Bug 197834 - [MSE][GStreamer] update the readyState correctly in MediaPlayerPrivateGStreamerMSE
Summary: [MSE][GStreamer] update the readyState correctly in MediaPlayerPrivateGStream...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-13 02:00 PDT by Yacine Bandou
Modified: 2019-06-06 10:19 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.76 KB, patch)
2019-05-13 02:12 PDT, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2019-05-15 08:50 PDT, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2019-05-28 09:19 PDT, Yacine Bandou
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yacine Bandou 2019-05-13 02:00:49 PDT
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".
Comment 1 Yacine Bandou 2019-05-13 02:12:21 PDT
Created attachment 369718 [details]
Patch
Comment 2 Philippe Normand 2019-05-13 03:22:18 PDT
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.
Comment 3 Alicia Boya García 2019-05-13 04:05:53 PDT
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?
Comment 4 Yacine Bandou 2019-05-15 08:50:40 PDT
Created attachment 369955 [details]
Patch
Comment 5 Olivier Blin 2019-05-22 13:13:52 PDT
Is it the same as bug 194775 ?
Comment 6 Alicia Boya García 2019-05-23 06:08:14 PDT
Passes the tests and LGTM. Thank you.
Comment 7 Xabier Rodríguez Calvar 2019-05-28 00:19:54 PDT
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.
Comment 8 Yacine Bandou 2019-05-28 09:19:15 PDT
Created attachment 370750 [details]
Patch
Comment 9 WebKit Commit Bot 2019-05-28 23:00:46 PDT
Comment on attachment 370750 [details]
Patch

Clearing flags on attachment: 370750

Committed r245848: <https://trac.webkit.org/changeset/245848>
Comment 10 WebKit Commit Bot 2019-05-28 23:00:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Charlie Turner 2019-06-06 10:19:02 PDT
I have proposed this for merging  on the 2.24 stable series: https://trac.webkit.org/wiki/WebKitGTK/2.24.x