Summary: | [GStreamer] Return a valid time values in unprerolled states | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Charlie Turner <cturner> | ||||||||||||
Component: | WebKitGTK | Assignee: | 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
Charlie Turner
2018-06-27 12:24:50 PDT
Created attachment 343746 [details]
Patch
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? (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. (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? (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 > 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.
OK, LGTM. Comment on attachment 343746 [details] Patch (In reply to Alicia Boya García from comment #7) > OK, LGTM. LGTM2 Created attachment 344570 [details]
Patch for landing
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 Created attachment 344571 [details]
Patch for landing
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. The commit-queue encountered the following flaky tests while processing attachment 344571 [details]:
The commit-queue is continuing to process your patch.
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 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
Created attachment 345344 [details]
Patch for landing
Comment on attachment 345344 [details] Patch for landing Clearing flags on attachment: 345344 Committed r233984: <https://trac.webkit.org/changeset/233984> All reviewed patches have been landed. Closing bug. |