Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
110173003:
Refactor audio manager for Android to avoid heavy tasks at startup (Closed)
|
Created:
7 years ago by henrika (OOO until Aug 14) Modified:
7 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor audio manager for Android to avoid heavy tasks at startup.
Main goal of this CL is to avoid all demanding tasks (e.g. initiate audio routing, detect BT devices etc.) at Chrome startup.
All we do now is to populate the list of available devices. BT support has been removed but will be added in an upcoming CL.
BUG=324464
TEST=media_unittests --gtest-filter=AudioAndroid*
R=bulach@chromium.org, tommi@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240883
Patch Set 1 #Patch Set 2 : First round #Patch Set 3 : Removed BT usage and improved state handling #
Total comments: 83
Patch Set 4 : First review round #Patch Set 5 : Simplified by removing use of updateState. It was redundant #Patch Set 6 : Improved thread safety #
Total comments: 10
Patch Set 7 : improved the state machine #
Total comments: 9
Patch Set 8 : tommi@ #
Total comments: 4
Patch Set 9 : installed checkstyle #Patch Set 10 : nits #
Total comments: 10
Patch Set 11 : Now uses HandlerThread #
Total comments: 5
Patch Set 12 : removed WithLock #Patch Set 13 : nit #Patch Set 14 : merged #
Total comments: 22
Patch Set 15 : removed mActive state #
Total comments: 18
Patch Set 16 : tommi@ again #Patch Set 17 : bulach@ #Patch Set 18 : has->had #
Total comments: 1
Patch Set 19 : nit #Patch Set 20 : Fixed reported findbugs issues #Patch Set 21 : rebased #
Messages
Total messages: 48 (0 generated)
What's new? BT support removed for now. Will be restored in separate CL. Starting Chrome => device list is populated but no audio routing is done. Hence, we can call getSources and list all devices. Remove devices, add devices => list is updated. Creating first input stream using default device => audio mode is set to communication mode and default device is selected: wired headset first, then speaker phone or earpiece. Creating first input stream using non-default device => audio mode is set to communication mode and specified device is selected if available. Stream is NULL otherwise. Output streams sets the communication mode as well but default device is always used. No option to specify. We restore the default audio mode when the last input or output stream is destroyed. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:43: private static final boolean DEBUG = true; will be restored to false https://codereview.chromium.org/110173003/diff/40001/testing/android/java/src... File testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java (right): https://codereview.chromium.org/110173003/diff/40001/testing/android/java/src... testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:38: if (true || extras != null && extras.containsKey(EXTRA_RUN_IN_SUB_THREAD)) { Will be removed. Only used for test to ensure that I can trigger broadcast events even in the media unittest.
tommi@: care to take a first round of sanity checks? Not done with thread safety but main parts should work fine now.
Great! Lots of comments but looks good. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:88: // default record followed by non-default device names. nit; took me a couple of reads to understand that "record" means "entry" and not "record" as it "record audio". Maybe change that? :) https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:91: if (!device_names.empty()) { nit: to reduce scopes: if (device_names.empty()) { LOG(WARNING) << ... return; } <rest> https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:101: // default name or id. good thing to test https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:563: AudioManagerAndroid* manager = Do you need AudioManagerAndroid* or is AudioManager* enough? https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:574: AudioManagerAndroid* manager = AudioManager* enough? These tests seem like valid cross-platform tests btw. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:130: if (stream && IsFirstCreatedAudioStream()) { you don't really need both IsFirst (or HasOne) and IsLast methods (or HasNo). Instead, you can just implement HasNoAudioStreams() and do: bool has_no_streams = HasNoAudioStreams(); AudioOutputStream* stream = AudioManagerBase::MakeAudioOutputStream(...); if (stream && has_no_streams) { // We just created the first stream. SetCommunicationAudioModeOn(true); } https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:131: SetCommunicationAudioModeOn(true); Here, we should store what the previous mode is. SetCommunicationAudioModeOn(false) should restore the mode to what it previously was. Can you file a bug for that? Also, it would be good to have a comment here that explains that the current implementation of the AudioManager on Android is for voice only. As is, I'm assuming wide band audio quality would suffer if Android used the AudioManager for rendering audio in general (which is what I expect will happen down the line), so this will need to change. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:260: return (out_count == 1 && in_count == 0) || (out_count == 0 && in_count == 1); return (output_stream_count() + input_stream_count()) == 1; (but I don't think you need this method actually. see above) https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:263: bool AudioManagerAndroid::IsLastDestroyedAudioStream() { HasNoAudioStreams? I don't see a stream being checked so 'last destroyed' doesn't have much meaning. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:264: return (output_stream_count() == 0 && input_stream_count() == 0); nit: remove superflous parenthesis https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:304: j_audio_manager_.obj(), on); see comment on this above. since this doesn't remember from what mode we changed the mode, this looks like a bug to me. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.h:70: bool IsFirstCreatedAudioStream(); HasOneAudioStream? (IsFirstXxx sounds like it would need a stream argument to check if that stream is the first one) https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.h:71: bool IsLastDestroyedAudioStream(); It's not clear how this is different from the function above, so would be good to have some documentation. Also, do you need to clarify whether it's an input or output stream? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:101: static final int STATE_BLUETOOTH_TURNING_ON = 5; unused? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:104: static final int STATE_BLUETOOTH_TURNING_OFF = 6; unused? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:169: synchronized (mLock) { is synchronization necessary in this method? Won't it always be called from the same thread, exactly once and before any other methods? I see that access to 'mIsInitialized' is not protected, so this feels futile. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:187: mIsInitialized = true; nit: set last? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:193: mSettingsObserverLock.wait(); I'm not sure we need this locking+waiting actually. See comment below about moving mSettingsObserver into the mSettingsObserverThread class. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:195: loge("unregisterHeadsetReceiver exception: " + e.getMessage()); nit: log message seems misleading https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:211: mSettingsObserverThread = null; does this gracefully end the thread? (does it end the thread at all?) https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:231: if (mSavedAudioMode == AudioManager.MODE_INVALID) { What about making it a programmer error if mSavedAudioMode isn't AudioManager.MODE_INVALID? Then you can skip this check and assert on the value being correct instead. Thanks for fixing this bug btw! Maybe you could file a proper issue just for tracking and tag this cl with that bug? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:237: loge("getMode exception: " + e.getMessage()); does this happen in practice? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:253: if (mSavedAudioMode != AudioManager.MODE_INVALID) { assert that mSavedAudioMode != AudioManager.MODE_INVALID? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:266: } set mSavedAudioMode to AudioManager.MODE_INVALID after restoring the previous mode. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:489: // TODO(henrika): add lock? on which thread are we here? If we're on a different thread than where we test mAudioDevices, won't we need synchronization for that too? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:559: // Deafault-device mode => shift to new default device. Default https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:564: // TODO(henrika): is there anything else we can do here? Could we keep track of what device was previously selected? In the case of bluetooth and A2DP, we might have to store more information if we are not really changing devices but rather switching profiles. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:574: private static boolean isNumeric(String str) { is this necessary? Would Integer.parseInt() give you what you need? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:575: for (char c : str.toCharArray()) for () { } https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:577: if (!Character.isDigit(c)) return false; return on a separate line https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:583: return (mAudioDeviceState == STATE_SPEAKERPHONE_ON && crazy idea: It seems that all the STATE_FOO_ON constants are duplicates of the DEVICE_FOO constants. Can we simply use one of them and set mAudioDeviceState to e.g. DEVICE_SPEAKERPHONE when the speakerphone is being used? That makes mAudioDevices and mAudioDeviceState compatible and this function can be rewritten as: private boolean selectedDeviceIsInList() { return mAudioDeviceState != DEVICE_INVALID && mAudioDevices[mAudioDeviceState]; } If you then rename the mAudioDeviceState variable to e.g. mSelectedAudioDevice, it's also more inline with selectedDeviceIsInList and to go out on a limb, arguably more correct since it currently doesn't really represent the state of the audio device but rather which one is selected. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:647: super("SettinsObserver"); fix all "Settins" :) https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:657: mSettingsObserverLock.notify(); what about making mSettingsObserver just a member variable of SettingsObserverThread and skip the lock?
https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:224: * TODO(henrika): add comments... Before landing this patch? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:551: /** TODO(henrika): add comments... */ Before landing this patch? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:574: private static boolean isNumeric(String str) { On 2013/12/10 21:19:11, tommi wrote: > is this necessary? Would Integer.parseInt() give you what you need? That would throw an exception if it's not an integer, and throwing exceptions is expensive, reserved for exceptional cases. If it's only in exceptional cases that the string is not an Integer, then it's fine to do that. If it's a common case then this seems better.
https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:574: private static boolean isNumeric(String str) { On 2013/12/11 10:52:04, Jói wrote: > On 2013/12/10 21:19:11, tommi wrote: > > is this necessary? Would Integer.parseInt() give you what you need? > > That would throw an exception if it's not an integer, and throwing exceptions is > expensive, reserved for exceptional cases. If it's only in exceptional cases > that the string is not an Integer, then it's fine to do that. If it's a common > case then this seems better. Yes, I think it would be an exceptional case and should basically never happen (i.e. I'm not sure we should even attempt to catch that error and let it just CHECK() in the C++ code.
Thanks. New round up for review. Passed on some questions since it was not "my code". Plan to improve thread safety next. PTAL https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:88: // default record followed by non-default device names. fixed. I was thinking in Pascal ;-) https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:91: if (!device_names.empty()) { On 2013/12/10 21:19:11, tommi wrote: > nit: to reduce scopes: > > if (device_names.empty()) { > LOG(WARNING) << ... > return; > } > > <rest> Done. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:101: // default name or id. Thx. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:563: AudioManagerAndroid* manager = My bad. Fixed. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:574: AudioManagerAndroid* manager = On 2013/12/10 21:19:11, tommi wrote: > AudioManager* enough? These tests seem like valid cross-platform tests btw. Done. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:130: if (stream && IsFirstCreatedAudioStream()) { On 2013/12/10 21:19:11, tommi wrote: > you don't really need both IsFirst (or HasOne) and IsLast methods (or HasNo). > Instead, you can just implement HasNoAudioStreams() and do: > > bool has_no_streams = HasNoAudioStreams(); > AudioOutputStream* stream = AudioManagerBase::MakeAudioOutputStream(...); > if (stream && has_no_streams) { > // We just created the first stream. > SetCommunicationAudioModeOn(true); > } Done. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:131: SetCommunicationAudioModeOn(true); I do save the state and restore the state in Java. OK? I have not noticed any degradation in BW due this mode. Note that we use MODE_IN_COMMUNICATION: "In communication audio mode. An audio/video chat or VoIP call is established." and *not* MODE_IN_CALL: the voice/in-call mode "A telephony call is established.". Hence, name of mode does not imply quality reduction IMHO. Still want comment? https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:260: return (out_count == 1 && in_count == 0) || (out_count == 0 && in_count == 1); removed https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:263: bool AudioManagerAndroid::IsLastDestroyedAudioStream() { On 2013/12/10 21:19:11, tommi wrote: > HasNoAudioStreams? I don't see a stream being checked so 'last destroyed' > doesn't have much meaning. Done. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:264: return (output_stream_count() == 0 && input_stream_count() == 0); On 2013/12/10 21:19:11, tommi wrote: > nit: remove superflous parenthesis Done. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:304: j_audio_manager_.obj(), on); So you want me to move the cache to this class instead? As mentioned above, I do that in Java today. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.h:70: bool IsFirstCreatedAudioStream(); Removed. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.h:71: bool IsLastDestroyedAudioStream(); Renamed. Regarding input & output. The old solution (before I did any work) only set the audio mode to comm mode when out stream was created, hence no mode shift if only capture was active. It feels wrong so my idea was to maintain the mode as long as any stream needs it. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:101: static final int STATE_BLUETOOTH_TURNING_ON = 5; Yes. Will remove. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:104: static final int STATE_BLUETOOTH_TURNING_OFF = 6; On 2013/12/10 21:19:11, tommi wrote: > unused? Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:169: synchronized (mLock) { Agree. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:187: mIsInitialized = true; On 2013/12/10 21:19:11, tommi wrote: > nit: set last? Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:193: mSettingsObserverLock.wait(); Wei wrote these things with assistance from reviewer from the Android team. I woul like to pass on these comments if that is OK for now. Or should I add Wei to the review list? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:195: loge("unregisterHeadsetReceiver exception: " + e.getMessage()); Changed. Code from Wei. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:211: mSettingsObserverThread = null; Refer to Wei. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:237: loge("getMode exception: " + e.getMessage()); It was added in the original version and I simply maintained it. Don't know the full history and therefore kept it. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:253: if (mSavedAudioMode != AudioManager.MODE_INVALID) { On 2013/12/10 21:19:11, tommi wrote: > assert that mSavedAudioMode != AudioManager.MODE_INVALID? Done. Assuming my "assert" is OK. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:266: } On 2013/12/10 21:19:11, tommi wrote: > set mSavedAudioMode to AudioManager.MODE_INVALID after restoring the previous > mode. Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:489: // TODO(henrika): add lock? Will take a second round and consider these issues. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:551: /** TODO(henrika): add comments... */ On 2013/12/11 10:52:04, Jói wrote: > Before landing this patch? Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:559: // Deafault-device mode => shift to new default device. On 2013/12/10 21:19:11, tommi wrote: > Default Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:564: // TODO(henrika): is there anything else we can do here? I think I simply have to develop this scheme as we start using the code. Am unable to come up with a perfect solution right now. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:574: private static boolean isNumeric(String str) { I tried to avoid exception. Will not happen often. Do you want me to switch to exception? https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:575: for (char c : str.toCharArray()) On 2013/12/10 21:19:11, tommi wrote: > for () { > } Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:577: if (!Character.isDigit(c)) return false; On 2013/12/10 21:19:11, tommi wrote: > return on a separate line Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:583: return (mAudioDeviceState == STATE_SPEAKERPHONE_ON && The idea crossed my mind as well but I had also added STATE_BLUETOOTH_TURNING_ON and STATE_BLUETOOTH_TURNING_OFF at that stage. I think we will need these as well in the upcoming BT CL. Perhaps I could use them separately and merge as you say. Ahh, it sounds so good when you describe it. Will change. Thanks! https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:647: super("SettinsObserver"); On 2013/12/10 21:19:11, tommi wrote: > fix all "Settins" :) Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:657: mSettingsObserverLock.notify(); Code from Wei in separate CL. OK to pass on it for now?
One more detail. The current version does not select a device until SetAudioDevice is called and it is only called when an input stream is selected. Before, we did an automatic device selection already at startup. If I added a call to set the default device when out stream is created, it could overwrite previous setting from input side. Will try to come up with a solution.
I have no further comments at the moment, but to be honest I didn't wrap my head around the threading model in the Java code since I think Tommi had. If you need me to dig into that, then please give an overview of what it's intended to be and I can take another look. Cheers, Jói On Wed, Dec 11, 2013 at 1:25 PM, <henrika@chromium.org> wrote: > One more detail. > > The current version does not select a device until SetAudioDevice is called > and > it is only called when an input stream is selected. Before, we did an > automatic > device selection already at startup. If I added a call to set the default > device > when out stream is created, it could overwrite previous setting from input > side. > Will try to come up with a solution. > > https://codereview.chromium.org/110173003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks Jói, uploaded a simplified version (still not thread safe) in the mean time.
Added locks. Nothing done for the "additional thread comments" added by Tommi.
comments and suggestions specifically for the SettingsObserver set of variables. https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:112: private SettingsObserver mSettingsObserver = null; suggest we remove this variable. (see more below). https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:115: private final Object mSettingsObserverLock = new Object(); suggest we remove this variable. (see more below). https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:162: synchronized(mSettingsObserverLock) { this block shouldn't be necessary https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:184: mSettingsObserverThread = null; can you verify that this does indeed terminate the thread gracefully? You should be able to check that by e.g. logging after this line as well as at the end of the run() method at the bottom of this file. If run() never completes, then the thread will be just be killed when the process is killed. Since there's no comment saying that this is intentional, it looks like a bug to me. Regardless, I think that this is easily fixed and that the code should have been something along these lines: if (mSettingsObserverThread != null) { mSettingsObserverThread.getLooper().quitSafely(); mSettingsObserverThread = null; } https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:186: if (mSettingsObserver != null) { this block shouldn't be necessary. As is, and considering that the thread actually has not stopped (I'm pretty certain), then this is a race since the thread is accessing this variable as well. So, good to get rid of it. https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:601: synchronized(mSettingsObserverLock) { After removing all the lines above, you can replace lines 601-607 with these three lines: SettingsObserver settingsObserver = new SettingsObserver(); Looper.loop(); mContentResolver.unregisterContentObserver(settingsObserver);
https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:184: mSettingsObserverThread = null; On 2013/12/11 15:54:52, tommi wrote: > can you verify that this does indeed terminate the thread gracefully? > You should be able to check that by e.g. logging after this line as well as at > the end of the run() method at the bottom of this file. If run() never > completes, then the thread will be just be killed when the process is killed. > Since there's no comment saying that this is intentional, it looks like a bug to > me. > > Regardless, I think that this is easily fixed and that the code should have been > something along these lines: > > if (mSettingsObserverThread != null) { > mSettingsObserverThread.getLooper().quitSafely(); > mSettingsObserverThread = null; > } to add to that, you might also want to do a join() on the thread object before setting the variable to null.
Looked at the new changes. Looking pretty good. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:131: SetCommunicationAudioModeOn(true); On 2013/12/11 13:16:38, henrika wrote: > I do save the state and restore the state in Java. OK? Yes that's great. I meant to update this comment but didn't get around to it. > > I have not noticed any degradation in BW due this mode. Note that we use > MODE_IN_COMMUNICATION: "In communication audio mode. An audio/video chat or VoIP > call is established." and *not* MODE_IN_CALL: the voice/in-call mode "A > telephony call is established.". Hence, name of mode does not imply quality > reduction IMHO. > > Still want comment? If "MODE_IN_COMMUNICATION" means that we'll be using SCO over bluetooth (as I think it does), then we may be switching from A2DP to SCO here and subsequently get a drop in audio quality. A2DP is typically a uni-directional stereo audio stream with focus on wide band codecs (if I'm not mistaken) whereas SCO is typically a low sample rate (16kHz or lower), bidirectional, mono, connection. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:304: j_audio_manager_.obj(), on); On 2013/12/11 13:16:38, henrika wrote: > So you want me to move the cache to this class instead? As mentioned above, I do > that in Java today. Doing it in Java as you're currently doing, is fine. I meant to remove my comment but forgot :) https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.h:71: bool IsLastDestroyedAudioStream(); On 2013/12/11 13:16:38, henrika wrote: > Renamed. > > Regarding input & output. The old solution (before I did any work) only set the > audio mode to comm mode when out stream was created, hence no mode shift if only > capture was active. It feels wrong so my idea was to maintain the mode as long > as any stream needs it. sgtm https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:574: private static boolean isNumeric(String str) { On 2013/12/11 13:16:38, henrika wrote: > I tried to avoid exception. Will not happen often. Do you want me to switch to > exception? yes I think that's OK.
#7 does not cover latest comments from tommi@. Improves the internal state machine and makes device handling thread safe. Will deal with comments from Tommi next round.
https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:105: private int mActiveAudioDevice = DEVICE_INVALID; I'm not following why this variable is needed. Looks like you only assign to it but never check it? https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:522: private int getDefaultDevice(boolean[] devices) { static?
More changes based on great feedback from Tommi. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:131: SetCommunicationAudioModeOn(true); OK, get your point but note that BT is just one case and the only case where Comm mode will lead to degradation. Will make a note about it; please check again. And i think that some hands-free profiles actually supports 16k as well. "Version 1.6 adds optional support for wide band speech with the mSBC codec, a 16 kHz monaural configuration of the SBC codec mandated by the A2DP profile." (http://en.wikipedia.org/wiki/Bluetooth_profile) The Android spec actually states that it supports 16k out and 8k in. It is actually not clear to me what profile we select when we say "switch to SCO". https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:304: j_audio_manager_.obj(), on); Great. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:224: * TODO(henrika): add comments... On 2013/12/11 10:52:04, Jói wrote: > Before landing this patch? Done. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:231: if (mSavedAudioMode == AudioManager.MODE_INVALID) { Will improve. https://codereview.chromium.org/110173003/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:574: private static boolean isNumeric(String str) { On 2013/12/11 17:25:05, tommi wrote: > On 2013/12/11 13:16:38, henrika wrote: > > I tried to avoid exception. Will not happen often. Do you want me to switch to > > exception? > > yes I think that's OK. Done. https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:112: private SettingsObserver mSettingsObserver = null; On 2013/12/11 15:54:52, tommi wrote: > suggest we remove this variable. (see more below). Done. https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:115: private final Object mSettingsObserverLock = new Object(); On 2013/12/11 15:54:52, tommi wrote: > suggest we remove this variable. (see more below). Done. https://codereview.chromium.org/110173003/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:162: synchronized(mSettingsObserverLock) { Removed. https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:105: private int mActiveAudioDevice = DEVICE_INVALID; Nice catch. My plan was to use it to avoid trying to activate a device upon state change which is already active. Now added usage. Hope you like it. https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:522: private int getDefaultDevice(boolean[] devices) { On 2013/12/12 10:03:40, tommi wrote: > static? Done.
As discussed offline, I'm looking into one other way to clean up the SettingsObserver code. I'll report back on that with a separate review comment (that way we may yet set a record for review-rounds-in-one-day! :)). https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:131: SetCommunicationAudioModeOn(true); On 2013/12/12 10:58:00, henrika wrote: > OK, get your point but note that BT is just one case and the only case where > Comm mode will lead to degradation. Will make a note about it; please check > again. > > And i think that some hands-free profiles actually supports 16k as well. > "Version 1.6 adds optional support for wide band speech with the mSBC codec, a > 16 kHz monaural configuration of the SBC codec mandated by the A2DP profile." > (http://en.wikipedia.org/wiki/Bluetooth_profile) > > The Android spec actually states that it supports 16k out and 8k in. > It is actually not clear to me what profile we select when we say "switch to > SCO". I'm not convinced that bluetooth is and forever will be the only case where communication mode will have different attributes than e.g. music mode. When you tested this, did you test stereo sources or only voice? I can understand that voice would sound more or less the same since the source signal would be tailored to the communication mode. In any case, my concern is that the AudioManager on android is currently implemented with the assumption that it will only be used for "communication mode". There can be differences in both routing and quality from 'normal' mode, so I think it's a good thing to document that somewhere. https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:105: private int mActiveAudioDevice = DEVICE_INVALID; On 2013/12/12 10:58:00, henrika wrote: > Nice catch. My plan was to use it to avoid trying to activate a device upon > state change which is already active. Now added usage. Hope you like it. I have a concern that mActiveAudioDevice and mSelectedAudioDevice represent basically the same thing, have similar names and it's not clear which does what or what happens when they're out of sync (now there's only a single check for the 'active' one). I think that the check you added for mActiveAudioDevice is actually not needed (premature optimization) since the same check is no doubt done inside each of the underlying methods. So, for the sake of simplicity, I'd like to not have it. https://codereview.chromium.org/110173003/diff/140001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/140001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:128: // If a Bluetooth headset is utilized, the audio stream will use the SCO grammar tip: http://writing.wikinut.com/Writing-Tip%3A-Use-and-Utilize-are-Not-the-Same/1c... https://codereview.chromium.org/110173003/diff/140001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/140001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:530: private static int getDefaultDevice(boolean[] devices) { nit: what about calling this selectDefaultDevice?
Nits and comments. Some questions as well. PTAL. https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/40001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:131: SetCommunicationAudioModeOn(true); Discussed offline; think we are in phase now. https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:105: private int mActiveAudioDevice = DEVICE_INVALID; OK, I'll remove it. My idea was to maintain these states since you can in fact have two devices, Speaker + Headset. Select Headset => selected == activated. Now, remove headset => active = speaker != selected. Add headset => back to first state. That was my idea; to note that we are no longer in a state where the selected device is active, hence try to restore it. Actually; given my explanation above. Do you still want me to remove it? It feels like a nice state to maintain IMHO. https://codereview.chromium.org/110173003/diff/140001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/140001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:128: // If a Bluetooth headset is utilized, the audio stream will use the SCO On 2013/12/12 11:43:15, tommi wrote: > grammar tip: > http://writing.wikinut.com/Writing-Tip%253A-Use-and-Utilize-are-Not-the-Same/... Done. https://codereview.chromium.org/110173003/diff/140001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/140001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:530: private static int getDefaultDevice(boolean[] devices) { On 2013/12/12 11:43:15, tommi wrote: > nit: what about calling this selectDefaultDevice? Done.
Since there's not a clear way to safely stop the current SettingsObserverThread, here's another suggestion which in addition to fixing the termination problem, simplifies things a bit and doesn't require a lock or startup synchronization. https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:105: private int mActiveAudioDevice = DEVICE_INVALID; On 2013/12/12 12:53:11, henrika wrote: > OK, I'll remove it. > > My idea was to maintain these states since you can in fact have two devices, > Speaker + Headset. Select Headset => selected == activated. > > Now, remove headset => active = speaker != selected. > > Add headset => back to first state. > > That was my idea; to note that we are no longer in a state where the selected > device is active, hence try to restore it. > > Actually; given my explanation above. Do you still want me to remove it? It > feels like a nice state to maintain IMHO. I don't see the code where we attempt to restore a state but that sounds like something related to getting state notifications. Do you plan on doing that in this CL? https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:116: private SettingsObserverThread mSettingsObserverThread = null; Change the type to be HandlerThread. You'll also have to import android.os.HandlerThread. https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:118: private final Object mSettingsObserverLock = new Object(); This lock is not needed. https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:163: mSettingsObserverThread = new SettingsObserverThread(); Change lines 163-171 to: mSettingsObserverThread = new HandlerThread("SettingsObserver"); mSettingsObserverThread.start(); mSettingsObserver = new SettingsObserver( new Handler(mSettingsObserverThread.getLooper())); https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:186: if (mSettingsObserverThread != null) { change lines 186-192 to: mSettingsObserverThread.quit(); mSettingsObserverThread.join(); mSettingsObserverThread = null; mContentResolver.unregisterContentObserver(mSettingsObserver); mSettingsObserver = null; https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:632: SettingsObserver() { change the constructor to be: SettingsObserver(Handler handler) { super(handler); ... } https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:647: private class SettingsObserverThread extends Thread { Remove this class.
One initial comment. Working on your change proposals now. https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:105: private int mActiveAudioDevice = DEVICE_INVALID; No. But I have not yet solved the problem I've described where we can start up by creating an output stream (which does not select a device). I have to solve that and I had intended to check if a device was active to solve it. Let me try to solve it and at the same time remove this state.
https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:105: private int mActiveAudioDevice = DEVICE_INVALID; On 2013/12/12 14:46:00, henrika wrote: > No. But I have not yet solved the problem I've described where we can start up > by creating an output stream (which does not select a device). I have to solve > that and I had intended to check if a device was active to solve it. Let me try > to solve it and at the same time remove this state. sounds good
New version now using HandlerThread. Added logs to verify that volume changes are received correctly and I can verify that we see this for a WebRTC demo: D/AudioManagerAndroid(16187): getAudioInputDeviceNames: [Speakerphone, Wired headset] D/AudioManagerAndroid(16187): setDevice: default D/AudioManagerAndroid(16187): reportUpdate: active=1, selected=-1, devices=[Speakerphone, Wired headset] D/AudioManagerAndroid(16187): setCommunicationAudioModeOn(true) D/AudioManagerAndroid(16187): SettingsObserver.onChange: false D/AudioManagerAndroid(16187): nativeSetMute: false D/AudioManagerAndroid(16187): SettingsObserver.onChange: false D/AudioManagerAndroid(16187): nativeSetMute: true where the volume is not all the way down to the left for 'true' but that is known stuff. Seems to work ;-) Just some more info. Start Youtube in one tab => music mode and volume slider shows speaker icon and can be pulled down to zero. Start audio => audio and not able to trigger true in our callback. Create new tab and start WebRTC loopback => Youtube goes silent and volume is now "phone volume" which can only be pulled down to 10%. Start loopback => audio and it is muted at 10% due to our callback. Switch back to Youtube => still phone icon in volume. Start Youtube => now audio is mixed (Y+WebRTC). https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:116: private SettingsObserverThread mSettingsObserverThread = null; On 2013/12/12 14:29:45, tommi wrote: > Change the type to be HandlerThread. You'll also have to import > android.os.HandlerThread. Done. https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:118: private final Object mSettingsObserverLock = new Object(); On 2013/12/12 14:29:45, tommi wrote: > This lock is not needed. Done. https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:186: if (mSettingsObserverThread != null) { On 2013/12/12 14:29:45, tommi wrote: > change lines 186-192 to: > mSettingsObserverThread.quit(); > mSettingsObserverThread.join(); > mSettingsObserverThread = null; > mContentResolver.unregisterContentObserver(mSettingsObserver); > mSettingsObserver = null; Done. https://codereview.chromium.org/110173003/diff/140007/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:647: private class SettingsObserverThread extends Thread { On 2013/12/12 14:29:45, tommi wrote: > Remove this class. Done.
> Just some more info. ... > Start Youtube => now audio is mixed (Y+WebRTC). Stop WebRTC => volume is now back to "music volume" since we restored old mode.
excellent! thanks for the update :) https://codereview.chromium.org/110173003/diff/180001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/180001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:573: private int getNumOfAudioDevicesWithLock() { can you clarify the 'WithLock' part? there doesn't appear to be any locking done https://codereview.chromium.org/110173003/diff/180001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:577: count++; nit: ++count
I can verify that the mode is restored a few seconds after we close the WebRTC tab and that stage the volume is restored to the speaker icon and we can drag it down to zero again.
comments only https://codereview.chromium.org/110173003/diff/180001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/180001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:573: private int getNumOfAudioDevicesWithLock() { I was asked by Wei to add it since it is called from getAudioInputDeviceNames and we have a lock when we call this method. https://codereview.chromium.org/110173003/diff/180001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:577: count++; On 2013/12/12 15:48:11, tommi wrote: > nit: ++count Done.
https://codereview.chromium.org/110173003/diff/180001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/180001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:573: private int getNumOfAudioDevicesWithLock() { On 2013/12/12 16:01:21, henrika wrote: > I was asked by Wei to add it since it is called from getAudioInputDeviceNames > and we have a lock when we call this method. ah, I guess I read the name differently. please ignore.
Adding bulach@ as OWNER. We are pretty close here but I need an owner as well. Main description summarizes what this CL is about. - Do less when Chrome starts. - Remove BT support (will be added in separate CL this time without use of BT persmission) - Clean up thread handling for volume observer. - Improved comments related to the fact that the audio manager for Android changes mode to COMMUNICATION. Thanks ;-)
tommi@: Have done tests where I only create an output stream (which does not select any device at all) and start render audio. On the devices I have tested (Nexus 7, Nexus 5), audio is routed OK even without setting the device explicitly. I would like to avoid adding "safety code" in this area if you are OK with it. It *could* be that older devices needs some "help" to route to the correct default device but I don't know for sure.
almost there :) https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:561: // Verify input device enumeration. nit: Since these two tests are now generally useful for all platforms that support device enumeration, can you add a TODO here to move them to the more generic unit test file for AudioManager that Joi added? https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:822: printf("\n"); do you mind removing this? I think it's a leftover since printf was used for logging some time ago. https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:130: if (stream && has_no_streams) { nit: no {} https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:152: if (stream && has_no_streams) { nit: no {} https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:163: if (HasNoAudioStreams()) { nit: no {} https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:175: if (HasNoAudioStreams()) { nit: no {} https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:36: private static final boolean DEBUG = true; remember to change to false before submitting https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:102: private int mActiveAudioDevice = DEVICE_INVALID; remove this variable in this cl since it's not used? https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:262: boolean devices[] = null; nit: you only need this for DEVICE_DEFAULT, so you can move this into the isEmpty() scope below. https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:277: Integer id = Integer.valueOf(deviceId); I still think that implementing isNumeric + using Integer.valueOf isn't buying us anything over just using Integer.parseInt(). * valueOf() can throw the same exception as parseInt since that's how it is implemented. * isNumeric() doesn't shield us from these exceptions since a string like "9999999999999" is still a number (but too large for 'int') So, I suggest: * remove isNumeric() * Have a check at the top of setDevice() like so (the try/catch thing might even be optional since we should never get IDs from the renderer process - i.e. they should be trusted): if (DEBUG) logd("setDevice: " + deviceId); int intDeviceId = DEVICE_DEFAULT; try { if (!deviceId.isEmpty()) intDeviceId = Integer.parseInt(deviceId); } catch (NumericException e) { loge("Invalid device ID: " + deviceId); return false; } * Or, the non-try-catch version: if (DEBUG) logd("setDevice: " + deviceId); int intDeviceId = deviceId.isEmpty() ? DEVICE_DEFAULT : Integer.parseInt(deviceId); * Then the rest of the body can be something like: if (intDeviceId == DEVICE_DEFAULT) { int defaultDevice = selectDefaultDevice(devices); setAudioDevice(defaultDevice); mSelectedAudioDevice = DEVICE_DEFAULT; return true; } // A non-default device is specified. Verify that it is valid // device, and if so, start using it. List<Integer> validIds = Arrays.asList(VALID_DEVICES); if (!validIds.contains(intDeviceId) || !mAudioDevices[intDeviceId]) return false; mSelectedAudioDevice = intDeviceId; setAudioDevice(intDeviceId); return true; https://codereview.chromium.org/110173003/diff/230001/testing/android/java/sr... File testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java (right): https://codereview.chromium.org/110173003/diff/230001/testing/android/java/sr... testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:38: if (true || extras != null && extras.containsKey(EXTRA_RUN_IN_SUB_THREAD)) { remember to remove before submitting
On 2013/12/13 10:43:12, henrika wrote: > tommi@: Have done tests where I only create an output stream (which does not > select any device at all) and start render audio. On the devices I have tested > (Nexus 7, Nexus 5), audio is routed OK even without setting the device > explicitly. I would like to avoid adding "safety code" in this area if you are > OK with it. It *could* be that older devices needs some "help" to route to the > correct default device but I don't know for sure. I'm not sure I follow. Do you mean that 'selectDevice' in the case of using the default device, is not necessary? That would make sense to me but perhaps you can clarify?
On 2013/12/13 11:37:27, tommi wrote: > On 2013/12/13 10:43:12, henrika wrote: > > tommi@: Have done tests where I only create an output stream (which does not > > select any device at all) and start render audio. On the devices I have tested > > (Nexus 7, Nexus 5), audio is routed OK even without setting the device > > explicitly. I would like to avoid adding "safety code" in this area if you are > > OK with it. It *could* be that older devices needs some "help" to route to the > > correct default device but I don't know for sure. > > I'm not sure I follow. Do you mean that 'selectDevice' in the case of using the > default device, is not necessary? That would make sense to me but perhaps you > can clarify? I would say that, given only two devices (wired and speaker), we could probably skip the explicit SetDevice(default) case. The audio routing that follows with COMM mode seems to be inline with what we want. Not 100% if that is the case when a BT headset exists as well. In any case, we could (in theory) redefine our selection scheme for default and then we would not be inline with the built-in routing.
good stuff! mostly nits and suggestions below, I'm sure tommi is far more knowledgeable in this area than I am. https://codereview.chromium.org/110173003/diff/250001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/250001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:130: if (stream && has_no_streams) { nit: this condition looks funky :) maybe rename s/has_no_streams/previous_streams/ or perhaps s/stream/new_stream/ so that the if would be more readable? ditto on the places below. https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:36: private static final boolean DEBUG = true; make sure you change it back :) https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:55: // Supported audio device types. nit: unindent https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:258: boolean setDevice(String deviceId) { nit: maybe private? https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:267: mRequestedAudioDevice = DEVICE_DEFAULT; nit: needs to be protected by mLock? more below, it's being read in the other thread within the lock.. https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:276: mRequestedAudioDevice = id.intValue(); nit: mLock? https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:465: synchronized (mLock) { as above, the comment on mLock says it only protects mAudioDevices, not mRequestedAudioDevice. update? https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:554: if (requested == DEVICE_DEFAULT || !devices[mRequestedAudioDevice]) { nit: s/mRequestedAudioDevice/requested/, otherwise this needs a lock. https://codereview.chromium.org/110173003/diff/250001/testing/android/java/sr... File testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java (right): https://codereview.chromium.org/110173003/diff/250001/testing/android/java/sr... testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:38: if (true || extras != null && extras.containsKey(EXTRA_RUN_IN_SUB_THREAD)) { remove it :)
Fixes based on latest round by Tommi. Marcus; you are next ;-) https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:561: // Verify input device enumeration. My bad; I should have mentioned that I took them from the existing AudioManagerTest. Is that the one you are referring to? I.e., they are in use today. I just wanted to move it into the unit test for Android audio to get a broader test in one place. https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:822: printf("\n"); On 2013/12/13 11:30:13, tommi wrote: > do you mind removing this? I think it's a leftover since printf was used for > logging some time ago. Done. https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:130: if (stream && has_no_streams) { On 2013/12/13 11:30:13, tommi wrote: > nit: no {} Done. https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:152: if (stream && has_no_streams) { On 2013/12/13 11:30:13, tommi wrote: > nit: no {} Done. https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:163: if (HasNoAudioStreams()) { On 2013/12/13 11:30:13, tommi wrote: > nit: no {} Done. https://codereview.chromium.org/110173003/diff/230001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:175: if (HasNoAudioStreams()) { On 2013/12/13 11:30:13, tommi wrote: > nit: no {} Done. https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:36: private static final boolean DEBUG = true; On 2013/12/13 11:30:13, tommi wrote: > remember to change to false before submitting Done. https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:102: private int mActiveAudioDevice = DEVICE_INVALID; On 2013/12/13 11:30:13, tommi wrote: > remove this variable in this cl since it's not used? Done. https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:262: boolean devices[] = null; Cool. Done. https://codereview.chromium.org/110173003/diff/230001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:277: Integer id = Integer.valueOf(deviceId); Removed two test cases and used the non-try-catch version. https://codereview.chromium.org/110173003/diff/230001/testing/android/java/sr... File testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java (right): https://codereview.chromium.org/110173003/diff/230001/testing/android/java/sr... testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:38: if (true || extras != null && extras.containsKey(EXTRA_RUN_IN_SUB_THREAD)) { On 2013/12/13 11:30:13, tommi wrote: > remember to remove before submitting Done.
lgtm
Thanks Marcus. Now more encapsulated and thread safe ;-) PTAL https://codereview.chromium.org/110173003/diff/250001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/110173003/diff/250001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:130: if (stream && has_no_streams) { Hmm, it is a result of this long review... Will try to merge views ;-) https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:36: private static final boolean DEBUG = true; On 2013/12/13 13:49:49, bulach wrote: > make sure you change it back :) Done. https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:55: // Supported audio device types. On 2013/12/13 13:49:49, bulach wrote: > nit: unindent Done. https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:258: boolean setDevice(String deviceId) { Done. Did same for all methods in this class. https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:267: mRequestedAudioDevice = DEVICE_DEFAULT; On 2013/12/13 13:49:49, bulach wrote: > nit: needs to be protected by mLock? more below, it's being read in the other > thread within the lock.. Done. https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:276: mRequestedAudioDevice = id.intValue(); On 2013/12/13 13:49:49, bulach wrote: > nit: mLock? Done. https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:465: synchronized (mLock) { On 2013/12/13 13:49:49, bulach wrote: > as above, the comment on mLock says it only protects mAudioDevices, not > mRequestedAudioDevice. update? Done. https://codereview.chromium.org/110173003/diff/250001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:554: if (requested == DEVICE_DEFAULT || !devices[mRequestedAudioDevice]) { Thanks! https://codereview.chromium.org/110173003/diff/250001/testing/android/java/sr... File testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java (right): https://codereview.chromium.org/110173003/diff/250001/testing/android/java/sr... testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:38: if (true || extras != null && extras.containsKey(EXTRA_RUN_IN_SUB_THREAD)) { Of course.
lgtm, tiny nit. thanks! https://codereview.chromium.org/110173003/diff/310001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/110173003/diff/310001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:95: LOG(WARNING) << "No input devices detected"; nit: perhaps return here? otherwise 101 will fail, since begin() == end()
Now it is perfect ;-)
w00t! ship it!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/110173003/330001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/110173003/330001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/110173003/340001
Failed to apply patch for media/audio/android/audio_manager_android.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file media/audio/android/audio_manager_android.cc
Hunk #1 succeeded at 37 (offset 1 line).
Hunk #2 succeeded at 123 (offset 5 lines).
Hunk #3 succeeded at 145 (offset 5 lines).
Hunk #4 FAILED at 201.
Hunk #5 succeeded at 269 with fuzz 1 (offset 19 lines).
Hunk #6 succeeded at 307 (offset 19 lines).
Hunk #7 succeeded at 323 (offset 19 lines).
1 out of 7 hunks FAILED -- saving rejects to file
media/audio/android/audio_manager_android.cc.rej
Patch: media/audio/android/audio_manager_android.cc
Index: media/audio/android/audio_manager_android.cc
diff --git a/media/audio/android/audio_manager_android.cc
b/media/audio/android/audio_manager_android.cc
index
b588768bbd08c1fcfbb75df70248a9da752a28a4..90e4d01bfdfb9f4de4ef57c10dd5b5ed27196b22
100644
--- a/media/audio/android/audio_manager_android.cc
+++ b/media/audio/android/audio_manager_android.cc
@@ -36,9 +36,6 @@ static void AddDefaultDevice(AudioDeviceNames* device_names) {
// Maximum number of output streams that can be open simultaneously.
static const int kMaxOutputStreams = 10;
-static const int kAudioModeNormal = 0x00000000;
-static const int kAudioModeInCommunication = 0x00000003;
-
static const int kDefaultInputBufferSize = 1024;
static const int kDefaultOutputBufferSize = 2048;
@@ -121,12 +118,17 @@ AudioOutputStream*
AudioManagerAndroid::MakeAudioOutputStream(
const AudioParameters& params,
const std::string& device_id,
const std::string& input_device_id) {
+ bool had_no_streams = HadNoAudioStreams();
AudioOutputStream* stream =
AudioManagerBase::MakeAudioOutputStream(params, std::string(),
std::string());
- if (stream && output_stream_count() == 1) {
- SetAudioMode(kAudioModeInCommunication);
- }
+
+ // The audio manager for Android creates streams intended for real-time
+ // VoIP sessions and therefore sets the audio mode to MODE_IN_COMMUNICATION.
+ // If a Bluetooth headset is used, the audio stream will use the SCO
+ // channel and therefore have a limited bandwidth (8-16kHz).
+ if (stream && had_no_streams)
+ SetCommunicationAudioModeOn(true);
{
base::AutoLock lock(streams_lock_);
@@ -138,22 +140,37 @@ AudioOutputStream*
AudioManagerAndroid::MakeAudioOutputStream(
AudioInputStream* AudioManagerAndroid::MakeAudioInputStream(
const AudioParameters& params, const std::string& device_id) {
+ bool had_no_streams = HadNoAudioStreams();
AudioInputStream* stream =
AudioManagerBase::MakeAudioInputStream(params, device_id);
+
+ // The audio manager for Android creates streams intended for real-time
+ // VoIP sessions and therefore sets the audio mode to MODE_IN_COMMUNICATION.
+ // If a Bluetooth headset is used, the audio stream will use the SCO
+ // channel and therefore have a limited bandwidth (8kHz).
+ if (stream && had_no_streams)
+ SetCommunicationAudioModeOn(true);
return stream;
}
void AudioManagerAndroid::ReleaseOutputStream(AudioOutputStream* stream) {
AudioManagerBase::ReleaseOutputStream(stream);
- if (!output_stream_count()) {
- SetAudioMode(kAudioModeNormal);
- }
+
+ // Restore the audio mode which was used before the first communication-
+ // mode stream was created.
+ if (HadNoAudioStreams())
+ SetCommunicationAudioModeOn(false);
base::AutoLock lock(streams_lock_);
streams_.erase(static_cast<OpenSLESOutputStream*>(stream));
}
void AudioManagerAndroid::ReleaseInputStream(AudioInputStream* stream) {
AudioManagerBase::ReleaseInputStream(stream);
+
+ // Restore the audio mode which was used before the first communication-
+ // mode stream was created.
+ if (HadNoAudioStreams())
+ SetCommunicationAudioModeOn(false);
}
AudioOutputStream* AudioManagerAndroid::MakeLinearOutputStream(
@@ -184,11 +201,14 @@ AudioInputStream*
AudioManagerAndroid::MakeLowLatencyInputStream(
const AudioParameters& params, const std::string& device_id) {
DCHECK_EQ(AudioParameters::AUDIO_PCM_LOW_LATENCY, params.format());
DLOG_IF(ERROR, device_id.empty()) << "Invalid device ID!";
- // Utilize the device ID to select the correct input device.
+ // Use the device ID to select the correct input device.
// Note that the input device is always associated with a certain output
// device, i.e., this selection does also switch the output device.
// All input and output streams will be affected by the device selection.
- SetAudioDevice(device_id);
+ if (!SetAudioDevice(device_id)) {
+ LOG(ERROR) << "Unable to select audio device!";
+ return NULL;
+ }
return new OpenSLESInputStream(this, params);
}
@@ -233,6 +253,10 @@ AudioParameters
AudioManagerAndroid::GetPreferredOutputStreamParameters(
sample_rate, bits_per_sample, buffer_size);
}
+bool AudioManagerAndroid::HadNoAudioStreams() {
+ return output_stream_count() == 0 && input_stream_count() == 0;
+}
+
// static
bool AudioManagerAndroid::RegisterAudioManager(JNIEnv* env) {
return RegisterNativesImpl(env);
@@ -267,13 +291,13 @@ void AudioManagerAndroid::DoSetMuteOnAudioThread(bool
muted) {
}
}
-void AudioManagerAndroid::SetAudioMode(int mode) {
- Java_AudioManagerAndroid_setMode(
+void AudioManagerAndroid::SetCommunicationAudioModeOn(bool on) {
+ Java_AudioManagerAndroid_setCommunicationAudioModeOn(
base::android::AttachCurrentThread(),
- j_audio_manager_.obj(), mode);
+ j_audio_manager_.obj(), on);
}
-void AudioManagerAndroid::SetAudioDevice(const std::string& device_id) {
+bool AudioManagerAndroid::SetAudioDevice(const std::string& device_id) {
JNIEnv* env = AttachCurrentThread();
// Send the unique device ID to the Java audio manager and make the
@@ -283,7 +307,7 @@ void AudioManagerAndroid::SetAudioDevice(const std::string&
device_id) {
env,
device_id == AudioManagerBase::kDefaultDeviceId ?
std::string() : device_id);
- Java_AudioManagerAndroid_setDevice(
+ return Java_AudioManagerAndroid_setDevice(
env, j_audio_manager_.obj(), j_device_id.obj());
}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/110173003/360001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Message was sent while issue was closed.
Committed patchset #21 manually as r240883 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
