Bug 35706

Summary: [GStreamer] Buffering logic is not correct, and does not work very well
Product: WebKit Reporter: Gustavo Noronha (kov) <gns>
Component: MediaAssignee: Gustavo Noronha (kov) <gns>
Status: RESOLVED FIXED    
Severity: Normal CC: pnormand, slomo, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
proposed change
gns: commit-queue-
fix buffering logic to also consider non-download cases
gns: commit-queue-
set GStreamer state directly instead of using pause()
gns: commit-queue-
handle all cases of buffering gns: commit-queue-

Description Gustavo Noronha (kov) 2010-03-03 15:17:50 PST
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.
Comment 1 Gustavo Noronha (kov) 2010-03-03 15:22:38 PST
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.
Comment 2 Philippe Normand 2010-03-04 00:26:53 PST
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 ;)
Comment 3 Philippe Normand 2010-03-04 00:50:46 PST
(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?
Comment 4 Gustavo Noronha (kov) 2010-03-08 09:45:28 PST
(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 =(.
Comment 5 Philippe Normand 2010-03-08 09:56:00 PST
(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 =(.
Comment 6 Gustavo Noronha (kov) 2010-03-08 12:44:11 PST
(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.
Comment 7 Gustavo Noronha (kov) 2010-03-08 12:51:41 PST
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.
Comment 8 Philippe Normand 2010-03-09 02:49:28 PST
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.
Comment 9 Gustavo Noronha (kov) 2010-03-09 07:39:06 PST
(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!
Comment 10 Philippe Normand 2010-03-09 07:53:43 PST
(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. :)
Comment 11 Gustavo Noronha (kov) 2010-03-09 09:21:54 PST
(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.
Comment 12 Gustavo Noronha (kov) 2010-03-09 09:39:15 PST
(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.
Comment 13 Gustavo Noronha (kov) 2010-03-09 09:41:45 PST
Created attachment 50317 [details]
set GStreamer state directly instead of using pause()
Comment 14 Gustavo Noronha (kov) 2010-03-09 09:42:48 PST
Created attachment 50318 [details]
handle all cases of buffering
Comment 15 Dirk Schulze 2010-03-09 10:14:42 PST
Comment on attachment 50318 [details]
handle all cases of buffering

lgtm r=me.
Comment 16 Gustavo Noronha (kov) 2010-03-09 10:36:18 PST
Comment on attachment 50317 [details]
set GStreamer state directly instead of using pause()

Landed as r55732. Thanks!
Comment 17 Gustavo Noronha (kov) 2010-03-09 10:44:13 PST
Comment on attachment 50318 [details]
handle all cases of buffering

Landed as r55733.