Mute audio when volume is zero on Android.

Mute audio when volume is zero on Android.

We are using Communication mode for audio backend on Android. By design, the platform voice
volume never goes to zero. This patch uses ContentObserver to listen to volume change. When
volume is set to zero, AudioManagerAndroid will mute all its output streams.

BUG=263399
R=henrika@chromium.org, qinmin@chromium.org, tommi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239007

[henrika (OOO until Aug 14), 2013-11-28 08:45:37]

Would appreciate if I jmtrivi@ could do the primary review (go first) here since
I don't know the details related to ContentObserver, or the threading model used
in this CL.

The OpenSL parts look OK. It is not clear to me how all this will work in
practice. Will the user still not be able to slide down the volume to 0 but the
audio will be totally muted anyhow?

[tommi (sloooow) - chröme, 2013-12-02 18:35:57]

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:86: AudioOutputStream*
AudioManagerAndroid::MakeAudioOutputStream(
Can we DCHECK that all methods that touch streams_ are running on the audio
thread?

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:97:
streams_.insert(reinterpret_cast<OpenSLESOutputStream*>(stream));
static_cast

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:114:
streams_.erase(reinterpret_cast<OpenSLESOutputStream*>(stream));
static_cast

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:204: for (OutputStreams::iterator
it=streams_.begin();
spaces around =

https://codereview.chromium.org/93233003/diff/1/media/audio/android/opensles_...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/android/opensles_...
media/audio/android/opensles_output.cc:176: void
OpenSLESOutputStream::SetMute(bool state) {
s/state/muted

https://codereview.chromium.org/93233003/diff/1/media/audio/android/opensles_...
File media/audio/android/opensles_output.h (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/android/opensles_...
media/audio/android/opensles_output.h:43: void SetMute(bool state);
nit: s/state/muted

https://codereview.chromium.org/93233003/diff/1/media/audio/audio_manager_base.h
File media/audio/audio_manager_base.h (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.h:161: scoped_refptr<base::MessageLoopProxy>
message_loop_;
I don't think we need this change so it would be great to revert (no other
platform needs it). Instead use GetMessageLoop.

https://codereview.chromium.org/93233003/diff/1/media/base/android/java/src/o...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/93233003/diff/1/media/base/android/java/src/o...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:106:
mSettinsObserverThread = new SettinsObserverThread();
s/mSettinsObserverThread/mSettingsObserverThread
s/SettinsObserverThread/SettingsObserverThread

[tommi (sloooow) - chröme, 2013-12-02 18:36:12]

oh, and great work Wei! :)

[wjia(left Chromium), 2013-12-02 21:49:49]

Thanks! PTAL.

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:86: AudioOutputStream*
AudioManagerAndroid::MakeAudioOutputStream(
On 2013/12/02 18:36:00, tommi wrote:
> Can we DCHECK that all methods that touch streams_ are running on the audio
> thread?

Not now. Please refer to comments in AudioManagerBase::MakeAudioOutputStream
(https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...).

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:97:
streams_.insert(reinterpret_cast<OpenSLESOutputStream*>(stream));
On 2013/12/02 18:36:00, tommi wrote:
> static_cast

Done.

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:114:
streams_.erase(reinterpret_cast<OpenSLESOutputStream*>(stream));
On 2013/12/02 18:36:00, tommi wrote:
> static_cast

Done.

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:204: for (OutputStreams::iterator
it=streams_.begin();
On 2013/12/02 18:36:00, tommi wrote:
> spaces around =

Done.

https://codereview.chromium.org/93233003/diff/1/media/audio/android/opensles_...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/android/opensles_...
media/audio/android/opensles_output.cc:176: void
OpenSLESOutputStream::SetMute(bool state) {
On 2013/12/02 18:36:00, tommi wrote:
> s/state/muted

Done.

https://codereview.chromium.org/93233003/diff/1/media/audio/android/opensles_...
File media/audio/android/opensles_output.h (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/android/opensles_...
media/audio/android/opensles_output.h:43: void SetMute(bool state);
On 2013/12/02 18:36:00, tommi wrote:
> nit: s/state/muted

Done.

https://codereview.chromium.org/93233003/diff/1/media/audio/audio_manager_base.h
File media/audio/audio_manager_base.h (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.h:161: scoped_refptr<base::MessageLoopProxy>
message_loop_;
On 2013/12/02 18:36:00, tommi wrote:
> I don't think we need this change so it would be great to revert (no other
> platform needs it). Instead use GetMessageLoop.

Done.

https://codereview.chromium.org/93233003/diff/1/media/base/android/java/src/o...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/93233003/diff/1/media/base/android/java/src/o...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:106:
mSettinsObserverThread = new SettinsObserverThread();
On 2013/12/02 18:36:00, tommi wrote:
> s/mSettinsObserverThread/mSettingsObserverThread
> s/SettinsObserverThread/SettingsObserverThread

Done.

[jmtrivi, 2013-12-02 23:57:58]

https://codereview.chromium.org/93233003/diff/30001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/93233003/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:596:
mContentResolver.registerContentObserver(Settings.System.CONTENT_URI, true,
this);
Be aware that this will be called for many more things than volume changes. I
know there isn't any public mechanism to monitor volume changes.

https://codereview.chromium.org/93233003/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:600:
public void onChange(boolean selfChange) {
This will only work for volume changes when the audio configuration remains the
same: as volume is device-dependent, when you switch the audio device (e.g. user
unplugs headset), your settings observer will not be called.

https://codereview.chromium.org/93233003/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:603:
nativeSetMute(mNativeAudioManagerAndroid, (volume == 0));
You should post a message to do this asynchronously to better handle the case of
multiple volume changes (as is the case when the user bring the volume down
quickly to 0). You could also maintain state so you only update the native layer
on a state change.

[tommi (sloooow) - chröme, 2013-12-03 08:56:23]

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:86: AudioOutputStream*
AudioManagerAndroid::MakeAudioOutputStream(
Ju On 2013/12/02 21:49:49, wjia wrote:
> On 2013/12/02 18:36:00, tommi wrote:
> > Can we DCHECK that all methods that touch streams_ are running on the audio
> > thread?
> 
> Not now. Please refer to comments in AudioManagerBase::MakeAudioOutputStream
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...).

Ah. OK in that case you need to post a task to the audio thread for inserting
into streams_ or use a lock to protect it.

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:114:
streams_.erase(reinterpret_cast<OpenSLESOutputStream*>(stream));
On 2013/12/02 21:49:49, wjia wrote:
> On 2013/12/02 18:36:00, tommi wrote:
> > static_cast
> 
> Done.

Looks like there's a race here too.

[henrika (OOO until Aug 14), 2013-12-03 11:02:28]

LGTM on cc-parts assuming latest input from tommi@ is fixed. Don't know the Java
parts well enough to comment on the details there.

https://codereview.chromium.org/93233003/diff/30001/media/audio/android/opens...
File media/audio/android/opensles_output.h (right):

https://codereview.chromium.org/93233003/diff/30001/media/audio/android/opens...
media/audio/android/opensles_output.h:43: void SetMute(bool muted);
Perhaps worth mentioning that muted=true does not lead to GetVolume()=0.

[wjia(left Chromium), 2013-12-03 23:08:05]

Thanks, Jean-Michel! Could you shed some light on why
AudioManager.getStreamVolume() could return 3 different results when
called inside BroadcastReceiver::onReceive()?

https://codereview.chromium.org/93233003/diff/30001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/93233003/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:596:
mContentResolver.registerContentObserver(Settings.System.CONTENT_URI, true,
this);
On 2013/12/02 23:57:59, jmtrivi wrote:
> Be aware that this will be called for many more things than volume changes. I
> know there isn't any public mechanism to monitor volume changes.

Thanks for the reminder! Yes, I am aware of this. Once a public mechanism is
available (if it will be), we will switch to listening to volume specific
changes.

https://codereview.chromium.org/93233003/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:600:
public void onChange(boolean selfChange) {
On 2013/12/02 23:57:59, jmtrivi wrote:
> This will only work for volume changes when the audio configuration remains
the
> same: as volume is device-dependent, when you switch the audio device (e.g.
user
> unplugs headset), your settings observer will not be called.

Right, switching audio device is a problem (I have mentioned it in an email
thread). I have tried to call mAudioManager.getStreamVolume() when device is
changed in BroadcastReceiver::onReceive() defined above (line 415). But somehow
the AudioManager doesn't give me inconsistent result. For example,
1. put earpiece volume at 0;
2. plug headset, query volume of STREAM_VOICE_CALL in onReceive(), it's 0;
3. change headset volume to 5;
4. unplug headset, query volume of STREAM_VOICE_CALL in onReceive(), it's 4 (I
expect it's 5 at least, but it's 4).

If I modify the step #3 to change the volume to 3, unplug headset, now
AudioManager gives me volume of 3 or 0.

Basically, AudioManager.getStreamVolume() could return 3 different results when
called inside BroadcastReceiver::onReceive():
a. the previous volume of the new device;
b. the volume of previous device;
c. 4 (which I have no clue why it's 4);

I am not sure if that's a bug, or I use it in a wrong way (e.g., on when/how to
query AudioManager for stream volume, etc). I hope you can shed some light on
it.

https://codereview.chromium.org/93233003/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:603:
nativeSetMute(mNativeAudioManagerAndroid, (volume == 0));
On 2013/12/02 23:57:59, jmtrivi wrote:
> You should post a message to do this asynchronously to better handle the case
of
> multiple volume changes (as is the case when the user bring the volume down
> quickly to 0). You could also maintain state so you only update the native
layer
> on a state change.

The message posting happens in c++ code, i.e., AudioManagerAndroid::SetMute(). 

I have tried maintaining a state here to avoid unnecessary function call to c++
code. But I gave it up because it messed up the case when device is changed.

[wjia(left Chromium), 2013-12-04 00:53:29]

PTAL. Thanks!

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:86: AudioOutputStream*
AudioManagerAndroid::MakeAudioOutputStream(
On 2013/12/03 08:56:24, tommi wrote:
> Ju On 2013/12/02 21:49:49, wjia wrote:
> > On 2013/12/02 18:36:00, tommi wrote:
> > > Can we DCHECK that all methods that touch streams_ are running on the
audio
> > > thread?
> > 
> > Not now. Please refer to comments in AudioManagerBase::MakeAudioOutputStream
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...).
> 
> Ah. OK in that case you need to post a task to the audio thread for inserting
> into streams_ or use a lock to protect it. 

Done.

https://codereview.chromium.org/93233003/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:114:
streams_.erase(reinterpret_cast<OpenSLESOutputStream*>(stream));
On 2013/12/03 08:56:24, tommi wrote:
> On 2013/12/02 21:49:49, wjia wrote:
> > On 2013/12/02 18:36:00, tommi wrote:
> > > static_cast
> > 
> > Done.
> 
> Looks like there's a race here too. 

Done.

https://codereview.chromium.org/93233003/diff/30001/media/audio/android/opens...
File media/audio/android/opensles_output.h (right):

https://codereview.chromium.org/93233003/diff/30001/media/audio/android/opens...
media/audio/android/opensles_output.h:43: void SetMute(bool muted);
On 2013/12/03 11:02:28, henrika wrote:
> Perhaps worth mentioning that muted=true does not lead to GetVolume()=0.

Done.

[tommi (sloooow) - chröme, 2013-12-04 08:55:31]

Thanks Wei.  Lgtm.

[qinmin, 2013-12-05 17:28:32]

lgtm

[wjia(left Chromium), 2013-12-05 19:10:30]

Committed patchset #6 manually as r239007 (presubmit successful).