Bug 31586 – [GTK] set playbin mute property depending on volume value

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

If user mutes the volume, the HTMLMediaElement calls MediaPlayer::setVolume(0). OTOH if the user mutes the volume with PulseAudio, PA sets the playbin::mute property to true. So in some cases we can end up with volume being muted but reported as unmuted in the audio/video controls (and vice-versa).

One proposal is to set the mute property depending on the volume value in our PrivatePlayer, so we remain coherent with both PA and HTMLMediaElement behavior. We also need to correctly react on the notify::mute playbin2 signal.

[Philippe Normand]

Created attachment 43852 [details]
2009-11-25  Philippe Normand  <[e-mail]>

[Adam Barth]

Attachment 43852 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Done processing WebCore/platform/graphics/MediaPlayer.h
Done processing WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.h
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:421:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:699:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:904:  Use 0 instead of NULL.  [readability/null] [5]
Done processing WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp
Total errors found: 3

[Holger Freyther]

Comment on attachment 43852 [details]
2009-11-25  Philippe Normand  <[e-mail]>

Who would/should implement setMuted?

[Philippe Normand]

(In reply to comment #3)
> (From update of attachment 43852 [details])
> Who would/should implement setMuted?

I don't know. I added it in the Client interface because I needed to call it from the media player when we receive a notification that sound has been muted.

[Gustavo Noronha (kov)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 43852 [details] [details])
> > Who would/should implement setMuted?
> 
> I don't know. I added it in the Client interface because I needed to call it
> from the media player when we receive a notification that sound has been muted.

Why exactly do you need to call it? It doesn't seem to do anything at all.

[Gustavo Noronha (kov)]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 43852 [details] [details] [details])
> > > Who would/should implement setMuted?
> > 
> > I don't know. I added it in the Client interface because I needed to call it
> > from the media player when we receive a notification that sound has been muted.
> 
> Why exactly do you need to call it? It doesn't seem to do anything at all.

I see it now. That interface is implemented by HTMLMediaElement. So the response to Holger's question is HTMLMediaElement is the implementor of that interface.

[Gustavo Noronha (kov)]

Comment on attachment 43852 [details]
2009-11-25  Philippe Normand  <[e-mail]>

> +static gboolean notify_mute_idle_cb(gpointer data)
> +{
> +    MediaPlayerPrivate* mp = reinterpret_cast<MediaPlayerPrivate*>(data);
> +    mp->muteChanged();
> +    return FALSE;
> +}
> +
[...]
> +void MediaPlayerPrivate::muteChanged()
> +{
> +    // Some application (PulseAudio for instance) changed the mute
> +    // playbin property. Propagate the change to the player's client,
> +    // the HTMLMediaElement.
> +    gboolean mute;
> +    g_object_get(G_OBJECT(m_playBin), "mute", &mute, NULL);
> +    m_player->mediaPlayerClient()->setMuted(static_cast<bool>(mute));
> +}
> +

I don't think adding muteChanged to the MediaPlayerPrivate gains us anything. You can do all the work that you are doing in MediaPlayerPrivate::muteChanged in the callback function.

>      int width = 0, height = 0;
> -    GstCaps *caps = gst_buffer_get_caps(m_buffer);
> +    GstCaps* caps = gst_buffer_get_caps(m_buffer);
>      GstVideoFormat format;

Commit this separately, please! r=me on this change alone.

> -    g_object_set(G_OBJECT(m_playBin), "uri", url.utf8().data(),
> -        "volume", static_cast<double>(m_player->volume()), NULL);
> +    g_signal_connect(G_OBJECT(m_playBin), "notify::mute", G_CALLBACK(mediaPlayerPrivateMuteCallback), this);
> +
> +    g_object_set(G_OBJECT(m_playBin), "uri", url.utf8().data(), NULL);
> +
>  
>      m_videoSink = webkit_video_sink_new();

You're adding a blank line unnecessarily here.

[Philippe Normand]

Created attachment 45053 [details]
2009-11-25  Philippe Normand  <[e-mail]>

renamed the mute_idle callback and removed the already applied coding
style fix.

[WebKit Review Bot]

Attachment 45053 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:443:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:731:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:949:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3

[Philippe Normand]

Created attachment 45077 [details]
2009-11-25  Philippe Normand  <[e-mail]>

I removed the un-needed calls to the G_OBJECT() macro.

[WebKit Review Bot]

Attachment 45077 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:449:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:737:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:955:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3

[Philippe Normand]

Comment on attachment 45077 [details]
2009-11-25  Philippe Normand  <[e-mail]>

This patch needs more work.

[Philippe Normand]

Created attachment 45270 [details]
new API in MediaPlayer for mute control and mute control in MediaPlayerPrivateGStreamer

I also added support for it in the MediaPlayerPrivateGStreamer

[WebKit Review Bot]

Attachment 45270 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:454:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:460:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:752:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:758:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:986:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 5

[WebKit Review Bot]

Attachment 45270 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/137052

[Philippe Normand]

Comment on attachment 45270 [details]
new API in MediaPlayer for mute control and mute control in MediaPlayerPrivateGStreamer

Will upload new version, the MediaPlayerPrivate subclasses need to be updated.

[Philippe Normand]

Created attachment 45274 [details]
new API in MediaPlayer for mute control and mute control in MediaPlayerPrivateGStreamer

[WebKit Review Bot]

Attachment 45274 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:454:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:460:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:752:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:758:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:986:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 5

[Eric Seidel (no email)]

Please file a bug about the style false positives if they are indeed false positives and you don't intend to fix them.

[Philippe Normand]

