Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
78033003:
Adding device enumeration to Android device manager (Closed)
|
Created:
7 years, 1 month ago by henrika (OOO until Aug 14) Modified:
7 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, bulach, Ami GONE FROM CHROMIUM, wjia(left Chromium) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding device enumeration to the Android device manager.
Main goal is to support the getSources API (see TEST) and to allow VoIP clients in Chrome to use Bluetooth headsets.
BUG=324464
TEST=https://simpl.info/getusermedia/sources/ and manual unittests in media.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237942
Patch Set 1 : Initial support for BT detection #
Total comments: 8
Patch Set 2 : tommi@, nits #
Total comments: 12
Patch Set 3 : Added setDevices #Patch Set 4 : cleanup #Patch Set 5 : nits #
Total comments: 38
Patch Set 6 : bulach@ and tommi@ #Patch Set 7 : try again #
Total comments: 21
Patch Set 8 : more fixes #
Total comments: 12
Patch Set 9 : nits #Patch Set 10 : final changes? #Patch Set 11 : nit #
Total comments: 14
Messages
Total messages: 26 (0 generated)
https://codereview.chromium.org/78033003/diff/30001/media/audio/audio_manager... File media/audio/audio_manager_unittest.cc (right): https://codereview.chromium.org/78033003/diff/30001/media/audio/audio_manager... media/audio/audio_manager_unittest.cc:79: VLOG(2) << "Device ID(" << it->unique_id Temporary debugging only. Will be removed.
https://codereview.chromium.org/78033003/diff/30001/testing/android/java/src/... File testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java (right): https://codereview.chromium.org/78033003/diff/30001/testing/android/java/src/... testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:39: if (true) { This is just a hack to allow us to receive broadcasted intents so we can set up the initial state with a headset if it is attached.
great! just some tiny drive-by nits. https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (left): https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:19: DCHECK(device_names->empty()); nit: maybe just dcheck instead that the default device isn't present in the list? https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:89: env, j_audio_manager_.obj()); nit: indent https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:103: AddDefaultDevice(device_names); couldn't you call this method before enumerating the devices and then you don't have to remove the dcheck from AddDefaultDevice?
https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (left): https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:19: DCHECK(device_names->empty()); Fixed based on you comment below ;-) https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:89: env, j_audio_manager_.obj()); On 2013/11/22 14:59:10, tommi wrote: > nit: indent Done. https://codereview.chromium.org/78033003/diff/30001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:103: AddDefaultDevice(device_names); Thanks ;-)
Just some drive-by nits. https://codereview.chromium.org/78033003/diff/110001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/110001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:91: AddDefaultDevice(device_names); Not sure you want to add the default device again (already done on line 85 above). https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:116: // it was unlpugged. If so, switch to speaker phone. unlpugged -> unplugged https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:117: // If'it was not in use, don't do anything. If'it -> If it https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:153: * Pupulate the list of available audio devices and register receivers Pupulate -> Populate https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:306: * Returns true if the device has a headset earpice and false if not. I think the convention for JavaDoc comments is for the * here to line up with the first * in the preceding line (here and elsewhere). https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:388: logd("BT device was connected at start of call"); The indent here seems off, shouldn't it be 4 in from line 384?
Thanks Jói, working on refactoring based on separate e-mail input from tommi@ (move to input-device version instead). Your proposed changes will be included in next version. https://codereview.chromium.org/78033003/diff/110001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/110001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:91: AddDefaultDevice(device_names); oops https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:116: // it was unlpugged. If so, switch to speaker phone. On 2013/11/25 13:09:35, Jói wrote: > unlpugged -> unplugged Done. https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:117: // If'it was not in use, don't do anything. On 2013/11/25 13:09:35, Jói wrote: > If'it -> If it Done. https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:153: * Pupulate the list of available audio devices and register receivers On 2013/11/25 13:09:35, Jói wrote: > Pupulate -> Populate Done. https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:306: * Returns true if the device has a headset earpice and false if not. On 2013/11/25 13:09:35, Jói wrote: > I think the convention for JavaDoc comments is for the * here to line up with > the first * in the preceding line (here and elsewhere). Done. https://codereview.chromium.org/78033003/diff/110001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:388: logd("BT device was connected at start of call"); On 2013/11/25 13:09:35, Jói wrote: > The indent here seems off, shouldn't it be 4 in from line 384? Done.
it mostly lgtm, a few comments and please wait for other reviewers as well... some notes on the commit message: - it's too long :) it's probably best to move some of that to a bug and link it from here. - the internal codename shouldn't be mentioned in public. - "every byte helps".. so whilst I appreciate the convenience, we're striving to keep binary size, memory footprint, etc.., to a minimum.. I suppose the logs can always be post-processed with minimum effort, so we'd really appreciate it being more compact and saving a few precious bytes! https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:127: SetAudioMode(kAudioModeInCommunication); nit: unindent https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:175: // Utilize the deviceI ID to select the correct input device. nit: s/deviceI/device/ https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... media/audio/android/audio_manager_android.h:66: void SetAudioDevice(std::string device_id); nit: const & ? https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:74: }; see more below.. where are these names going to be shown? if the user will ever see this, then it needs to be a resource string, translated, etc... if it's internal for devs, it'd be much better to save a few bytes and just have the integer value directly.. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:92: private static final String[] STATE_NAMES = new String[] { this is only used for logging purposes, and it'd be extra work to keep in sync... would it be too painful to just log the value itself, and then translate the value when reading the logs (or perhaps even a simple script to this out-of-band translation if it's going to be that common? https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:101: // Use 44.1kHz as the default sampling rate. nit: this comment is a bit redundant :) perhaps just have DEFAULT_SAMPLING_RATE_HZ would be as clear? https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:372: private static final int HAS_MIC = 1; nit: indent 368-372 https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:385: logd("==> WiredHeadsetReceiver.onReceive: state=" + state as per our chat, if this is absolutely required, I'd suggest keeping it more compact: the "tag" in logd already says where it's coming from, so perhaps: "onReceive s=" ... "m=" ... "n="... https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:392: case STATE_UNPLUGGED: nit: indent this block inside the switch
https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:74: }; On 2013/11/29 10:58:39, bulach wrote: > see more below.. where are these names going to be shown? if the user will ever > see this, then it needs to be a resource string, translated, etc... if it's > internal for devs, it'd be much better to save a few bytes and just have the > integer value directly.. These strings will eventually be shown to users*. We've talked about doing this correctly in a future cl. It would be ideal to get this from the OS so that we can be guaranteed to show a consistent friendly name in the appropriate language. Is there a way to do that? Henrik: Can we change these to be more UI friendly for now? E.g. "Bluetooth headset" instead of "BLUETOOTH_HEADSET" etc. * The way these strings become available to web applications is via the MediaStreamTrack.getSources() API and the 'label' attribute of a track, is intended to be used by web apps when showing the user what devices are available. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:77: static final int STATE_SPEAKERPHONE_ON = 0; what about reserving 0 for no device selected. I'm sure there are android devices that do not have a speaker phone (chromecast being one example). https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:101: // Use 44.1kHz as the default sampling rate. is 44.1kHz the most common sample rate in practice or was it chosen here for some other reason? (are there devices that will not support 44.1?) If 48kHz is possible, that would be a more SRC friendly rate and match better with e.g. the webrtc stack. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:116: private Integer mAudioDeviceState = STATE_SPEAKERPHONE_ON; suggest to initialize this to no device until we know better. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:119: private Set<Integer> mAudioDevices = new HashSet<Integer>(); A hashset of integers seems overkill to me - but I'm not a java programmer. In C(++) I would have just used a fixed sized array of bool probably :-/ https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:488: case DEVICE_BLUETOOTH_HEADSET: indent https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:492: break; 'break' should be indented from 'case' (not at the same indent). https://codereview.chromium.org/78033003/diff/340001/testing/android/AndroidM... File testing/android/AndroidManifest.xml (right): https://codereview.chromium.org/78033003/diff/340001/testing/android/AndroidM... testing/android/AndroidManifest.xml:27: <uses-permission android:name="android.permission.BLUETOOTH" /> remove this from this cl?
Thanks. https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:127: SetAudioMode(kAudioModeInCommunication); On 2013/11/29 10:58:39, bulach wrote: > nit: unindent Done. https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:175: // Utilize the deviceI ID to select the correct input device. On 2013/11/29 10:58:39, bulach wrote: > nit: s/deviceI/device/ Done. https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... media/audio/android/audio_manager_android.h:66: void SetAudioDevice(std::string device_id); I actually modify it if it contains "default". Sound OK? https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:74: }; I will modify according to proposal from Tommi. To make them "perfect" I will hahve to follow up and get some assistance from the Android team. I honestly don't know how to access and localize these names. Adding TODO as well. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:77: static final int STATE_SPEAKERPHONE_ON = 0; On 2013/11/29 11:45:47, tommi wrote: > what about reserving 0 for no device selected. I'm sure there are android > devices that do not have a speaker phone (chromecast being one example). Done. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:92: private static final String[] STATE_NAMES = new String[] { Removed https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:101: // Use 44.1kHz as the default sampling rate. I would like to pass on that question. I did not write this code and can't really comment. It is only used when getProperty(AudioManager.PROPERTY_OUTPUT_SAMPLE_RATE) returns null and I don't know when that happens or the background for why 44.1 is used here. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:116: private Integer mAudioDeviceState = STATE_SPEAKERPHONE_ON; On 2013/11/29 11:45:47, tommi wrote: > suggest to initialize this to no device until we know better. Done. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:119: private Set<Integer> mAudioDevices = new HashSet<Integer>(); Well, it gives me add(), remove(), contains(), avoids duplicates and an iterator. Thanks for allowing me to keep it :-) https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:372: private static final int HAS_MIC = 1; On 2013/11/29 10:58:39, bulach wrote: > nit: indent 368-372 Done. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:385: logd("==> WiredHeadsetReceiver.onReceive: state=" + state On 2013/11/29 10:58:39, bulach wrote: > as per our chat, if this is absolutely required, I'd suggest keeping it more > compact: the "tag" in logd already says where it's coming from, so perhaps: > "onReceive s=" ... "m=" ... "n="... Done. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:392: case STATE_UNPLUGGED: On 2013/11/29 10:58:39, bulach wrote: > nit: indent this block inside the switch Done. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:488: case DEVICE_BLUETOOTH_HEADSET: On 2013/11/29 11:45:47, tommi wrote: > indent Done. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:492: break; On 2013/11/29 11:45:47, tommi wrote: > 'break' should be indented from 'case' (not at the same indent). Done. https://codereview.chromium.org/78033003/diff/340001/testing/android/AndroidM... File testing/android/AndroidManifest.xml (right): https://codereview.chromium.org/78033003/diff/340001/testing/android/AndroidM... testing/android/AndroidManifest.xml:27: <uses-permission android:name="android.permission.BLUETOOTH" /> hhmmm, yes.
tommi@: PTAL
https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... media/audio/android/audio_manager_android.h:66: void SetAudioDevice(std::string device_id); On 2013/11/29 12:20:20, henrika wrote: > I actually modify it if it contains "default". Sound OK? we actually never pass strings by value so +1 on const& https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:74: }; On 2013/11/29 12:20:20, henrika wrote: > I will modify according to proposal from Tommi. To make them "perfect" I will > hahve to follow up and get some assistance from the Android team. I honestly > don't know how to access and localize these names. Adding TODO as well. I'm assuming we'll just use grit as usual for localization if we have to go that route. I also assume that we'll then load the resources from C++ land. In any case, I think that's something we can do in a later CL unless Marcus disagrees. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:119: private Set<Integer> mAudioDevices = new HashSet<Integer>(); On 2013/11/29 12:20:20, henrika wrote: > Well, it gives me add(), remove(), contains(), avoids duplicates and an > iterator. Thanks for allowing me to keep it :-) bool[] has all of that, even without having to call any methods. Since we're concerned about bloat, think about the amount of code behind a hash set vs an array of 4 bools. mAudioDevices.contains(foo)) vs if (mAudioDevices[foo]) mAudioDevices.add(foo) vs mAudioDevices[foo] = true mAudioDevices.remove(foo) vs mAudioDevices[foo] = false for iteration, you just go from the first to last id to check the state.
https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:101: // Use 44.1kHz as the default sampling rate. On 2013/11/29 12:20:20, henrika wrote: > I would like to pass on that question. I did not write this code and can't > really comment. It is only used when > getProperty(AudioManager.PROPERTY_OUTPUT_SAMPLE_RATE) returns null and I don't > know when that happens or the background for why 44.1 is used here. OK, let's take that offline and talk to Wei.
LGTM with just a few nits. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:67: // Maps audio device types to string values. Maybe it would be better to have this right after lines 53 through 57 and mention that the indexes there need to be in sync with this array. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:78: // The device does not have any audio device. It would be cool to document the valid state transmissions. Maybe for a later change, once you are turning Bluetooth on/off at the opportune moments. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:141: mSavedSpeakerPhoneState = mAudioManager.isSpeakerphoneOn(); Naming suggestion: mSavedIsSpeakerphoneOn and mSavedIsMicrophoneMute would make it clear what false vs. true means for these flags. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:331: boolean wasOn = mAudioManager.isMicrophoneMute(); nit: wasMuted maybe instead of wasOn? https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:345: boolean hasFeature = mContext.getPackageManager().hasSystemFeature( could just return directly https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:371: if (action.equals(Intent.ACTION_HEADSET_PLUG)) { Will it not always equal this, since you have a filter? Any need to check it? https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:419: nit: just one blank line?
No longer using HashSet. https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/78033003/diff/340001/media/audio/android/audi... media/audio/android/audio_manager_android.h:66: void SetAudioDevice(std::string device_id); Will fix. https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/340001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:119: private Set<Integer> mAudioDevices = new HashSet<Integer>(); Will simplify.
https://codereview.chromium.org/78033003/diff/380001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/380001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:77: // Get all available devices and add these to the list. nit: redundant comment (since we're inside GetAudioInputDeviceNames) https://codereview.chromium.org/78033003/diff/380001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:78: AudioDeviceName device; nit: declare above the loop where it's used. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:371: if (action.equals(Intent.ACTION_HEADSET_PLUG)) { On 2013/11/29 14:40:46, Jói wrote: > Will it not always equal this, since you have a filter? Any need to check it? +1 - and if it isn't then it might be better to save on indentation and return early if the action isn't ACTION_HEADSET_PLUG. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:480: logd("--- TO BE IMPLEMENTED ---"); use |if (DEBUG)| for debug logging?
Done. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:67: // Maps audio device types to string values. On 2013/11/29 14:40:46, Jói wrote: > Maybe it would be better to have this right after lines 53 through 57 and > mention that the indexes there need to be in sync with this array. Done. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:78: // The device does not have any audio device. Added TODO. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:141: mSavedSpeakerPhoneState = mAudioManager.isSpeakerphoneOn(); thx https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:331: boolean wasOn = mAudioManager.isMicrophoneMute(); On 2013/11/29 14:40:46, Jói wrote: > nit: wasMuted maybe instead of wasOn? Done. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:345: boolean hasFeature = mContext.getPackageManager().hasSystemFeature( On 2013/11/29 14:40:46, Jói wrote: > could just return directly Done. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:371: if (action.equals(Intent.ACTION_HEADSET_PLUG)) { You guys ;-) https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:419: On 2013/11/29 14:40:46, Jói wrote: > nit: just one blank line? Done. https://codereview.chromium.org/78033003/diff/380001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:480: logd("--- TO BE IMPLEMENTED ---"); Forgot, will do.
Tommi?
forgot, sorry. https://codereview.chromium.org/78033003/diff/380001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/380001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:77: // Get all available devices and add these to the list. On 2013/11/29 15:04:33, tommi wrote: > nit: redundant comment (since we're inside GetAudioInputDeviceNames) Done. https://codereview.chromium.org/78033003/diff/380001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:78: AudioDeviceName device; On 2013/11/29 15:04:33, tommi wrote: > nit: declare above the loop where it's used. Done.
still lgtm, small suggestions you may or may not want to do now.. https://codereview.chromium.org/78033003/diff/400001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/400001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:231: base::android::AttachCurrentThread(), nit: wrong indent, should be 4 (ditto following block) https://codereview.chromium.org/78033003/diff/400001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:259: ConvertUTF8ToJavaString(env, id); how about just: // Provide an empty string to the Java audio manager if the default // device is selected. ConvertUTF8ToJavaString( env, device_id == AudioManagerBase::kDefaultDeviceId ? std::string() : device_id); https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:66: // localize the name strings. if this is going to be user visible, any chance of using resource strings now? https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:118: private boolean[] mAudioDevices = new boolean[] { false, false, false, false }; would this work? new boolean[DEVICE_MAX_ID] = ... alternatively, it could be a single integer and a bitmask.. https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:535: Log.i(TAG, "Manufacturer:" + Build.MANUFACTURER + ditto for shorter abbreviations..
lgtm https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:61: private static final int DEVICE_MAX_ID = 4; nit: Since this is an invalid device id and not actually the max value (which is 3), suggest to call this DEVICE_COUNT.
Thanks again. https://codereview.chromium.org/78033003/diff/400001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/400001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:231: base::android::AttachCurrentThread(), On 2013/11/29 15:41:07, bulach wrote: > nit: wrong indent, should be 4 (ditto following block) Done. https://codereview.chromium.org/78033003/diff/400001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:259: ConvertUTF8ToJavaString(env, id); Nice. https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:61: private static final int DEVICE_MAX_ID = 4; On 2013/11/29 15:48:35, tommi wrote: > nit: Since this is an invalid device id and not actually the max value (which is > 3), suggest to call this DEVICE_COUNT. Done. https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:66: // localize the name strings. I would really like to pass on that one this time. Thanks. https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:118: private boolean[] mAudioDevices = new boolean[] { false, false, false, false }; > new boolean[DEVICE_MAX_ID] = ... Done! https://codereview.chromium.org/78033003/diff/400001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:535: Log.i(TAG, "Manufacturer:" + Build.MANUFACTURER + Only active when exception happens. Hope it is OK to keep for now.
still LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/78033003/450001
Message was sent while issue was closed.
Change committed as 237942
Message was sent while issue was closed.
Nice work! Sorry for the late review. https://codereview.chromium.org/78033003/diff/450001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/450001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:166: DCHECK_EQ(AudioParameters::AUDIO_PCM_LINEAR, params.format()); Would you need to SetAudioDevice() here as in MakeLowLatencyInputStream? https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:23: import java.util.ArrayList; nit: Arrays is after ArrayList. https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:255: if (mAudioDevices[id]) { I am concerned about the racing condition on accessing mAudioDevices. When the BroadcastReceiver::onReceive is called, on which thread is it? It's possibly the main thread, not the audio device thread on which getAudioInputDeviceNames is called. The reason is that JAVA is not aware of other thread. But please double check if that's true or not. https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:256: array[i] = new AudioDeviceName(id, DEVICE_NAMES[id]); DEVICE_NAMES[] contains output device names, not input ones. Are you using output device names as input ones? I think we don't need to expose input device names. Only output device names would be exposed to JS, and so would microphone mute/unmute. https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:367: filter.addAction(Intent.ACTION_HEADSET_PLUG); You can use another ctor: IntentFilter filter = new IntentFilter(Intent.ACTION_HEADSET_PLUG); https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:381: if (!action.equals(Intent.ACTION_HEADSET_PLUG)) nit: JAVA code style requires {} or the body should be on the same line of "if". https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:527: * notifies a registered state change listener (if any) that there has nit: notify.
Message was sent while issue was closed.
Thanks Wei. See https://codereview.chromium.org/99063002 for changes. https://codereview.chromium.org/78033003/diff/450001/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/78033003/diff/450001/media/audio/android/audi... media/audio/android/audio_manager_android.cc:166: DCHECK_EQ(AudioParameters::AUDIO_PCM_LINEAR, params.format()); Good question Wei. I don't know of any client that would use it actually. I will add a DCHECK for now; best I can do. OK? https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:23: import java.util.ArrayList; Done. PS, any tool (jlint?) I can use to detect things like these. I did use clank/bin/lint.py but it did not detect any issues. https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:255: if (mAudioDevices[id]) { onReceive is called on : @thread: Thread[main,5,main] and getAudioInputDeviceNames: @thread: Thread[Thread-357,5,main] hence, different threads as you state. Let me try to come up with a locking mechanism (first time using Java...) https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:256: array[i] = new AudioDeviceName(id, DEVICE_NAMES[id]); We have had a long internal (off-line) discussion about that and my initial approach was as you suggest (and what I started out documenting in my design doc). However, the first API that needs this is getSources and it does only enum the input side. We (proposal from tommi@, se design doc) then proposed to simply use the output names when enumerating the input devices and assume that it is well known that the selected mic is associated with the output device. As an example, a wired headset with mic is the same device no matter from which direction we see it. Note that the final names are not yet written in stone (not localized) either. https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:367: filter.addAction(Intent.ACTION_HEADSET_PLUG); On 2013/12/01 22:18:02, wjia wrote: > You can use another ctor: > IntentFilter filter = new IntentFilter(Intent.ACTION_HEADSET_PLUG); Done. https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:381: if (!action.equals(Intent.ACTION_HEADSET_PLUG)) On 2013/12/01 22:18:02, wjia wrote: > nit: JAVA code style requires {} or the body should be on the same line of "if". Done. https://codereview.chromium.org/78033003/diff/450001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:527: * notifies a registered state change listener (if any) that there has On 2013/12/01 22:18:02, wjia wrote: > nit: notify. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
