Bug 36299

Summary: Volume control not correctly initialized
Product: WebKit Reporter: Alexander Hunziker <alex.hunziker>
Component: WebKitGTKAssignee: 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 Flags
proposed patch
none
proposed patch
none
simpler patch
none
proposed patch
mrobinson: review-
updated patch mrobinson: review+

Description Alexander Hunziker 2010-03-18 09:26:06 PDT
When watching HTML5 video, WebkitGTK seems to use the browser's volume channel in PulseAudio to control the volume, which is good. However, when first showing a website, the volume level is not correctly initialized.

When i open my sound preferences, I see that epiphany's channel there is set to, say, 30%. The video viewer on e.g. openvideo.dailymotion.com however shows the volume as maximised. The problem now arises when one uses the GUI on the website to change the volume. At the first small change in the GUI (say from 100% to 98%, the real volume jumps from the 30% it was set to to 98%, which potentially blasts one's ears out when wearing headphones.

Solution, when loading a website with sound or video, the volume controls there should be initialised to the volume currently set in Pulse.
Comment 1 Philippe Normand 2010-11-23 03:27:53 PST
Created attachment 74636 [details]
proposed patch
Comment 2 Philippe Normand 2010-11-23 03:34:46 PST
Created attachment 74637 [details]
proposed patch
Comment 3 Philippe Normand 2010-11-23 03:42:20 PST
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.
Comment 4 Philippe Normand 2010-11-23 05:54:00 PST
Another, less intrusive and simpler, option would be to replace the Timers in the player with g_timeouts, like it was before.
Comment 5 Philippe Normand 2010-11-23 08:43:46 PST
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 :)
Comment 6 Philippe Normand 2010-11-24 01:08:38 PST
Created attachment 74731 [details]
simpler patch
Comment 7 Philippe Normand 2010-11-24 01:10:12 PST
(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.
Comment 8 Philippe Normand 2010-11-30 02:50:11 PST
Created attachment 75117 [details]
proposed patch
Comment 9 Martin Robinson 2010-11-30 09:30:58 PST
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"?
Comment 10 Philippe Normand 2010-12-01 00:08:09 PST
Created attachment 75259 [details]
updated patch
Comment 11 Martin Robinson 2010-12-01 02:13:58 PST
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.
Comment 12 Philippe Normand 2010-12-01 03:42:34 PST
Committed r73014: <http://trac.webkit.org/changeset/73014>
Comment 13 WebKit Review Bot 2010-12-01 09:18:09 PST
http://trac.webkit.org/changeset/73014 might have broken GTK Linux 32-bit Debug