Summary: | [GStreamer] replace g_idle_add / g_timeout_add calls with Timers in the gstreamer player | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||
Component: | WebKitGTK | Assignee: | Philippe Normand <pnormand> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, gustavo, xan.lopez | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Philippe Normand
2010-03-04 06:32:48 PST
Created attachment 50021 [details]
proposed patch
Comment on attachment 50021 [details]
proposed patch
OK.
Comment on attachment 50021 [details]
proposed patch
OK.
Attachment 50021 [details] was posted by a committer and has review+, assigning to Philippe Normand for commit.
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. As per comment #6. (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... (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. (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. Created attachment 50695 [details]
proposed patch
(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. (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 on attachment 50695 [details]
proposed patch
Great, since it's good for Xan, I think we can go ahead with this one again, then.
Re-landed in r55996 |