Bug 35706 – [GStreamer] Buffering logic is not correct, and does not work very well

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Gustavo Noronha (kov)]

The main problem is this:

        if (m_fillTimeoutId) {
            m_networkState = MediaPlayer::Loading;
            // Buffering has just started, we should now have enough
            // data to restart playback if it was internally paused by
            // GStreamer.
            if (m_paused && !m_startedPlaying)
                gst_element_set_state(m_playBin, GST_STATE_PLAYING);
        }

This pretty much guarantees that the pipeline is playing while buffering, which is not the behavior we want, nor what GStreamer expects.

[Gustavo Noronha (kov)]

Created attachment 49955 [details]
proposed change

This works much better to me (I have quite a bit of latency, though I do have quite a bit of bandwidth too). Specially watching Youtube videos using my HTML5 extension.

[Philippe Normand]

This patch breaks on-disk buffering... Just try an apple trailer for instance:

<video preload controls src="http://movies.apple.com/movies/paramount/ironman2/ironman2-z7r459-tlr1_480p.mov"/>

Without the patch you can see the download in-progress filling the dark progressbar of the controls.

So maybe the patch fixes live buffering when you play youtube videos but it'd be nice to keep the on-disk buffering feature working as well ;)

[Philippe Normand]

(In reply to comment #0)
> The main problem is this:
> 
>         if (m_fillTimeoutId) {
>             m_networkState = MediaPlayer::Loading;
>             // Buffering has just started, we should now have enough
>             // data to restart playback if it was internally paused by
>             // GStreamer.
>             if (m_paused && !m_startedPlaying)
>                 gst_element_set_state(m_playBin, GST_STATE_PLAYING);
>         }
> 
> This pretty much guarantees that the pipeline is playing while buffering, which
> is not the behavior we want, nor what GStreamer expects.

This code is executed only in the case of on-disk buffering. I didn't know youtube had enabled this, or is your extension setting the preload attribute on the video?

[Gustavo Noronha (kov)]

(In reply to comment #3)
> (In reply to comment #0)
> > The main problem is this:
> > 
> >         if (m_fillTimeoutId) {
> >             m_networkState = MediaPlayer::Loading;
> >             // Buffering has just started, we should now have enough
> >             // data to restart playback if it was internally paused by
> >             // GStreamer.
> >             if (m_paused && !m_startedPlaying)
> >                 gst_element_set_state(m_playBin, GST_STATE_PLAYING);
> >         }
> > 
> > This pretty much guarantees that the pipeline is playing while buffering, which
> > is not the behavior we want, nor what GStreamer expects.
> 
> This code is executed only in the case of on-disk buffering. I didn't know
> youtube had enabled this, or is your extension setting the preload attribute on
> the video?

Not really. It is executed for any buffering GStreamer decides to perform, which it does decide to perform for many videos, even without the preload attribute. I am becoming pretty sure that our state tracking needs a complete rewrite, to be honest. 

And yeah, this patch of mine is not really correct =(.

[Philippe Normand]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #0)
> > > The main problem is this:
> > > 
> > >         if (m_fillTimeoutId) {
> > >             m_networkState = MediaPlayer::Loading;
> > >             // Buffering has just started, we should now have enough
> > >             // data to restart playback if it was internally paused by
> > >             // GStreamer.
> > >             if (m_paused && !m_startedPlaying)
> > >                 gst_element_set_state(m_playBin, GST_STATE_PLAYING);
> > >         }
> > > 
> > > This pretty much guarantees that the pipeline is playing while buffering, which
> > > is not the behavior we want, nor what GStreamer expects.
> > 
> > This code is executed only in the case of on-disk buffering. I didn't know
> > youtube had enabled this, or is your extension setting the preload attribute on
> > the video?
> 
> Not really. It is executed for any buffering GStreamer decides to perform,
> which it does decide to perform for many videos, even without the preload
> attribute.

Err no, the fillTimer is started only if the buffering mode is GST_BUFFERING_DOWNLOAD (see processBufferingStats) or did I miss some case?

> I am becoming pretty sure that our state tracking needs a complete
> rewrite, to be honest. 
> 
> And yeah, this patch of mine is not really correct =(.

[Gustavo Noronha (kov)]

(In reply to comment #5)
> > Not really. It is executed for any buffering GStreamer decides to perform,
> > which it does decide to perform for many videos, even without the preload
> > attribute.
> 
> Err no, the fillTimer is started only if the buffering mode is
> GST_BUFFERING_DOWNLOAD (see processBufferingStats) or did I miss some case?

Nope, you're right, sorry for being confused =D. So the problem really is we are not handling other kinds of buffering requests correctly. New patch to come, which makes it work fine for me, while keeping the other use cases working.

[Gustavo Noronha (kov)]

Created attachment 50242 [details]
fix buffering logic to also consider non-download cases

So it turns out that the real problem was in using the on-disk-buffering logic to pause/play the pipeline. GStreamer uses a buffering-percent value in the struct that is sent to the bus in the buffering message to provide information on how much of the immediate buffering it needs to play is actually complete. That is the information we need to use to decide when to restart playing the pipeline that got paused for buffering, not the fact that the on-disk buffering has started.

[Philippe Normand]

Comment on attachment 50242 [details]
fix buffering logic to also consider non-download cases


>     g_object_set(m_playBin, "uri", url.utf8().data(), NULL);
>-    pause();
>+
>+    // GStreamer needs to have the pipeline set to a paused state to
>+    // start providing anything useful.
>+    gst_element_set_state(m_playBin, GST_STATE_PAUSED);

Why that change?

+    const GstStructure *structure = gst_message_get_structure(message);

Should be GstStructure* structure no? I wonder why the style bot didn't complain. Or maybe it's not a strong rule ;) 
What about using gst_message_parse_buffering instead of reading directly the structure?

The rest of the patch is fine by me.

[Gustavo Noronha (kov)]

(In reply to comment #8)
> (From update of attachment 50242 [details])
> 
> >     g_object_set(m_playBin, "uri", url.utf8().data(), NULL);
> >-    pause();
> >+
> >+    // GStreamer needs to have the pipeline set to a paused state to
> >+    // start providing anything useful.
> >+    gst_element_set_state(m_playBin, GST_STATE_PAUSED);
> 
> Why that change?

I want to make sure the setting of the state for GStreamer is not a real "pause" on the player. This change is to guarantee that we don't get side-effects if we change the pause method to, say, change m_paused, or change some other flag. This is just an implementation detail for GStreamer, so I think it should not be confused with the actual actions in the player.

This could go in separately, before the other changes, what do you think?

> +    const GstStructure *structure = gst_message_get_structure(message);
> 
> Should be GstStructure* structure no? I wonder why the style bot didn't
> complain. Or maybe it's not a strong rule ;) 

It should be that indeed. Not sure why the style bot didn't catch it.

> What about using gst_message_parse_buffering instead of reading directly the
> structure?

Sounds like a good idea as well. I'll fix these both when I come back from lunch, thanks for your comments!

[Philippe Normand]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 50242 [details] [details])
> > 
> > >     g_object_set(m_playBin, "uri", url.utf8().data(), NULL);
> > >-    pause();
> > >+
> > >+    // GStreamer needs to have the pipeline set to a paused state to
> > >+    // start providing anything useful.
> > >+    gst_element_set_state(m_playBin, GST_STATE_PAUSED);
> > 
> > Why that change?
> 
> I want to make sure the setting of the state for GStreamer is not a real
> "pause" on the player. This change is to guarantee that we don't get
> side-effects if we change the pause method to, say, change m_paused, or change
> some other flag. This is just an implementation detail for GStreamer, so I
> think it should not be confused with the actual actions in the player.
> 
> This could go in separately, before the other changes, what do you think?
> 

Hum ok then, but still, a state change will trigger an updateStates() call because we get a state-changed message on the bus and this might update some internal variables of the player. :)

[Gustavo Noronha (kov)]

(In reply to comment #10)
> Hum ok then, but still, a state change will trigger an updateStates() call
> because we get a state-changed message on the bus and this might update some
> internal variables of the player. :)

Right. But this already happens currently. We currently call pause(), which changes the state.

[Gustavo Noronha (kov)]

(In reply to comment #11)
> (In reply to comment #10)
> > Hum ok then, but still, a state change will trigger an updateStates() call
> > because we get a state-changed message on the bus and this might update some
> > internal variables of the player. :)
> 
> Right. But this already happens currently. We currently call pause(), which
> changes the state.

FWIW, this is required (which is why  is done currently) because, as the comment I added implies, GStreamer will not start doing its thing before it is put in the paused state. What we could investigate is moving this to the prepareToPlay() implementation instead.

[Gustavo Noronha (kov)]

Created attachment 50317 [details]
set GStreamer state directly instead of using pause()

[Gustavo Noronha (kov)]

Created attachment 50318 [details]
handle all cases of buffering

[Dirk Schulze]

Comment on attachment 50318 [details]
handle all cases of buffering

lgtm r=me.

[Gustavo Noronha (kov)]

Comment on attachment 50317 [details]
set GStreamer state directly instead of using pause()

Landed as r55732. Thanks!

[Gustavo Noronha (kov)]

Comment on attachment 50318 [details]
handle all cases of buffering

Landed as r55733.