Bug 187111

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.