Summary: | [GTK] set playbin mute property depending on volume value | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric.carlson, eric, gustavo, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Philippe Normand
2009-11-17 07:24:35 PST
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! :) |