Bug 187111 - [GStreamer] Return a valid time values in unprerolled states
Summary: [GStreamer] Return a valid time values in unprerolled states
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charlie Turner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-27 12:24 PDT by Charlie Turner
Modified: 2018-07-19 07:51 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.17 KB, patch)
2018-06-27 13:21 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch for landing (2.13 KB, patch)
2018-07-09 02:59 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch for landing (2.14 KB, patch)
2018-07-09 03:35 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-01 for mac-sierra (2.34 MB, application/zip)
2018-07-09 04:41 PDT, WebKit Commit Bot
no flags Details
Patch for landing (2.18 KB, patch)
2018-07-19 07:12 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2018-06-27 12:24:50 PDT
[GStreamer] Return a valid time values in unprerolled states
Comment 1 Charlie Turner 2018-06-27 13:21:24 PDT
Created attachment 343746 [details]
Patch
Comment 2 Alicia Boya García 2018-06-27 13:31:33 PDT
Comment on attachment 343746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343746&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:467
> +        return MediaTime::positiveInfiniteTime();

So the assert raises because it's being given a range with invalid edges and this fixes it by making one of them infinite... That sounds a bit fishy. Why is the range being added in this case (when duration is unknown) in the first place?
Comment 3 Charlie Turner 2018-06-28 01:27:43 PDT
(In reply to Alicia Boya García from comment #2)
> Comment on attachment 343746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343746&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:467
> > +        return MediaTime::positiveInfiniteTime();
> 
> So the assert raises because it's being given a range with invalid edges and
> this fixes it by making one of them infinite... That sounds a bit fishy.

An infinite edge is not invalid, which is the point here to not hit an assertion. I chose the infinite edge because just below this check we have the condition it's trying to avoid calling when it knows it will fail,

    if (!gst_element_query_duration(m_pipeline.get(), GST_FORMAT_TIME, &timeLength) || !GST_CLOCK_TIME_IS_VALID(timeLength)) {
        GST_DEBUG("Time duration query failed for %s", m_url.string().utf8().data());
        return MediaTime::positiveInfiniteTime();
    }

I would argue it's pretty fishy to return an infinite time for a failed query. But if we have a check to avoid testing a condition that will certainly fail, returning infinite time, then if said check fails, we should also return the same result. That's my reasoning.

> Why is the range being added in this case (when duration is unknown) in the
> first place?

The seekable property can be evaluated at any moment in the media lifecycle (such as before play()),

virtual std::unique_ptr<PlatformTimeRanges> seekable() const { return maxMediaTimeSeekable() == MediaTime::zeroTime() ? std::make_unique<PlatformTimeRanges>() : std::make_unique<PlatformTimeRanges>(minMediaTimeSeekable(), maxMediaTimeSeekable()); }

It might make more sense in these cases (for both this condition and the failed query check) to return a zeroTime instead and invoke the default ctor for PlatformTimeRanges.
Comment 4 Alicia Boya García 2018-06-28 10:54:24 PDT
(In reply to Charlie Turner from comment #3)
> The seekable property can be evaluated at any moment in the media lifecycle
> (such as before play()),

What is the "seekable" property?
Comment 5 Charlie Turner 2018-06-29 01:50:59 PDT
(In reply to Alicia Boya García from comment #4)
> (In reply to Charlie Turner from comment #3)
> > The seekable property can be evaluated at any moment in the media lifecycle
> > (such as before play()),
> 
> What is the "seekable" property?

https://html.spec.whatwg.org/multipage/media.html#dom-media-seekable
Comment 6 Alicia Boya García 2018-07-02 03:45:18 PDT
> User agents should adopt a very liberal and optimistic view of what is seekable. User agents should also buffer recent content where possible to enable seeking to be fast.

I guess [0, +inf) can be considered very liberal and optimistic.
Comment 7 Alicia Boya García 2018-07-02 06:51:16 PDT
OK, LGTM.
Comment 8 Xabier Rodríguez Calvar 2018-07-02 06:52:07 PDT
Comment on attachment 343746 [details]
Patch

(In reply to Alicia Boya García from comment #7)
> OK, LGTM.

LGTM2
Comment 9 Charlie Turner 2018-07-09 02:59:33 PDT
Created attachment 344570 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-07-09 03:01:32 PDT
Comment on attachment 344570 [details]
Patch for landing

Rejecting attachment 344570 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 344570, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/8480861
Comment 11 Charlie Turner 2018-07-09 03:35:32 PDT
Created attachment 344571 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-07-09 04:26:48 PDT
The commit-queue encountered the following flaky tests while processing attachment 344571 [details]:

fast/files/file-reader-immediate-abort.html bug 187464 (author: msaboff@apple.com)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2018-07-09 04:26:49 PDT
The commit-queue encountered the following flaky tests while processing attachment 344571 [details]:

The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2018-07-09 04:41:48 PDT
Comment on attachment 344571 [details]
Patch for landing

Rejecting attachment 344571 [details] from commit-queue.

New failing tests:
media/media-fullscreen-return-to-inline.html
Full output: https://webkit-queues.webkit.org/results/8481144
Comment 15 WebKit Commit Bot 2018-07-09 04:41:50 PDT
Created attachment 344574 [details]
Archive of layout-test-results from webkit-cq-01 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 16 Charlie Turner 2018-07-19 07:12:36 PDT
Created attachment 345344 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2018-07-19 07:51:22 PDT
Comment on attachment 345344 [details]
Patch for landing

Clearing flags on attachment: 345344

Committed r233984: <https://trac.webkit.org/changeset/233984>
Comment 18 WebKit Commit Bot 2018-07-19 07:51:23 PDT
All reviewed patches have been landed.  Closing bug.