Summary: | Volume control not correctly initialized | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Hunziker <alex.hunziker> | ||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, eric, mrobinson, pnormand, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
Attachments: |
|
Description
Alexander Hunziker
2010-03-18 09:26:06 PDT
Created attachment 74636 [details]
proposed patch
Created attachment 74637 [details]
proposed patch
So after some investigation on this issue, the previous mute/volume notifications we got from PulseAudio were not going up to the MediaPlayer (and HTMLMediaElement). The one-shot Timers used to do so were simply never fired. It seems that the loop processing the Timers in ThreadTimers.cpp is in some cases triggered too late for the one-shot-now (startOneShot(0)) Timers and thus they are ignored. This whole code relying on currentTime() value is quite fragile IMHO. I tried to replace the Timer class implementation with a new implementation using g_timeout and skipping the ThreadTimers processing but then some tests started to fail, I could fix some but not all, so for now I decided to add that TimerGlib standalone implementation and keep the Timer one. Another, less intrusive and simpler, option would be to replace the Timers in the player with g_timeouts, like it was before. Comment on attachment 74637 [details]
proposed patch
Pulling out of review after discussion with Martin. The Timer issue is more general and will be dealt with in a separate patch.
Will send a new patch for this issue asap :)
Created attachment 74731 [details]
simpler patch
(In reply to comment #6) > Created an attachment (id=74731) [details] > simpler patch Not flagged for review yet, I have some issues here with a debug build, it seems the mute/volume callbacks get called during destruction of the MediaPlayer. I'll investigate this further later on. Created attachment 75117 [details]
proposed patch
Comment on attachment 75117 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=75117&action=review Sorry, that I missed this! Looks great in general, but I have a few questions... > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:184 > + MediaPlayerPrivateGStreamer* mp = reinterpret_cast<MediaPlayerPrivateGStreamer*>(data); > + mp->notifyVolumeToPlayer(); Instead of doing the reinterpret_cast here, why not do it when you set the callback. What I mean is this: gboolean mediaPlayerPrivateVolumeChangeTimeoutCallback(MediaPlayerPrivateGStreamer* player) ... m_volumeTimerHandler = g_timeout_add(0, reinterpret_cast<GSourceFunc>(mediaPlayerPrivateVolumeChangeTimeoutCallback), this); If you do it here, it only needs to be a static_cast too. > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:201 > +gboolean mediaPlayerPrivateMuteChangeTimeoutCallback(gpointer data) > +{ > + // This is the callback of the timeout source created in ::muteChanged. > + MediaPlayerPrivateGStreamer* mp = reinterpret_cast<MediaPlayerPrivateGStreamer*>(data); > + mp->notifyMuteToPlayer(); > + return FALSE; > +} Same comment here. > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:357 > + if (m_muteTimerHandler) > + g_source_remove(m_muteTimerHandler); > + m_muteTimerHandler = 0; > + > + if (m_volumeTimerHandler) > + g_source_remove(m_volumeTimerHandler); > + m_volumeTimerHandler = 0; Is there some chance that these id's may be re-used after the callback fires? Maybe you should set them to 0 in notifyMuteToPlayer and notifyVolumeToPlayer. If they are re-used you'll be removing some unknown GSource. > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:91 > - void volumeChangedTimerFired(Timer<MediaPlayerPrivateGStreamer>*); > + void notifyVolumeToPlayer(); > > bool supportsMuting() const; > void setMuted(bool); > void muteChanged(); > - void muteChangedTimerFired(Timer<MediaPlayerPrivateGStreamer>*); > + void notifyMuteToPlayer(); The names of the methods is just slightly funky. Maybe "notifyPlayerOfMute" and "notifyPlayerOfVolumeChange"? Created attachment 75259 [details]
updated patch
Comment on attachment 75259 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=75259&action=review Only one small issue, but it's fine to fix before landing it. > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:618 > + m_volumeTimerHandler = 0; I think this should happen before the early return. > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1197 > + m_muteTimerHandler = 0; Same here. Committed r73014: <http://trac.webkit.org/changeset/73014> http://trac.webkit.org/changeset/73014 might have broken GTK Linux 32-bit Debug |