Bug 35735 – [GStreamer] replace g_idle_add / g_timeout_add calls with Timers in the gstreamer player

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

So the playber backend can be reused by other ports more easily.

[Philippe Normand]

Created attachment 50021 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 50021 [details]
proposed patch

OK.

[Eric Seidel (no email)]

Comment on attachment 50021 [details]
proposed patch

OK.

[WebKit Commit Bot]

Attachment 50021 [details] was posted by a committer and has review+, assigning to Philippe Normand for commit.

[Philippe Normand]

Landed in r55657

[Gustavo Noronha (kov)]

Comment on attachment 50021 [details]
proposed patch

We rolled this out in r55789, as it caused very bad behaviour on Youtube HTML5 for Xan, that was improved by reverting this change.

[Gustavo Noronha (kov)]

As per comment #6.

[Philippe Normand]

(In reply to comment #6)
> (From update of attachment 50021 [details])
> We rolled this out in r55789, as it caused very bad behaviour on Youtube HTML5
> for Xan, that was improved by reverting this change.

I don't see how this patch can break youtube HTML5 stuff because:

- the 2 idle sources for mute/volume are in the end just moved to SharedTimerGtk and they are fired only if you change or mute the volume
- the timeout replaced by a Timer is use only during on-disk-buffering, which AFAIK is not enabled for youtube...

[Gustavo Noronha (kov)]

(In reply to comment #8)
> - the timeout replaced by a Timer is use only during on-disk-buffering, which
> AFAIK is not enabled for youtube...

If that is controlled by the preload attribute, then I think you're wrong here. When I was working on my buffering patch I believe I noticed on-disk-buffering triggering all the time. This seems to be related to the reason why media/video-preload.html fails. If you watch the media tests running you'll see most of them are painting the on-disk-buffering on top of the position bar, which is weird. I'm working on a patch to fix media/video-preload.html, which will likely fix it being triggered always.

[Philippe Normand]

(In reply to comment #9)
> (In reply to comment #8)
> > - the timeout replaced by a Timer is use only during on-disk-buffering, which
> > AFAIK is not enabled for youtube...
> 
> If that is controlled by the preload attribute, then I think you're wrong here.

I checked again and I see only the "live" buffering in the logs and no message like:

[Buffering] Starting on-disk buffering.


> When I was working on my buffering patch I believe I noticed on-disk-buffering
> triggering all the time. This seems to be related to the reason why
> media/video-preload.html fails. If you watch the media tests running you'll see
> most of them are painting the on-disk-buffering on top of the position bar,
> which is weird. I'm working on a patch to fix media/video-preload.html, which
> will likely fix it being triggered always.

[Philippe Normand]

Created attachment 50695 [details]
proposed patch

[Xan Lopez]

(In reply to comment #11)
> Created an attachment (id=50695) [details]
> proposed patch

I have been testing this for some minutes and I can't reproduce any of the weird things that happened to me before.

[Philippe Normand]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > - the timeout replaced by a Timer is use only during on-disk-buffering, which
> > > AFAIK is not enabled for youtube...
> > 
> > If that is controlled by the preload attribute, then I think you're wrong here.
> 
> I checked again and I see only the "live" buffering in the logs and no message
> like:
> 
> [Buffering] Starting on-disk buffering.
> 
> 

Some precisions :) The preload attribute is set to "auto" (by default), which means on-disk buffering for our player. But GStreamer doesn't support buffering h264/mp4 yet and it fallsback to "stream buffering" mode.

The autoplay flag is set as well, which means preload value should be ignored, according to http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#attr-media-preload

[Gustavo Noronha (kov)]

Comment on attachment 50695 [details]
proposed patch

Great, since it's good for Xan, I think we can go ahead with this one again, then.

[Philippe Normand]

Re-landed in r55996