Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8162015:
The first step to add device selection to linux. (Closed)
|
Created:
9 years, 2 months ago by no longer working on chromium Modified:
9 years, 2 months ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionThe first step to add device selection to linux.
This patch will loop through the soundcard and return a list of available devices when the AudioInputDeviceManager does the device enumeration.
Previously, only default device will be returned.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106513
Patch Set 1 #
Total comments: 48
Patch Set 2 : '' #
Total comments: 18
Patch Set 3 : rebase and update #
Messages
Total messages: 14 (0 generated)
Hello, This is patch is a follow-up to the device enumeration in linux. What it does is that it queries ALSA and returns a list of available devices. This patch does not support opening a non-default device yet, which will be done by another follow-up CL after CLs of platform-dependent device enumeration.
[-awong, +enal] Let me know if there's something specifically you want me to look at, otherwise I think it'd be better to get formal media team members to review these kind of changes.
Nice. I have a few comments. In addition, please add to the description that you have added a "proper" device detection on the output side. It would also be nice with an example output from the unit test. http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.cc File media/audio/audio_device_name.cc (right): http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.c... media/audio/audio_device_name.cc:17: } // namespace media http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.h File media/audio/audio_device_name.h (right): http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.h... media/audio/audio_device_name.h:17: std::string device_name; // Name and also display name of the device. Please make comment more clear. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:176: // Empty the name list. remove http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:182: // Pre-pend the default device to the list. Perhaps extend the comments why this is needed. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:185: AudioManagerBase::kDefaultDeviceName, "0")); indent by 4 http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:197: // Loop through the sound cards to get Alsa device hints. ALSA? http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:199: int error = wrapper_->DeviceNameHint(card, kPcmInterfaceName, &hints); Do you allocate memory here? I think you should move hint declaration inside while. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:201: // Get devices remove http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:206: hints = NULL; remove http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:208: LOG(WARNING) << "GetAudioInputDevices error: unable to get device hints: " DLOG http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:232: // Find out if the device is available. empty line before http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:313: LOG(WARNING) << "HasAnyAudioDevice warning: unable to get device hints: " DLOG http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:313: LOG(WARNING) << "HasAnyAudioDevice warning: unable to get device hints: " remove warning http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... File media/audio/linux/audio_manager_linux.h (right): http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.h:42: // Types of the stream. Comment not really needed. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.h:48: // Helper methods to query if there is any valid input device Do you mean "if there are any valid input devices"? And comments end with period (.). http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.h:49: void GetAlsaAudioInputDevices(media::AudioDeviceNames* device_names); Some more comments here perhaps?
http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.cc File media/audio/audio_device_name.cc (right): http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.c... media/audio/audio_device_name.cc:9: AudioDeviceName::AudioDeviceName() { nit: collapse this to {} http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:182: // Pre-pend the default device to the list. nit: Pre-pend -> Prepend also agree w/ henrik on explanation as to why http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:191: nit: remove blank line http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:266: strlen(kInvalidAudioInputDevices[i]))) indent by 1 more space http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:273: if (wrapper_->PcmOpen(&device_handle, device_name, how long does this take to run? The call stack is: GetAudioInputDeviceNames() GetAlsaAudioInputDevices() GetAlsaDevicesInfo() IsAlsaDeviceAvailable() // For each device PcmOpen() / PcmClose() // For each device Is it possible to execute this code from javascript by invoking WebRTC methods? http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:275: wrapper_->PcmClose(device_handle) < 0) { could you re-write this to be more explicit? using the || makes it harder to figure out the error condition if (!PcmOpen) { return false; } else if (!PcmClose) { return false; } return true; http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:293: // inside this ALSA API with libasound.so.2.0.0. do you know whether we can do a compile time/run time check? have you checked whether this is fixed upstream / filed a bug with ALSA? http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:296: if(!error) { space between if and ( http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:309: // Destroy the hints now that we're done with it. nit: blank line before this one
Changed the code based on the comments from Henrik and Andrew. http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.cc File media/audio/audio_device_name.cc (right): http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.c... media/audio/audio_device_name.cc:9: AudioDeviceName::AudioDeviceName() { On 2011/10/07 16:54:45, scherkus wrote: > nit: collapse this to {} Done. http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.c... media/audio/audio_device_name.cc:17: } On 2011/10/07 12:07:51, henrika1 wrote: > // namespace media Done. http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.h File media/audio/audio_device_name.h (right): http://codereview.chromium.org/8162015/diff/1/media/audio/audio_device_name.h... media/audio/audio_device_name.h:17: std::string device_name; // Name and also display name of the device. On 2011/10/07 12:07:51, henrika1 wrote: > Please make comment more clear. Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:176: // Empty the name list. On 2011/10/07 12:07:51, henrika1 wrote: > remove Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:182: // Pre-pend the default device to the list. On 2011/10/07 16:54:45, scherkus wrote: > nit: Pre-pend -> Prepend > > also agree w/ henrik on explanation as to why Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:182: // Pre-pend the default device to the list. On 2011/10/07 12:07:51, henrika1 wrote: > Perhaps extend the comments why this is needed. Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:185: AudioManagerBase::kDefaultDeviceName, "0")); On 2011/10/07 12:07:51, henrika1 wrote: > indent by 4 Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:191: On 2011/10/07 16:54:45, scherkus wrote: > nit: remove blank line Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:197: // Loop through the sound cards to get Alsa device hints. On 2011/10/07 12:07:51, henrika1 wrote: > ALSA? Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:199: int error = wrapper_->DeviceNameHint(card, kPcmInterfaceName, &hints); On 2011/10/07 12:07:51, henrika1 wrote: > Do you allocate memory here? > I think you should move hint declaration inside while. Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:201: // Get devices On 2011/10/07 12:07:51, henrika1 wrote: > remove Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:206: hints = NULL; On 2011/10/07 12:07:51, henrika1 wrote: > remove Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:208: LOG(WARNING) << "GetAudioInputDevices error: unable to get device hints: " On 2011/10/07 12:07:51, henrika1 wrote: > DLOG Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:232: // Find out if the device is available. On 2011/10/07 12:07:51, henrika1 wrote: > empty line before Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:266: strlen(kInvalidAudioInputDevices[i]))) On 2011/10/07 16:54:45, scherkus wrote: > indent by 1 more space Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:273: if (wrapper_->PcmOpen(&device_handle, device_name, On 2011/10/07 16:54:45, scherkus wrote: > how long does this take to run? > > The call stack is: > GetAudioInputDeviceNames() > GetAlsaAudioInputDevices() > GetAlsaDevicesInfo() > IsAlsaDeviceAvailable() // For each device > PcmOpen() / PcmClose() // For each device > I have totally 3 physical devices attached to PC, one is built-in device, another is USB headset, the last one is camera with mic. The whole call stack takes 39467us. > PcmOpen() / PcmClose() // For each device DEVICE front:CARD=Intel,DEV=0TIME open/close time: 441us DEVICE front:CARD=Q9000,DEV=0TIME open/close time: 527us DEVICE iec958:CARD=Q9000,DEV=0TIME open/close time: 628us DEVICE front:CARD=Headset,DEV=0TIME open/close time: 462us DEVICE iec958:CARD=Headset,DEV=0TIME open/close time: 675us So roughly 1ms for each device. >Is it possible to execute this code from javascript by invoking WebRTC methods? As far as I know, we don't have a specific javascript for device enumeration. But We have javascript for the MediaStream, which will execute this code. We feed MediaStream with a list of devices, and MediaStream handles the device internally like popping up a windows and letting users the select the device. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:275: wrapper_->PcmClose(device_handle) < 0) { On 2011/10/07 16:54:45, scherkus wrote: > could you re-write this to be more explicit? using the || makes it harder to > figure out the error condition > > if (!PcmOpen) { > return false; > } else if (!PcmClose) { > return false; > } > return true; Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:293: // inside this ALSA API with libasound.so.2.0.0. On 2011/10/07 16:54:45, scherkus wrote: > do you know whether we can do a compile time/run time check? > I am not sure, probably yes. But I am not sure if we will gain much by doing it, having two solutions for different versions can be quite messy. Furthermore, what snd_device_name_hint(-1 does it looping through the cards in ALSA, right now we are doing the same thing but looping them in chrome instead. The behavior should be more or less the same. > have you checked whether this is fixed upstream / filed a bug with ALSA? Sorry, I haven't. I will do it later, thanks for reminding. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:296: if(!error) { On 2011/10/07 16:54:45, scherkus wrote: > space between if and ( Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.cc:309: // Destroy the hints now that we're done with it. On 2011/10/07 16:54:45, scherkus wrote: > nit: blank line before this one Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... File media/audio/linux/audio_manager_linux.h (right): http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.h:42: // Types of the stream. On 2011/10/07 12:07:51, henrika1 wrote: > Comment not really needed. Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.h:48: // Helper methods to query if there is any valid input device On 2011/10/07 12:07:51, henrika1 wrote: > Do you mean "if there are any valid input devices"? And comments end with period > (.). Done. http://codereview.chromium.org/8162015/diff/1/media/audio/linux/audio_manager... media/audio/linux/audio_manager_linux.h:49: void GetAlsaAudioInputDevices(media::AudioDeviceNames* device_names); On 2011/10/07 12:07:51, henrika1 wrote: > Some more comments here perhaps? Done.
LGTM with nits. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:184: // We use index 0 to make up the unique_id to identify the default devices. More than one default device? http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:207: << wrapper_->StrError(error); indentation? http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:270: // device, return false if it fails either of operations. . http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:314: << wrapper_->StrError(error); indentation? http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.h (right): http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.h:50: // Get the ALSA devices' information like names and ids. Nit. We only get name and ID here. Not clear. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.h:56: // Query if there is any valid ALSA input device? ??
Hi Andrew and Eugene, any update on this CL?
LGTM w/ nits http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:275: else if (wrapper_->PcmClose(device_handle)) nit: no need for else http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:284: static const char* kNotWantedDevice = what happens if you call this with kStreamPlayback then kStreamCapture? being static won't its value remain constant? http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.h (right): http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.h:56: // Query if there is any valid ALSA input device? this comment would be clearer if stated as "Returns true if a device is present for the given stream type."
Thanks guys so much. I have already updated the code based on comments from Andrew and Henrik, and will commit it soon. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:184: // We use index 0 to make up the unique_id to identify the default devices. On 2011/10/13 09:41:17, henrika1 wrote: > More than one default device? Done. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:207: << wrapper_->StrError(error); On 2011/10/13 09:41:17, henrika1 wrote: > indentation? Done. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:270: // device, return false if it fails either of operations. On 2011/10/13 09:41:17, henrika1 wrote: > . Done. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:275: else if (wrapper_->PcmClose(device_handle)) On 2011/10/18 23:22:51, scherkus wrote: > nit: no need for else Done. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:284: static const char* kNotWantedDevice = On 2011/10/18 23:22:51, scherkus wrote: > what happens if you call this with kStreamPlayback then kStreamCapture? > > being static won't its value remain constant? Oh, sorry, my mistake. Thanks. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:314: << wrapper_->StrError(error); On 2011/10/13 09:41:17, henrika1 wrote: > indentation? Done. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.h (right): http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.h:50: // Get the ALSA devices' information like names and ids. On 2011/10/13 09:41:17, henrika1 wrote: > Nit. We only get name and ID here. Not clear. Done. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.h:56: // Query if there is any valid ALSA input device? On 2011/10/18 23:22:51, scherkus wrote: > this comment would be clearer if stated as "Returns true if a device is present > for the given stream type." Done. http://codereview.chromium.org/8162015/diff/12002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.h:56: // Query if there is any valid ALSA input device? On 2011/10/13 09:41:17, henrika1 wrote: > ?? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8162015/21001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8162015/21001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is win_rel, revision is 106488, job name was 8162015-21001 (retry) (retry) (retry) (retry).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8162015/21001
Change committed as 106513 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
