Enable platform echo cancellation through the AudioRecord path.

content/renderer/media/media_stream_dependency_factory.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/media_stream_dependency_factory.h"
 
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 78 matching lines @@
                                 &already_set_value, NULL)) {
       constraints->AddMandatory(kDefaultAudioConstraints[i].key,
           kDefaultAudioConstraints[i].value, false);
     } else {
       DVLOG(1) << "Constraint " << kDefaultAudioConstraints[i].key
                << " already set to " << already_set_value;
     }
   }
 }
 
+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(
       P2PSocketDispatcher* socket_dispatcher,
       talk_base::NetworkManager* network_manager,
       talk_base::PacketSocketFactory* socket_factory,
       blink::WebFrame* web_frame)
       : socket_dispatcher_(socket_dispatcher),
         network_manager_(network_manager),
         socket_factory_(socket_factory),
@@ skipping 207 matching lines @@
         static_cast<MediaStreamSourceExtraData*>(source.extraData());
 
     // Check if the source has already been created. This happens when the same
     // source is used in multiple MediaStreams as a result of calling
     // getUserMedia.
     if (source_data->local_audio_source())
       continue;
 
     // 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()) {
       DLOG(WARNING) << "Failed to create the capturer for device "
                     << device_info.device.id;
       sources_created.Run(web_stream, false);
       // TODO(xians): Don't we need to check if source_observer is observing
       // something? If not, then it looks like we have a leak here.
       // OTOH, if it _is_ observing something, then the callback might
       // be called multiple times which is likely also a bug.
       return;
     }
     source_data->SetAudioCapturer(capturer);
 
     // 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());
   }
 
   source_observer->StartObservering();
 }
 
 void MediaStreamDependencyFactory::CreateNativeLocalMediaStream(
     blink::WebMediaStream* web_stream) {
   DVLOG(1) << "MediaStreamDependencyFactory::CreateNativeLocalMediaStream()";
   if (!EnsurePeerConnectionFactory()) {
@@ skipping 548 matching lines @@
 
   if (!capturer->Initialize(
           render_view_id,
           static_cast<media::ChannelLayout>(
               device_info.device.input.channel_layout),
           device_info.device.input.sample_rate,
           device_info.device.input.frames_per_buffer,
           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;
   }
 
   // Add the capturer to the WebRtcAudioDeviceImpl if it is a new capturer.
   if (is_new_capturer)
     GetWebRtcAudioDevice()->AddAudioCapturer(capturer);
 
   return capturer;
 }
 
@@ skipping 28 matching lines @@
 MediaStreamDependencyFactory::GetNativeMediaStreamTrack(
       const blink::WebMediaStreamTrack& track) {
   if (track.isNull())
     return NULL;
   MediaStreamTrackExtraData* extra_data =
       static_cast<MediaStreamTrackExtraData*>(track.extraData());
   return extra_data ? extra_data->track().get() : NULL;
 }
 
 }  // namespace content