[Webkit-unassigned] [Bug 197834] [MSE][GStreamer] update the readyState correctly in MediaPlayerPrivateGStreamerMSE

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=197834

--- Comment #3 from Alicia Boya García <aboya at igalia.com> ---
Comment on attachment 369718
  --> https://bugs.webkit.org/attachment.cgi?id=369718
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190513/7f579e70/attachment.html>