RESOLVED FIXED 197834
[MSE][GStreamer] update the readyState correctly in MediaPlayerPrivateGStreamerMSE
https://bugs.webkit.org/show_bug.cgi?id=197834
Summary [MSE][GStreamer] update the readyState correctly in MediaPlayerPrivateGStream...
Yacine Bandou
Reported 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".
Attachments
Patch (3.76 KB, patch)
2019-05-13 02:12 PDT, Yacine Bandou
no flags
Patch (6.50 KB, patch)
2019-05-15 08:50 PDT, Yacine Bandou
no flags
Patch (6.36 KB, patch)
2019-05-28 09:19 PDT, Yacine Bandou
no flags
Yacine Bandou
Comment 1 2019-05-13 02:12:21 PDT
Philippe Normand
Comment 2 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.
Alicia Boya García
Comment 3 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?
Yacine Bandou
Comment 4 2019-05-15 08:50:40 PDT
Olivier Blin
Comment 5 2019-05-22 13:13:52 PDT
Is it the same as bug 194775 ?
Alicia Boya García
Comment 6 2019-05-23 06:08:14 PDT
Passes the tests and LGTM. Thank you.
Xabier Rodríguez Calvar
Comment 7 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.
Yacine Bandou
Comment 8 2019-05-28 09:19:15 PDT
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2019-05-28 23:00:48 PDT
All reviewed patches have been landed. Closing bug.
Charlie Turner
Comment 11 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
Note You need to log in before you can comment on or make changes to this bug.