Bug 187111 – [GStreamer] Return a valid time values in unprerolled states

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Charlie Turner]

[GStreamer] Return a valid time values in unprerolled states

[Charlie Turner]

Created attachment 343746 [details]
Patch

[Alicia Boya García]

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]

(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]

(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]

(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]

> 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]

OK, LGTM.

[Xabier Rodríguez Calvar]

Comment on attachment 343746 [details]
Patch

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

LGTM2

[Charlie Turner]

Created attachment 344570 [details]
Patch for landing

[WebKit Commit Bot]

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]

Created attachment 344571 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 344571 [details]:

fast/files/file-reader-immediate-abort.html bug 187464 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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 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]

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]

Created attachment 345344 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345344 [details]
Patch for landing

Clearing flags on attachment: 345344

Committed r233984: <https://trac.webkit.org/changeset/233984>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.