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;
     }
   }
 }
 
+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(
       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 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()) {
       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.use_platform_aec)) {
     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