Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
227743004:
Added a kEchoCancellation constraint to turn off the audio processing. (Closed)
|
Created:
6 years, 8 months ago by no longer working on chromium Modified:
6 years, 7 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdded a kEchoCancellation constraint to turn off the audio processing. Also, change the the existing code not to use the libjingle interfaces.
BUG=264611
TEST=content_unittests --getst_filter="*MediaStreamAudioProcessor*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270728
Patch Set 1 : #Patch Set 2 : #
Total comments: 59
Patch Set 3 : addressed the comments. #
Total comments: 25
Patch Set 4 : moved some of the constraints code to MediaAudioConstraints class, and hope it makes the code more … #
Total comments: 19
Patch Set 5 : addressed Per's comments. #
Total comments: 45
Patch Set 6 : addressed the comments and added more unittests #Patch Set 7 : added missing new unittest #
Total comments: 5
Patch Set 8 : added a GetEchoCancellationProperty method. #
Total comments: 1
Patch Set 9 : GetEchoCancellationProperty handles both kEchoCancellation and kGoogEchoCancellation #
Total comments: 26
Patch Set 10 : addressed Tommi's comments. #
Total comments: 6
Patch Set 11 : minor changes: adding CONTENT_EXPORT, KSourceId to fix all the bots #
Total comments: 3
Patch Set 12 : addressed the comments. #Patch Set 13 : when kEchoCancellation is false, default values of all goog* constraints are false, added unittests #
Total comments: 15
Patch Set 14 : addressed Tommi's comments. #
Total comments: 2
Patch Set 15 : #
Total comments: 2
Patch Set 16 : #Messages
Total messages: 41 (0 generated)
Hi Per, could you please review this CL. Tommi, feel free to take a look if you have time. Thanks, SX
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:327: if (!echo_calcellation || The main change is here, echo_calcellation is true by default, if it is false, we turn off the audio processing. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:78: void ApplyFixedAudioConstraints(RTCMediaConstraints* constraints) { These two methods are still used in WebRtcAudioCapturer and MediaStreamDependencyFactory for the old path, we can't remove them until the old path goes away.
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:293: const bool echo_calcellation = GetPropertyFromConstraints( nit: move to where its used. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:327: if (!echo_calcellation || fix name echo_calcellation https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:62: MediaStreamType type) { You can read the type from the original constrains instead. Not sure if that is better or not... https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:113: return false; What does this mean? Can echo cancellation on/off not be controlled from js? https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.h (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.h:11: #include "content/public/common/media_stream_request.h" this seems like a weird inclusion. This is not for a new request right and should probably not be dependent on classes used for requesting media devices from the browser process. Is MediaStreamType the correct type selector? https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.h:35: // Specified by draft-alvestrand-constraints-resolution-00b. No- this comment is wrong. this draft has nothing to do with these constraints. Document them please. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_unittest.cc:305: Add test to see that gum fail if an unsupported mandatory constraint is specified?
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:293: const bool echo_calcellation = GetPropertyFromConstraints( echo_cancellation (not calcellation) Actually, I'm not sure this is the desired behavior. We want echo cancellation to be turned on by default. If this constraint is missing, the echo_cancellation will be set to false which is incorrect. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:301: const bool enable_aec = GetPropertyFromConstraints( can you change the name of this variable to e.g. goog_aec or goog_echo_cancellation to properly distinguish it from the variable above? Do we actually need to call GetPropertyFromConstraints if echo_cancellation is false? It feels like this could be: const bool enable_aec = echo_cancellation && GetPropertyFromConstraints(...); https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:303: const bool enable_agc = GetPropertyFromConstraints( same here and below. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:324: // |echo_calcellation| is used as a master control on enabling/disabling fix typo here as well https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:327: if (!echo_calcellation || On 2014/04/08 10:02:53, xians1 wrote: > The main change is here, echo_calcellation is true by default, if it is false, > we turn off the audio processing. I think that we're unnecessarily querying all the properties in the case where echo_cancellation is false. However, there's also a bug above which sets echo_cancellation to false if the constraint is not present. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:55: { content::kMediaStreamAudioDucking, true }, remove content:: prefix here and in the line below. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:57: { content::kMediaStreamAudioDucking, false }, why adding this? https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:64: if (type != MEDIA_DEVICE_AUDIO_CAPTURE) the name of the function is GetDefaultValueForConstraint so this check seems a bit strange. Is it needed and is the MediaStreamType parameter really needed since we're in the audio processor source file? https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:72: return false; I'm a bit wary of this sort of implementation. I'd rather that the function accepts a default in the case where the constraint is not found and then return the default here. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:84: base::IntToString(kDefaultAudioConstraints[i].value), false); ugh, isn't this is a big change in functionality? We can't start mixing integers and strings and then feed either user provided values or generated values through the same parse routines. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:110: if (effects & media::AudioParameters::ECHO_CANCELLER && Please get Andrew to review this. The name of this method is GetPropertyFromConstraints and that does not reflect this sort of special casing. At the very least this check is in the wrong place and I think that there was previously a better way to do what you want. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:118: bool value = GetDefaultValueForConstraint(key, type); better to do: if (!GetConstraintValue(constraints, key, &value)) value = GetDefaultValueForConstraint(key, type); https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_unittest.cc:264: kEchoCancellation, fix indent https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_unittest.cc:279: constraint_factory.AddMandatory(kDefaultAudioConstraints[i], false); looks like you're exchanging bool for what used to be a string... https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:21: *value = static_cast<bool>(constraint_value); Are you sure this works? The values of these constraints have previously been checked as strings with kValueTrue and kValueFalse. Now it looks like you're attempting to convert that to an int and from there to a bool... https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:32: bool GetManatoryConstraintValue(const blink::WebMediaConstraints& constraints, GetMandatoryConstraintValue (missing 'd') https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:40: base::StringToInt(value_str.utf8(), value); return base::StringToInt(value_str.utf8(), value); so that if the value isn't an int, we will report an error. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:49: value_str)) {} https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:52: base::StringToInt(value_str.utf8(), value); return base::StringToInt(value_str.utf8(), value); so that if the value isn't an int, we will report an error. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_constraints_util.h (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.h:33: bool GetManatoryConstraintValue(const blink::WebMediaConstraints& constraints, fix typo https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_video_source.cc:314: GetManatoryConstraintValue(constraints, kMaxWidth, &max_requested_width); typo (here and throughout)
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:293: const bool echo_calcellation = GetPropertyFromConstraints( On 2014/04/08 10:28:12, perkj wrote: > nit: move to where its used. Added code to return immediately when the echo_cancellation is false. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:293: const bool echo_calcellation = GetPropertyFromConstraints( On 2014/04/08 10:49:30, tommi wrote: > echo_cancellation (not calcellation) > Done. > Actually, I'm not sure this is the desired behavior. > > We want echo cancellation to be turned on by default. If this constraint is > missing, the echo_cancellation will be set to false which is incorrect. echo_cancellation will be true if the constraint is missing. The default value is specified in media_stream_audio_processor_options.cc::kDefaultAudioConstraints https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:301: const bool enable_aec = GetPropertyFromConstraints( On 2014/04/08 10:49:30, tommi wrote: > can you change the name of this variable to e.g. goog_aec or > goog_echo_cancellation to properly distinguish it from the variable above? > Done. > Do we actually need to call GetPropertyFromConstraints if echo_cancellation is > false? > kGoogEchoCancellation can go away if we don't support disabling AEC while enabling other components. Otherwise we will have to keep it. what do you think here? > It feels like this could be: > const bool enable_aec = echo_cancellation && GetPropertyFromConstraints(...); We can't do this until our client (M*) specify kEchoCancellation to be false in studio mode, I added a TODO to remind it. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:303: const bool enable_agc = GetPropertyFromConstraints( On 2014/04/08 10:49:30, tommi wrote: > same here and below. Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:324: // |echo_calcellation| is used as a master control on enabling/disabling On 2014/04/08 10:49:30, tommi wrote: > fix typo here as well Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:327: if (!echo_calcellation || On 2014/04/08 10:28:12, perkj wrote: > fix name echo_calcellation Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:327: if (!echo_calcellation || On 2014/04/08 10:49:30, tommi wrote: > On 2014/04/08 10:02:53, xians1 wrote: > > The main change is here, echo_calcellation is true by default, if it is > false, > > we turn off the audio processing. > > I think that we're unnecessarily querying all the properties in the case where > echo_cancellation is false. However, there's also a bug above which sets > echo_cancellation to false if the constraint is not present. Done with avoid querying the constraints when echo_cancellation is false. FYI, the previous code was to avoid duplicating RecordProcessingState(AUDIO_PROCESSING_DISABLED). https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:55: { content::kMediaStreamAudioDucking, true }, On 2014/04/08 10:49:30, tommi wrote: > remove content:: prefix here and in the line below. Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:57: { content::kMediaStreamAudioDucking, false }, On 2014/04/08 10:49:30, tommi wrote: > why adding this? I think it is good to explicitly define the default values to all constraints in one place, and we know exactly what will be set to true or false. If you disagree, I can fall back to the previous code, which implicitly sets to false if the key could not be found in kDefaultAudioConstraints. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:62: MediaStreamType type) { On 2014/04/08 10:28:12, perkj wrote: > You can read the type from the original constrains instead. Not sure if that is > better or not... Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:64: if (type != MEDIA_DEVICE_AUDIO_CAPTURE) On 2014/04/08 10:49:30, tommi wrote: > the name of the function is GetDefaultValueForConstraint so this check seems a > bit strange. Is it needed and is the MediaStreamType parameter really needed > since we're in the audio processor source file? Per pointed out that we should be able to get the type from the constraints, so I remove the type. But we still need to check if that is from default source or not, since for gUM of tab source or system source, the default value will be false. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:72: return false; On 2014/04/08 10:49:30, tommi wrote: > I'm a bit wary of this sort of implementation. I'd rather that the function > accepts a default in the case where the constraint is not found and then return > the default here. Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:84: base::IntToString(kDefaultAudioConstraints[i].value), false); On 2014/04/08 10:49:30, tommi wrote: > ugh, isn't this is a big change in functionality? We can't start mixing > integers and strings and then feed either user provided values or generated > values through the same parse routines. Right, we have to keep using the webrtc::MediaConstraintsInterface::kValueXXX https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:110: if (effects & media::AudioParameters::ECHO_CANCELLER && On 2014/04/08 10:49:30, tommi wrote: > Please get Andrew to review this. > Will do. > The name of this method is GetPropertyFromConstraints and that does not reflect > this sort of special casing. At the very least this check is in the wrong place > and I think that there was previously a better way to do what you want. I agree. Previously we mapped the blink constraints to a writable RTCMediaConstraints, and modified the AEC constraint if effect was on, but we can't do it any more with the new code since we need to deal with the non-writable blink constraint directly. I will check with Andrew to see if we can have another better alternative here. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:113: return false; On 2014/04/08 10:28:12, perkj wrote: > What does this mean? Can echo cancellation on/off not be controlled from js? Yes, when the effect is on, we need to disable the software AEC. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:118: bool value = GetDefaultValueForConstraint(key, type); On 2014/04/08 10:49:30, tommi wrote: > better to do: > > if (!GetConstraintValue(constraints, key, &value)) > value = GetDefaultValueForConstraint(key, type); Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.h (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.h:11: #include "content/public/common/media_stream_request.h" On 2014/04/08 10:28:12, perkj wrote: > this seems like a weird inclusion. This is not for a new request right and > should probably not be dependent on classes used for requesting media devices > from the browser process. > > Is MediaStreamType the correct type selector? Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.h:35: // Specified by draft-alvestrand-constraints-resolution-00b. On 2014/04/08 10:28:12, perkj wrote: > No- this comment is wrong. this draft has nothing to do with these constraints. > Document them please. removed. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_unittest.cc:264: kEchoCancellation, On 2014/04/08 10:49:30, tommi wrote: > fix indent Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_unittest.cc:279: constraint_factory.AddMandatory(kDefaultAudioConstraints[i], false); On 2014/04/08 10:49:30, tommi wrote: > looks like you're exchanging bool for what used to be a string... MockMediaConstraintFactory::AddMandatory takes int or double as input. I guess you meant RTCMediaConstraints, which takes string as input. Some background information, RTCMediaConstraints is the helper class to parse blink constraints to libjingle constraints, it takes strings as input. While blink::WebMediaConstraints tales int or double converted strings as values. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_unittest.cc:305: On 2014/04/08 10:28:12, perkj wrote: > Add test to see that gum fail if an unsupported mandatory constraint is > specified? Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:21: *value = static_cast<bool>(constraint_value); On 2014/04/08 10:49:30, tommi wrote: > Are you sure this works? > > The values of these constraints have previously been checked as strings with > kValueTrue and kValueFalse. Now it looks like you're attempting to convert that > to an int and from there to a bool... Yes, the value is stored as blink::WebString in blink, and we need to convert it to boolean (audio constraints) or int (video constraints). We have been doing this for long time. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:32: bool GetManatoryConstraintValue(const blink::WebMediaConstraints& constraints, On 2014/04/08 10:49:30, tommi wrote: > GetMandatoryConstraintValue > (missing 'd') Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:40: base::StringToInt(value_str.utf8(), value); On 2014/04/08 10:49:30, tommi wrote: > return base::StringToInt(value_str.utf8(), value); > > so that if the value isn't an int, we will report an error. Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:49: value_str)) On 2014/04/08 10:49:30, tommi wrote: > {} Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.cc:52: base::StringToInt(value_str.utf8(), value); On 2014/04/08 10:49:30, tommi wrote: > return base::StringToInt(value_str.utf8(), value); > > so that if the value isn't an int, we will report an error. Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_constraints_util.h (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_constraints_util.h:33: bool GetManatoryConstraintValue(const blink::WebMediaConstraints& constraints, On 2014/04/08 10:49:30, tommi wrote: > fix typo Done. https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_video_source.cc:314: GetManatoryConstraintValue(constraints, kMaxWidth, &max_requested_width); On 2014/04/08 10:49:30, tommi wrote: > typo (here and throughout) Done. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:115: return false; Andrew, could you please take a look at this? Previously, we mapped a const blink::WebMediaConstraints object into a writable RTCMediaConstraints object, so we could modify the RTC constraints when the effect was enabled, but we can't do it any more, so I simply return false for kGoogEchoCancellation when the effect is enabled.
https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:163: goog_audio_mirroring_(false), why prefix with goog? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:305: // TODO(xians): goog_aec should be just echo_cancellation. ??? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:189: // It can be accessed by the capture audio thread and by the libjingle thread is this still true? is libjingle thread involved? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:63: // Audio processing is false for gUM of non-default kMediaStreamSource. ? of non-default ? isn't kMediaStreamSource always true for tab capture. Comment seems to be misleading. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:115: return false; On 2014/04/11 08:56:30, xians1 wrote: > Andrew, could you please take a look at this? > > Previously, we mapped a const blink::WebMediaConstraints object into a writable > RTCMediaConstraints object, so we could modify the RTC constraints when the > effect was enabled, but we can't do it any more, so I simply return false for > kGoogEchoCancellation when the effect is enabled. Forgot to add Andrew? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.h (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:60: // Returns true if the key is found and has a valid boolean value; Otherwise "and has a valid boolean value that is true." I think this is a bit confusing. How about having the return value mean that the key exist and have an out parameter with the actual value of it? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:61: // false. document |effects| https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:62: bool GetPropertyFromConstraints( I think you have to but these functions in class scope. The names are two general to add as a function in content. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:69: bool IsValid(const blink::WebMediaConstraints& constraints); dito - this is audio specific. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:282: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDefaultAudioConstraints); ++i) { why not array_size() ? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_local_audio_track_adapter_unittest.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/webrtc/webrtc_local_audio_track_adapter_unittest.cc:5: #include "content/renderer/media/mock_media_constraint_factory.h" up to you but you don't need this factory to create an empty blink constraint. just do blink::WebMediaStreamConstraints constraints; constraints.initialize(); here and elsewhere.
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:84: base::IntToString(kDefaultAudioConstraints[i].value), false); On 2014/04/11 08:56:30, xians1 wrote: > On 2014/04/08 10:49:30, tommi wrote: > > ugh, isn't this is a big change in functionality? We can't start mixing > > integers and strings and then feed either user provided values or generated > > values through the same parse routines. > > Right, we have to keep using the webrtc::MediaConstraintsInterface::kValueXXX I don't think you understand my comments. You're changing "True" to "1" here. They are not the same. If the constraint exists and is set to "True" or "False" you cannot convert that to an integer in the same way you'd convert "1" and "0".
Hi Per and Tommi, PTAL. Andrew, could you please take a look at media_stream_audio_processor_options.cc::GetProperty()? And please feel free to review other part of code as well. Thanks. SX https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:163: goog_audio_mirroring_(false), On 2014/04/11 11:45:04, perkj wrote: > why prefix with goog? They are from goog* constraints, and Tommi suggested: can you change the name of this variable to e.g. goog_aec or goog_echo_cancellation to properly distinguish it from the variable above? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:305: // TODO(xians): goog_aec should be just echo_cancellation. On 2014/04/11 11:45:04, perkj wrote: > ??? I am thinking that kEchoCancellation can replace kGoogEchoCancellation. Or do you think we need to keep kGoogEchoCancellation? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:189: // It can be accessed by the capture audio thread and by the libjingle thread On 2014/04/11 11:45:04, perkj wrote: > is this still true? is libjingle thread involved? Yes, by GetStats() from libjingle. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:63: // Audio processing is false for gUM of non-default kMediaStreamSource. On 2014/04/11 11:45:04, perkj wrote: > ? of non-default ? isn't kMediaStreamSource always true for tab capture. > Comment seems to be misleading. changed the comment to // The default audio processing is false for gUM with a specific kMediaStreamSource. wdyt? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:115: return false; On 2014/04/11 11:45:04, perkj wrote: > On 2014/04/11 08:56:30, xians1 wrote: > > Andrew, could you please take a look at this? > > > > Previously, we mapped a const blink::WebMediaConstraints object into a > writable > > RTCMediaConstraints object, so we could modify the RTC constraints when the > > effect was enabled, but we can't do it any more, so I simply return false for > > kGoogEchoCancellation when the effect is enabled. > > Forgot to add Andrew? Thanks. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.h (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:60: // Returns true if the key is found and has a valid boolean value; Otherwise On 2014/04/11 11:45:04, perkj wrote: > "and has a valid boolean value that is true." > > I think this is a bit confusing. How about having the return value mean that the > key exist and have an out parameter with the actual value of it? Returning two values might not be the best. The usage of GetPropertyFromConstraints() is to tell the property is true or not. If the |key| exists, then it returns the value; otherwise returns the default value of the default value. I changed the comment and hope it is clear. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:61: // false. On 2014/04/11 11:45:04, perkj wrote: > document |effects| Added comment to the constructor of the new MediaAudioConstraints class. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:62: bool GetPropertyFromConstraints( On 2014/04/11 11:45:04, perkj wrote: > I think you have to but these functions in class scope. The names are two > general to add as a function in content. Done. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:69: bool IsValid(const blink::WebMediaConstraints& constraints); On 2014/04/11 11:45:04, perkj wrote: > dito - this is audio specific. Done. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:282: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDefaultAudioConstraints); ++i) { On 2014/04/11 11:45:04, perkj wrote: > why not array_size() ? I guess you meant arraysize, if so, done. https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_local_audio_track_adapter_unittest.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/webrtc/webrtc_local_audio_track_adapter_unittest.cc:5: #include "content/renderer/media/mock_media_constraint_factory.h" On 2014/04/11 11:45:04, perkj wrote: > up to you but you don't need this factory to create an empty blink constraint. > > just do > > blink::WebMediaStreamConstraints constraints; > constraints.initialize(); > > > here and elsewhere. Good to know, but lets keep what it is since MockMediaConstraintFactory should become the default way to construct a blink constraints for the unittests. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:121: if (effects_ & media::AudioParameters::ECHO_CANCELLER && Andrew, could you please take a look at this? Previously, we mapped a const blink::WebMediaConstraints object into a writable RTCMediaConstraints object, so we could modify the RTC constraints when the effect was enabled, but we can't do it any more, so I simply return false for kGoogEchoCancellation when the effect is enabled. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:168: if (!audio_constraints.IsValid()) { Per, after thinking a bit more, I am not sure if we should check the mandatory constraints. For example, if we are deprecating some of our goog* constraints, and remove it from our code base, then our users putting those constraints to mandatory filed will fail on AddTrack.
https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:305: // TODO(xians): goog_aec should be just echo_cancellation. On 2014/04/11 16:47:05, xians1 wrote: > On 2014/04/11 11:45:04, perkj wrote: > > ??? > > I am thinking that kEchoCancellation can replace kGoogEchoCancellation. > > Or do you think we need to keep kGoogEchoCancellation? no. Maybe refrase to goog_aec should be renamed to echo_can... or remove goog_aec once . https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:63: // Audio processing is false for gUM of non-default kMediaStreamSource. On 2014/04/11 16:47:05, xians1 wrote: > On 2014/04/11 11:45:04, perkj wrote: > > ? of non-default ? isn't kMediaStreamSource always true for tab capture. > > Comment seems to be misleading. > > changed the comment to > // The default audio processing is false for gUM with a specific > kMediaStreamSource. > wdyt? > Please explain what kMediaStreamSource is. The name kMediaStreamSource is misleading it itself since it actually means TAB or screen capture right? https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:281: DCHECK(audio_constraints.IsValid()); I don't think you should crash if a user user insert invalid constraints from JS. That is something you need to handle according to the spec. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:71: // kMediaStreamSource. Please explain what kMediaSource is. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:76: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDefaultAudioConstraints); ++i) { arraysize ? https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:89: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDefaultAudioConstraints); ++i) { arraysize ? https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:195: void EnableTypingDetection(AudioProcessing* audio_processing, nit: allign paramers or move audio_processing to next line. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:213: const base::PlatformFile& aec_dump_file) { dio https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:317: EXPECT_FALSE(audio_constraints.IsValid()); Add test to make sure guM fail? ie, media_stream_impl_unittests https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:212: GetConstraintValue(constraints, MediaStreamVideoSource::kMaxHeight, this is an unwanted behaviour change. Ie- if with or height is set as mandatory- don't check optional. This new code will always get max widh and height regardless if one of them was set and mandatory and the other as optional. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:168: if (!audio_constraints.IsValid()) { On 2014/04/11 16:47:05, xians1 wrote: > Per, after thinking a bit more, I am not sure if we should check the mandatory > constraints. For example, if we are deprecating some of our goog* constraints, > and remove it from our code base, then our users putting those constraints to > mandatory filed will fail on AddTrack. If they are mandatory, we should fail. But if that is not the behavior we currently have with the goog prefixed, you can consider filtering them out and only fail if its a w3c defined constraint.
Per, PTAL. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:281: DCHECK(audio_constraints.IsValid()); On 2014/04/14 12:15:19, perkj wrote: > I don't think you should crash if a user user insert invalid constraints from > JS. That is something you need to handle according to the spec. I was thinking WebRtcAudioCapture::Initialize() will safe guard the IsValid(), but taking it a second look, it unfortunately created the processor before the Initialize() method. DCHECK is removed. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:71: // kMediaStreamSource. On 2014/04/14 12:15:19, perkj wrote: > Please explain what kMediaSource is. Done with adding "which is used by tab capture and screen capture." That is all I know. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:76: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDefaultAudioConstraints); ++i) { On 2014/04/14 12:15:19, perkj wrote: > arraysize ? It is not possible before of the struct type used by kDefaultAudioConstraints. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:89: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDefaultAudioConstraints); ++i) { On 2014/04/14 12:15:19, perkj wrote: > arraysize ? ditto. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:195: void EnableTypingDetection(AudioProcessing* audio_processing, On 2014/04/14 12:15:19, perkj wrote: > nit: allign paramers or move audio_processing to next line. Done. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:213: const base::PlatformFile& aec_dump_file) { On 2014/04/14 12:15:19, perkj wrote: > dio Done. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:317: EXPECT_FALSE(audio_constraints.IsValid()); On 2014/04/14 12:15:19, perkj wrote: > Add test to make sure guM fail? ie, media_stream_impl_unittests I don't see any existing framework in media_stream_impl_unittests to allow me test a gUM failure there. Probably the best place to add the unittest is media_stream_audio_source_unittest, which does not exist yet. So I added a unittest to webrtc_audio_capturer_unittest to make sure we will fail to create a capturer when the constraint is invalid. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:212: GetConstraintValue(constraints, MediaStreamVideoSource::kMaxHeight, On 2014/04/14 12:15:19, perkj wrote: > this is an unwanted behaviour change. > > Ie- if with or height is set as mandatory- don't check optional. This new code > will always get max widh and height regardless if one of them was set and > mandatory and the other as optional. I changed it back and added a comment to explain. Though, that is a really odd behaviour, may I ask why skipping the option constraint if the one mandatory constraint is specified?
https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:294: const bool echo_cancellation = audio_constraints.GetProperty( Is this defaulted to true, and only overridden if the user sets it? Otherwise apps that don't currently set this will suddenly have their audio processing disabled. Also, shouldn't enabling echo_cancellation also enable the AEC? Currently, kGoogEchoCancellation still needs to be specified to do this. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:303: const bool goog_aec = false; I'm not sure that having these match the constraint name is the best approach. This variable determines if the AEC will be enabled, so "enable_aec" seems more clear to me.
https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:294: const bool echo_cancellation = audio_constraints.GetProperty( On 2014/04/15 01:29:58, ajm wrote: > Is this defaulted to true, and only overridden if the user sets it? Yes. > Otherwise apps that don't currently set this will suddenly have their > audio processing > disabled. > > Also, shouldn't enabling echo_cancellation also enable the AEC? Currently, > kGoogEchoCancellation still needs to be specified to do this. Yes, but this needs to be done after our "user" switch to echo_cancellation. Otherwise we will run into problem in music mode, where all the goog* constraints are set to false but echo_cancellation is unset. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:303: const bool goog_aec = false; On 2014/04/15 01:29:58, ajm wrote: > I'm not sure that having these match the constraint name is the best approach. > This variable determines if the AEC will be enabled, so "enable_aec" seems more > clear to me. The change is to avoid the confusion between standardized constraint and goog constraints. Tommi had a comment in his first review: > can you change the name of this variable to e.g. goog_aec or > goog_echo_cancellation to properly distinguish it from the variable > above? If you don't like goog_aec, how about enable_goog_aec?
LGTM with nits below. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:292: for (size_t i = 0; i < arraysize(kDefaultAudioConstraints); ++i) { So how come you can use arraysize here? https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:310: TEST_F(MediaStreamAudioProcessorTest, VerifyAllMandatoryConstraints) { Rename the test to match what it test. Also, the comment "Verify gUM will fail if getting an invalid mandatory audio constraint". is a bit weird since this is not gUm. Remove the comment. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:221: desired_width); nit: indentation https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:324: &max_requested_height); fits on line above? https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:169: DLOG(ERROR) << "Constraints contain invalid mandatory keys"; This is already logged in !audio_constraints.IsValid.
lgtm on media_stream_audio_processor.cc. Have you verified with some real-time testing that this works properly? https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:294: const bool echo_cancellation = audio_constraints.GetProperty( On 2014/04/15 12:43:46, xians1 wrote: > On 2014/04/15 01:29:58, ajm wrote: > > Also, shouldn't enabling echo_cancellation also enable the AEC? Currently, > > kGoogEchoCancellation still needs to be specified to do this. > > Yes, but this needs to be done after our "user" switch to echo_cancellation. > Otherwise we will run into problem in music mode, where all the goog* > constraints are set to false but echo_cancellation is unset. Ah, OK. I see you have a TODO below to deal with this. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:303: const bool goog_aec = false; On 2014/04/15 12:43:46, xians1 wrote: > The change is to avoid the confusion between standardized constraint and goog > constraints. > Tommi had a comment in his first review: > > can you change the name of this variable to e.g. goog_aec or > > goog_echo_cancellation to properly distinguish it from the variable > > above? OK, I can buy it. > > If you don't like goog_aec, how about enable_goog_aec? No, I think that's worse ;)
sorry for the delay in reviewing. I reviewed through 'media_stream_dependency_factory.cc' this time around. Will continue in the next round. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:295: MediaAudioConstraints::kEchoCancellation); It's not clear from the code that the return value will be |true| when the constraint doesn't exist. Can you elaborate in the comments here how this works? https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:306: // TODO(xians): goog_aec should be just echo_cancellation. Here I think we need to only check the value of kGoogEchoCancellation if kEchoCancellation was *not* explicitly set. If kEchoCancellation is explicitly set, then goog_aec can be set to the same value as echo_cancellation. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:330: // Return immediately if no goog constraint is enabled. Can you add that in this case the goog constraints would have been explicitly set to false and that |echo_cancellation| must *not* have been set to false but must be missing from the set of constraints? - can we DCHECK that at least? ..actually, if |echo_cancellation| was set to true explicitly, then goog_aec should be set to true (since the former overrides the latter), but I don't see that handled here. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:183: bool goog_audio_mirroring_; my comment on the 'goog' prefix had more to do with the local variables. I don't think goog here disambiguates anything so it's better to skip it. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:191: base::subtle::Atomic32 goog_typing_detected_; I don't think we should change this variable name. This is not the value of a goog constraint. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:113: if (GetProperty(kDefaultAudioConstraints[i].key)) kMediaStreamAudioDucking does not require the APM https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:123: // If platform echo canceller is enabled, disable the software AEC. I don't think this is the right place for this (I feel like I've mentioned this before or was that a different method?) https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:131: value = GetDefaultValueForConstraint(constraints_, key); I think it would be useful for the caller to know if the returned value is the default value or if it was explicitly specified. Especially in the case of kEchoCancellation, we need to know that. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:269: MediaAudioConstraints::kEchoCancellation, nit: can you sort these? https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:21: *value = static_cast<bool>(constraint_value); can you DCHECK that constraint_value is either 1 or 0? https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:56: return base::StringToInt(value_str.utf8(), value); I think I've asked about this as well before. It seems you might be trying to convert a string like "True" or "False" to an int. That's not going to give you the results you want. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:68: return base::StringToInt(value_str.utf8(), value); here too https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.h (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.h:32: // Returns true if the constraint is specified in either manatory or optinal fix all "optinal" to "optional" and all "manatory" to "mandatory"
I agree with Tommi's comments around checking if kEchoCancellation was explicitly set by the app. If we know that, we should be able to implement everything correctly here, without needing to wait for apps to update. If kEchoCancellation was explicitly set, it can be used as a switch for audio processing and in particular should override the kGoogEchoCancellation setting. If it was not explicitly set you can either: a) ignore it entirely, if you want to continue using kGoogEchoCancellation in Chrome's default list; or b) override kGoogEchoCancellation in the case that it was not explicitly set. Does that make sense?
Tommi and Andrew, PTAL. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:295: MediaAudioConstraints::kEchoCancellation); On 2014/04/17 10:56:41, tommi wrote: > It's not clear from the code that the return value will be |true| when the > constraint doesn't exist. Can you elaborate in the comments here how this > works? Done. FYI, there is also comment in MediaAudioConstraints::GetProperty // Gets the property of the constraint named by |key| in |constraints_|. // Returns the constraint's value if the key is found; Otherwise returns the // default value of the constraint. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:306: // TODO(xians): goog_aec should be just echo_cancellation. On 2014/04/17 10:56:41, tommi wrote: > Here I think we need to only check the value of kGoogEchoCancellation if > kEchoCancellation was *not* explicitly set. If kEchoCancellation is explicitly > set, then goog_aec can be set to the same value as echo_cancellation. Done with taking care the case where kEchoCancellation is set to true, while kGoogEchoCancellation is false. FYI, I was hoping our client could switch over to kEchoCancellation, so we don't need to take care of this conner case. But it turned out that not much code was required to cover it. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:330: // Return immediately if no goog constraint is enabled. On 2014/04/17 10:56:41, tommi wrote: > Can you add that in this case the goog constraints would have been explicitly > set to false and that |echo_cancellation| must *not* have been set to false but > must be missing from the set of constraints? > - can we DCHECK that at least? No, we can't. The constraints can be whatever JS developers set to. > ..actually, if |echo_cancellation| was set to true explicitly, then goog_aec > should be set to true (since the former overrides the latter), but I don't see > that handled here. now GetPropery() handles it. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:183: bool goog_audio_mirroring_; On 2014/04/17 10:56:41, tommi wrote: > my comment on the 'goog' prefix had more to do with the local variables. I > don't think goog here disambiguates anything so it's better to skip it. Done. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:183: bool goog_audio_mirroring_; On 2014/04/17 10:56:41, tommi wrote: > my comment on the 'goog' prefix had more to do with the local variables. I > don't think goog here disambiguates anything so it's better to skip it. Done. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:191: base::subtle::Atomic32 goog_typing_detected_; On 2014/04/17 10:56:41, tommi wrote: > I don't think we should change this variable name. This is not the value of a > goog constraint. Done. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:191: base::subtle::Atomic32 goog_typing_detected_; On 2014/04/17 10:56:41, tommi wrote: > I don't think we should change this variable name. This is not the value of a > goog constraint. Done. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:113: if (GetProperty(kDefaultAudioConstraints[i].key)) On 2014/04/17 10:56:41, tommi wrote: > kMediaStreamAudioDucking does not require the APM Right, just FTR, the old code enables the APM on windows even when only kMediaStreamAudioDucking is on. Done with fixing it. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:123: // If platform echo canceller is enabled, disable the software AEC. On 2014/04/17 10:56:41, tommi wrote: > I don't think this is the right place for this (I feel like I've mentioned this > before or was that a different method?) GetProperty() is used by both MediaStreamAudioProcessor and NeedsAudioProcessing() a few lines above, if we don't want to have this if code in multiple places, this method is the best place to handle all the constraints. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:131: value = GetDefaultValueForConstraint(constraints_, key); On 2014/04/17 10:56:41, tommi wrote: > I think it would be useful for the caller to know if the returned value is the > default value or if it was explicitly specified. Especially in the case of > kEchoCancellation, we need to know that. I have already handled the kEchoCancellation and kGoogEchoCancellation in GetProperty(), PTAL. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:269: MediaAudioConstraints::kEchoCancellation, On 2014/04/17 10:56:41, tommi wrote: > nit: can you sort these? Done. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:292: for (size_t i = 0; i < arraysize(kDefaultAudioConstraints); ++i) { On 2014/04/15 13:01:06, perkj wrote: > So how come you can use arraysize here? kDefaultAudioConstraints are different in the unittest and production code. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:310: TEST_F(MediaStreamAudioProcessorTest, VerifyAllMandatoryConstraints) { On 2014/04/15 13:01:06, perkj wrote: > Rename the test to match what it test. Done. > Also, the comment "Verify gUM will fail if getting an invalid mandatory audio > constraint". is a bit weird since this is not gUm. Remove the comment. I removed the comment since it does not tell much. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:21: *value = static_cast<bool>(constraint_value); On 2014/04/17 10:56:41, tommi wrote: > can you DCHECK that constraint_value is either 1 or 0? No, that is constraint from JS, users can set it to whatever. I changed the code to be: *value = constraint_value == 1 ? true : false; https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.h (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.h:32: // Returns true if the constraint is specified in either manatory or optinal On 2014/04/17 10:56:41, tommi wrote: > fix all "optinal" to "optional" and all "manatory" to "mandatory" Done. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:221: desired_width); On 2014/04/15 13:01:06, perkj wrote: > nit: indentation Done. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:324: &max_requested_height); On 2014/04/15 13:01:06, perkj wrote: > fits on line above? No. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:169: DLOG(ERROR) << "Constraints contain invalid mandatory keys"; On 2014/04/15 13:01:06, perkj wrote: > This is already logged in !audio_constraints.IsValid. OK, I removed it.
https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:56: return base::StringToInt(value_str.utf8(), value); On 2014/04/17 10:56:41, tommi wrote: > I think I've asked about this as well before. It seems you might be trying to > convert a string like "True" or "False" to an int. That's not going to give you > the results you want. Thanks for pointing it out. I misunderstood that the blink code will convert strings to 0 or 1 internally. I added overload methods to convert true anf false strings to boolean. So this method will be used by video to get int values. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:68: return base::StringToInt(value_str.utf8(), value); On 2014/04/17 10:56:41, tommi wrote: > here too Ditto.
Andrew, FYI, the code will do a) if kEchoCancellation is not set explicitly. > If kEchoCancellation was explicitly set, it can be used as a switch for audio > processing and in particular should override the kGoogEchoCancellation setting. > If it was not explicitly set you can either: > a) ignore it entirely, if you want to continue using kGoogEchoCancellation in Chrome's default list; or https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:21: *value = static_cast<bool>(constraint_value); On 2014/04/23 14:59:06, xians1 wrote: > On 2014/04/17 10:56:41, tommi wrote: > > can you DCHECK that constraint_value is either 1 or 0? > > No, that is constraint from JS, users can set it to whatever. > > I changed the code to be: > *value = constraint_value == 1 ? true : false; Wrong reply, I added overloaded methods for the GetMandatoryConstraintValue() and GetOptionalConstraintValue() to convert "true" and "false" strings to boolean.
A gentle ping.
lgtm again on media_stream_audio_processor.cc, but with some potential concerns. https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:127: if (effects_ & media::AudioParameters::ECHO_CANCELLER) You'll need to apply this to kEchoCancellation if you later intend to remove kGoogEchoCancellation. But then you'll run into a problem with the dual meaning of kEchoCancellation: you want to disable the AEC, but not all audio processing. https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:132: if (GetConstraintValue(constraints_, kEchoCancellation, &value)) Not sure we want to make GetProperty this "smart", but I'll leave that up to you and Tommi.
will do a more thorough review later but I think I've mentioned the below point before so calling it out explicitly now. https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:132: if (GetConstraintValue(constraints_, kEchoCancellation, &value)) On 2014/04/24 21:39:12, ajm wrote: > Not sure we want to make GetProperty this "smart", but I'll leave that up to you > and Tommi. I don't think that checking kEchoCancellation or kGoogEchoCancellation in this function is the right thing to do. I think the function should return whether or not the property was found and set the value (whether default or not) via an out argument: bool GetProperty(const std::string& key, bool* value);
Tommi, PTAL. Thanks, SX https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:127: if (effects_ & media::AudioParameters::ECHO_CANCELLER) On 2014/04/24 21:39:12, ajm wrote: > You'll need to apply this to kEchoCancellation if you later intend to remove > kGoogEchoCancellation. But then you'll run into a problem with the dual meaning > of kEchoCancellation: you want to disable the AEC, but not all audio processing. Yes, I will keep it in mind. And I think we have to keep kGoogEchoCancellation in our chromium code for a while before it can be completely deprecated. https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:132: if (GetConstraintValue(constraints_, kEchoCancellation, &value)) On 2014/04/25 15:09:31, tommi wrote: > On 2014/04/24 21:39:12, ajm wrote: > > Not sure we want to make GetProperty this "smart", but I'll leave that up to > you > > and Tommi. > > I don't think that checking kEchoCancellation or kGoogEchoCancellation in this > function is the right thing to do. > As offline discussed, I added a GetEchoCancellationProperty() method. But frankly, it requires all the clients of MediaAudioConstraints to explicitly check if the constraint is kGoogEchoCancellation or not and call different methods correspondingly. It might a bit more error prone in a way that users might use the wrong method. > I think the function should return whether or not the property was found and set > the value (whether default or not) via an out argument: > > bool GetProperty(const std::string& key, bool* value); As offline discussed, you probably meant GetConstraintValue() in media_stream_constraints_util.h, GetProperty() is used by audio only, and the purpose is to make it easier for the audio processor to get the property of audio constraints, and no need to know if the constraint is explicitly set.
On 2014/04/28 12:42:53, xians1 wrote: > Tommi, PTAL. > > Thanks, > > SX > > https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... > File content/renderer/media/media_stream_audio_processor_options.cc (right): > > https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... > content/renderer/media/media_stream_audio_processor_options.cc:127: if (effects_ > & media::AudioParameters::ECHO_CANCELLER) > On 2014/04/24 21:39:12, ajm wrote: > > You'll need to apply this to kEchoCancellation if you later intend to remove > > kGoogEchoCancellation. But then you'll run into a problem with the dual > meaning > > of kEchoCancellation: you want to disable the AEC, but not all audio > processing. > > Yes, I will keep it in mind. > > And I think we have to keep kGoogEchoCancellation in our chromium code for a > while before it can be completely deprecated. > > https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/... > content/renderer/media/media_stream_audio_processor_options.cc:132: if > (GetConstraintValue(constraints_, kEchoCancellation, &value)) > On 2014/04/25 15:09:31, tommi wrote: > > On 2014/04/24 21:39:12, ajm wrote: > > > Not sure we want to make GetProperty this "smart", but I'll leave that up to > > you > > > and Tommi. > > > > I don't think that checking kEchoCancellation or kGoogEchoCancellation in this > > function is the right thing to do. > > > > As offline discussed, I added a GetEchoCancellationProperty() method. > But frankly, it requires all the clients of MediaAudioConstraints to explicitly > check if the constraint is kGoogEchoCancellation or not and call different > methods correspondingly. It might a bit more error prone in a way that users > might use the wrong method. > Please ignore what I said, we have enough unittests to protect misusing methods, it is not a valid concern. > > I think the function should return whether or not the property was found and > set > > the value (whether default or not) via an out argument: > > > > bool GetProperty(const std::string& key, bool* value); > > As offline discussed, you probably meant GetConstraintValue() in > media_stream_constraints_util.h, GetProperty() is used by audio only, and the > purpose is to make it easier for the audio processor to get the property of > audio constraints, and no need to know if the constraint is explicitly set.
one comment... more coming today https://codereview.chromium.org/227743004/diff/190001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/190001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:307: const bool echo_cancellation = audio_constraints.GetProperty( Can we just do audio_constraints.GetEchoCancellation() here and encapsulate all the logic related to both the 'goog' property and the standard one, in there? I still don't think that it's a good idea that GetProperty() doesn't let the caller know whether the constraint was set or not. Here you're making assumptions about the default value of kEchoCancellation and assuming that if the return value was false, then it must have been set explicitly. I think that's both fragile and non-obvious to others that will also maintain this code.
got half way through it. We're still not communicating wrt GetEchoCancellationProperty. Also, I'm still seeing bugs in the string->bool conversion code that I've mentioned repeatedly now including in my first code review 3 weeks ago. Please go back and read my comments since this honestly feels like we're wasting both of our time. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:308: MediaAudioConstraints::kEchoCancellation); Here I was thinking that GetEchoCancellationProperty() would not take any arguments but would handle both kEchoCancellation and kGoogEchoCancellation correctly internally. This code would not need a goog_aec variable at all and no special checks for it since that would all be done by GetEchoCancellationProperty. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:70: // The default audio processing is false for gUM with a specific "The default audio processing is false" doesn't make sense https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:73: if (GetConstraintValue(constraints, kMediaStreamSource, &value)) This seems surprising behavior (breaking the rule of least surprise) of a function called GetDefaultValueForConstraint. I would expect this function to always return the same value for a given constraint and not be affected by other potentially set constraints. If there's a special case for kMediaStreamSource, I think that should be handled outside of this function. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:116: return true; See other comment about how GetEchoCancellationProperty should work. As is, this function could now return true if kGoogEchoCancellation is set to true but kEchoCancellation is set to false. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:163: if (mandatory[i].m_name.utf8() == kMediaStreamSource) does utf8() create a new string? If so, only call it once and store the utf8 string. As is you're calling it multiple times per iteration. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:19: bool FromString(const std::string& string, bool* value) { nit: FromString doesn't quite describe what the function does https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:20: std::istringstream iss(string); using istringstream here is complete overkill! just compare the string to "True" and "False" and dcheck if it is anything else. I also suspect that there's a bug here since the values we're going to be dealing with are (iirc) "True" and "False" and not "true", "false". Have you tested this? https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:77: return base::StringToInt(value_str.utf8(), value); StringToInt? https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:101: return base::StringToInt(value_str.utf8(), value); StringToInt?
Tommi, I hope I have already explained clearly that the stringtoint is needed and used by video, and I wrote a new media_stream_constraints_util_unittest.cc to test all the methods there. About GetEchoCancellationProperty() and GetDefaultValueForConstraint(), please read my inline comments, let me know if you strongly want things to be done in the other way. FYI, I have already done quite some manual tests to make sure all the constraints are parsed correctly in MediaStreamAudioProcessor, except that the studio mode has been broken for quite a while and I can't fully test it until the problem is fixed in webrtc. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:308: MediaAudioConstraints::kEchoCancellation); On 2014/04/30 10:58:44, tommi wrote: > Here I was thinking that GetEchoCancellationProperty() would not take any > arguments but would handle both kEchoCancellation and kGoogEchoCancellation > correctly internally. This code would not need a goog_aec variable at all and > no special checks for it since that would all be done by > GetEchoCancellationProperty. I think Andrew had already asked similar questions and I had answered it, I copied the comments below so it will be easier for you. Also to point out the problem with your proposal: our client need to support kEchoCancellation before we can remove goog_aec variable, otherwise it will break the music mode. And I have a relevant TODO 4 lines below. ajm: > Otherwise apps that don't currently set this will suddenly have their > audio processing > disabled. > > Also, shouldn't enabling echo_cancellation also enable the AEC? Currently, > kGoogEchoCancellation still needs to be specified to do this. me: > Yes, but this needs to be done after our "user" switch to echo_cancellation. > Otherwise we will run into problem in music mode, where all the goog* > constraints are set to false but echo_cancellation is unset. ajm: > Ah, OK. I see you have a TODO below to deal with this. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:70: // The default audio processing is false for gUM with a specific On 2014/04/30 10:58:44, tommi wrote: > "The default audio processing is false" doesn't make sense Changed it to Audio constraints are false by default. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:73: if (GetConstraintValue(constraints, kMediaStreamSource, &value)) On 2014/04/30 10:58:44, tommi wrote: > This seems surprising behavior (breaking the rule of least surprise) of a > function called GetDefaultValueForConstraint. I would expect this function to > always return the same value for a given constraint and not be affected by other > potentially set constraints. > The fact is that we will have different default values for different clients. And kMediaStreamSource is the constraint telling us what client it is. > If there's a special case for kMediaStreamSource, I think that should be handled > outside of this function. Putting it inside GetDefaultValueForConstraint consolidate the handling of default values for all clients. Moving it out (then, it will be in GetProperty()) means we get default value for some clients but not for for others, where the code becomes: bool MediaAudioConstraints::GetProperty(const std::string& key) { bool value = false; if (!GetConstraintValue(constraints_, key, &value)) { std::string value; // Tab capture, screen capture if (GetConstraintValue(constraints, kMediaStreamSource, &value)) return false; // normal gUM value = GetDefaultValueForConstraint(constraints_, key); } } If you strongly want it this way, of course I can do that. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:116: return true; On 2014/04/30 10:58:44, tommi wrote: > See other comment about how GetEchoCancellationProperty should work. > > As is, this function could now return true if kGoogEchoCancellation is set to > true but kEchoCancellation is set to false. Added code to return false when kEchoCancellation is false. Due to that studio mode is broken by other things now, I was barely able to verify the configurations of APM in WebRTC were correct, I will further verify everything is working after the studio mode problem is solved. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:163: if (mandatory[i].m_name.utf8() == kMediaStreamSource) On 2014/04/30 10:58:44, tommi wrote: > does utf8() create a new string? If so, only call it once and store the utf8 > string. As is you're calling it multiple times per iteration. Done. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:19: bool FromString(const std::string& string, bool* value) { On 2014/04/30 10:58:44, tommi wrote: > nit: FromString doesn't quite describe what the function does changed it to ConvertStringToBoolean, is it ok? https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:20: std::istringstream iss(string); On 2014/04/30 10:58:44, tommi wrote: > using istringstream here is complete overkill! > just compare the string to "True" and "False" and dcheck if it is anything else. > That is what libjingle is using. I discussed this with Per explicitly and we decided to follow what libjingle is doing to make sure we had the same behavior. Does this info change your mind? If not, I can switch to compare the string instead. > I also suspect that there's a bug here since the values we're going to be > dealing with are (iirc) "True" and "False" and not "true", "false". Have you > tested this? Does the TEST_F(MediaStreamConstraintsUtilTest, WrongBooleanConstraints) in media_stream_constraints_util_unittest.cc answer your question? https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:77: return base::StringToInt(value_str.utf8(), value); On 2014/04/30 10:58:44, tommi wrote: > StringToInt? Yes, I think I have already answered this question before. And after the offline discussion, it seem you misunderstood that it was used for audio constraints. And I change the names of all the constraint methods, but we will still use StringToInt. On 2014/04/17 10:56:41, tommi wrote: > I think I've asked about this as well before. It seems you might be trying to > convert a string like "True" or "False" to an int. That's not going to give you > the results you want. me: > Thanks for pointing it out. I misunderstood that the blink code will convert > strings to 0 or 1 internally. > I added overload methods to convert true anf false strings to boolean. > So this method will be used by video to get int values. If you don't trust my reply, look at the change in media_stream_video_source.cc https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:101: return base::StringToInt(value_str.utf8(), value); On 2014/04/30 10:58:44, tommi wrote: > StringToInt? Ditto.
Almost there. I still believe we can do better with the GetEchoCancellation function. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:308: MediaAudioConstraints::kEchoCancellation); On 2014/05/02 12:24:02, xians1 wrote: > On 2014/04/30 10:58:44, tommi wrote: > > Here I was thinking that GetEchoCancellationProperty() would not take any > > arguments but would handle both kEchoCancellation and kGoogEchoCancellation > > correctly internally. This code would not need a goog_aec variable at all and > > no special checks for it since that would all be done by > > GetEchoCancellationProperty. > > I think Andrew had already asked similar questions and I had answered it, I > copied the comments below so it will be easier for you. Also to point out the > problem with your proposal: our client need to support kEchoCancellation before > we can remove goog_aec variable, otherwise it will break the music mode. And I > have a relevant TODO 4 lines below. > ajm: > > Otherwise apps that don't currently set this will suddenly have their > > audio processing > > disabled. > > > > Also, shouldn't enabling echo_cancellation also enable the AEC? Currently, > > kGoogEchoCancellation still needs to be specified to do this. > > me: > > Yes, but this needs to be done after our "user" switch to echo_cancellation. > > Otherwise we will run into problem in music mode, where all the goog* > > constraints are set to false but echo_cancellation is unset. > > ajm: > > Ah, OK. I see you have a TODO below to deal with this. > Would we still have that problem if GetEchoCancellation implemented the following algorithm (pseudo code): bool aec, goog_aec; bool explicitly_set = GetProperty(kEchoCancellation, &aec); if (!explicitly_set) { explicitly_set = GetProperty(kGoogEchoCancellation, &goog_aec); aec = !explicitly_set || goog_aec; } return aec; https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:73: if (GetConstraintValue(constraints, kMediaStreamSource, &value)) On 2014/05/02 12:24:02, xians1 wrote: > On 2014/04/30 10:58:44, tommi wrote: > > This seems surprising behavior (breaking the rule of least surprise) of a > > function called GetDefaultValueForConstraint. I would expect this function to > > always return the same value for a given constraint and not be affected by > other > > potentially set constraints. > > > > The fact is that we will have different default values for different clients. > And kMediaStreamSource is the constraint telling us what client it is. > > > If there's a special case for kMediaStreamSource, I think that should be > handled > > outside of this function. > > Putting it inside GetDefaultValueForConstraint consolidate the handling of > default values for all clients. Moving it out (then, it will be in > GetProperty()) means we get default value for some clients but not for for > others, where the code becomes: > > bool MediaAudioConstraints::GetProperty(const std::string& key) { > bool value = false; > if (!GetConstraintValue(constraints_, key, &value)) { > std::string value; > // Tab capture, screen capture > if (GetConstraintValue(constraints, kMediaStreamSource, &value)) > return false; > > // normal gUM > value = GetDefaultValueForConstraint(constraints_, key); > } > } > > If you strongly want it this way, of course I can do that. Ah, I see. Fine to keep as is then. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:19: bool FromString(const std::string& string, bool* value) { On 2014/05/02 12:24:02, xians1 wrote: > On 2014/04/30 10:58:44, tommi wrote: > > nit: FromString doesn't quite describe what the function does > > changed it to ConvertStringToBoolean, is it ok? Yes thanks. that's very clear. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:20: std::istringstream iss(string); On 2014/05/02 12:24:02, xians1 wrote: > On 2014/04/30 10:58:44, tommi wrote: > > using istringstream here is complete overkill! > > just compare the string to "True" and "False" and dcheck if it is anything > else. > > > > That is what libjingle is using. I discussed this with Per explicitly and we > decided to follow what libjingle is doing to make sure we had the same behavior. > Does this info change your mind? If not, I can switch to compare the string > instead. Well, imho istringstream here is overkill and if you look at the code generated by this you'll hopefully agree with me. Here's a very simple implementation that does exactly what you already have but doesn't do any extra allocations or copies, is easily inlined, in the best case does only a single strcmp and it's one line shorter :) bool ConvertStringToBoolean(const std::string& string, bool* value) { *value = (string.compare("true") == 0); return *value || string.compare("false") == 0; } > > > I also suspect that there's a bug here since the values we're going to be > > dealing with are (iirc) "True" and "False" and not "true", "false". Have you > > tested this? > > Does the TEST_F(MediaStreamConstraintsUtilTest, WrongBooleanConstraints) in > media_stream_constraints_util_unittest.cc answer your question? Yes. I somehow thought that the constants were "True", "False" and not "true", "false". https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:119: if (GetEchoCancellationProperty(kGoogEchoCancellation)) If GetEchoCancellation doesn't take any arguments (handles both EC and googEC properties), you can make a single call at the top of the function and return true if it returns true. In the loop you can then just skip (continue) over the k[Goog]EchoCancellation properties. https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:34: bool GetConstraintValueAsInteger(const blink::WebMediaConstraints& constraints, Thanks for changing these. It was still stuck in my head when you used these functions in earlier patch sets to convert string->int->bool. https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:45: if (!constraints.getMandatoryConstraintValue(base::UTF8ToUTF16(key), nit: Do the base::UTF8ToUTF16 conversion only once https://codereview.chromium.org/227743004/diff/260001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/260001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:42: const char kSourceId[] = "sourceId"; Isn't this also defined elsewhere? https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Would be good to only have it in one place if possible and since this isn't audio specific (or video specific either) maybe we should think about moving all these constants into a specific source file? If you agree, I'm fine with putting down a todo here (for me or you) to do that cleanup in a separate cl.
Tommi, PTAL. Thanks. SX https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:308: MediaAudioConstraints::kEchoCancellation); On 2014/05/03 10:26:28, tommi wrote: > On 2014/05/02 12:24:02, xians1 wrote: > > On 2014/04/30 10:58:44, tommi wrote: > > > Here I was thinking that GetEchoCancellationProperty() would not take any > > > arguments but would handle both kEchoCancellation and kGoogEchoCancellation > > > correctly internally. This code would not need a goog_aec variable at all > and > > > no special checks for it since that would all be done by > > > GetEchoCancellationProperty. > > > > I think Andrew had already asked similar questions and I had answered it, I > > copied the comments below so it will be easier for you. Also to point out the > > problem with your proposal: our client need to support kEchoCancellation > before > > we can remove goog_aec variable, otherwise it will break the music mode. And I > > have a relevant TODO 4 lines below. > > ajm: > > > Otherwise apps that don't currently set this will suddenly have their > > > audio processing > > > disabled. > > > > > > Also, shouldn't enabling echo_cancellation also enable the AEC? Currently, > > > kGoogEchoCancellation still needs to be specified to do this. > > > > me: > > > Yes, but this needs to be done after our "user" switch to echo_cancellation. > > > Otherwise we will run into problem in music mode, where all the goog* > > > constraints are set to false but echo_cancellation is unset. > > > > ajm: > > > Ah, OK. I see you have a TODO below to deal with this. > > > > Would we still have that problem if GetEchoCancellation implemented the > following algorithm (pseudo code): > > bool aec, goog_aec; > bool explicitly_set = GetProperty(kEchoCancellation, &aec); > if (!explicitly_set) { > explicitly_set = GetProperty(kGoogEchoCancellation, &goog_aec); > aec = !explicitly_set || goog_aec; nit, tab capture needs a "false" as value if explicitly_set is false here, so you might mean: if (!explicitly_set) { if (tab capture) aec = false; else aec = true; } else aec = goog_aec; > } > > return aec; AFAIK, there one conner case that prevents us doing this: # It also turns kGoogEchoCancellation into a master control for audio processing. If kEchoCancellation is not explicitly set, kGoogEchoCancellation is explicitly set to false, this algorithm returns a false as aec, which will turn off the audio processing even though users might set other goog* constraints to true. btw, it is a good algorithm. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:73: if (GetConstraintValue(constraints, kMediaStreamSource, &value)) On 2014/05/03 10:26:28, tommi wrote: > On 2014/05/02 12:24:02, xians1 wrote: > > On 2014/04/30 10:58:44, tommi wrote: > > > This seems surprising behavior (breaking the rule of least surprise) of a > > > function called GetDefaultValueForConstraint. I would expect this function > to > > > always return the same value for a given constraint and not be affected by > > other > > > potentially set constraints. > > > > > > > The fact is that we will have different default values for different clients. > > And kMediaStreamSource is the constraint telling us what client it is. > > > > > If there's a special case for kMediaStreamSource, I think that should be > > handled > > > outside of this function. > > > > Putting it inside GetDefaultValueForConstraint consolidate the handling of > > default values for all clients. Moving it out (then, it will be in > > GetProperty()) means we get default value for some clients but not for for > > others, where the code becomes: > > > > bool MediaAudioConstraints::GetProperty(const std::string& key) { > > bool value = false; > > if (!GetConstraintValue(constraints_, key, &value)) { > > std::string value; > > // Tab capture, screen capture > > if (GetConstraintValue(constraints, kMediaStreamSource, &value)) > > return false; > > > > // normal gUM > > value = GetDefaultValueForConstraint(constraints_, key); > > } > > } > > > > If you strongly want it this way, of course I can do that. > > Ah, I see. Fine to keep as is then. Thanks. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:20: std::istringstream iss(string); On 2014/05/03 10:26:28, tommi wrote: > On 2014/05/02 12:24:02, xians1 wrote: > > On 2014/04/30 10:58:44, tommi wrote: > > > using istringstream here is complete overkill! > > > just compare the string to "True" and "False" and dcheck if it is anything > > else. > > > > > > > That is what libjingle is using. I discussed this with Per explicitly and we > > decided to follow what libjingle is doing to make sure we had the same > behavior. > > Does this info change your mind? If not, I can switch to compare the string > > instead. > > Well, imho istringstream here is overkill and if you look at the code generated > by this you'll hopefully agree with me. > Here's a very simple implementation that does exactly what you already have but > doesn't do any extra allocations or copies, is easily inlined, in the best case > does only a single strcmp and it's one line shorter :) > > bool ConvertStringToBoolean(const std::string& string, bool* value) { > *value = (string.compare("true") == 0); > return *value || string.compare("false") == 0; > } > I agree with you, done with changing it to using string comparison. > > > > > I also suspect that there's a bug here since the values we're going to be > > > dealing with are (iirc) "True" and "False" and not "true", "false". Have > you > > > tested this? > > > > Does the TEST_F(MediaStreamConstraintsUtilTest, WrongBooleanConstraints) in > > media_stream_constraints_util_unittest.cc answer your question? > > Yes. I somehow thought that the constants were "True", "False" and not "true", > "false". I am glad that it is sorted out. FYI, JS constraints boolean has to be either true or false, True, False, TRUE or FALSE will fail the JS code. https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:119: if (GetEchoCancellationProperty(kGoogEchoCancellation)) On 2014/05/03 10:26:28, tommi wrote: > If GetEchoCancellation doesn't take any arguments (handles both EC and googEC > properties), you can make a single call at the top of the function and return > true if it returns true. > > In the loop you can then just skip (continue) over the k[Goog]EchoCancellation > properties. Lets have the discussion in the previous comment about this, if we are turning kGoogEchoCancellation into a master control, I am fine. https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:34: bool GetConstraintValueAsInteger(const blink::WebMediaConstraints& constraints, On 2014/05/03 10:26:28, tommi wrote: > Thanks for changing these. It was still stuck in my head when you used these > functions in earlier patch sets to convert string->int->bool. NP. https://codereview.chromium.org/227743004/diff/240001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:45: if (!constraints.getMandatoryConstraintValue(base::UTF8ToUTF16(key), On 2014/05/03 10:26:28, tommi wrote: > nit: Do the base::UTF8ToUTF16 conversion only once Done. https://codereview.chromium.org/227743004/diff/260001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/260001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:42: const char kSourceId[] = "sourceId"; On 2014/05/03 10:26:28, tommi wrote: > Isn't this also defined elsewhere? > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > Would be good to only have it in one place if possible and since this isn't > audio specific (or video specific either) maybe we should think about moving all > these constants into a specific source file? If you agree, I'm fine with > putting down a todo here (for me or you) to do that cleanup in a separate cl. Discussed with Per and decided to move to media_stream_source.h
https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:308: MediaAudioConstraints::kEchoCancellation); On 2014/05/05 12:55:56, xians1 wrote: > On 2014/05/03 10:26:28, tommi wrote: > > On 2014/05/02 12:24:02, xians1 wrote: > > > On 2014/04/30 10:58:44, tommi wrote: > > > > Here I was thinking that GetEchoCancellationProperty() would not take any > > > > arguments but would handle both kEchoCancellation and > kGoogEchoCancellation > > > > correctly internally. This code would not need a goog_aec variable at all > > and > > > > no special checks for it since that would all be done by > > > > GetEchoCancellationProperty. > > > > > > I think Andrew had already asked similar questions and I had answered it, I > > > copied the comments below so it will be easier for you. Also to point out > the > > > problem with your proposal: our client need to support kEchoCancellation > > before > > > we can remove goog_aec variable, otherwise it will break the music mode. And > I > > > have a relevant TODO 4 lines below. > > > ajm: > > > > Otherwise apps that don't currently set this will suddenly have their > > > > audio processing > > > > disabled. > > > > > > > > Also, shouldn't enabling echo_cancellation also enable the AEC? Currently, > > > > kGoogEchoCancellation still needs to be specified to do this. > > > > > > me: > > > > Yes, but this needs to be done after our "user" switch to > echo_cancellation. > > > > Otherwise we will run into problem in music mode, where all the goog* > > > > constraints are set to false but echo_cancellation is unset. > > > > > > ajm: > > > > Ah, OK. I see you have a TODO below to deal with this. > > > > > > > Would we still have that problem if GetEchoCancellation implemented the > > following algorithm (pseudo code): > > > > bool aec, goog_aec; > > bool explicitly_set = GetProperty(kEchoCancellation, &aec); > > if (!explicitly_set) { > > explicitly_set = GetProperty(kGoogEchoCancellation, &goog_aec); > > aec = !explicitly_set || goog_aec; > > nit, tab capture needs a "false" as value if explicitly_set is false here, so > you might mean: > if (!explicitly_set) { > if (tab capture) > aec = false; > else > aec = true; > } else > aec = goog_aec; > > > > } > > > > return aec; > > AFAIK, there one conner case that prevents us doing this: > # It also turns kGoogEchoCancellation into a master control for audio > processing. > If kEchoCancellation is not explicitly set, kGoogEchoCancellation is explicitly > set to false, this algorithm returns a false as aec, which will turn off the > audio processing even though users might set other goog* constraints to true. > > btw, it is a good algorithm. I don't think echo cancellation applies to tab capture? The standard echo cancellation constraint should be a master switch in the absense of other constraints. If e.g. hpf is enabled, audio processing is required.
Tommi, another look?
this is great. I've got a couple of minor requests and then we're done https://codereview.chromium.org/227743004/diff/260001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/260001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:42: const char kSourceId[] = "sourceId"; On 2014/05/05 12:55:57, xians1 wrote: > On 2014/05/03 10:26:28, tommi wrote: > > Isn't this also defined elsewhere? > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > Would be good to only have it in one place if possible and since this isn't > > audio specific (or video specific either) maybe we should think about moving > all > > these constants into a specific source file? If you agree, I'm fine with > > putting down a todo here (for me or you) to do that cleanup in a separate cl. > > Discussed with Per and decided to move to media_stream_source.h Makes sense. Thanks. https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:305: audio_constraints.GetEchoCancellationProperty(); ah, this is nice. It makes this function so much easier to parse. https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:105: turn_off_constraints_as_default_ = true; this applies only to the audio-processing-module, right? and not other constraints such as ducking and I'm assuming future audio related constraints such as sample rate and buffer size. I think the name should reflect that it only applies to the apm. also, the name 'turn off" and setting it to |true| to *not* do something, is a little confusing. Can we change the name to be e.g. default_audio_processing_constraint_value_? By default it would be set to true, but here we would set it to false. You would also have to reverse the checks below. https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:147: // overwrite the value of |kGoogEchoCancellation|. nit: s/overwrite/override https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:182: one empty line https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:189: if (turn_off_constraints_as_default_ && key != kMediaStreamAudioDucking) I wonder if it's worth it now to factor the second check out into a separate function instead of checking explicitly for the ducking property here. I think it would make the code slightly more readable and easier to update in the future. Something like: if (IsAudioProcessingConstraint(key)) return default_audio_processing_constraint_value_; where IsAudioProcessingConstraint() would for now be simply: bool IsAudioProcessingConstraint(const std::string& key) { return key != kMediaStreamAudioDucking; } Later we don't have to change the logic in this function, but just update that function. https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:204: if (group_name.empty() || (group_name != "Enabled")) { nit: no need for extra parenthesis https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:333: }MockMediaConstraintFactory constraint_factory; missing \n? is the intent that constraint_factory be a part of the next scope perhaps?
https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:105: turn_off_constraints_as_default_ = true; On 2014/05/07 14:19:45, tommi wrote: > this applies only to the audio-processing-module, right? and not other > constraints such as ducking and I'm assuming future audio related constraints > such as sample rate and buffer size. I think the name should reflect that it > only applies to the apm. > > also, the name 'turn off" and setting it to |true| to *not* do something, is a > little confusing. Can we change the name to be e.g. > default_audio_processing_constraint_value_? By default it would be set to true, > but here we would set it to false. You would also have to reverse the checks > below. how about simply default_constraint_value_? https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:147: // overwrite the value of |kGoogEchoCancellation|. On 2014/05/07 14:19:45, tommi wrote: > nit: s/overwrite/override Done. https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:182: On 2014/05/07 14:19:45, tommi wrote: > one empty line Done. https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:189: if (turn_off_constraints_as_default_ && key != kMediaStreamAudioDucking) On 2014/05/07 14:19:45, tommi wrote: > I wonder if it's worth it now to factor the second check out into a separate > function instead of checking explicitly for the ducking property here. I think > it would make the code slightly more readable and easier to update in the > future. > Something like: > > if (IsAudioProcessingConstraint(key)) > return default_audio_processing_constraint_value_; > > where IsAudioProcessingConstraint() would for now be simply: > > bool IsAudioProcessingConstraint(const std::string& key) { > return key != kMediaStreamAudioDucking; > } > > Later we don't have to change the logic in this function, but just update that > function. Done. https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:204: if (group_name.empty() || (group_name != "Enabled")) { On 2014/05/07 14:19:45, tommi wrote: > nit: no need for extra parenthesis Done, though it is from a rebase. https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:333: }MockMediaConstraintFactory constraint_factory; On 2014/05/07 14:19:45, tommi wrote: > missing \n? > > is the intent that constraint_factory be a part of the next scope perhaps? No idea how it got moved here, corrected now.
https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:105: turn_off_constraints_as_default_ = true; On 2014/05/07 15:45:00, xians1 wrote: > On 2014/05/07 14:19:45, tommi wrote: > > this applies only to the audio-processing-module, right? and not other > > constraints such as ducking and I'm assuming future audio related constraints > > such as sample rate and buffer size. I think the name should reflect that it > > only applies to the apm. > > > > also, the name 'turn off" and setting it to |true| to *not* do something, is a > > little confusing. Can we change the name to be e.g. > > default_audio_processing_constraint_value_? By default it would be set to > true, > > but here we would set it to false. You would also have to reverse the checks > > below. > > how about simply default_constraint_value_? That name is technically not correct. See the first part of my comment.
https://codereview.chromium.org/227743004/diff/340001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/340001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:78: return base::StringToInt(value_str.utf8(), value); Humm - you are aware that there is a difference between that a key doesn't exist and that it can't be translated? If the constraint is mandatory, the call should fail if it exists but the value can not be applied. If it does not exist everything should be per the default value. If its optional - it is probably correct to just ignore that its there at all. So parsing errors and that a value doen't exist is not necessarily the same thing.
https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:105: turn_off_constraints_as_default_ = true; On 2014/05/07 17:01:06, tommi wrote: > On 2014/05/07 15:45:00, xians1 wrote: > > On 2014/05/07 14:19:45, tommi wrote: > > > this applies only to the audio-processing-module, right? and not other > > > constraints such as ducking and I'm assuming future audio related > constraints > > > such as sample rate and buffer size. I think the name should reflect that > it > > > only applies to the apm. > > > > > > also, the name 'turn off" and setting it to |true| to *not* do something, is > a > > > little confusing. Can we change the name to be e.g. > > > default_audio_processing_constraint_value_? By default it would be set to > > true, > > > but here we would set it to false. You would also have to reverse the > checks > > > below. > > > > how about simply default_constraint_value_? > > That name is technically not correct. See the first part of my comment. ah, right, I was trying to avoid the long name, obviously the intention failed with default_constraint_value_. Done with changing it to default_audio_processing_constraint_value_ https://codereview.chromium.org/227743004/diff/340001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/340001/content/renderer/media/... content/renderer/media/media_stream_constraints_util.cc:78: return base::StringToInt(value_str.utf8(), value); On 2014/05/08 13:42:24, perkj wrote: > Humm - you are aware that there is a difference between that a key doesn't exist > and that it can't be translated? > > If the constraint is mandatory, the call should fail if it exists but the value > can not be applied. If it does not exist everything should be per the default > value. > If its optional - it is probably correct to just ignore that its there at all. > > So parsing errors and that a value doen't exist is not necessarily the same > thing. Added a GetMandatoryConstraintValueAsBoolean(key) in MediaAudioConstraints::IsValid() check so that we know all the mandatory constraints are valid.
lgtm https://codereview.chromium.org/227743004/diff/360001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/360001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:173: if (key == kDefaultAudioConstraints[j].key && should this perhaps rather be: if (key == kDefaultAudioConstraints[j].key) { valid = GetMandatoryConstraintValueAsBoolean(constraints_, key, &value); break; }
landing now. https://codereview.chromium.org/227743004/diff/360001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/360001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:173: if (key == kDefaultAudioConstraints[j].key && On 2014/05/13 14:19:40, tommi wrote: > should this perhaps rather be: > > if (key == kDefaultAudioConstraints[j].key) { > valid = GetMandatoryConstraintValueAsBoolean(constraints_, key, &value); > break; > } Done.
The CQ bit was checked by xians@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/227743004/380001
Message was sent while issue was closed.
Change committed as 270728 |
