Bug 197834 – [MSE][GStreamer] update the readyState correctly in MediaPlayerPrivateGStreamerMSE

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Yacine Bandou]

The buffering state and the m_downloadFinished boolean aren't supported in MSE case.

When the readyState is already "HaveEnoughData", we don't want to revert it to "HaveFutureData",
else the MediaPlayer would send a "canplay" event instead of "canplaythrough".

[Yacine Bandou]

Created attachment 369718 [details]
Patch

[Philippe Normand]

Comment on attachment 369718 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The buffering state and the m_downloadFinished boolean aren't supported in MSE case.

Why do you think "buffering state" is not supported? m_buffering is about pipeline buffering, not on-disk buffering.

[Alicia Boya García]

Comment on attachment 369718 [details]
Patch

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

Is there any LayoutTest (existing or new) that is fixed by this patch? If you can provide one, that's informal r+ from me.

Even if I end up reworking the readyState handling completely (which will still take quite a while) having the test will help in avoiding me breaking it on accident.

>> Source/WebCore/ChangeLog:8
>> +        The buffering state and the m_downloadFinished boolean aren't supported in MSE case.
> 
> Why do you think "buffering state" is not supported? m_buffering is about pipeline buffering, not on-disk buffering.

m_buffering is set by the parent class in response to GST_MESSAGE_BUFFERING. These messages are sent by network sources, but not by WebKitMediaSrc or appsrc.

Also, in the WebKit MSE design the ready state (including HaveFutureData vs HaveEnoughData) is calculated and set by the multi-platform code (see MediaSource::monitorSourceBuffers()), and the player is expected to respect it. This is not what is happening in our current implementation, but I'm working on a revamp of the player where that will be the case and that should fix the issue once released.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:-538
> -            } else if (m_buffering) {
> -                if (m_bufferingPercentage == 100) {
> -                    GST_DEBUG("[Buffering] Complete.");
> -                    m_buffering = false;
> -                    m_readyState = MediaPlayer::HaveEnoughData;
> -                    GST_DEBUG("m_readyState=%s", dumpReadyState(m_readyState));
> -                    m_networkState = m_downloadFinished ? MediaPlayer::Idle : MediaPlayer::Loading;
> -                } else {
> -                    m_readyState = MediaPlayer::HaveCurrentData;
> -                    GST_DEBUG("m_readyState=%s", dumpReadyState(m_readyState));
> -                    m_networkState = MediaPlayer::Loading;
> -                }
> -            } else if (m_downloadFinished) {
> -                m_readyState = MediaPlayer::HaveEnoughData;
> -                GST_DEBUG("m_readyState=%s", dumpReadyState(m_readyState));
> -                m_networkState = MediaPlayer::Loaded;

I agree this code is unnecessary in the MSE player for the aforementioned reasons.

I would argue that long term the same thing could be said for most of the stuff done in the updateStates() method in the MSE player though.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:525
> -                m_readyState = MediaPlayer::HaveFutureData;
> +                if (m_readyState < MediaPlayer::HaveFutureData)
> +                    m_readyState = MediaPlayer::HaveFutureData;

Makes sense that we don't downgrade the readyState. I wonder, would it work as is if you just completely removed this piece of code?

[Yacine Bandou]

Created attachment 369955 [details]
Patch

[Olivier Blin]

Is it the same as bug 194775 ?

[Alicia Boya García]

Passes the tests and LGTM. Thank you.

[Xabier Rodríguez Calvar]

Comment on attachment 369955 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The buffering state and the m_downloadFinished boolean aren't supported in MSE case.

in the MSE case

> Source/WebCore/ChangeLog:10
> +        else the MediaPlayer would send a "canplay" event instead of "canplaythrough".

else -> or else, instead of a

> LayoutTests/ChangeLog:8
> +        Add a new test that check if the MediaElement receive the "canplaythrough"

checks, receives

> LayoutTests/media/media-source/media-source-canplaythrough-event.html:18
> +        waitForEventOn(video, 'canplaythrough', oncanplaythrough, false, true);

I think we can remove oncanplaythrough if we use:

waitForEventAndEnd('canplaythrough', null);

> LayoutTests/media/media-source/media-source-canplaythrough-event.html:25
> +    function oncanplaythrough() {
> +        passTest("Receive canplaythrough event");
> +    }

I think we could remove this.

[Yacine Bandou]

Created attachment 370750 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 370750 [details]
Patch

Clearing flags on attachment: 370750

Committed r245848: <https://trac.webkit.org/changeset/245848>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Charlie Turner]

I have proposed this for merging  on the 2.24 stable series: https://trac.webkit.org/wiki/WebKitGTK/2.24.x