Summary: | [GStreamer] Buffering logic is not correct, and does not work very well | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gustavo Noronha (kov) <gustavo> | ||||||||||
Component: | Media | Assignee: | Gustavo Noronha (kov) <gustavo> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | pnormand, slomo, xan.lopez | ||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Gustavo Noronha (kov)
2010-03-03 15:17:50 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.
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 ;) (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? (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 =(. (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 =(. (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. 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 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. (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! (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. :) (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. (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. Created attachment 50317 [details]
set GStreamer state directly instead of using pause()
Created attachment 50318 [details]
handle all cases of buffering
Comment on attachment 50318 [details]
handle all cases of buffering
lgtm r=me.
Comment on attachment 50317 [details] set GStreamer state directly instead of using pause() Landed as r55732. Thanks! Comment on attachment 50318 [details] handle all cases of buffering Landed as r55733. |