Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1275783003:
Add a virtual beamforming audio device on ChromeOS. (Closed)
|
Created:
5 years, 4 months ago by ajm Modified:
5 years, 3 months ago Reviewers:
Henrik Grunell, tommi (sloooow) - chröme, aluebs-chromium, jam, mcasas, DaleCurtis, Tom Sepez, dgreid, Charlie CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, stevenjb+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, oshima+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a virtual beamforming audio device on ChromeOS.
AudioManagerCras checks with CrasAudioHandler for an internal mic
device with a valid positions field. If present, it adds a virtual
beamforming device.
When this device is selected by a web app, the mic positions are plumbed
up to MediaStreamAudioProcessor via AudioParameters. MSAP will enable
beamforming when it sees valid mic positions.
See the design doc for background:
go/virtual-beamforming-device
BUG=497001
TEST=Selecting the beamforming device on a swanky indeed enables
beamforming for the internal mic and continues to work fine for an
external USB mic. Selecting the non-beamforming device disables
beamforming for the internal mic.
Committed: https://crrev.com/42979149ac67727de85b1e5fc5cf874cf709eb36
Cr-Commit-Position: refs/heads/master@{#348668}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : Fix more constructors and disable for now. #
Total comments: 1
Patch Set 4 : More constructors. #Patch Set 5 : More constructors. #
Total comments: 35
Patch Set 6 : Use a vector instead of a string. #
Total comments: 12
Patch Set 7 : Rebase. #
Total comments: 2
Patch Set 8 : mcasas comments #
Total comments: 29
Patch Set 9 : Fix some cross-platform errors. #
Total comments: 2
Patch Set 10 : More build errors. #Patch Set 11 : Only add virtual device if we have at least two mics. #
Total comments: 1
Patch Set 12 : Fix Chromebook not booting. #
Total comments: 6
Patch Set 13 : Rebase. #Patch Set 14 : Use gfx::Point3F. #
Total comments: 1
Patch Set 15 : Add gfx dependency. #
Total comments: 4
Patch Set 16 : gfx_test_support should depend on gfx_geometry. #Patch Set 17 : ...and gfx. #
Total comments: 4
Patch Set 18 : Remove audio_manager_openbsd. #
Total comments: 1
Patch Set 19 : Actually remove openbsd from .gyp. #Patch Set 20 : Update the beamforming device names. #Patch Set 21 : Revert chromeos/audio changes. #
Total comments: 19
Patch Set 22 : aluebs comments. #Messages
Total messages: 160 (48 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/40001
ajm@chromium.org changed reviewers: + aluebs@chromium.org, dgreid@chromium.org, grunell@chromium.org
Please have a look at the design doc first: go/virtual-beamforming-device I need to add a few more OWNERS eventually, but it would be great if you guys could have a crack at it. Henrik, I added you because of your somewhat related work on the keyboard mic stuff awhile ago. This touches a lot of files, but many are very minor due to the AudioParameters refactor. Here's a list of the files of interest: audio_device.h audio_device.cc cras_audio_handler.cc audio_input_device_manager.cc media_param_traits.cc media_stream_messages.h media_stream_request.h media_stream_audio_processor.h media_stream_audio_processor.cc media_stream_audio_processor_options.h media_stream_audio_processor_options.cc media_stream_audio_processor_unittest.cc audio_parameters.h audio_parameters.cc audio_manager_cras.h audio_manager_cras.cc
https://codereview.chromium.org/1275783003/diff/60001/media/audio/audio_param... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1275783003/diff/60001/media/audio/audio_param... media/audio/audio_parameters.h:74: int effects = NO_EFFECTS); Some of the parameters are reordered (channels <-> channel_layout, effects moved to the end) for type safety with the default parameters. That is, it's harder to use the wrong int value now.
https://codereview.chromium.org/1275783003/diff/60001/media/audio/audio_param... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1275783003/diff/60001/media/audio/audio_param... media/audio/audio_parameters.h:76: void Reset(Format format, ChannelLayout channel_layout, I could similarly reorder Reset, but I'll wait for a follow-up CL.
https://codereview.chromium.org/1275783003/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:70: {MediaAudioConstraints::kGoogBeamforming, false}, Disabled by default for now. I'll default enable in a focused follow-up.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Took a first round. Looks good in general. Could the CL be split up in smaller parts? https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:869: // TODO(ajm): This is a temporary proof-of-concept hack. These values should Do you intend to fix the todo before landing? https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... media/audio/audio_parameters.h:140: std::string mic_positions_; I think we should have some other type for this. I understand it can be any number of mics, what about use a vector of MicPosition (or such name), where struct MicPosition contains x,y,z? https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... File media/audio/audio_parameters_unittest.cc (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... media/audio/audio_parameters_unittest.cc:12: TEST(AudioParameters, Constructor_Default) { I suppose you'll add tests for the new ctors when they are finalized? https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (left): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:28: DCHECK(device_names->empty()); Why remove the dcheck? https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:35: std::string GetMicPositions() { Add comment. Can the function be called on any thread? https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.h:66: const AudioDeviceName beamforming_device_name_; Why is it restricted to only the default device? Does it make sense to abstract a device, instead of hard coding the virtual one here? https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.h:67: // Stores the mic positions field from the device. Nit: Empty line before comment.
Would it be possible to have Cras expose the virtual beamforming device? IIRC
now, I think it only exposes one device ("default"). Maybe this is not a time to
break that design.
A note about the CL description "This refactors the unwieldly AudioParameter
constructors to use now-allowed default arguments".
You are actually allowed to use default arguments in constructors.
On 2015/08/27 08:38:18, Henrik Grunell wrote: > Took a first round. Looks good in general. > > Could the CL be split up in smaller parts? Believe me, I would have liked to. I thought about splitting off the AudioParameters constructor refactoring. However, that would have required either a) adding a temporary additional constructor to AudioParameters and removing it later or b) refactoring to add a mic_positions parameter which wasn't used anywhere. I hope you're OK with this, but if you see obvious opportunities for dividing it, let me know.
On 2015/08/27 08:51:01, Henrik Grunell wrote:
> Would it be possible to have Cras expose the virtual beamforming device? IIRC
> now, I think it only exposes one device ("default"). Maybe this is not a time
to
> break that design.
If by CRAS you mean the system that sits in ChromeOS, then I don't think that's
the right choice. CRAS exposes all of the hardware devices to Chromium; it's
AudioManagerCras that decides to expose a single virtual device. And extending
that design, it's where I've put knowledge of the virtual beamforming device.
>
> A note about the CL description "This refactors the unwieldly AudioParameter
> constructors to use now-allowed default arguments".
> You are actually allowed to use default arguments in constructors.
Right, but that's a new-ish allowance which is what I meant by "now-allowed".
https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:869: // TODO(ajm): This is a temporary proof-of-concept hack. These values should On 2015/08/27 08:38:18, Henrik Grunell wrote: > Do you intend to fix the todo before landing? I wasn't going to, but it looks like Hsin-yu might land the needed CRAS CLs before this one. If not, I intend to keep it like this until they do land. https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... media/audio/audio_parameters.h:140: std::string mic_positions_; On 2015/08/27 08:38:18, Henrik Grunell wrote: > I think we should have some other type for this. I understand it can be any > number of mics, what about use a vector of MicPosition (or such name), where > struct MicPosition contains x,y,z? The reasons I want to use a string are: 1. It will be provided that way from CRAS. 2. The googArrayGeometry media constraint also uses a string; it's nice to have uniformity when parsing them in media_stream_audio_processing.cc. 3. Avoid yet-another-Point-class. We have many floating around, notably webrtc::Point which this is parsed to in MSAP. The most significant counter-argument I can see is in the case that this field is used outside of MSAP. Then we'll have to parse the string more than once. I'll think about it some more tomorrow. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.h:66: const AudioDeviceName beamforming_device_name_; On 2015/08/27 08:38:18, Henrik Grunell wrote: > Why is it restricted to only the default device? > > Does it make sense to abstract a device, instead of hard coding the virtual one > here? AudioManagerCras only exposes a single device called "Default" today. Not sure exactly what you mean by "abstract a device".
On 2015/08/27 23:10:22, ajm wrote: > On 2015/08/27 08:38:18, Henrik Grunell wrote: > > Took a first round. Looks good in general. > > > > Could the CL be split up in smaller parts? > > Believe me, I would have liked to. I thought about splitting off the > AudioParameters constructor refactoring. However, that would have required > either a) adding a temporary additional constructor to AudioParameters and > removing it later or b) refactoring to add a mic_positions parameter which > wasn't used anywhere. > > I hope you're OK with this, but if you see obvious opportunities for dividing > it, let me know. Yeah, most files a AP ctor changes. I think it's OK.
On 2015/08/28 07:26:03, ajm wrote:
> On 2015/08/27 08:51:01, Henrik Grunell wrote:
> > Would it be possible to have Cras expose the virtual beamforming device?
IIRC
> > now, I think it only exposes one device ("default"). Maybe this is not a
time
> to
> > break that design.
>
> If by CRAS you mean the system that sits in ChromeOS, then I don't think
that's
> the right choice. CRAS exposes all of the hardware devices to Chromium; it's
> AudioManagerCras that decides to expose a single virtual device. And extending
> that design, it's where I've put knowledge of the virtual beamforming device.
Oh, it's in the audio manager? My core point was to expose the new virtual
device where the current single virtual device decision is made. Then it's the
right place.
>
> >
> > A note about the CL description "This refactors the unwieldly AudioParameter
> > constructors to use now-allowed default arguments".
> > You are actually allowed to use default arguments in constructors.
>
> Right, but that's a new-ish allowance which is what I meant by "now-allowed".
Ha, I read "non-allowed". Sorry. :)
Sorry for not reviewing more today. Maybe other reviewers could take a round? https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:869: // TODO(ajm): This is a temporary proof-of-concept hack. These values should On 2015/08/28 07:36:13, ajm wrote: > On 2015/08/27 08:38:18, Henrik Grunell wrote: > > Do you intend to fix the todo before landing? > > I wasn't going to, but it looks like Hsin-yu might land the needed CRAS CLs > before this one. If not, I intend to keep it like this until they do land. Acknowledged. https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... media/audio/audio_parameters.h:140: std::string mic_positions_; On 2015/08/28 07:36:13, ajm wrote: > On 2015/08/27 08:38:18, Henrik Grunell wrote: > > I think we should have some other type for this. I understand it can be any > > number of mics, what about use a vector of MicPosition (or such name), where > > struct MicPosition contains x,y,z? > > The reasons I want to use a string are: > 1. It will be provided that way from CRAS. > 2. The googArrayGeometry media constraint also uses a string; it's nice to have > uniformity when parsing them in media_stream_audio_processing.cc. > 3. Avoid yet-another-Point-class. We have many floating around, notably > webrtc::Point which this is parsed to in MSAP. I really hope there's only one in Chrome. :) Are there several? What about using the (or "an" if several) existing Chrome point type, add a common parser function if there is none, use that at any place we want to convert? I assume you don't want to have that extra step between string and webrtc::Point, but I think that conversion should be trivial. > > The most significant counter-argument I can see is in the case that this field > is used outside of MSAP. Then we'll have to parse the string more than once. > I'll think about it some more tomorrow.
https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:13: #include "base/sys_info.h" Does this need to be guarded by #if defined(OS_CHROMEOS)? https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:872: const std::string& board = base::SysInfo::GetLsbReleaseBoard(); Does this need to be guarded by #if defined(OS_CHROMEOS)? https://codereview.chromium.org/1275783003/diff/120001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1275783003/diff/120001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:24: #include "base/sys_info.h" This is not needed anymore, right? https://codereview.chromium.org/1275783003/diff/120001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:108: const auto& geometry = ParseArrayGeometry(geometry_str); Personally I think in this case having the type spelled out it useful, but maybe it is only me. https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/audio_para... media/audio/audio_parameters.h:140: std::string mic_positions_; On 2015/08/28 15:04:34, Henrik Grunell wrote: > On 2015/08/28 07:36:13, ajm wrote: > > On 2015/08/27 08:38:18, Henrik Grunell wrote: > > > I think we should have some other type for this. I understand it can be any > > > number of mics, what about use a vector of MicPosition (or such name), where > > > struct MicPosition contains x,y,z? > > > > The reasons I want to use a string are: > > 1. It will be provided that way from CRAS. > > 2. The googArrayGeometry media constraint also uses a string; it's nice to > have > > uniformity when parsing them in media_stream_audio_processing.cc. > > 3. Avoid yet-another-Point-class. We have many floating around, notably > > webrtc::Point which this is parsed to in MSAP. > > I really hope there's only one in Chrome. :) Are there several? > > What about using the (or "an" if several) existing Chrome point type, add a > common parser function if there is none, use that at any place we want to > convert? I assume you don't want to have that extra step between string and > webrtc::Point, but I think that conversion should be trivial. > > > > > The most significant counter-argument I can see is in the case that this field > > is used outside of MSAP. Then we'll have to parse the string more than once. > > I'll think about it some more tomorrow. > I agree with Henrik, this also allows you to only surface devices which actually have a valid geometry with number of mics greater than 1 and check this in AudioManagerCras. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:63: const char kBeamformingDeviceIdSuffix[] = "-beamforming"; Since they are user-facing strings, is this the right thing for these strings? Isn't there a standard way of handling them? I am thinking specially about internationalization, etc. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:106: if (!mic_positions_.empty()) { Shouldn't we be more restrictive here? For instance check that tha mic_positions_ represents a valid geometry and has more than one mic, for example? Else we will be surfacing this device and then don't know how to handle it later. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:135: CHANNEL_LAYOUT_STEREO, kDefaultSampleRate, 16, What does CHANNEL_LAYOUT_STEREO represent? Is this the number of channels before or after AudioProcessing? Because when beamforming this is going to be different. Also , does this mean that it doesn't support higher number of channels than 2? What about mono? https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.h:66: const AudioDeviceName beamforming_device_name_; Do these really need to be part of the state of this class? Or can't you just query or concatenate them when needed?
https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:63: const char kBeamformingDeviceIdSuffix[] = "-beamforming"; On 2015/08/28 19:14:30, aluebs-chromium wrote: > Since they are user-facing strings, is this the right thing for these strings? > Isn't there a standard way of handling them? I am thinking specially about > internationalization, etc. Great question, I wonder the same thing! The existing "Default" doesn't appear to have any i8ln. Dylan do you know anything about this case? If not, we need to find someone that does.
https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:63: const char kBeamformingDeviceIdSuffix[] = "-beamforming"; On 2015/08/28 20:17:18, ajm wrote: > On 2015/08/28 19:14:30, aluebs-chromium wrote: > > Since they are user-facing strings, is this the right thing for these strings? > > Isn't there a standard way of handling them? I am thinking specially about > > internationalization, etc. > > Great question, I wonder the same thing! The existing "Default" doesn't appear > to have any i8ln. Dylan do you know anything about this case? If not, we need to > find someone that does. I don't think we ever bothered to translate "Default" because without another option it didn't matter. That might have to be revisited now.
https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:13: #include "base/sys_info.h" On 2015/08/28 19:14:29, aluebs-chromium wrote: > Does this need to be guarded by #if defined(OS_CHROMEOS)? See below. https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:872: const std::string& board = base::SysInfo::GetLsbReleaseBoard(); On 2015/08/28 19:14:30, aluebs-chromium wrote: > Does this need to be guarded by #if defined(OS_CHROMEOS)? This should only run on ChromeOS so I don't think so. The trybots don't have a problem with it. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:106: if (!mic_positions_.empty()) { On 2015/08/28 19:14:30, aluebs-chromium wrote: > Shouldn't we be more restrictive here? For instance check that tha > mic_positions_ represents a valid geometry and has more than one mic, for > example? Else we will be surfacing this device and then don't know how to handle > it later. That's a good point. I have those checks in MSAP for uniformity with the media constraint, but you're right that we shouldn't expose a beamforming device if it's not possible. My thinking was that the CRAS configs would be responsible for this. Don't provide non-empty mic positions if we don't have a beamforming capable device. If we do the string parsing here, it's a stronger argument for doing checks here as well. Will consider. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:135: CHANNEL_LAYOUT_STEREO, kDefaultSampleRate, 16, On 2015/08/28 19:14:30, aluebs-chromium wrote: > What does CHANNEL_LAYOUT_STEREO represent? Is this the number of channels before > or after AudioProcessing? Because when beamforming this is going to be > different. Also , does this mean that it doesn't support higher number of > channels than 2? What about mono? This represents the channel layout for the audio coming from the device. We'll eventually have to refactor this to support > 2 mics. We may use the CHANNEL_LAYOUT_DISCRETE format for that, which lets us specify an arbitrary number of channels. I don't want to do that in this CL though. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.h:66: const AudioDeviceName beamforming_device_name_; On 2015/08/28 19:14:30, aluebs-chromium wrote: > Do these really need to be part of the state of this class? Or can't you just > query or concatenate them when needed? They're immutable, so we're not expanding the "state-space" if you will. I don't mind computing beamforming_device_name_ on demand, but I'd prefer to leave mic_positions_ as a member to avoid the unknown complexity of querying CRAS for the device list.
https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/120001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:872: const std::string& board = base::SysInfo::GetLsbReleaseBoard(); On 2015/08/28 23:38:31, ajm wrote: > On 2015/08/28 19:14:30, aluebs-chromium wrote: > > Does this need to be guarded by #if defined(OS_CHROMEOS)? > > This should only run on ChromeOS so I don't think so. The trybots don't have a > problem with it. Great! :) https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:106: if (!mic_positions_.empty()) { On 2015/08/28 23:38:31, ajm wrote: > On 2015/08/28 19:14:30, aluebs-chromium wrote: > > Shouldn't we be more restrictive here? For instance check that tha > > mic_positions_ represents a valid geometry and has more than one mic, for > > example? Else we will be surfacing this device and then don't know how to > handle > > it later. > > That's a good point. I have those checks in MSAP for uniformity with the media > constraint, but you're right that we shouldn't expose a beamforming device if > it's not possible. > > My thinking was that the CRAS configs would be responsible for this. Don't > provide non-empty mic positions if we don't have a beamforming capable device. > > If we do the string parsing here, it's a stronger argument for doing checks here > as well. Will consider. Yes, I know it was in MSAP, but if we do it here we are protected against potential string bugs in CRAS config. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:135: CHANNEL_LAYOUT_STEREO, kDefaultSampleRate, 16, On 2015/08/28 23:38:31, ajm wrote: > On 2015/08/28 19:14:30, aluebs-chromium wrote: > > What does CHANNEL_LAYOUT_STEREO represent? Is this the number of channels > before > > or after AudioProcessing? Because when beamforming this is going to be > > different. Also , does this mean that it doesn't support higher number of > > channels than 2? What about mono? > > This represents the channel layout for the audio coming from the device. > > We'll eventually have to refactor this to support > 2 mics. We may use the > CHANNEL_LAYOUT_DISCRETE format for that, which lets us specify an arbitrary > number of channels. I don't want to do that in this CL though. Agreed that this CL is big enough. But it needs to be added at some point. https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.h (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.h:66: const AudioDeviceName beamforming_device_name_; On 2015/08/28 23:38:31, ajm wrote: > On 2015/08/28 19:14:30, aluebs-chromium wrote: > > Do these really need to be part of the state of this class? Or can't you just > > query or concatenate them when needed? > > They're immutable, so we're not expanding the "state-space" if you will. I don't > mind computing beamforming_device_name_ on demand, but I'd prefer to leave > mic_positions_ as a member to avoid the unknown complexity of querying CRAS for > the device list. Agreed.
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/140001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2015/09/02 01:18:24, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. Only full committers are accepted. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Whoops, meant to kick off a dry run.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Tiny drive-by :) https://codereview.chromium.org/1275783003/diff/140001/media/audio/audio_para... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1275783003/diff/140001/media/audio/audio_para... media/audio/audio_parameters.h:141: std::vector<Point> mic_positions_; const? https://codereview.chromium.org/1275783003/diff/140001/media/audio/audio_para... media/audio/audio_parameters.h:143: int effects_; // Bitmask using PlatformEffectsMask. const? And perhaps l.121-l.127 as well? Is strange that |mic_positions_| and |effects_| are not cleared by Reset(). https://codereview.chromium.org/1275783003/diff/140001/media/audio/audio_para... media/audio/audio_parameters.h:143: int effects_; // Bitmask using PlatformEffectsMask. const? And perhaps l.121-l.127 as well? Is strange that |mic_positions_| and |effects_| are not cleared by Reset(). https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc File media/audio/point.cc (right): https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc#n... media/audio/point.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. // Copyright 2015, with no (c) https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc#n... media/audio/point.cc:28: if (points_string.empty()) { No {} https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc#n... media/audio/point.cc:41: float_tokens.reserve(tokens.size()); std::vector<float> float_tokens(tokens.size()); https://codereview.chromium.org/1275783003/diff/160001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/1275783003/diff/160001/content/public/common/... content/public/common/media_stream_request.h:129: struct AudioDeviceParameters { A struct that contains a class? [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C...
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks for having a look Miguel. https://codereview.chromium.org/1275783003/diff/140001/media/audio/audio_para... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1275783003/diff/140001/media/audio/audio_para... media/audio/audio_parameters.h:141: std::vector<Point> mic_positions_; On 2015/09/02 03:05:37, mcasas wrote: > const? The members aren't const to allow copy assignment and Reset(). That's hinted at on l.119. https://codereview.chromium.org/1275783003/diff/140001/media/audio/audio_para... media/audio/audio_parameters.h:143: int effects_; // Bitmask using PlatformEffectsMask. On 2015/09/02 03:05:37, mcasas wrote: > const? > And perhaps l.121-l.127 as well? As above. > Is strange that |mic_positions_| and |effects_| are not cleared > by Reset(). I didn't change it because Reset() has no need to alter effects_ and mic_positions_ in all uses. I agree resetting all members would be less surprising though. Done. https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc File media/audio/point.cc (right): https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc#n... media/audio/point.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/09/02 03:05:37, mcasas wrote: > // Copyright 2015, > with no (c) Thanks, copied from an old file. https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc#n... media/audio/point.cc:41: float_tokens.reserve(tokens.size()); On 2015/09/02 03:05:37, mcasas wrote: > std::vector<float> float_tokens(tokens.size()); That will insert tokens.size() default-initialized floats, which is not what I want. https://codereview.chromium.org/1275783003/diff/160001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/1275783003/diff/160001/content/public/common/... content/public/common/media_stream_request.h:129: struct AudioDeviceParameters { On 2015/09/02 03:05:37, mcasas wrote: > A struct that contains a class? > > [1] > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C... I don't see that necessarily as a violation. AudioDeviceParameters is still a "passive object that carries data". There are plenty of existing examples: https://code.google.com/p/chromium/codesearch#search/&q=pcre:yes%20struct.*%7...
Thanks for suggesting the strongly-typed container for mic_positions_, I think it worked out well. I added a Point class and some more tests. There are a slew of irritating platform-specific failures I need to fix, but otherwise this should be in good shape for another round. https://codereview.chromium.org/1275783003/diff/180001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/1275783003/diff/180001/content/public/common/... content/public/common/media_stream_request.h:129: struct AudioDeviceParameters { This should probably _be_ a media::AudioParameters. I added a checkdeps exception for this to make that clear. I'll add a TODO on myself here if you agree, but would prefer not to make the change in this CL.
ajm@chromium.org changed reviewers: + tommi@chromium.org
Tommi, could you have a look as well? You're an owner on the majority of the files.
+Dale as FYI that AudioParameters is growing. My only concern is that we're adding an array type to AudioParameters, which is used all over the place, for a feature that doesn't exist on any device that's out there :-/ I don't suppose there's a way around that? https://codereview.chromium.org/1275783003/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:870: // come from the cras configs for each board. will this be removed before checkin? https://codereview.chromium.org/1275783003/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:872: const std::string& board = base::SysInfo::GetLsbReleaseBoard(); GetLsbReleaseBoard returns std::string (non-ref), so using ref here is a bit misleading (although compilers will do the right thing and give you reference to a temporary) https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... File components/audio_modem/audio_recorder_impl.cc (right): https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... components/audio_modem/audio_recorder_impl.cc:105: params = media::AudioParameters( is this only to clear effects? If so, can you add a comment? I think we might add a set_effects() method to AudioParameters since this is unfortunately common throughout the code base. https://codereview.chromium.org/1275783003/diff/180001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/1275783003/diff/180001/content/public/common/... content/public/common/media_stream_request.h:129: struct AudioDeviceParameters { On 2015/09/02 06:47:01, ajm wrote: > This should probably _be_ a media::AudioParameters. I added a checkdeps > exception for this to make that clear. > > I'll add a TODO on myself here if you agree, but would prefer not to make the > change in this CL. I agree. It's basically a poor man's copy of it. The reason this is here though is that we got push back for adding AudioParameters here way back when. We started out with only needing a couple of parameters, but things have grown quite a bit since then. If you'd like to fix this (not in this cl), then that would be great. https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:39: const char MediaAudioConstraints::kGoogBeamforming[] = "googBeamforming"; was this constraint added recently? If so, is it too late to change it to beamForming? (we don't want to use the 'goog' prefix) https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:54: {MediaAudioConstraints::kEchoCancellation, true}, would be nice to keep the whitespace and indent consistent as it was. https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:111: webrtc_points.reserve(webrtc_points.size()); appreciate the attention to detail :) https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:399: const std::string& constraints_geometry = nit: no ref https://codereview.chromium.org/1275783003/diff/180001/media/audio/audio_para... File media/audio/audio_parameters.cc (right): https://codereview.chromium.org/1275783003/diff/180001/media/audio/audio_para... media/audio/audio_parameters.cc:60: effects_ = NO_EFFECTS; I wonder if this will have a side effect. Can you double check that there's no code path that calls Reset() on an AudioParameters whose effects() may be something else than NO_EFFECTS? :-D hmm... I guess it would be a bug in that case anyway, so... nevermind.
ajm@chromium.org changed reviewers: + ckehoe@chromium.org
Thanks for the quick review Tommi. No new patch; responding to comments. Charlie could you look at the audio_modem change? Question for you inline. https://codereview.chromium.org/1275783003/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:870: // come from the cras configs for each board. On 2015/09/02 07:43:07, tommi wrote: > will this be removed before checkin? hychao@ is doing work to plumb this through on the ChromeOS side. Looks like that will be finished before this lands, but if not, I intend to keep this. (It doesn't make anything worse; these values were previously hard-coded in MediaStreamAudioProcessor.) https://codereview.chromium.org/1275783003/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:872: const std::string& board = base::SysInfo::GetLsbReleaseBoard(); On 2015/09/02 07:43:07, tommi wrote: > GetLsbReleaseBoard returns std::string (non-ref), so using ref here is a bit > misleading (although compilers will do the right thing and give you reference to > a temporary) Right; I had a discussion with emerican@ about this in an earlier CL: https://codereview.chromium.org/1224623014/#msg17 Given GetLsbReleaseBoard's non-ref return, there is no performance advantage of making the receiver a ref and it's a bit misleading, as you mention. The one benefit I can think of is if the return type were to change to a const ref; we'd then be making a copy unnecessarily if we changed the receiver to non-ref. What's your opinion? There are other examples of this pattern in the CL. https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... File components/audio_modem/audio_recorder_impl.cc (right): https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... components/audio_modem/audio_recorder_impl.cc:105: params = media::AudioParameters( On 2015/09/02 07:43:07, tommi wrote: > is this only to clear effects? If so, can you add a comment? I think we might > add a set_effects() method to AudioParameters since this is unfortunately common > throughout the code base. Good question. Adding a set_effects() method breaks an apparent AudioParameters convention to not support individual member mutation. See: https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... I'm sure that audio processing should be disabled for the audio_modem and I'm guessing this is an unnecessary effort to do that. Since the effects only represent the availability of platform processing, not an enabled/disabled state, removing them in no way guarantees disabling processing. ckehoe: Can you confirm? I think we should remove this line. https://codereview.chromium.org/1275783003/diff/180001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/1275783003/diff/180001/content/public/common/... content/public/common/media_stream_request.h:129: struct AudioDeviceParameters { On 2015/09/02 07:43:07, tommi wrote: > On 2015/09/02 06:47:01, ajm wrote: > > This should probably _be_ a media::AudioParameters. I added a checkdeps > > exception for this to make that clear. > > > > I'll add a TODO on myself here if you agree, but would prefer not to make the > > change in this CL. > > I agree. It's basically a poor man's copy of it. The reason this is here > though is that we got push back for adding AudioParameters here way back when. > We started out with only needing a couple of parameters, but things have grown > quite a bit since then. > > If you'd like to fix this (not in this cl), then that would be great. I'll add the TODO. https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:39: const char MediaAudioConstraints::kGoogBeamforming[] = "googBeamforming"; On 2015/09/02 07:43:07, tommi wrote: > was this constraint added recently? If so, is it too late to change it to > beamForming? (we don't want to use the 'goog' prefix) It was added awhile ago, but AFAIK no apps are using it yet. Do you have a link to the background here? I thought non-prefixed constraints should be present in the gUM draft? https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:54: {MediaAudioConstraints::kEchoCancellation, true}, On 2015/09/02 07:43:07, tommi wrote: > would be nice to keep the whitespace and indent consistent as it was. Ah sorry, I forgot to fix this after a rebase. I'd like to use clang-format on this whole block, which will result in the style you see on this line, with a four-space indent and no space inside the braces. Is that fine, or would you prefer to keep the original style? The trouble with not following clang-format is having to manually revert this block after every run. https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:111: webrtc_points.reserve(webrtc_points.size()); On 2015/09/02 07:43:07, tommi wrote: > appreciate the attention to detail :) Thanks :) https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:399: const std::string& constraints_geometry = On 2015/09/02 07:43:07, tommi wrote: > nit: no ref I agree, but would like to get your opinion on the other comment.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/09/02 07:43:07, tommi wrote: > +Dale as FYI that AudioParameters is growing. > > My only concern is that we're adding an array type to AudioParameters, which is > used all over the place, for a feature that doesn't exist on any device that's > out there :-/ I don't suppose there's a way around that? We need a way to pass audio hardware/platform information to higher layers. We could potentially split AudioParameters into AudioStreamParameters (needed all over the place) and AudioDeviceParameters (needed rarely). I'd be happy to discuss some refactoring along these lines for later consideration. On the other hand, the empty vector needed in the majority of cases consumes only (an implementation-defined, but presumably) 12 bytes.
https://codereview.chromium.org/1275783003/diff/180001/media/audio/point_unit... File media/audio/point_unittest.cc (right): https://codereview.chromium.org/1275783003/diff/180001/media/audio/point_unit... media/audio/point_unittest.cc:27: const Point point(0, 0, INFINITY); MSVC gave an "overflow in constant arithmetic" error on INFINITY here so I just removed the test. Apparently this is thrown for any floating point expression which compile-time evaluates to infinity. https://codereview.chromium.org/1275783003/diff/200001/content/common/media/m... File content/common/media/media_param_traits.cc (right): https://codereview.chromium.org/1275783003/diff/200001/content/common/media/m... content/common/media/media_param_traits.cc:63: void ParamTraits<media::Point>::Write(Message* m, const media::Point& p) { OS X has a "struct Point" in the global namespace so we need to fully qualify here. https://codereview.chromium.org/1275783003/diff/200001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/200001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:70: {MediaAudioConstraints::kGoogBeamforming, false}, Retaining false in this CL. I'll flip it on later.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Audio modem changes LGTM.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/220001
https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... File components/audio_modem/audio_recorder_impl.cc (right): https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... components/audio_modem/audio_recorder_impl.cc:105: params = media::AudioParameters( On 2015/09/02 17:00:55, ajm wrote: > On 2015/09/02 07:43:07, tommi wrote: > > is this only to clear effects? If so, can you add a comment? I think we > might > > add a set_effects() method to AudioParameters since this is unfortunately > common > > throughout the code base. > > Good question. Adding a set_effects() method breaks an apparent AudioParameters > convention to not support individual member mutation. See: > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... > > I'm sure that audio processing should be disabled for the audio_modem and I'm > guessing this is an unnecessary effort to do that. Since the effects only > represent the availability of platform processing, not an enabled/disabled > state, removing them in no way guarantees disabling processing. > > ckehoe: Can you confirm? I think we should remove this line. Charlie, to confirm, do you think we can remove this line? Or did you have some reason I'm not understanding to remove the effects?
https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/120001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:106: if (!mic_positions_.empty()) { On 2015/08/29 00:00:26, aluebs-chromium wrote: > On 2015/08/28 23:38:31, ajm wrote: > > On 2015/08/28 19:14:30, aluebs-chromium wrote: > > > Shouldn't we be more restrictive here? For instance check that tha > > > mic_positions_ represents a valid geometry and has more than one mic, for > > > example? Else we will be surfacing this device and then don't know how to > > handle > > > it later. > > > > That's a good point. I have those checks in MSAP for uniformity with the media > > constraint, but you're right that we shouldn't expose a beamforming device if > > it's not possible. > > > > My thinking was that the CRAS configs would be responsible for this. Don't > > provide non-empty mic positions if we don't have a beamforming capable device. > > > > If we do the string parsing here, it's a stronger argument for doing checks > here > > as well. Will consider. > > Yes, I know it was in MSAP, but if we do it here we are protected against > potential string bugs in CRAS config. Forgot to switch the check to > 1 mic. Done in PS#11.
https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... File components/audio_modem/audio_recorder_impl.cc (right): https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... components/audio_modem/audio_recorder_impl.cc:105: params = media::AudioParameters( On 2015/09/02 20:29:30, ajm wrote: > On 2015/09/02 17:00:55, ajm wrote: > > On 2015/09/02 07:43:07, tommi wrote: > > > is this only to clear effects? If so, can you add a comment? I think we > > might > > > add a set_effects() method to AudioParameters since this is unfortunately > > common > > > throughout the code base. > > > > Good question. Adding a set_effects() method breaks an apparent > AudioParameters > > convention to not support individual member mutation. See: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... > > > > I'm sure that audio processing should be disabled for the audio_modem and I'm > > guessing this is an unnecessary effort to do that. Since the effects only > > represent the availability of platform processing, not an enabled/disabled > > state, removing them in no way guarantees disabling processing. > > > > ckehoe: Can you confirm? I think we should remove this line. > > Charlie, to confirm, do you think we can remove this line? Or did you have some > reason I'm not understanding to remove the effects? Aren't effects set to NO_EFFECTS now by default, with the changes in audio_parameters.h?
https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... File components/audio_modem/audio_recorder_impl.cc (right): https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... components/audio_modem/audio_recorder_impl.cc:105: params = media::AudioParameters( On 2015/09/02 20:42:01, Charlie wrote: > Aren't effects set to NO_EFFECTS now by default, with the changes in > audio_parameters.h? Yes it is, so this doesn't change the existing behavior. The question Tommi raised is: why do you want to overwrite the value returned from GetInputStreamParameters?
https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... File components/audio_modem/audio_recorder_impl.cc (right): https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... components/audio_modem/audio_recorder_impl.cc:105: params = media::AudioParameters( On 2015/09/02 21:24:08, ajm wrote: > On 2015/09/02 20:42:01, Charlie wrote: > > Aren't effects set to NO_EFFECTS now by default, with the changes in > > audio_parameters.h? > > Yes it is, so this doesn't change the existing behavior. > > The question Tommi raised is: why do you want to overwrite the value returned > from GetInputStreamParameters? I think the intent was to explicitly turn off effects, in which case this code is still needed.
https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... File components/audio_modem/audio_recorder_impl.cc (right): https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... components/audio_modem/audio_recorder_impl.cc:105: params = media::AudioParameters( On 2015/09/02 23:11:09, Charlie wrote: > On 2015/09/02 21:24:08, ajm wrote: > > On 2015/09/02 20:42:01, Charlie wrote: > > > Aren't effects set to NO_EFFECTS now by default, with the changes in > > > audio_parameters.h? > > > > Yes it is, so this doesn't change the existing behavior. > > > > The question Tommi raised is: why do you want to overwrite the value returned > > from GetInputStreamParameters? > > I think the intent was to explicitly turn off effects, in which case this code > is still needed. Right, that's what I guessed in an earlier comment (sorry, should have been more clear in pointing that out). The effects field represents the availability of platform processing, not a request to enable. Setting NO_EFFECTS in no way guarantees that processing will be disabled. As a result I think this line is more misleading than anything and should be removed.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Where did you see that default arguments are allowed now? The google c++ style guide still says their disallowed.
I don't understand why we're plumbing this everywhere for a single client. Would it be cleaner as a lookup-type API where MediaStreamAudioProcessor can asynchronously look up the mic array data for a given device?
On 2015/09/03 01:13:50, DaleCurtis wrote: > Where did you see that default arguments are allowed now? The google c++ style > guide still says their disallowed. "In addition, default function parameters are allowed in constructors." https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu...
On 2015/09/03 01:19:32, DaleCurtis wrote: > I don't understand why we're plumbing this everywhere for a single client. Would > it be cleaner as a lookup-type API where MediaStreamAudioProcessor can > asynchronously look up the mic array data for a given device? The intention was to enable this feature for any client. Beamforming will work on any device where we have access to the mic positions. I admit though that we don't have a current plan to enable it outside of ChromeOS. This follows the model of AudioParameters providing everything the processor needs to know about the device/stream format. It's possible we could use a new API, but can MSAP easily query the AudioManager? I haven't looked, but will. This CL is huge, but it should provide some long-term benefit. The constructor refactoring will make future member additions to AudioParameters largely transparent.
https://codereview.chromium.org/1275783003/diff/240001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/240001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:94: mic_positions_(ParsePointsFromString(MicPositions())) { I've no idea what, but something changed in the ChromeOS image and checking MicPositions() in the constructor was now preventing my swanky from booting. Moved the check to GetAudioInputDeviceNames instead. https://codereview.chromium.org/1275783003/diff/260001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/260001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:94: beamforming_device_name_(BeamformingDeviceName()) { Alex, I reverted to storing this as a const member, as I think it's more clear that the value will never change than calling a function where it's used.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks for the link, I didn't see that caveat. I like the constructor cleanup, but I wonder if instead of default parameters we should just delete those parameters unused in most places and given them setters. We have many places where we're reseting the entire parameters struct just to set one value and the rest are copies. https://codereview.chromium.org/1275783003/diff/260001/media/audio/point.h File media/audio/point.h (right): https://codereview.chromium.org/1275783003/diff/260001/media/audio/point.h#ne... media/audio/point.h:5: #ifndef MEDIA_AUDIO_MIC_POSITIONS_H_ Header tag is not correct. Is it worth reusing ui/gfx/geometry/point3_f.h instead?
On 2015/09/03 01:58:37, DaleCurtis wrote: > Thanks for the link, I didn't see that caveat. > > I like the constructor cleanup, but I wonder if instead of default parameters we > should just delete those parameters unused in most places and given them > setters. We have many places where we're reseting the entire parameters struct > just to set one value and the rest are copies. It seemed counter to the conventions of AudioParameters (if it can be said to have such a thing), but I don't mind changing the convention. The one advantage I can see of providing them through the constructor is to support const AudioParameters, but that doesn't seem to be widely used. Should we add setters for every parameter for consistency? What about the Reset method, which then becomes redundant? I think we should instead require a copy or setting each parameter individually if you want to change it. Other reviewers, what do you think?
https://codereview.chromium.org/1275783003/diff/260001/media/audio/point.h File media/audio/point.h (right): https://codereview.chromium.org/1275783003/diff/260001/media/audio/point.h#ne... media/audio/point.h:5: #ifndef MEDIA_AUDIO_MIC_POSITIONS_H_ On 2015/09/03 01:58:37, DaleCurtis wrote: > Header tag is not correct. Is it worth reusing ui/gfx/geometry/point3_f.h > instead? It appears to do everything we need except for the IsValid method. I added that because it seemed to be desirable for IPC; if it's required we can always add a non-member function to handle it. Not sure what kind of checkdeps problems we're going to run into, but I can give it a whirl.
There's been a whole lot of reviewing going on so I've paused a bit. Seems to be some outstanding questions to resolve. Let me know when/if you want me to have another look. And what parts. https://codereview.chromium.org/1275783003/diff/260001/media/audio/point.h File media/audio/point.h (right): https://codereview.chromium.org/1275783003/diff/260001/media/audio/point.h#ne... media/audio/point.h:5: #ifndef MEDIA_AUDIO_MIC_POSITIONS_H_ On 2015/09/03 04:19:29, ajm wrote: > On 2015/09/03 01:58:37, DaleCurtis wrote: > > Header tag is not correct. Is it worth reusing ui/gfx/geometry/point3_f.h > > instead? > > It appears to do everything we need except for the IsValid method. I added that > because it seemed to be desirable for IPC; if it's required we can always add a > non-member function to handle it. > > Not sure what kind of checkdeps problems we're going to run into, but I can give > it a whirl. > If we can't use an existing point class, maybe a bug for consolidating point classes/structs in Chromium as a whole? Obviously shouldn't be done in this large CL. https://codereview.chromium.org/1275783003/diff/260001/media/audio/point_unit... File media/audio/point_unittest.cc (right): https://codereview.chromium.org/1275783003/diff/260001/media/audio/point_unit... media/audio/point_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. Remove "(c)".
https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:39: const char MediaAudioConstraints::kGoogBeamforming[] = "googBeamforming"; On 2015/09/02 17:00:55, ajm wrote: > On 2015/09/02 07:43:07, tommi wrote: > > was this constraint added recently? If so, is it too late to change it to > > beamForming? (we don't want to use the 'goog' prefix) > > It was added awhile ago, but AFAIK no apps are using it yet. > > Do you have a link to the background here? I thought non-prefixed constraints > should be present in the gUM draft? The background is that we shouldn't be adding constraints we're not trying to standardize. The constraints also need to be defined in blink and not here (see https://code.google.com/p/chromium/issues/detail?id=526734), since it's too easy to bypass things at this level that should be going through the web platform process. We've had several discussions with the blink team about this and recently had a regression when one more 'goog' constraint got added. If it's not too late to remove the prefix here, I'd really like to do that since I think that's what good citizens would do :)
https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc File media/audio/point.cc (right): https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc#n... media/audio/point.cc:41: float_tokens.reserve(tokens.size()); On 2015/09/02 06:41:48, ajm wrote: > On 2015/09/02 03:05:37, mcasas wrote: > > std::vector<float> float_tokens(tokens.size()); > > That will insert tokens.size() default-initialized floats, which is not what I > want. Actually both are the same, reserve() will also insert stuff "If case of reallocation, the storage is allocated using the container's allocator, " http://www.cplusplus.com/reference/vector/vector/reserve/
On 2015/09/03 04:19:05, ajm wrote: > On 2015/09/03 01:58:37, DaleCurtis wrote: > > Thanks for the link, I didn't see that caveat. > > > > I like the constructor cleanup, but I wonder if instead of default parameters > we > > should just delete those parameters unused in most places and given them > > setters. We have many places where we're reseting the entire parameters struct > > just to set one value and the rest are copies. > > It seemed counter to the conventions of AudioParameters (if it can be said to > have such a thing), but I don't mind changing the convention. The one advantage > I can see of providing them through the constructor is to support const > AudioParameters, but that doesn't seem to be widely used. > > Should we add setters for every parameter for consistency? What about the Reset > method, which then becomes redundant? I think we should instead require a copy > or setting each parameter individually if you want to change it. I'd just add setters in this patch set and then remove Reset() in a later patch set. I used to agree with making a copy / using reset, but after dealing with it for several years I don't think it's buying us anything other than syntactic pain. We have const for a reason if we need these values to remain unchanged :) > > Other reviewers, what do you think?
https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc File media/audio/point.cc (right): https://codereview.chromium.org/1275783003/diff/140001/media/audio/point.cc#n... media/audio/point.cc:41: float_tokens.reserve(tokens.size()); On 2015/09/03 15:29:27, mcasas wrote: > On 2015/09/02 06:41:48, ajm wrote: > > On 2015/09/02 03:05:37, mcasas wrote: > > > std::vector<float> float_tokens(tokens.size()); > > > > That will insert tokens.size() default-initialized floats, which is not what I > > want. > > Actually both are the same, reserve() will also insert stuff > "If case of reallocation, the storage is allocated using the > container's allocator, " > > http://www.cplusplus.com/reference/vector/vector/reserve/ They are most certainly not the same. reserve() allocates memory but does not insert elements. Run this example if you'd like a demonstration: #include <iostream> #include <vector> template <typename T> void PrintVector(const std::vector<T>& vec) { std::cout << "size: " << vec.size() << std::endl; std::cout << "elements: "; for (const auto& v : vec) { std::cout << v << " "; } std::cout << std::endl; } int main() { std::cout << "vec_initialized" << std::endl; std::vector<int> vec_initialized(10); PrintVector(vec_initialized); std::cout << std::endl << "vec_reserved" << std::endl; std::vector<int> vec_reserved; vec_reserved.reserve(10); PrintVector(vec_reserved); return 0; } $ ./a.out vec_initialized size: 10 elements: 0 0 0 0 0 0 0 0 0 0 vec_reserved size: 0 elements:
https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:39: const char MediaAudioConstraints::kGoogBeamforming[] = "googBeamforming"; On 2015/09/03 12:01:57, tommi wrote: > On 2015/09/02 17:00:55, ajm wrote: > > On 2015/09/02 07:43:07, tommi wrote: > > > was this constraint added recently? If so, is it too late to change it to > > > beamForming? (we don't want to use the 'goog' prefix) > > > > It was added awhile ago, but AFAIK no apps are using it yet. > > > > Do you have a link to the background here? I thought non-prefixed constraints > > should be present in the gUM draft? > > The background is that we shouldn't be adding constraints we're not trying to > standardize. The constraints also need to be defined in blink and not here (see > https://code.google.com/p/chromium/issues/detail?id=526734), since it's too easy > to bypass things at this level that should be going through the web platform > process. We've had several discussions with the blink team about this and > recently had a regression when one more 'goog' constraint got added. > If it's not too late to remove the prefix here, I'd really like to do that since > I think that's what good citizens would do :) Understood. I'm not sure we can faithfully say that we want beamforming to become a standardized constraint, so should we really un-prefix it? That suggests to me that we intend to standardize, rather than it being recognized as legacy badness. Also, this kind of breaks the model here and I'd like to know what we're going to replace it with. The custom constraints were mainly used as an easy way to control features from the apprtc URL. If that's going away, what should we switch to? Chrome command-line flags?
On 2015/09/03 20:40:48, ajm wrote: > https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... > File content/renderer/media/media_stream_audio_processor_options.cc (right): > > https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... > content/renderer/media/media_stream_audio_processor_options.cc:39: const char > MediaAudioConstraints::kGoogBeamforming[] = "googBeamforming"; > On 2015/09/03 12:01:57, tommi wrote: > > On 2015/09/02 17:00:55, ajm wrote: > > > On 2015/09/02 07:43:07, tommi wrote: > > > > was this constraint added recently? If so, is it too late to change it to > > > > beamForming? (we don't want to use the 'goog' prefix) > > > > > > It was added awhile ago, but AFAIK no apps are using it yet. > > > > > > Do you have a link to the background here? I thought non-prefixed > constraints > > > should be present in the gUM draft? > > > > The background is that we shouldn't be adding constraints we're not trying to > > standardize. The constraints also need to be defined in blink and not here > (see > > https://code.google.com/p/chromium/issues/detail?id=526734), since it's too > easy > > to bypass things at this level that should be going through the web platform > > process. We've had several discussions with the blink team about this and > > recently had a regression when one more 'goog' constraint got added. > > If it's not too late to remove the prefix here, I'd really like to do that > since > > I think that's what good citizens would do :) > > Understood. I'm not sure we can faithfully say that we want beamforming to > become a standardized constraint, so should we really un-prefix it? That > suggests to me that we intend to standardize, rather than it being recognized as > legacy badness. > > Also, this kind of breaks the model here and I'd like to know what we're going > to replace it with. The custom constraints were mainly used as an easy way to > control features from the apprtc URL. If that's going away, what should we > switch to? Chrome command-line flags? I see. Let's take if offline then and figure out the longer term solution. I.e. I don't have a good answer :)
On 2015/09/03 21:17:28, tommi wrote: > I see. Let's take if offline then and figure out the longer term solution. > I.e. I don't have a good answer :) Will do. I won't change it in this CL at least then.
On 2015/09/03 16:42:00, DaleCurtis wrote: > I'd just add setters in this patch set and then remove Reset() in a later patch > set. I used to agree with making a copy / using reset, but after dealing with it > for several years I don't think it's buying us anything other than syntactic > pain. We have const for a reason if we need these values to remain unchanged :) Took a crack at this here: https://codereview.chromium.org/1304973005/ since it's orthogonal to mic_positions. Hold off reviewing this further until we sort that out.
https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... File components/audio_modem/audio_recorder_impl.cc (right): https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... components/audio_modem/audio_recorder_impl.cc:105: params = media::AudioParameters( On 2015/09/03 00:40:13, ajm wrote: > On 2015/09/02 23:11:09, Charlie wrote: > > On 2015/09/02 21:24:08, ajm wrote: > > > On 2015/09/02 20:42:01, Charlie wrote: > > > > Aren't effects set to NO_EFFECTS now by default, with the changes in > > > > audio_parameters.h? > > > > > > Yes it is, so this doesn't change the existing behavior. > > > > > > The question Tommi raised is: why do you want to overwrite the value > returned > > > from GetInputStreamParameters? > > > > I think the intent was to explicitly turn off effects, in which case this code > > is still needed. > > Right, that's what I guessed in an earlier comment (sorry, should have been more > clear in pointing that out). > > The effects field represents the availability of platform processing, not a > request to enable. Setting NO_EFFECTS in no way guarantees that processing will > be disabled. As a result I think this line is more misleading than anything and > should be removed. Looked up the history. It was added here: https://codereview.chromium.org/1207553004/ due to problems with the ducking flag on Windows. I'm not too familiar with that, so it's possible it behaves differently than the rest (which, if true, is bad and should be fixed). In any case, in the new refactoring CL, I maintain the existing behavior, but the change makes the disabling effects intent more clear: https://codereview.chromium.org/1304973005/diff/20001/components/audio_modem/...
I've removed the ducking flag now, so maybe things have changed. I agree with Dale that reconstruction of AudioParameters in order to change a single value is proving to be ill maintainable :) On Fri, Sep 4, 2015 at 8:27 AM <ajm@chromium.org> wrote: > > > https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... > File components/audio_modem/audio_recorder_impl.cc (right): > > > https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... > components/audio_modem/audio_recorder_impl.cc:105 > <https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem...>: > params = > media::AudioParameters( > On 2015/09/03 00:40:13, ajm wrote: > > On 2015/09/02 23:11:09, Charlie wrote: > > > On 2015/09/02 21:24:08, ajm wrote: > > > > On 2015/09/02 20:42:01, Charlie wrote: > > > > > Aren't effects set to NO_EFFECTS now by default, with the > changes in > > > > > audio_parameters.h? > > > > > > > > Yes it is, so this doesn't change the existing behavior. > > > > > > > > The question Tommi raised is: why do you want to overwrite the > value > > returned > > > > from GetInputStreamParameters? > > > > > > I think the intent was to explicitly turn off effects, in which case > this code > > > is still needed. > > > Right, that's what I guessed in an earlier comment (sorry, should have > been more > > clear in pointing that out). > > > The effects field represents the availability of platform processing, > not a > > request to enable. Setting NO_EFFECTS in no way guarantees that > processing will > > be disabled. As a result I think this line is more misleading than > anything and > > should be removed. > > Looked up the history. It was added here: > https://codereview.chromium.org/1207553004/ > due to problems with the ducking flag on Windows. I'm not too familiar > with that, so it's possible it behaves differently than the rest (which, > if true, is bad and should be fixed). > > In any case, in the new refactoring CL, I maintain the existing > behavior, but the change makes the disabling effects intent more clear: > > https://codereview.chromium.org/1304973005/diff/20001/components/audio_modem/... > > https://codereview.chromium.org/1275783003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/09/04 06:52:27, tommi wrote: > I've removed the ducking flag now, so maybe things have changed. > I agree with Dale that reconstruction of AudioParameters in order to change > a single value is proving to be ill maintainable :) > > On Fri, Sep 4, 2015 at 8:27 AM <mailto:ajm@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... > > File components/audio_modem/audio_recorder_impl.cc (right): > > > > > > > https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... > > components/audio_modem/audio_recorder_impl.cc:105 > > > <https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem...>: > > params = > > media::AudioParameters( > > On 2015/09/03 00:40:13, ajm wrote: > > > On 2015/09/02 23:11:09, Charlie wrote: > > > > On 2015/09/02 21:24:08, ajm wrote: > > > > > On 2015/09/02 20:42:01, Charlie wrote: > > > > > > Aren't effects set to NO_EFFECTS now by default, with the > > changes in > > > > > > audio_parameters.h? > > > > > > > > > > Yes it is, so this doesn't change the existing behavior. > > > > > > > > > > The question Tommi raised is: why do you want to overwrite the > > value > > > returned > > > > > from GetInputStreamParameters? > > > > > > > > I think the intent was to explicitly turn off effects, in which case > > this code > > > > is still needed. > > > > > Right, that's what I guessed in an earlier comment (sorry, should have > > been more > > > clear in pointing that out). > > > > > The effects field represents the availability of platform processing, > > not a > > > request to enable. Setting NO_EFFECTS in no way guarantees that > > processing will > > > be disabled. As a result I think this line is more misleading than > > anything and > > > should be removed. > > > > Looked up the history. It was added here: > > https://codereview.chromium.org/1207553004/ > > due to problems with the ducking flag on Windows. I'm not too familiar > > with that, so it's possible it behaves differently than the rest (which, > > if true, is bad and should be fixed). > > > > In any case, in the new refactoring CL, I maintain the existing > > behavior, but the change makes the disabling effects intent more clear: > > > > > https://codereview.chromium.org/1304973005/diff/20001/components/audio_modem/... > > > > https://codereview.chromium.org/1275783003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Wait, so this CL is now defunct? Or it will just be rebased against the other one once it gets checked in?
On 2015/09/04 17:52:55, Charlie wrote: > On 2015/09/04 06:52:27, tommi wrote: > > I've removed the ducking flag now, so maybe things have changed. > > I agree with Dale that reconstruction of AudioParameters in order to change > > a single value is proving to be ill maintainable :) > > > > On Fri, Sep 4, 2015 at 8:27 AM <mailto:ajm@chromium.org> wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... > > > File components/audio_modem/audio_recorder_impl.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem... > > > components/audio_modem/audio_recorder_impl.cc:105 > > > > > > <https://codereview.chromium.org/1275783003/diff/180001/components/audio_modem...>: > > > params = > > > media::AudioParameters( > > > On 2015/09/03 00:40:13, ajm wrote: > > > > On 2015/09/02 23:11:09, Charlie wrote: > > > > > On 2015/09/02 21:24:08, ajm wrote: > > > > > > On 2015/09/02 20:42:01, Charlie wrote: > > > > > > > Aren't effects set to NO_EFFECTS now by default, with the > > > changes in > > > > > > > audio_parameters.h? > > > > > > > > > > > > Yes it is, so this doesn't change the existing behavior. > > > > > > > > > > > > The question Tommi raised is: why do you want to overwrite the > > > value > > > > returned > > > > > > from GetInputStreamParameters? > > > > > > > > > > I think the intent was to explicitly turn off effects, in which case > > > this code > > > > > is still needed. > > > > > > > Right, that's what I guessed in an earlier comment (sorry, should have > > > been more > > > > clear in pointing that out). > > > > > > > The effects field represents the availability of platform processing, > > > not a > > > > request to enable. Setting NO_EFFECTS in no way guarantees that > > > processing will > > > > be disabled. As a result I think this line is more misleading than > > > anything and > > > > should be removed. > > > > > > Looked up the history. It was added here: > > > https://codereview.chromium.org/1207553004/ > > > due to problems with the ducking flag on Windows. I'm not too familiar > > > with that, so it's possible it behaves differently than the rest (which, > > > if true, is bad and should be fixed). > > > > > > In any case, in the new refactoring CL, I maintain the existing > > > behavior, but the change makes the disabling effects intent more clear: > > > > > > > > > https://codereview.chromium.org/1304973005/diff/20001/components/audio_modem/... > > > > > > https://codereview.chromium.org/1275783003/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Wait, so this CL is now defunct? Or it will just be rebased against the other > one once it gets checked in? You mean the ducking flag? That was removed a week ago (or more?) so rebase would do it if you don't already have that change.
On 2015/09/04 17:52:55, Charlie wrote: > Wait, so this CL is now defunct? Or it will just be rebased against the other > one once it gets checked in? No I'll rebase this one against the other AudioParameters refactor CL once it crystallizes.
On 2015/09/04 17:56:59, tommi wrote: > You mean the ducking flag? > That was removed a week ago (or more?) so rebase would do it if you don't > already have that change. Charlie meant the AudioParameters CL, and I have the ducking removal here already. Given that the effects removal originated in an actual bug, I'm not going to change the behavior in these CLs. But if there's a test case that verifies it, happy to remove it later.
Do you have a reviewer yet for the content/public changes?
Patchset #13 (id:280001) has been deleted
Patchset #14 (id:320001) has been deleted
Patchset #14 (id:340001) has been deleted
ajm@chromium.org changed reviewers: + jam@chromium.org, tsepez@chromium.org
I rebased this against the AudioParameters refactor: https://codereview.chromium.org/1304973005/ significantly reducing its size and am now using gfx::Point3F. Please have another look. New reviewers, I need OWNERs approval as follows: jam: content/public/common/DEPS content/public/common/media_stream_request.cc content/public/common/media_stream_request.h tsepez: IPC review for: content/common/media/media_param_traits.cc content/common/media/media_stream_messages.h ui/gfx/ipc/gfx_param_traits.cc ui/gfx/ipc/gfx_param_traits.h Thanks! https://codereview.chromium.org/1275783003/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1275783003/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:870: // come from the cras configs for each board. On 2015/09/02 17:00:55, ajm wrote: > On 2015/09/02 07:43:07, tommi wrote: > > will this be removed before checkin? > > hychao@ is doing work to plumb this through on the ChromeOS side. Looks like > that will be finished before this lands, but if not, I intend to keep this. (It > doesn't make anything worse; these values were previously hard-coded in > MediaStreamAudioProcessor.) Now planning to remove this before landing. hychao's work is almost finished. https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:399: const std::string& constraints_geometry = On 2015/09/02 17:00:55, ajm wrote: > On 2015/09/02 07:43:07, tommi wrote: > > nit: no ref > > I agree, but would like to get your opinion on the other comment. Done. https://codereview.chromium.org/1275783003/diff/260001/media/audio/point.h File media/audio/point.h (right): https://codereview.chromium.org/1275783003/diff/260001/media/audio/point.h#ne... media/audio/point.h:5: #ifndef MEDIA_AUDIO_MIC_POSITIONS_H_ On 2015/09/03 07:14:20, Henrik Grunell wrote: > On 2015/09/03 04:19:29, ajm wrote: > > On 2015/09/03 01:58:37, DaleCurtis wrote: > > > Header tag is not correct. Is it worth reusing ui/gfx/geometry/point3_f.h > > > instead? > > > > It appears to do everything we need except for the IsValid method. I added > that > > because it seemed to be desirable for IPC; if it's required we can always add > a > > non-member function to handle it. > > > > Not sure what kind of checkdeps problems we're going to run into, but I can > give > > it a whirl. > > > > If we can't use an existing point class, maybe a bug for consolidating point > classes/structs in Chromium as a whole? Obviously shouldn't be done in this > large CL. gfx::Point3F works fine. I added a typedef below, which makes it easy to swap in a different point implementation that conforms to the same interface. On the other hand, it could make it harder to search for uses of gfx::Point3F. I can replace all media::Points with gfx::Point3Fs if you prefer. https://codereview.chromium.org/1275783003/diff/360001/media/audio/openbsd/au... File media/audio/openbsd/audio_manager_openbsd.cc (right): https://codereview.chromium.org/1275783003/diff/360001/media/audio/openbsd/au... media/audio/openbsd/audio_manager_openbsd.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. It looks like there aren't any bots building the "OS=openbsd" configuration. Should I delete this file in a follow up?
The CQ bit was checked by ajm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ckehoe@chromium.org Link to the patchset: https://codereview.chromium.org/1275783003/#ps360001 (title: "Use gfx::Point3F.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2015/09/09 01:15:32, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) Arg, meant to do a CQ dry run. Apologies.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm. nicely done. https://codereview.chromium.org/1275783003/diff/380001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/380001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:54: {MediaAudioConstraints::kEchoCancellation, true}, nit: Would be nice if we could revert all the whitespace changes here and keep the blame/praise information a little while longer. We're cleaning this up, so it can be good to use that information to go back to CLs that added them for more context. https://codereview.chromium.org/1275783003/diff/380001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:112: for (const auto& point : points) { nit: no {} as elsewhere
Messages LGTM
https://codereview.chromium.org/1275783003/diff/380001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1275783003/diff/380001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:54: {MediaAudioConstraints::kEchoCancellation, true}, On 2015/09/09 13:21:35, tommi wrote: > nit: Would be nice if we could revert all the whitespace changes here and keep > the blame/praise information a little while longer. We're cleaning this up, so > it can be good to use that information to go back to CLs that added them for > more context. Done. https://codereview.chromium.org/1275783003/diff/380001/content/renderer/media... content/renderer/media/media_stream_audio_processor_options.cc:112: for (const auto& point : points) { On 2015/09/09 13:21:35, tommi wrote: > nit: no {} as elsewhere Done.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/390001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/410001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1275783003/diff/410001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1275783003/diff/410001/media/BUILD.gn#newcode792 media/BUILD.gn:792: "//ui/gfx/geometry", Hmm, this might break NaCl, but if it passes the bots, it's fine with me. https://codereview.chromium.org/1275783003/diff/410001/media/audio/openbsd/au... File media/audio/openbsd/audio_manager_openbsd.cc (right): https://codereview.chromium.org/1275783003/diff/410001/media/audio/openbsd/au... media/audio/openbsd/audio_manager_openbsd.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Should just delete this, I don't think it even works anymore.
https://codereview.chromium.org/1275783003/diff/410001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1275783003/diff/410001/media/BUILD.gn#newcode792 media/BUILD.gn:792: "//ui/gfx/geometry", On 2015/09/09 22:51:52, DaleCurtis wrote: > Hmm, this might break NaCl, but if it passes the bots, it's fine with me. Acknowledged. https://codereview.chromium.org/1275783003/diff/410001/media/audio/openbsd/au... File media/audio/openbsd/audio_manager_openbsd.cc (right): https://codereview.chromium.org/1275783003/diff/410001/media/audio/openbsd/au... media/audio/openbsd/audio_manager_openbsd.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/09/09 22:51:52, DaleCurtis wrote: > Should just delete this, I don't think it even works anymore. Agreed, done. https://codereview.chromium.org/1275783003/diff/430001/media/audio/openbsd/au... File media/audio/openbsd/audio_manager_openbsd.cc (left): https://codereview.chromium.org/1275783003/diff/430001/media/audio/openbsd/au... media/audio/openbsd/audio_manager_openbsd.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This is deleted; not sure why Rietveld's marking it with an 'M'.
I reverted the chromeos/audio changes, which are now covered here: https://codereview.chromium.org/1320923005/ Following the discussion in crbug.com/497001, the beamforming device names are now: "Default (pick up just one person)" and "Default (pick up everything)" Adding proper localized strings shouldn't be a problem and will be done in a follow-up CL. grunell, aluebs: any more comments? jam: Can I get OWNERs approval for content/public/common? Thanks!
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/490001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
lgtm % some small comments https://codereview.chromium.org/1275783003/diff/120001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1275783003/diff/120001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:24: #include "base/sys_info.h" On 2015/08/28 19:14:30, aluebs-chromium wrote: > This is not needed anymore, right? right? https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... File content/public/common/media_stream_request.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... content/public/common/media_stream_request.cc:98: MediaStreamDevice::AudioDeviceParameters::~AudioDeviceParameters() {} Is there a reason to have an empty destructor here? https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... content/renderer/media/media_stream_audio_processor_unittest.cc:413: EXPECT_EQ(input_device_geometry, actual_geometry); Can't you use input_params.mic_positions here instead of having to define input_device_geometry above? Similar on all cases. https://codereview.chromium.org/1275783003/diff/490001/media/audio/audio_para... File media/audio/audio_parameters.cc (right): https://codereview.chromium.org/1275783003/diff/490001/media/audio/audio_para... media/audio/audio_parameters.cc:24: AudioParameters::~AudioParameters() {} Again, maybe I am missing something, but why do you need an empty constructor here? https://codereview.chromium.org/1275783003/diff/490001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/490001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:50: static const char kBeamformingOnNameSuffix[] = " (pick up just one person)"; Add a TODO to remove these hardcoded strings? https://codereview.chromium.org/1275783003/diff/490001/media/audio/point.h File media/audio/point.h (right): https://codereview.chromium.org/1275783003/diff/490001/media/audio/point.h#ne... media/audio/point.h:8: #include <cmath> Why is this include necessary here?
https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... File content/public/common/media_stream_request.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... content/public/common/media_stream_request.cc:88: can this be inlined? https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... content/public/common/media_stream_request.cc:98: MediaStreamDevice::AudioDeviceParameters::~AudioDeviceParameters() {} On 2015/09/10 02:41:39, aluebs-chromium wrote: > Is there a reason to have an empty destructor here? also wondering this..
https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... File content/public/common/media_stream_request.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... content/public/common/media_stream_request.cc:88: On 2015/09/10 19:24:19, jam wrote: > can this be inlined? Both of these are chromium-style violations due to adding the vector member. "Complex class/struct needs an explicit out-of-line {constructor,destructor}" Described a bit here: https://www.chromium.org/developers/coding-style/chromium-style-checker-errors https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... content/public/common/media_stream_request.cc:98: MediaStreamDevice::AudioDeviceParameters::~AudioDeviceParameters() {} On 2015/09/10 19:24:19, jam wrote: > On 2015/09/10 02:41:39, aluebs-chromium wrote: > > Is there a reason to have an empty destructor here? > > also wondering this.. As above. I should have added a review comment about it. https://codereview.chromium.org/1275783003/diff/490001/media/audio/audio_para... File media/audio/audio_parameters.cc (right): https://codereview.chromium.org/1275783003/diff/490001/media/audio/audio_para... media/audio/audio_parameters.cc:24: AudioParameters::~AudioParameters() {} On 2015/09/10 02:41:39, aluebs-chromium wrote: > Again, maybe I am missing something, but why do you need an empty constructor > here? chromium-style violation, as in MediaStreamDevice. https://codereview.chromium.org/1275783003/diff/490001/media/audio/audio_para... media/audio/audio_parameters.cc:27: AudioParameters& AudioParameters::operator=(const AudioParameters&) = default; Likewise, had to move these to the implementation due to: "Complex constructor has an inlined body."
https://codereview.chromium.org/1275783003/diff/120001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1275783003/diff/120001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:24: #include "base/sys_info.h" On 2015/09/10 02:41:39, aluebs-chromium wrote: > On 2015/08/28 19:14:30, aluebs-chromium wrote: > > This is not needed anymore, right? > > right? Thanks for the reminder; removed. https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... content/renderer/media/media_stream_audio_processor_unittest.cc:413: EXPECT_EQ(input_device_geometry, actual_geometry); On 2015/09/10 02:41:39, aluebs-chromium wrote: > Can't you use input_params.mic_positions here instead of having to define > input_device_geometry above? Similar on all cases. No, because input_params.mic_positions is a vector<media::Points> and input_device_geometry is a vector<webrtc::Points>. I could write an equality operator to compare them, but figured it was better to explicitly compare the same types. https://codereview.chromium.org/1275783003/diff/490001/media/audio/cras/audio... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/1275783003/diff/490001/media/audio/cras/audio... media/audio/cras/audio_manager_cras.cc:50: static const char kBeamformingOnNameSuffix[] = " (pick up just one person)"; On 2015/09/10 02:41:40, aluebs-chromium wrote: > Add a TODO to remove these hardcoded strings? Done. https://codereview.chromium.org/1275783003/diff/490001/media/audio/point.h File media/audio/point.h (right): https://codereview.chromium.org/1275783003/diff/490001/media/audio/point.h#ne... media/audio/point.h:8: #include <cmath> On 2015/09/10 02:41:40, aluebs-chromium wrote: > Why is this include necessary here? It's not any longer; removed.
The CQ bit was checked by ajm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/510001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm % with a tiny question https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... File content/public/common/media_stream_request.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/public/common/... content/public/common/media_stream_request.cc:98: MediaStreamDevice::AudioDeviceParameters::~AudioDeviceParameters() {} On 2015/09/10 19:33:04, ajm wrote: > On 2015/09/10 19:24:19, jam wrote: > > On 2015/09/10 02:41:39, aluebs-chromium wrote: > > > Is there a reason to have an empty destructor here? > > > > also wondering this.. > > As above. I should have added a review comment about it. Oh, I see. Thank you for clarifying :) https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... content/renderer/media/media_stream_audio_processor_unittest.cc:413: EXPECT_EQ(input_device_geometry, actual_geometry); On 2015/09/10 21:56:57, ajm wrote: > On 2015/09/10 02:41:39, aluebs-chromium wrote: > > Can't you use input_params.mic_positions here instead of having to define > > input_device_geometry above? Similar on all cases. > > No, because input_params.mic_positions is a vector<media::Points> and > input_device_geometry is a vector<webrtc::Points>. I could write an equality > operator to compare them, but figured it was better to explicitly compare the > same types. Good point! But maybe you can use one to get the other with the appropriate method? Or do you think being verbose here adds to the test simplicity? https://codereview.chromium.org/1275783003/diff/490001/media/audio/audio_para... File media/audio/audio_parameters.cc (right): https://codereview.chromium.org/1275783003/diff/490001/media/audio/audio_para... media/audio/audio_parameters.cc:24: AudioParameters::~AudioParameters() {} On 2015/09/10 19:33:04, ajm wrote: > On 2015/09/10 02:41:39, aluebs-chromium wrote: > > Again, maybe I am missing something, but why do you need an empty constructor > > here? > > chromium-style violation, as in MediaStreamDevice. Thanks for clarifying :)
https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... content/renderer/media/media_stream_audio_processor_unittest.cc:413: EXPECT_EQ(input_device_geometry, actual_geometry); On 2015/09/11 01:58:42, aluebs-chromium wrote: > On 2015/09/10 21:56:57, ajm wrote: > > On 2015/09/10 02:41:39, aluebs-chromium wrote: > > > Can't you use input_params.mic_positions here instead of having to define > > > input_device_geometry above? Similar on all cases. > > > > No, because input_params.mic_positions is a vector<media::Points> and > > input_device_geometry is a vector<webrtc::Points>. I could write an equality > > operator to compare them, but figured it was better to explicitly compare the > > same types. > > Good point! But maybe you can use one to get the other with the appropriate > method? Or do you think being verbose here adds to the test simplicity? I could, but that's part of the functionality I'm testing here: https://codereview.chromium.org/1275783003/diff/510001/content/renderer/media... in WebrtcPointsFromMediaPoints. Ideally we wouldn't repeat that here. I'd be willing to add the aforementioned equality operator if you prefer though.
lgtm https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1275783003/diff/490001/content/renderer/media... content/renderer/media/media_stream_audio_processor_unittest.cc:413: EXPECT_EQ(input_device_geometry, actual_geometry); On 2015/09/11 05:06:32, ajm wrote: > On 2015/09/11 01:58:42, aluebs-chromium wrote: > > On 2015/09/10 21:56:57, ajm wrote: > > > On 2015/09/10 02:41:39, aluebs-chromium wrote: > > > > Can't you use input_params.mic_positions here instead of having to define > > > > input_device_geometry above? Similar on all cases. > > > > > > No, because input_params.mic_positions is a vector<media::Points> and > > > input_device_geometry is a vector<webrtc::Points>. I could write an equality > > > operator to compare them, but figured it was better to explicitly compare > the > > > same types. > > > > Good point! But maybe you can use one to get the other with the appropriate > > method? Or do you think being verbose here adds to the test simplicity? > > I could, but that's part of the functionality I'm testing here: > https://codereview.chromium.org/1275783003/diff/510001/content/renderer/media... > > in WebrtcPointsFromMediaPoints. Ideally we wouldn't repeat that here. I'd be > willing to add the aforementioned equality operator if you prefer though. Yes, you are right. You can leave it as is.
Verified that this continues to work with the mic positions hychao plumbed up through ChromeOS. jam: could you have another look?
lgtm
The CQ bit was checked by ajm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ckehoe@chromium.org, tommi@chromium.org, tsepez@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1275783003/#ps510001 (title: "aluebs comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/510001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275783003/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275783003/510001
Message was sent while issue was closed.
Committed patchset #22 (id:510001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/42979149ac67727de85b1e5fc5cf874cf709eb36 Cr-Commit-Position: refs/heads/master@{#348668}
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/42979149ac67727de85b1e5fc5cf874cf709eb36 Cr-Commit-Position: refs/heads/master@{#348668} |
