WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Yacine Bandou
Comment 1
2019-05-13 02:12:21 PDT
Created
attachment 369718
[details]
Patch
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
Created
attachment 369955
[details]
Patch
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
Created
attachment 370750
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug