Bug 31586 - [GTK] set playbin mute property depending on volume value
Summary: [GTK] set playbin mute property depending on volume value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-17 07:24 PST by Philippe Normand
Modified: 2010-02-01 08:23 PST (History)
6 users (show)

See Also:


Attachments
2009-11-25 Philippe Normand <pnormand@igalia.com> (6.20 KB, patch)
2009-11-25 09:21 PST, Philippe Normand
gustavo: review-
gustavo: commit-queue-
Details | Formatted Diff | Diff
2009-11-25 Philippe Normand <pnormand@igalia.com> (5.90 KB, patch)
2009-12-17 04:44 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
2009-11-25 Philippe Normand <pnormand@igalia.com> (5.86 KB, patch)
2009-12-17 09:27 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
new API in MediaPlayer for mute control and mute control in MediaPlayerPrivateGStreamer (14.23 KB, patch)
2009-12-20 04:39 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
new API in MediaPlayer for mute control and mute control in MediaPlayerPrivateGStreamer (22.94 KB, patch)
2009-12-20 06:35 PST, Philippe Normand
gustavo: review-
gustavo: commit-queue-
Details | Formatted Diff | Diff
new API in MediaPlayer for mute control (23.20 KB, patch)
2010-01-27 05:52 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
new API in MediaPlayer for mute control (23.05 KB, patch)
2010-01-27 06:11 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
new API in MediaPlayer for mute control (23.01 KB, patch)
2010-01-27 06:52 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
new API in MediaPlayer for mute control (23.34 KB, patch)
2010-01-27 07:43 PST, Philippe Normand
eric.carlson: review-
Details | Formatted Diff | Diff
new API in MediaPlayer for mute control (17.17 KB, patch)
2010-01-29 00:52 PST, Philippe Normand
eric.carlson: review-
Details | Formatted Diff | Diff
new API in MediaPlayer for mute control (17.21 KB, patch)
2010-02-01 01:03 PST, Philippe Normand
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2009-11-17 07:24:35 PST
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.
Comment 1 Philippe Normand 2009-11-25 09:21:08 PST
Created attachment 43852 [details]
2009-11-25  Philippe Normand  <pnormand@igalia.com>
Comment 2 Adam Barth 2009-11-30 12:40:47 PST
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
Comment 3 Holger Freyther 2009-12-12 22:16:37 PST
Comment on attachment 43852 [details]
2009-11-25  Philippe Normand  <pnormand@igalia.com>

Who would/should implement setMuted?
Comment 4 Philippe Normand 2009-12-14 00:26:42 PST
(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.
Comment 5 Gustavo Noronha (kov) 2009-12-17 01:23:31 PST
(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.
Comment 6 Gustavo Noronha (kov) 2009-12-17 01:25:24 PST
(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.
Comment 7 Gustavo Noronha (kov) 2009-12-17 01:30:41 PST
Comment on attachment 43852 [details]
2009-11-25  Philippe Normand  <pnormand@igalia.com>

> +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.
Comment 8 Philippe Normand 2009-12-17 04:44:43 PST
Created attachment 45053 [details]
2009-11-25  Philippe Normand  <pnormand@igalia.com>

renamed the mute_idle callback and removed the already applied coding
style fix.
Comment 9 WebKit Review Bot 2009-12-17 04:47:50 PST
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
Comment 10 Philippe Normand 2009-12-17 09:27:56 PST
Created attachment 45077 [details]
2009-11-25  Philippe Normand  <pnormand@igalia.com>

I removed the un-needed calls to the G_OBJECT() macro.
Comment 11 WebKit Review Bot 2009-12-17 09:32:24 PST
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
Comment 12 Philippe Normand 2009-12-19 09:19:46 PST
Comment on attachment 45077 [details]
2009-11-25  Philippe Normand  <pnormand@igalia.com>

This patch needs more work.
Comment 13 Philippe Normand 2009-12-20 04:39:25 PST
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
Comment 14 WebKit Review Bot 2009-12-20 04:41:21 PST
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
Comment 15 WebKit Review Bot 2009-12-20 04:44:33 PST
Attachment 45270 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/137052
Comment 16 Philippe Normand 2009-12-20 05:46:13 PST
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.
Comment 17 Philippe Normand 2009-12-20 06:35:51 PST
Created attachment 45274 [details]
new API in MediaPlayer for mute control and mute control in MediaPlayerPrivateGStreamer
Comment 18 WebKit Review Bot 2009-12-20 06:39:13 PST
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
Comment 19 Eric Seidel (no email) 2009-12-20 10:29:43 PST
Please file a bug about the style false positives if they are indeed false positives and you don't intend to fix them.
Comment 20 Philippe Normand 2009-12-20 10:32:12 PST
(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.
Comment 21 Gustavo Noronha (kov) 2009-12-29 11:29:10 PST
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  <pnormand@igalia.com>
 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.
Comment 22 Philippe Normand 2010-01-07 05:46:25 PST
(In reply to comment #21)
> (From update of attachment 45274 [details])
> WebCore/ChangeLog
> 
>  1 2009-12-20  Philippe Normand  <pnormand@igalia.com>
>  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? :)
Comment 23 Philippe Normand 2010-01-21 01:47:45 PST
Ping Gustavo? :)
Comment 24 Philippe Normand 2010-01-27 05:52:56 PST
Created attachment 47522 [details]
new API in MediaPlayer for mute control
Comment 25 Eric Seidel (no email) 2010-01-27 05:58:09 PST
Attachment 47522 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/214963
Comment 26 Philippe Normand 2010-01-27 06:11:14 PST
Created attachment 47524 [details]
new API in MediaPlayer for mute control
Comment 27 WebKit Review Bot 2010-01-27 06:19:45 PST
Attachment 47522 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/215072
Comment 28 WebKit Review Bot 2010-01-27 06:31:25 PST
Attachment 47524 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/215078
Comment 29 Philippe Normand 2010-01-27 06:52:42 PST
Created attachment 47527 [details]
new API in MediaPlayer for mute control
Comment 30 WebKit Review Bot 2010-01-27 07:34:35 PST
Attachment 47527 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/216010
Comment 31 Philippe Normand 2010-01-27 07:43:55 PST
Created attachment 47533 [details]
new API in MediaPlayer for mute control

should now build on chromium too, sorry for the noise.
Comment 32 Eric Carlson 2010-01-27 15:52:45 PST
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.
Comment 33 Philippe Normand 2010-01-29 00:52:20 PST
Created attachment 47688 [details]
new API in MediaPlayer for mute control

Removed the not-needed platform-specific changes.
Comment 34 Eric Carlson 2010-01-29 07:51:35 PST
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.
Comment 35 Philippe Normand 2010-02-01 01:03:12 PST
Created attachment 47819 [details]
new API in MediaPlayer for mute control
Comment 36 Philippe Normand 2010-02-01 01:06:47 PST
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.
Comment 37 Eric Carlson 2010-02-01 07:35:31 PST
Comment on attachment 47819 [details]
new API in MediaPlayer for mute control

Thanks!

r=me
Comment 38 Philippe Normand 2010-02-01 08:23:31 PST
Landed in r54136. Thanks for the review and patience! :)