Bug 116431 – [GStreamer] Get rid of fill timer

• Status: RESOLVED
• Resolution: INVALID
• Component: Media

[Balazs Kelemen]

I don't think we need this timer. We can query buffering stats as needed. The fill timer only active when preload is auto. This is bad because our internal behavior is different in preload="none" and nothing guarantees that tests that are passing with auto preload would pass with none or metadata.

[Balazs Kelemen]

Created attachment 202269 [details]
Patch

[Philippe Normand]

Can the patch be rebased against ToT so the EWS can process it?

[Balazs Kelemen]

(In reply to comment #2)
> Can the patch be rebased against ToT so the EWS can process it?

I forgot to set the dependency. It's based on bug 11628. I only tested it upon the other. Can we resolve that one first? Than I can upload it again so we can see how it goes on ews.

[Balazs Kelemen]

Created attachment 202418 [details]
Patch

[Philippe Normand]

Comment on attachment 202418 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-860
> -        gst_element_set_state(m_playBin.get(), GST_STATE_PAUSED);

Please don't remove this, the pipeline really needs to be paused in this case.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:865
> +        gst_query_parse_buffering_percent(query, 0, &percent);

Does this really return the percentage of data buffered on-disk?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:873
> +        if (m_mediaDuration)
> +            m_maxTimeLoaded = percent == 100 ? m_mediaDuration : (m_mediaDuration * percent) / 100.0f;

Perhaps this can be moved to a small private method, that code is called at two different places

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1170
> -        if (m_fillTimer.isActive())
> -            m_networkState = MediaPlayer::Loading;

So where is this done now?

[Philippe Normand]

Comment on attachment 202418 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1170
>> -            m_networkState = MediaPlayer::Loading;
> 
> So where is this done now?

Sorry I see it's done above, line 1063 now

[Balazs Kelemen]

(In reply to comment #5)
> (From update of attachment 202418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202418&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-860
> > -        gst_element_set_state(m_playBin.get(), GST_STATE_PAUSED);
> 
> Please don't remove this, the pipeline really needs to be paused in this case.

Ok.

> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:865
> > +        gst_query_parse_buffering_percent(query, 0, &percent);
> 
> Does this really return the percentage of data buffered on-disk?

Good observation. Only if we are doing download buffering (on-disk buffering in other words). I was not sure about what this function should tell, I just considered how it is used internally. Now I guess it should be the maximal position that has been loaded, no matter if we reached it by fetching into memory via normal buffering or by downloading the file.

Actually this reveals a few other things:
 - readystate should be updated based on the state of the playback (memory) buffer (not on |maxTimeLoaded() == duration()|)
 - Why do we only do download buffering if preload=auto? preload=none means don't download before playback starts, but it's our internal decision whether to use download buffering after that. As preload=none is work correctly after recent patches we maybe want to always do download buffering?

[Balazs Kelemen]

I abandon this. Seems like a timer is really necessary to track the state of download buffering. The buffering messages only describe the state of the memory buffer. (The documentation is quite ambiguous here.) There are still things annoying me:
 1. there is no rationale of disabling download buffering with preload="none"
 2. maxTimeLoaded()

float loaded = m_maxTimeLoaded;
if (!loaded && !m_fillTimer.isActive())
    loaded = duration();

That's wrong. What if the fill timer was never active? What if duration is not known? I will try to fix these.

[Balazs Kelemen]

> 
> float loaded = m_maxTimeLoaded;
> if (!loaded && !m_fillTimer.isActive())
>     loaded = duration();
> 
> That's wrong. What if the fill timer was never active? What if duration is not known? I will try to fix these.

Forgot the most annoying: if I remove the if, there are plenty of tests start failing mostly because canplaythrough does not fire.