Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7060011:
Adding GetAudioInputDeviceNames to AudioManager, this function is supposed to do device enumerati... (Closed)
|
Created:
9 years, 7 months ago by no longer working on chromium Modified:
9 years, 6 months ago CC:
chromium-reviews, hclam+watch_chromium.org, sjl, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., acolwell GONE FROM CHROMIUM, annacc, pam+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), henrika (OOO until Aug 14), mflodman1, niklase Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdding GetAudioInputDeviceNames to AudioManager, this function is supposed to do device enumeration.
And the current version only support "default" device
Example on how to use the function is
AudioInputDeviceNames device_names;
AudioManager::GetAudioManager()->GetAudioInputDeviceNames(&device_names);
Patch Set 1 #
Total comments: 30
Patch Set 2 : '' #
Total comments: 17
Patch Set 3 : '' #
Total comments: 25
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 7
Patch Set 7 : '' #
Messages
Total messages: 12 (0 generated)
Hello Albert, This is Shijing at Stockholm. As we talked a little before, I am making for a patch for the audio input device enumeration. And I changed the design a bit and integrated it into the AudioManager instead, as the AudioManager has some existing infrastructure for the device handling. This patch adds AudioManager a function called GetAudioInputDeviceNames(AudioInputDeviceName* device_name). The implementation of the function is to check if the system has any valid input device, if it does, it returns a list containing only default device, otherwise, an empty list is returned. In Windows and Mac, I am mainly using the existing code to achieve the functionality. In linux, I make the HasAudioInputDevice() a real query to the OS, and implement the GetAudioInputDeviceNames on top. This patch, together with my next patch for AudioInputDeviceManager, are dependencies for the webrtc audio for chrome. So I am hoping to have it in ASAP. Thanks a lot. BR, /SX
here's my first set of comments. My largest concern is that it feels like you're trying to make Linux work, while doing the minimal to fix compile failures for other platforms. I realize you have time pressure, but we should avoid committing code that doesn't make sense in the larger architecture, or isn't right for other platforms. :) -Albert http://codereview.chromium.org/7060011/diff/1/media/audio/audio_device_types.h File media/audio/audio_device_types.h (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_device_types.... media/audio/audio_device_types.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. have you run lint? http://codereview.chromium.org/7060011/diff/1/media/audio/audio_device_types.... media/audio/audio_device_types.h:11: struct AudioInputDeviceName { Is there a reason to distinguish between input and output devices here? This struct seems generic to both. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... File media/audio/audio_input_device_unittest.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:10: TEST(AudioInputDeviceTest, EnumerateDevices) { Add a 1-line (keep it very short) comment explaining the assertion you are trying to validate with the unittest. Side-effect is that if you can't name it in one line, this test is probably too unfocused. ;) http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:13: AudioManager::GetAudioManager()->GetAudioInputDeviceNames(&device_names); This looks like it's going to actually query ALSA during a unittest. That's generally a bad idea since we don't know if the buildbots will have audio devices. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:15: int i(0); Most code in Chromium doesn't use the constructor syntax to initialize primitives. Doing "int i = 0;" is preferred. That being said, in this code, I suggest just keeping it out of the loop, and removeing i all together. So AIDN::const_iterator it = device_names.begin(); EXPECT_EQ("Default", it->device_name); EXPECT_EQ("Default_0", it->unique_id); ++it; for (; it != device_names.end(); ++it) { EXPECT_NE("", it->device_name); EXPECT_NE("", it->unique_id); } http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:26: DLOG(INFO) << i << " Device Name: " << it->device_name; Don't unconditionally dump output inside a unittest. Gets really annoying in the log files. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:30: device_names.clear(); device_names is a temporary...no reason to expicitly call clear() here. Remove? http://codereview.chromium.org/7060011/diff/1/media/audio/audio_manager.h File media/audio/audio_manager.h (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_manager.h#new... media/audio/audio_manager.h:51: // recording. Change to say "Appends a list of available input devices". http://codereview.chromium.org/7060011/diff/1/media/audio/audio_manager_base.h File media/audio/audio_manager_base.h (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_manager_base.... media/audio/audio_manager_base.h:19: static const char kDefaultDeviceName[]; "default" isn't cross platform...is the AudioManagerBase comment incorrect? http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:33: bool AudioManagerLinux::HasAudioInputDevices() { It's slightly confusing to me that you don't populate the AudioInputDevices list here if you're iterating and rejecting devices. If you're only pass back the device named "default", shouldn't you only search for the device named default? http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:43: // Since "default", "pulse" and "dmix" devices are virtual devices mapped to Newline before comment. http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:64: // NOTE: Do not early return from inside this if statement. The This function is long anyways. Throw the whole body of this if statement into another function that just returns has_device() and then you can keep the memory management safe from early returns. http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:103: // Destory the hint now that we're done with it. Newline before comment. http://codereview.chromium.org/7060011/diff/1/media/audio/mac/audio_manager_m... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:135: // Add the default device to the list. Does this make sense in Mac? http://codereview.chromium.org/7060011/diff/1/media/audio/win/audio_manager_w... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/win/audio_manager_w... media/audio/win/audio_manager_win.cc:265: name.device_name = AudioManagerBase::kDefaultDeviceName; ...This really seems odd now...
Thanks Albert for quick response. It is very appreciated. I changed the code based on the review feedback, please take the second run. Thanks. BR, /SX http://codereview.chromium.org/7060011/diff/1/media/audio/audio_device_types.h File media/audio/audio_device_types.h (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_device_types.... media/audio/audio_device_types.h:11: struct AudioInputDeviceName { On 2011/05/23 17:36:15, awong wrote: > Is there a reason to distinguish between input and output devices here? This > struct seems generic to both. Good point! Done. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... File media/audio/audio_input_device_unittest.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:10: TEST(AudioInputDeviceTest, EnumerateDevices) { On 2011/05/23 17:36:15, awong wrote: > Add a 1-line (keep it very short) comment explaining the assertion you are > trying to validate with the unittest. Side-effect is that if you can't name it > in one line, this test is probably too unfocused. ;) Done. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:13: AudioManager::GetAudioManager()->GetAudioInputDeviceNames(&device_names); On 2011/05/23 17:36:15, awong wrote: > This looks like it's going to actually query ALSA during a unittest. > > That's generally a bad idea since we don't know if the buildbots will have audio > devices. True. I changed the test to handle the case when there is no soundcard in the machine. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:15: int i(0); On 2011/05/23 17:36:15, awong wrote: > Most code in Chromium doesn't use the constructor syntax to initialize > primitives. Doing "int i = 0;" is preferred. > > That being said, in this code, I suggest just keeping it out of the loop, and > removeing i all together. So > > AIDN::const_iterator it = device_names.begin(); > EXPECT_EQ("Default", it->device_name); > EXPECT_EQ("Default_0", it->unique_id); > ++it; > > for (; it != device_names.end(); ++it) { > EXPECT_NE("", it->device_name); > EXPECT_NE("", it->unique_id); > } Done. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:26: DLOG(INFO) << i << " Device Name: " << it->device_name; On 2011/05/23 17:36:15, awong wrote: > Don't unconditionally dump output inside a unittest. Gets really annoying in > the log files. Done. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:30: device_names.clear(); On 2011/05/23 17:36:15, awong wrote: > device_names is a temporary...no reason to expicitly call clear() here. Remove? Done. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_manager.h File media/audio/audio_manager.h (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_manager.h#new... media/audio/audio_manager.h:51: // recording. On 2011/05/23 17:36:15, awong wrote: > Change to say "Appends a list of available input devices". Done. http://codereview.chromium.org/7060011/diff/1/media/audio/audio_manager_base.h File media/audio/audio_manager_base.h (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_manager_base.... media/audio/audio_manager_base.h:19: static const char kDefaultDeviceName[]; On 2011/05/23 17:36:15, awong wrote: > "default" isn't cross platform...is the AudioManagerBase comment incorrect? Sorry, I should have written more comments here. It is actually the design. For all the platforms, we always prepend the default device to the list, and use this default device if the users do not explicitly choose another device for the call. So this string is mainly used as a display name to the users. And we dont use it to open the device. http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:43: // Since "default", "pulse" and "dmix" devices are virtual devices mapped to On 2011/05/23 17:36:15, awong wrote: > Newline before comment. Done. http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:64: // NOTE: Do not early return from inside this if statement. The On 2011/05/23 17:36:15, awong wrote: > This function is long anyways. Throw the whole body of this if statement into > another function that just returns has_device() and then you can keep the memory > management safe from early returns. I made a helper function to do this. But I am actually not sure if the name I made fits the design style in chrome. Please give me suggestion if it doesnt look good to you. http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:103: // Destory the hint now that we're done with it. On 2011/05/23 17:36:15, awong wrote: > Newline before comment. Done. http://codereview.chromium.org/7060011/diff/1/media/audio/mac/audio_manager_m... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/mac/audio_manager_m... media/audio/mac/audio_manager_mac.cc:135: // Add the default device to the list. On 2011/05/23 17:36:15, awong wrote: > Does this make sense in Mac? We only prepend the default device to the list to display to the users. This "Default" string is only used as the name of the default device in the list, and when opening the device, we still use kAudioHardwarePropertyDefaultOutputDevice. http://codereview.chromium.org/7060011/diff/1/media/audio/win/audio_manager_w... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/win/audio_manager_w... media/audio/win/audio_manager_win.cc:265: name.device_name = AudioManagerBase::kDefaultDeviceName; On 2011/05/23 17:36:15, awong wrote: > ...This really seems odd now... We only prepend the default device to the list to display to the users. This "Default" string is only used as the name of the default device in the list, and when opening the device, we still use WAVE_MAPPER.
I added some more comments. But taking a step back here, I'm actually no longer sure what you are trying to do. It seems like what you want: 1) open the default audio device 2) bubble up information to present in the UI #1 makes sense to me. #2 makes me uneasy, especially since the main funcitonality is just to open up one device. Questions that immediately pop into head. How will we internationalize these strings? How do we standardize them for display (eg., in the UI, how many chars to you allocate)? What if the vendor or driver presents really crappy device names? What attributes and structure data is actually important to bubble up? Without a UX, or a spec about what information is needed, it feels like we're designing an API without a client. I'd rather not have this complexity without a use case. Otherwise it feels like premature generalization (an insidious form of premature optimization. :P). -Albert http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... File media/audio/audio_input_device_unittest.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/audio_input_device_... media/audio/audio_input_device_unittest.cc:13: AudioManager::GetAudioManager()->GetAudioInputDeviceNames(&device_names); On 2011/05/23 20:15:59, xians wrote: > On 2011/05/23 17:36:15, awong wrote: > > This looks like it's going to actually query ALSA during a unittest. > > > > That's generally a bad idea since we don't know if the buildbots will have > audio > > devices. > > True. I changed the test to handle the case when there is no soundcard in the > machine. Would it be possible to mock out the Alsa responses like we do in the other tests? http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... File media/audio/audio_device_types.h (right): http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... media/audio/audio_device_types.h:8: #include <list> This should all be in the media namespace. http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... media/audio/audio_device_types.h:11: struct AudioDeviceName { BTW, this struct has non-POD data members. I think this might not pass the clang trybots. Give it a run through? http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... media/audio/audio_device_types.h:12: std::string device_name; // Name of the device. If this name is meant to be a display name, it should be labeled as such. http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_input_devi... File media/audio/audio_input_device_unittest.cc (right): http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_input_devi... media/audio/audio_input_device_unittest.cc:10: // Test that devices can be enumerated. I'm going to be a bit picky about this. The comment basically says the test name. But what I was alluding to is I'm not sure what invariant you want to guarantee from your API. Reading through your last response, I think what you're testing to ensure that the first device is always "Default" regardless of what order the lower level audio layer returns things in. You can even exercise this further by adjusting the mock to send back different orderings of devices. You also have a separate test which ensure there are no empty names. These sound like 2 invariants, which might be clearer as 2 different unittests, rather than one end-to-end which isn't quite a unit... http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:50: if (error == 0) { If you invert the control flow, you can indeed early return on error, and not need to worry about keeping an extra state variable. http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:51: // NOTE: Do not early return from inside this if statement. The I think you can clean up the control flow of this function now to not need this NOTE. http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:60: hints = NULL; not necesasry to reset a local to NULL. http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:126: // Make sure we stop the thread first. If we let the default destructor to "let the default destructor to destruct" -> "allow the default destructor to destroy"
Hello Albert, Let me provide more background here. Basically what I want here is to get a list of valid devices. And the information used to present in the UI is the names of the devices, which are got from the OS except for the default device which is prepended to the list. So we dont need to worry about how those strings look like as loon as they truly reveal what they are. It is actually a really good question about the char size. We talked to the UI guys and told them we used std::string. I am not sure if they still need to know the maximum length of the string, but in case they do, 128 chars should be enough. The unique_id is used to identify the devices in the list. For example, if we have two same devices in the machines, they may have the same names, but different unique_id. Using this unique_id helps us know which device the users have chosen. Currently we only support default device, because it is not that easy to support the full list of devices due to: 1, We have to make sure all the devices in the list can be opened for recording. This gets quite difficult in ALSA as the devices can be locked by other applications or other tags which are making calls. There is no way to get a valid list unless we try opening the devices one by one. 2, What happen if the use choose a non-default device, and unplug the device during a call? We have to test the behavior before we can say it is safe. 3, How does the volume APIs behave when we open a non-default devices. ..etc. There are actually plenty of things to be sorted out before we support a full list of devices. This patch is the first step to have the infrastructure to move forward. And it may also be a good idea to keep the steps small, then we dont need to have a huge CL. BR, /SX http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:33: bool AudioManagerLinux::HasAudioInputDevices() { On 2011/05/23 17:36:15, awong wrote: > It's slightly confusing to me that you don't populate the AudioInputDevices list > here if you're iterating and rejecting devices. > > If you're only pass back the device named "default", shouldn't you only search > for the device named default? Sorry that I forgot to reply this. We cant do this because the default device isnnt always listed by snd_device_name_hint() (depending on ALSA configuration in the userspace) http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... File media/audio/audio_device_types.h (right): http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... media/audio/audio_device_types.h:8: #include <list> On 2011/05/23 21:17:58, awong wrote: > This should all be in the media namespace. Done. http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... media/audio/audio_device_types.h:11: struct AudioDeviceName { On 2011/05/23 21:17:58, awong wrote: > BTW, this struct has non-POD data members. I think this might not pass the clang > trybots. Give it a run through? I tried the Clang compiler, but dont have the access to run the Trybots, likely because I am not a committer to chrome. http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... media/audio/audio_device_types.h:12: std::string device_name; // Name of the device. On 2011/05/23 21:17:58, awong wrote: > If this name is meant to be a display name, it should be labeled as such. Done. http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:50: if (error == 0) { On 2011/05/23 21:17:58, awong wrote: > If you invert the control flow, you can indeed early return on error, and not > need to worry about keeping an extra state variable. Hold until the details here are sorted out. And I removed the comment to see if it is better to just get rid of it. http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:60: hints = NULL; On 2011/05/23 21:17:58, awong wrote: > not necesasry to reset a local to NULL. Done. http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:126: // Make sure we stop the thread first. If we let the default destructor to On 2011/05/23 21:17:58, awong wrote: > "let the default destructor to destruct" -> "allow the default destructor to > destroy" Done.
low-level code looks okay now. Before continuing though, we should discuss through how/why/if we should be bubbling up the device names. -Albert http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... File media/audio/audio_device_types.h (right): http://codereview.chromium.org/7060011/diff/8001/media/audio/audio_device_typ... media/audio/audio_device_types.h:11: struct AudioDeviceName { On 2011/05/24 13:50:27, xians wrote: > On 2011/05/23 21:17:58, awong wrote: > > BTW, this struct has non-POD data members. I think this might not pass the > clang > > trybots. Give it a run through? > > I tried the Clang compiler, but dont have the access to run the Trybots, likely > because I am not a committer to chrome. Ah...I see. I'll run it through the try bot for you. If they fail, then my expectation is that you need declare the default constructor/destructor here and put the impelmetnation with an empty body in the .cc file. Long explanation: Our Clang bots have extra extensions that enforce some style + binary-reduction rules. In particular, non-POD datatypes inside a struct/class causes the generated constructor/destructor to no longer be "trivial". Instead, they can often become inlined versions of the member constructors, which dumps a version in each .o file that the .h is listed in. http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:50: if (error == 0) { On 2011/05/24 13:50:27, xians wrote: > On 2011/05/23 21:17:58, awong wrote: > > If you invert the control flow, you can indeed early return on error, and not > > need to worry about keeping an extra state variable. > > Hold until the details here are sorted out. > And I removed the comment to see if it is better to just get rid of it. Ah...I missed that hints might need to be freed. I guess the API doesn't guarantee that hints is not allocated on failure. Looks fine as is then. http://codereview.chromium.org/7060011/diff/8001/media/audio/linux/audio_mana... media/audio/linux/audio_manager_linux.cc:51: // NOTE: Do not early return from inside this if statement. The On 2011/05/23 21:17:58, awong wrote: > I think you can clean up the control flow of this function now to not need this > NOTE. This comment is still out of date. http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_input_dev... File media/audio/audio_input_device_unittest.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_input_dev... media/audio/audio_input_device_unittest.cc:10: media::AudioDeviceNames device_names; The standard is just to throw the whole unittest into namespace media. Sometimes people even do namespace media { namespace { --- tests go here --- } // namespace } // namespace media
http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_device_ty... File media/audio/audio_device_types.h (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_device_ty... media/audio/audio_device_types.h:5: #ifndef MEDIA_AUDIO_AUDIO_DEVICE_TYPES_H_ naming nit... do you intend to add more types in here? otherwise the name of this file should match the contents (audio_device_name.h) http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_manager_b... File media/audio/audio_manager_base.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_manager_b... media/audio/audio_manager_base.cc:11: const char AudioManagerBase::kDefaultDeviceName[] = "Default"; random thought... will this have to be a localized string? http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:42: void **hints = NULL; pointers go with typename void ** --> void** http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:56: // Destory the hint now that we're done with it. Destory -> Destroy http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:199: static const char KNotWantedDefaultDevice[] = "default"; nit: K -> k http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:203: static const char kNotWantedSurroundDevice[] = "surround"; what about adding these to an array and iterating instead of having lots of constants + a really big if statement? you can loop the array via arraysize() http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:208: scoped_ptr_malloc<char> io(wrapper_->DeviceNameGetHint(*hint_iter, sanity check: DeviceNameGetHint mallocs() and we're required to free()? http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:238: // Did not find a valid device comments end w/ period http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:133: // TODO(xians): query a full list of valid devices. would it hurt to leave this as NOTIMPLEMENTED()? right now it seems like GetAudioInputDeviceNames() is a wrapper around HasAudioInputDevices() -- how soon do you expect to follow up w/ implementing audio input device name querying? http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:136: // We use (device_name)_(index) to make up the unique_ids to identify the I don't quite understand the need for unique IDs does this method of generating IDs also apply to non-default devices? if we have indices wouldn't it make sense to use an integer to specify the index? right now for a device "Foo" with 3 indices, we'd end up with the following AudioDeviceNames: { "Foo", "Foo_0" }, { "Foo", "Foo_1" }, { "Foo", "Foo_2" }, ...since we already have an AudioDeviceName struct we can probably do away with the duplicated "Foo" in the device ID Hope I'm making sense here and feel free to correct me if I'm misunderstanding things here! http://codereview.chromium.org/7060011/diff/11003/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7060011/diff/11003/media/media.gyp#newcode31 media/media.gyp:31: 'audio/audio_device_types.ḧ́', crazy "h" unicode character here
Change the code according to the review from Albert and Andrew. There are still some ongoing discussions with Andrew, and I will change the relevant code after getting feedback. Thanks, SX http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_device_ty... File media/audio/audio_device_types.h (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_device_ty... media/audio/audio_device_types.h:5: #ifndef MEDIA_AUDIO_AUDIO_DEVICE_TYPES_H_ On 2011/05/26 06:13:12, scherkus wrote: > naming nit... do you intend to add more types in here? > > otherwise the name of this file should match the contents (audio_device_name.h) Done. http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_input_dev... File media/audio/audio_input_device_unittest.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_input_dev... media/audio/audio_input_device_unittest.cc:10: media::AudioDeviceNames device_names; On 2011/05/24 23:30:32, awong wrote: > The standard is just to throw the whole unittest into namespace media. > > Sometimes people even do > > namespace media { > namespace { > > --- tests go here --- > > } // namespace > } // namespace media Done. http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_manager_b... File media/audio/audio_manager_base.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/audio_manager_b... media/audio/audio_manager_base.cc:11: const char AudioManagerBase::kDefaultDeviceName[] = "Default"; On 2011/05/26 06:13:12, scherkus wrote: > random thought... will this have to be a localized string? Not really. I dont have any personal preference on this. I did this because there are some examples chrome code doing things like this. Please just let me know if you think other way is better. http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:42: void **hints = NULL; On 2011/05/26 06:13:12, scherkus wrote: > pointers go with typename > > void ** --> void** Done. http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:56: // Destory the hint now that we're done with it. On 2011/05/26 06:13:12, scherkus wrote: > Destory -> Destroy Done. http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:199: static const char KNotWantedDefaultDevice[] = "default"; On 2011/05/26 06:13:12, scherkus wrote: > nit: K -> k Done. http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:208: scoped_ptr_malloc<char> io(wrapper_->DeviceNameGetHint(*hint_iter, On 2011/05/26 06:13:12, scherkus wrote: > sanity check: DeviceNameGetHint mallocs() a yes, there is a "if (io != NULL.." just right below. > we're required to free()? I suppose scoped_ptr_malloc should automatically free the memory. http://codereview.chromium.org/7060011/diff/11003/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:238: // Did not find a valid device On 2011/05/26 06:13:12, scherkus wrote: > comments end w/ period Done. http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:133: // TODO(xians): query a full list of valid devices. On 2011/05/26 06:13:12, scherkus wrote: > would it hurt to leave this as NOTIMPLEMENTED()? > > right now it seems like GetAudioInputDeviceNames() is a wrapper around > HasAudioInputDevices() -- how soon do you expect to follow up w/ implementing > audio input device name querying? They are actually very good questions. The story is that this GetAudioInputDeviceName is used by AudioInputDeviceManager(I am currently working on, and will submit a patch soon). So I need to prepend the default device to the list either in GetAudioInputDeviceNames or inside AudioInputDeviceManager. The reason why I prefer to do it in GetAudioInputDeviceNames is that I want to use the same function for now and the future. But I understand it is a bit against the chrome concept "dont design for the future". My current plan is that I am going to make a patch for AudioInputDeviceManager, then hooks it up to media stream and chrome recording. After that I suppose I may have time to work on this input device querying. But the risk here is that it is a good-to-have feature, and its priority might be lowered if something else jumps in. So I dont really have a concrete time for it, but I am hoping to start it soon. A question about the approach to add features in chrome, can I add a feature to some specific platforms first while putting others in a TODO list? I ask this because this device selection is easier in windows and mac, but quite complicated in linux, as we have ALSA, PulseAudio, and other sound servers mixing around there. http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:136: // We use (device_name)_(index) to make up the unique_ids to identify the On 2011/05/26 06:13:12, scherkus wrote: > I don't quite understand the need for unique IDs > > does this method of generating IDs also apply to non-default devices? > > if we have indices wouldn't it make sense to use an integer to specify the > index? > > right now for a device "Foo" with 3 indices, we'd end up with the following > AudioDeviceNames: > { "Foo", "Foo_0" }, > { "Foo", "Foo_1" }, > { "Foo", "Foo_2" }, > > ...since we already have an AudioDeviceName struct we can probably do away with > the duplicated "Foo" in the device ID > > Hope I'm making sense here and feel free to correct me if I'm misunderstanding > things here! I was also thinking about it before, but I decided not to do it because it reduce the readability. But I am open to the idea. I pasted the code here to let you decide which one is better: static const char* kNotWantedDevices[] = { "default", "null", "pulse", "dmix", "surround"}; // Now check if if it is a valid device. for (int i = 0; i < arraysize(kNotWantedDevices); ++i) { if (strncmp(kNotWantedDevices[i], hint_device_name.get(), strlen(kNotWantedDevices[i])) == 0) break; // Invalid device. else if (i == (arraysize(kNotWantedDevices) -1)) return true; // Valid device, return. } } http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:136: // We use (device_name)_(index) to make up the unique_ids to identify the On 2011/05/26 06:13:12, scherkus wrote: > I don't quite understand the need for unique IDs > > does this method of generating IDs also apply to non-default devices? > > if we have indices wouldn't it make sense to use an integer to specify the > index? > > right now for a device "Foo" with 3 indices, we'd end up with the following > AudioDeviceNames: > { "Foo", "Foo_0" }, > { "Foo", "Foo_1" }, > { "Foo", "Foo_2" }, > > ...since we already have an AudioDeviceName struct we can probably do away with > the duplicated "Foo" in the device ID > > Hope I'm making sense here and feel free to correct me if I'm misunderstanding > things here! Sorry for the bad design here. There are two ways to generate the IDs. 1, make our own internal unique id by the index. 2, get the IDs from the OS. Option 2 is slightly better than 1 because it handles the following user case: Assuming we have USB0 and USB1, they are having a same name. If the user chooses USB0, but unplugs the device before the call starts, then USB1 will become USB0 and will be used for the call if we are using index. But if we are using a string unique id from OS, we will find out the selected device does not exist any more and return an error at the early state. I confess this scenario is hidden in the corner and may not be important at all. I was trying to have the framework cover both option 1 and 2, so I made the unique id a string but indicating it can be made up by the index, and will make the decision when I get there. But right now I slightly prefer to get the IDs from the OS. How do you think here? Anyhow, it is quite a bad idea to use default_0 for the default id, 0 should be enough. I did it because it used to be the style for my team.
http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:136: // We use (device_name)_(index) to make up the unique_ids to identify the On 2011/05/26 13:54:57, xians wrote: > On 2011/05/26 06:13:12, scherkus wrote: > > I don't quite understand the need for unique IDs > > > > does this method of generating IDs also apply to non-default devices? > > > > if we have indices wouldn't it make sense to use an integer to specify the > > index? > > > > right now for a device "Foo" with 3 indices, we'd end up with the following > > AudioDeviceNames: > > { "Foo", "Foo_0" }, > > { "Foo", "Foo_1" }, > > { "Foo", "Foo_2" }, > > > > ...since we already have an AudioDeviceName struct we can probably do away > with > > the duplicated "Foo" in the device ID > > > > Hope I'm making sense here and feel free to correct me if I'm misunderstanding > > things here! > > I was also thinking about it before, but I decided not to do it because it > reduce the readability. But I am open to the idea. I pasted the code here to let > you decide which one is better: > static const char* kNotWantedDevices[] = { > "default", > "null", > "pulse", > "dmix", > "surround"}; > > // Now check if if it is a valid device. > for (int i = 0; i < arraysize(kNotWantedDevices); ++i) { > if (strncmp(kNotWantedDevices[i], > hint_device_name.get(), > strlen(kNotWantedDevices[i])) == 0) > break; // Invalid device. > else if (i == (arraysize(kNotWantedDevices) -1)) > return true; // Valid device, return. > } > } Ignore my other comment... but I do like this better :) http://codereview.chromium.org/7060011/diff/11003/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:136: // We use (device_name)_(index) to make up the unique_ids to identify the On 2011/05/26 13:54:57, xians wrote: > On 2011/05/26 06:13:12, scherkus wrote: > > I don't quite understand the need for unique IDs > > > > does this method of generating IDs also apply to non-default devices? > > > > if we have indices wouldn't it make sense to use an integer to specify the > > index? > > > > right now for a device "Foo" with 3 indices, we'd end up with the following > > AudioDeviceNames: > > { "Foo", "Foo_0" }, > > { "Foo", "Foo_1" }, > > { "Foo", "Foo_2" }, > > > > ...since we already have an AudioDeviceName struct we can probably do away > with > > the duplicated "Foo" in the device ID > > > > Hope I'm making sense here and feel free to correct me if I'm misunderstanding > > things here! > > Sorry for the bad design here. > There are two ways to generate the IDs. > 1, make our own internal unique id by the index. > 2, get the IDs from the OS. > Option 2 is slightly better than 1 because it handles the following user case: > Assuming we have USB0 and USB1, they are having a same name. > If the user chooses USB0, but unplugs the device before the call starts, then > USB1 will become USB0 and will be used for the call if we are using index. But > if we are using a string unique id from OS, we will find out the selected device > does not exist any more and return an error at the early state. > > I confess this scenario is hidden in the corner and may not be important at all. > I was trying to have the framework cover both option 1 and 2, so I made the > unique id a string but indicating it can be made up by the index, and will make > the decision when I get there. But right now I slightly prefer to get the IDs > from the OS. > How do you think here? > > Anyhow, it is quite a bad idea to use default_0 for the default id, 0 should be > enough. I did it because it used to be the style for my team. OK... what about integers? If we're enumerating? http://codereview.chromium.org/7060011/diff/22001/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/22001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:201: static const char kNotWantedSurroundDevice[] = "surround"; you missed this comment.. thoughts? "what about adding these to an array and iterating instead of having lots of constants + a really big if statement? you can loop the array via arraysize()" http://codereview.chromium.org/7060011/diff/22001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/7060011/diff/22001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:139: name.unique_id = std::string("0"); nit: "0" http://codereview.chromium.org/7060011/diff/22001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/7060011/diff/22001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.cc:265: name.unique_id = std::string("0"); nit: "0"
Just added reply to some comments. No new upload. Thanks, /SX http://codereview.chromium.org/7060011/diff/22001/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7060011/diff/22001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:201: static const char kNotWantedSurroundDevice[] = "surround"; Sorry, I wrote my replay in somewhere else by mistake. I was also thinking about it before, but I decided not to do it because it reduce the readability. But I am open to the idea. I pasted the code here to let you decide which one is better: static const char* kNotWantedDevices[] = { "default", "null", "pulse", "dmix", "surround"}; // Now check if if it is a valid device. for (int i = 0; i < arraysize(kNotWantedDevices); ++i) { if (strncmp(kNotWantedDevices[i], hint_device_name.get(), strlen(kNotWantedDevices[i])) == 0) break; // Invalid device. else if (i == (arraysize(kNotWantedDevices) -1)) return true; // Valid device, return. } } On 2011/05/27 19:10:37, scherkus wrote: > you missed this comment.. thoughts? > > "what about adding these to an array and iterating instead of having lots of > constants + a really big if statement? > > you can loop the array via arraysize()" http://codereview.chromium.org/7060011/diff/22001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/7060011/diff/22001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:139: name.unique_id = std::string("0"); I will take care of it when I am back to office on Monday. On 2011/05/27 19:10:37, scherkus wrote: > nit: "0"
Change the code to name.unique_id = "0" accordingly. http://codereview.chromium.org/7060011/diff/22001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/7060011/diff/22001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:139: name.unique_id = std::string("0"); On 2011/05/27 19:10:37, scherkus wrote: > nit: "0" Done. http://codereview.chromium.org/7060011/diff/22001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/7060011/diff/22001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.cc:265: name.unique_id = std::string("0"); On 2011/05/27 19:10:37, scherkus wrote: > nit: "0" Done.
LGTM I'll land it for you |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