(In reply to comment #19)
> Please file a bug about the style false positives if they are indeed false
> positives and you don't intend to fix them.

Maybe someone could review https://bugs.webkit.org/show_bug.cgi?id=32661 ?
and then I can update this patch if needed.

[Gustavo Noronha (kov)]

Comment on attachment 45274 [details]
new API in MediaPlayer for mute control and mute control in MediaPlayerPrivateGStreamer

WebCore/ChangeLog

 1 2009-12-20  Philippe Normand  <[e-mail]>
 2 
 3         Reviewed by NOBODY (OOPS!).
 4 
 5         [GTK] set playbin mute property depending on volume value
 6         https://bugs.webkit.org/show_bug.cgi?id=31586
 7 
 8         Adapted the other MediaPlayer implementations.,

You should write only 1 ChangeLog entry for the full change.

@@ void HTMLMediaElement::mediaPlayerTimeChanged(MediaPlayer*)
14101415 void HTMLMediaElement::mediaPlayerVolumeChanged(MediaPlayer*)
14111416 {
14121417     beginProcessingMediaPlayerCallback();
 1418     if (m_player)
 1419         m_volume = m_player->volume();
14131420     updateVolume();
14141421     endProcessingMediaPlayerCallback();

This looks wrong. Why are you changing this behavior?

 1424 void HTMLMediaElement::mediaPlayerMuteChanged(MediaPlayer*)
 1425 {
 1426     beginProcessingMediaPlayerCallback();
 1427     if (m_player)
 1428         m_muted = m_player->muted();
 1429     if (renderer())
 1430         renderer()->updateFromElement();
 1431     endProcessingMediaPlayerCallback();
 1432 }

I don't think you should duplicate this code here. Make a setMuted call, and make it behave like updateVolume does (checking if it is handling a player notification, and treating it specially).

 79     virtual bool supportsMuting() { return false; }
 80     virtual void setMuted(bool) = 0;

supportsMuting should be have the const qualifier, like hasClosedCaptions.

 457 void MediaPlayerPrivate::volumeChangedCallback()
 458 {
 459     double volume;
 460     g_object_get(m_playBin, "volume", &volume, NULL);
 461     m_player->volumeChanged(static_cast<float>(volume));

I think you are mixing unrelated changes in this patch. And this looks a bit weird. Shouldn't you just set the volume?

I believe it would be a good idea to grab Eric Carlson and ask him about the layering, and where you want to insert this.

[Philippe Normand]

(In reply to comment #21)
> (From update of attachment 45274 [details])
> WebCore/ChangeLog
> 
>  1 2009-12-20  Philippe Normand  <[e-mail]>
>  2 
>  3         Reviewed by NOBODY (OOPS!).
>  4 
>  5         [GTK] set playbin mute property depending on volume value
>  6         https://bugs.webkit.org/show_bug.cgi?id=31586
>  7 
>  8         Adapted the other MediaPlayer implementations.,
> 
> You should write only 1 ChangeLog entry for the full change.
> 

Ok sorry, I did 3 commits for that in my branch, that's why there were 3 entries, I will merge them then.

> @@ void HTMLMediaElement::mediaPlayerTimeChanged(MediaPlayer*)
> 14101415 void HTMLMediaElement::mediaPlayerVolumeChanged(MediaPlayer*)
> 14111416 {
> 14121417     beginProcessingMediaPlayerCallback();
>  1418     if (m_player)
>  1419         m_volume = m_player->volume();
> 14131420     updateVolume();
> 14141421     endProcessingMediaPlayerCallback();
> 
> This looks wrong. Why are you changing this behavior?
> 

MediaPlayer received a volume notification from its "private player" instance so m_volume needs to be updated to reflect the change downstream. Maybe I can move that to updateVolume(), I don't know... 

>  1424 void HTMLMediaElement::mediaPlayerMuteChanged(MediaPlayer*)
>  1425 {
>  1426     beginProcessingMediaPlayerCallback();
>  1427     if (m_player)
>  1428         m_muted = m_player->muted();
>  1429     if (renderer())
>  1430         renderer()->updateFromElement();
>  1431     endProcessingMediaPlayerCallback();
>  1432 }
> 
> I don't think you should duplicate this code here. Make a setMuted call, and
> make it behave like updateVolume does (checking if it is handling a player
> notification, and treating it specially).
> 

ok :)

>  79     virtual bool supportsMuting() { return false; }
>  80     virtual void setMuted(bool) = 0;
> 
> supportsMuting should be have the const qualifier, like hasClosedCaptions.
> 

ok 

>  457 void MediaPlayerPrivate::volumeChangedCallback()
>  458 {
>  459     double volume;
>  460     g_object_get(m_playBin, "volume", &volume, NULL);
>  461     m_player->volumeChanged(static_cast<float>(volume));
> 
> I think you are mixing unrelated changes in this patch. And this looks a bit
> weird. Shouldn't you just set the volume?
> 

I need to get the new volume value because we received a notify::volume signal.

> I believe it would be a good idea to grab Eric Carlson and ask him about the
> layering, and where you want to insert this.

Once we found an agreement on all the issues above I can send a new patch and we can discuss with Eric. Ok? :)

[Philippe Normand]

Ping Gustavo? :)

[Philippe Normand]

Created attachment 47522 [details]
new API in MediaPlayer for mute control

[Eric Seidel (no email)]

Attachment 47522 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/214963

[Philippe Normand]

Created attachment 47524 [details]
new API in MediaPlayer for mute control

[WebKit Review Bot]

Attachment 47522 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/215072

[WebKit Review Bot]

Attachment 47524 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/215078

[Philippe Normand]

Created attachment 47527 [details]
new API in MediaPlayer for mute control

[WebKit Review Bot]

Attachment 47527 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/216010

[Philippe Normand]

Created attachment 47533 [details]
new API in MediaPlayer for mute control

should now build on chromium too, sorry for the noise.

[Eric Carlson]

Comment on attachment 47533 [details]
new API in MediaPlayer for mute control

> +    virtual bool supportsMuting() const { return false; }
> +    virtual void setMuted(bool) { }
> 
These methods are unnecessary because MediaPlayerPrivateInterface has default implementations. I know that many other methods in NullMediaPlayerPrivate use this pattern, but we should change the pattern (and come back and clean this up).

> diff --git a/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.h 
> diff --git a/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm 
> diff --git a/WebCore/platform/graphics/qt/MediaPlayerPrivatePhonon.cpp 
> diff --git a/WebCore/platform/graphics/qt/MediaPlayerPrivatePhonon.h 
> diff --git a/WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp 
> diff --git a/WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.h 
> diff --git a/WebCore/platform/graphics/wince/MediaPlayerPrivateWince.h 
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> diff --git a/WebKit/chromium/public/WebMediaPlayerClient.h 
> diff --git a/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp 
> diff --git a/WebKit/chromium/src/WebMediaPlayerClientImpl.h 
None of these is necessary as each media engine subclasses MediaPlayerPrivateInterface, which has default implementation for both methods. I would prefer that we leave them out for now and add them when a port adds the functionality.

[Philippe Normand]

Created attachment 47688 [details]
new API in MediaPlayer for mute control

Removed the not-needed platform-specific changes.

[Eric Carlson]

Comment on attachment 47688 [details]
new API in MediaPlayer for mute control

> +++ b/WebCore/html/HTMLMediaElement.cpp
> @@ -1235,8 +1235,16 @@ void HTMLMediaElement::setMuted(bool muted)
>  {
>      if (m_muted != muted) {
>          m_muted = muted;
> -        updateVolume();
> -        scheduleEvent(eventNames().volumechangeEvent);
> +        // Avoid recursion when the player reports volume changes.
> +        if (!processingMediaPlayerCallback()) {
> +            if (m_player && m_player->supportsMuting()) {
> +                m_player->setMuted(m_muted);
> +                if (renderer())
> +                    renderer()->updateFromElement();
> +            } else
> +                updateVolume();
> +            scheduleEvent(eventNames().volumechangeEvent);
> +        }
>      }
>  }
>  
If a media engine's muted property changes without HTMLMediaElement knowing about it, we won't post a 'volumechanged' notification because processingMediaPlayerCallback() will be true. 


> @@ -275,6 +291,7 @@ MediaPlayerPrivate::MediaPlayerPrivate(MediaPlayer* player)
>      , m_errorOccured(false)
>      , m_volumeIdleId(-1)
>      , m_mediaDuration(0.0)
> +    , m_muteIdleId(-1)
>  {
>      doGstInit();
>  }
> @@ -286,6 +303,11 @@ MediaPlayerPrivate::~MediaPlayerPrivate()
>          m_volumeIdleId = -1;
>      }
>  
> +    if (m_muteIdleId) {
> +        g_source_remove(m_muteIdleId);
> +        m_muteIdleId = -1;
> +    }
> +
Why are m_muteIdleId and m_volumeIdleId set to -1 when the source is invalid, but "!=0" is the test for validity? Shouldn't you set them to 0 when invalid?

Sorry I missed both of these the last time around!

r- for the missing notification.

[Philippe Normand]

Created attachment 47819 [details]
new API in MediaPlayer for mute control

[Philippe Normand]

BTW this patch can be manually tested on linux (or any PulseAudio enabled OS) like this:

- open the volume control applet, go in the "Applications" tab
- open GtkLauncher http://people.mozilla.com/~prouget/demos/simpleVideo/video.html
- start playback

- if you mute GtkLauncher volume from the applet you should be able to unmute it from GtkLauncher
- if you mute GtkLauncher volume you should see the "muted" checkbox ticked in the volume applet.

[Eric Carlson]

Comment on attachment 47819 [details]
new API in MediaPlayer for mute control

Thanks!

r=me

[Philippe Normand]

Landed in r54136. Thanks for the review and patience! :)