Bug 35735 - [GStreamer] replace g_idle_add / g_timeout_add calls with Timers in the gstreamer player
Summary: [GStreamer] replace g_idle_add / g_timeout_add calls with Timers in the gstre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-04 06:32 PST by Philippe Normand
Modified: 2010-03-15 08:37 PDT (History)
3 users (show)

See Also:


Attachments
proposed patch (9.16 KB, patch)
2010-03-04 07:36 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (7.65 KB, patch)
2010-03-15 02:16 PDT, Philippe Normand
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-03-04 06:32:48 PST
So the playber backend can be reused by other ports more easily.
Comment 1 Philippe Normand 2010-03-04 07:36:10 PST
Created attachment 50021 [details]
proposed patch
Comment 2 Eric Seidel (no email) 2010-03-05 13:16:33 PST
Comment on attachment 50021 [details]
proposed patch

OK.
Comment 3 Eric Seidel (no email) 2010-03-05 13:16:33 PST
Comment on attachment 50021 [details]
proposed patch

OK.
Comment 4 WebKit Commit Bot 2010-03-05 14:05:20 PST
Attachment 50021 [details] was posted by a committer and has review+, assigning to Philippe Normand for commit.
Comment 5 Philippe Normand 2010-03-08 00:14:30 PST
Landed in r55657
Comment 6 Gustavo Noronha (kov) 2010-03-10 11:01:26 PST
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.
Comment 7 Gustavo Noronha (kov) 2010-03-10 11:02:39 PST
As per comment #6.
Comment 8 Philippe Normand 2010-03-11 01:10:23 PST
(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...
Comment 9 Gustavo Noronha (kov) 2010-03-12 11:44:16 PST
(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.
Comment 10 Philippe Normand 2010-03-15 02:05:13 PDT
(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.
Comment 11 Philippe Normand 2010-03-15 02:16:56 PDT
Created attachment 50695 [details]
proposed patch
Comment 12 Xan Lopez 2010-03-15 02:32:18 PDT
(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.
Comment 13 Philippe Normand 2010-03-15 05:00:03 PDT
(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
Comment 14 Gustavo Noronha (kov) 2010-03-15 07:54:17 PDT
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.
Comment 15 Philippe Normand 2010-03-15 08:37:24 PDT
Re-landed in r55996