Bug 36299 – Volume control not correctly initialized

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alexander Hunziker]

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.

[Philippe Normand]

Created attachment 74636 [details]
proposed patch

[Philippe Normand]

Created attachment 74637 [details]
proposed patch

[Philippe Normand]

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.

[Philippe Normand]

Another, less intrusive and simpler, option would be to replace the Timers in the player with g_timeouts, like it was before.

[Philippe Normand]

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 :)

[Philippe Normand]

Created attachment 74731 [details]
simpler patch

[Philippe Normand]

(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.

[Philippe Normand]

Created attachment 75117 [details]
proposed patch

[Martin Robinson]

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"?

[Philippe Normand]

Created attachment 75259 [details]
updated patch

[Martin Robinson]

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.

[Philippe Normand]

Committed r73014: <http://trac.webkit.org/changeset/73014>

[WebKit Review Bot]

http://trac.webkit.org/changeset/73014 might have broken GTK Linux 32-bit Debug