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.
Created attachment 43852 [details] 2009-11-25 Philippe Normand <pnormand@igalia.com>
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 on attachment 43852 [details] 2009-11-25 Philippe Normand <pnormand@igalia.com> Who would/should implement setMuted?
(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.
(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.
(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 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.
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.
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
Created attachment 45077 [details] 2009-11-25 Philippe Normand <pnormand@igalia.com> I removed the un-needed calls to the G_OBJECT() macro.
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 on attachment 45077 [details] 2009-11-25 Philippe Normand <pnormand@igalia.com> This patch needs more work.
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
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
Attachment 45270 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/137052
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.
Created attachment 45274 [details] new API in MediaPlayer for mute control and mute control in MediaPlayerPrivateGStreamer
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
Please file a bug about the style false positives if they are indeed false positives and you don't intend to fix them.
(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 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.
(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? :)
Ping Gustavo? :)
Created attachment 47522 [details] new API in MediaPlayer for mute control
Attachment 47522 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/214963
Created attachment 47524 [details] new API in MediaPlayer for mute control
Attachment 47522 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/215072
Attachment 47524 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/215078
Created attachment 47527 [details] new API in MediaPlayer for mute control
Attachment 47527 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/216010
Created attachment 47533 [details] new API in MediaPlayer for mute control should now build on chromium too, sorry for the noise.
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.
Created attachment 47688 [details] new API in MediaPlayer for mute control Removed the not-needed platform-specific changes.
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.
Created attachment 47819 [details] new API in MediaPlayer for mute control
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 on attachment 47819 [details] new API in MediaPlayer for mute control Thanks! r=me
Landed in r54136. Thanks for the review and patience! :)