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..f5995e16a2b6f84aa79883d9b8f409debe2892f1 100644
--- a/content/renderer/media/media_stream_dependency_factory.cc
+++ b/content/renderer/media/media_stream_dependency_factory.cc
@@ -96,6 +96,55 @@ void ApplyFixedAudioConstraints(RTCMediaConstraints* constraints) {
   }
 }
 
+typedef media::AudioParameters::PlatformEffects PlatformEffects;

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

Is it a good idea to create a new type for this?  I'm guessing you're only
interested in saving on keystrokes.  Is |using media::AudioParameters| at the
top of the file good enough?

[ajm, 2013/12/12 01:51:27]

Removed.

+
+// |key| is the constraint for which |constraints| should be updated, and
+// |effect| is the state of the corresponding platform effect.

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

This function is still confusing to me since both |key| and |constraints| will
change the state of the other and it's not clear which one wins.

What about removing the ReconcileConstraintAndPlatformEffect +
ReconcileConstraintsAndPlatformEffects, and instead do:

if (device_info.device.input.effects.echo_canceller) {
  // Try to set the matching mandatory constraint.
  // If the feature has been disabled via constraints,
  // echo_canceller will be set to false.
  device_info.device.input.effects.echo_canceller =
      platform_constraints.AddMandatory(
          webrtc::MediaConstraintsInterface::kEchoCancellation,
          webrtc::MediaConstraintsInterface::kValueTrue, false);
}


AddMandatory() returns true when it sets the constraint or when the constraint
is already set to the same value.  It return false (since I pass false for
|override_if_exists|) if the constraint has been set already but to a different
value.

This is much less code + comments and I think it accomplishes what you want
(unless of course Iḿ completely missing the point which is always possible).

[ajm, 2013/12/11 17:46:49]

That works, and I agree is much cleaner.

[ajm, 2013/12/12 01:51:27]

Ugh, on further reflection, it doesn't work. First that's not how AddMandatory()
works: when |override_if_exists| is false, it returns false if the key is found,
regardless of the value. It only returns true if the key wasn't found.

Even under the previous assumption this doesn't quite do what we want. If both
the effect is available and the constraint is enabled, we want to disable the
constraint in order to disable the SW effect.

I've removed these functions and tried to make the implementation as clear as
possible inline.

+// |constraints| and |effect| are both input and output parameters.
+//
+// See the function below for a full description of the purpose.
+void ReconcileConstraintAndPlatformEffect(const char* key,
+                                          RTCMediaConstraints* constraints,
+                                          bool* effect) {
+  bool value;
+  if (!webrtc::FindConstraint(constraints, key, &value, NULL) ||

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

nit: this fits on one line:
  if (!webrtc::FindConstraint(constraints, key, &value, NULL) || !value) {

[ajm, 2013/12/12 01:51:27]

Removed.

+      value == false) {
+    // If the constraint does not exist, or is set to false, disable the
+    // corresponding platform effect.
+    *effect = false;
+    DVLOG(1) << "Disabling platform effect: " << key;
+  } else if (*effect) {
+    // 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;
+  }
+}
+
+// Updates |constraints| and |effects| to be consistent with each other. At
+// input, |constraints| must be the media constraints requested for the gUM
+// stream. At output |constraints| will be updated to reflect what should be
+// applied to the source (e.g. PeerConnection). At input, |effects| must be the
+// available platform (i.e. built-in, typically hardware) audio effects which
+// should be used in preference to software effects. At output, |effects| will
+// be updated to reflect what should be applied to the platform capturer.
+//
+// In particular this means:
+// - Any true constraint with an available corresponding platform effect at
+// input will be set to false at output.
+// - Any false constraint at input will cause the corresponding platform effect
+// to be disabled at output.
+void ReconcileConstraintsAndPlatformEffects(RTCMediaConstraints* constraints,
+                                            PlatformEffects* effects) {
+  // This currently only deals with AEC, but is prepared for expansion to other
+  // platform effects.
+  ReconcileConstraintAndPlatformEffect(
+      webrtc::MediaConstraintsInterface::kEchoCancellation,
+      constraints,
+      &effects->echo_canceller);
+}
+
 class P2PPortAllocatorFactory : public webrtc::PortAllocatorFactoryInterface {
  public:
   P2PPortAllocatorFactory(
@@ -323,7 +372,16 @@ 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 has the potential to update both the constraints and the platform
+    // effects, such that we prefer platform to software effects but disable
+    // platform effects when requested by constraints.
+    //
+    // See the function documentation for a full description.
+    ReconcileConstraintsAndPlatformEffects(&platform_constraints,
+                                           &device_info.device.input.effects);
     scoped_refptr<WebRtcAudioCapturer> capturer(
         MaybeCreateAudioCapturer(render_view_id, device_info));
     if (!capturer.get()) {
@@ -341,7 +399,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 +968,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.effects)) {
     return NULL;
   }