Bug 187111

Summary: [GStreamer] Return a valid time values in unprerolled states
Product: WebKit Reporter: Charlie Turner <cturner>
Component: WebKitGTKAssignee: Charlie Turner <cturner>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, bugs-noreply, calvaris, commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=180253
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing
none
Archive of layout-test-results from webkit-cq-01 for mac-sierra
none
Patch for landing none

Charlie Turner
Reported 2018-06-27 12:24:50 PDT
[GStreamer] Return a valid time values in unprerolled states
Attachments
Patch (2.17 KB, patch)
2018-06-27 13:21 PDT, Charlie Turner
no flags
Patch for landing (2.13 KB, patch)
2018-07-09 02:59 PDT, Charlie Turner
no flags
Patch for landing (2.14 KB, patch)
2018-07-09 03:35 PDT, Charlie Turner
no flags
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
Patch for landing (2.18 KB, patch)
2018-07-19 07:12 PDT, Charlie Turner
no flags
Charlie Turner
Comment 1 2018-06-27 13:21:24 PDT
Alicia Boya García
Comment 2 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?
Charlie Turner
Comment 3 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.
Alicia Boya García
Comment 4 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?
Charlie Turner
Comment 5 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
Alicia Boya García
Comment 6 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.
Alicia Boya García
Comment 7 2018-07-02 06:51:16 PDT
OK, LGTM.
Xabier Rodríguez Calvar
Comment 8 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
Charlie Turner
Comment 9 2018-07-09 02:59:33 PDT
Created attachment 344570 [details] Patch for landing
WebKit Commit Bot
Comment 10 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
Charlie Turner
Comment 11 2018-07-09 03:35:32 PDT
Created attachment 344571 [details] Patch for landing
WebKit Commit Bot
Comment 12 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.
WebKit Commit Bot
Comment 13 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.
WebKit Commit Bot
Comment 14 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
WebKit Commit Bot
Comment 15 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
Charlie Turner
Comment 16 2018-07-19 07:12:36 PDT
Created attachment 345344 [details] Patch for landing
WebKit Commit Bot
Comment 17 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>
WebKit Commit Bot
Comment 18 2018-07-19 07:51:23 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.