Enable platform echo cancellation through the AudioRecord path.

content/renderer/media/media_stream_dependency_factory.cc

Index: content/renderer/media/media_stream_dependency_factory.cc
diff --git a/content/renderer/media/media_stream_dependency_factory.cc b/content/renderer/media/media_stream_dependency_factory.cc
index 6172e06a8e31da352c32b056fc1cf6585ef2dcc6..231eafe635e107e8a713a90cbb446d01f5446255 100644
--- a/content/renderer/media/media_stream_dependency_factory.cc
+++ b/content/renderer/media/media_stream_dependency_factory.cc
@@ -96,6 +96,40 @@ void ApplyFixedAudioConstraints(RTCMediaConstraints* constraints) {
   }
 }
 
+void TransferConstraintToPlatformEffect(RTCMediaConstraints* constraints,

[tommi (sloooow) - chröme, 2013/12/06 12:11:34]

Please document all the arguments.  |platform_effect| in particular seems to be
an in+out argument which is non-obvious. In one case it's always overwritten (no
check for what value it was set to on input) and in the other case it's only
checked but not set...

Does |platform_effect| mean that the effect should be turned on or does it mean
that the effect is supported by the platform?

would it make sense to use two separate arguments?  Maybe the return value could
be used to indicate whether or not the effect should be turned on and the input
argument could indicate whether the platform actually supports it?
If you think that makes sense, I'd like to change the variable name to
platform_effect_supported or something like that since I'm having problems
parsing the code as is.

[ajm, 2013/12/10 06:37:16]

I tried returning the output effect and using just an input parameter, but it
served to complicate the one call site (since I would set the return to the same
variable as I used for input anyway).

I left them both as input/output parameters, but hopefully with the rename and
new comments, it's clear what the intent is. LMK.

+                                        const char* key,
+                                        bool* platform_effect) {
+  bool value;
+  if (!webrtc::FindConstraint(constraints, key, &value, NULL) ||
+      value == false) {
+    // If the constraint does not exist, or is set to false, disable the
+    // corresponding platform effect.
+    *platform_effect = false;
+    DVLOG(1) << "Disabling platform effect: " << key;
+  } else {
+    if (*platform_effect) {

[tommi (sloooow) - chröme, 2013/12/06 12:11:34]

nit: you can save one scope:
} else if (*platform_effect) {

[ajm, 2013/12/10 06:37:16]

Done.

+      // If the constraint is set to true, and the corresponding platform effect
+      // is available, disable the constraint.
+      constraints->AddMandatory(key,
+          webrtc::MediaConstraintsInterface::kValueFalse, true);
+      DVLOG(1) << "Disabling constraint: " << key;
+    }
+  }
+}
+
+// Any true constraint with an available corresponding platform effect will be
+// set to false. Any false constraint will cause the corresponding platform
+// effect to be disabled.
+//
+// This currently only deals with AEC, but is prepared for expansion to other
+// platform effects.
+void TransferConstraintsToPlatformEffects(RTCMediaConstraints* constraints,
+    MediaStreamDevice::AudioDeviceParameters* params) {

[tommi (sloooow) - chröme, 2013/12/06 12:11:34]

please document that both parameters are in+out and that the current value of
one will have an effect on the other.

Just from looking at the function, I would have assumed that I could use it like
this:

// Assume |constraints| is valid at this point.
// I want to use |constraints| to initialize my |params|.
// Since the function has the name "ToPlatformEffects"
// it's not obvious to me that constraints will be modified.

AudioDeviceParameters params;  // just use ctor initialized values for now.
TransferConstraintsToPlatformEffects(constraints, &params);

// BUG: If I understand correctly, what has happened here is
// that my |params| were unaffected by calling the Transfer function
// but my params actually may have caused |constraints| to be modified
// and that to me feels unexpected.

[ajm, 2013/12/10 06:37:16]

Done. Hopefully you think it's sufficiently clear now ;)

+  TransferConstraintToPlatformEffect(constraints,
+      webrtc::MediaConstraintsInterface::kEchoCancellation,
+      &params->use_platform_aec);
+}
+
 class P2PPortAllocatorFactory : public webrtc::PortAllocatorFactoryInterface {
  public:
   P2PPortAllocatorFactory(
@@ -323,7 +357,17 @@ void MediaStreamDependencyFactory::CreateNativeMediaSources(
 
     // TODO(xians): Create a new capturer for difference microphones when we
     // support multiple microphones. See issue crbug/262117 .
-    const StreamDeviceInfo device_info = source_data->device_info();
+    StreamDeviceInfo device_info = source_data->device_info();
+
+    RTCMediaConstraints platform_constraints = native_audio_constraints;
+    // This causes constraint settings to be transferred to their corresponding
+    // platform effects. For example, if a platform AEC is available, and the
+    // AEC constraint is set to true, we should leave the platform AEC
+    // enabled, and disable the software AEC by setting the constraint to false.
+    // Contrariwise, if the AEC constraint is set to false, we should disable
+    // the platform AEC even if available.

[tommi (sloooow) - chröme, 2013/12/06 12:11:34]

This comment is good but I think it would be good to refactor the code so that
it will be more obvious what's going on.

[henrika (OOO until Aug 14), 2013/12/06 12:52:21]

is it correct to assume that everything should work as before on all platforms
but on Android? What happens on all other platforms if we try to enable a
platform AEC?

[ajm, 2013/12/10 06:37:16]

If an AudioManager claims that a platform AEC exists where it doesn't then we'll
have neither platform (HW) nor SW AEC enabled.

But, yes, it's correct to assume that everything should work as before, since no
other platform is claiming existence of effects.

+    TransferConstraintsToPlatformEffects(&platform_constraints,
+                                         &device_info.device.input);
     scoped_refptr<WebRtcAudioCapturer> capturer(
         MaybeCreateAudioCapturer(render_view_id, device_info));
     if (!capturer.get()) {
@@ -341,7 +385,7 @@ void MediaStreamDependencyFactory::CreateNativeMediaSources(
     // Creates a LocalAudioSource object which holds audio options.
     // TODO(xians): The option should apply to the track instead of the source.
     source_data->SetLocalAudioSource(
-        CreateLocalAudioSource(&native_audio_constraints).get());
+        CreateLocalAudioSource(&platform_constraints).get());
     source_observer->AddSource(source_data->local_audio_source());
   }
 
@@ -910,7 +954,8 @@ MediaStreamDependencyFactory::MaybeCreateAudioCapturer(
           device_info.session_id,
           device_info.device.id,
           device_info.device.matched_output.sample_rate,
-          device_info.device.matched_output.frames_per_buffer)) {
+          device_info.device.matched_output.frames_per_buffer,
+          device_info.device.input.use_platform_aec)) {
     return NULL;
   }