Only turn on the audio processing by default for MEDIA_DEVICE_AUDIO_CAPTURE

Only turn on the audio processing by default for MEDIA_DEVICE_AUDIO_CAPTURE.

Then other clients of MEDIA_TAB_AUDIO_CAPTURE, MEDIA_TAB_VIDEO_CAPTURE, MEDIA_LOOPBACK_AUDIO_CAPTURE won't have the audio processing unless the users explicitly turn on the constraints.

NOTRY=true

BUG=264611
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255975

[no longer working on chromium, 2014-03-07 13:57:26]

Hi Alpha or Yuri,

Could either of you please review this CL? It prevents turning on the audio
processing for other media types except for MEDIA_DEVICE_AUDIO_CAPTURE.

[Alpha Left Google, 2014-03-07 21:27:28]

LGTM after one nit.

https://codereview.chromium.org/190713004/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_unittest.cc (right):

https://codereview.chromium.org/190713004/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_unittest.cc:196:
TEST_F(MediaStreamAudioProcessorTest, VerifyCastWithoutAudioProcessing) {
Should be VerifyTabCapture...

[no longer working on chromium, 2014-03-07 21:56:52]

https://codereview.chromium.org/190713004/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_unittest.cc (right):

https://codereview.chromium.org/190713004/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_unittest.cc:196:
TEST_F(MediaStreamAudioProcessorTest, VerifyCastWithoutAudioProcessing) {
On 2014/03/07 21:27:29, Alpha wrote:
> Should be VerifyTabCapture...

But we only verified MEDIA_LOOPBACK_AUDIO_CAPTURE from line 217...

[Alpha Left Google, 2014-03-07 22:09:42]

https://codereview.chromium.org/190713004/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_unittest.cc (right):

https://codereview.chromium.org/190713004/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_unittest.cc:196:
TEST_F(MediaStreamAudioProcessorTest, VerifyCastWithoutAudioProcessing) {
On 2014/03/07 21:56:52, xians1 wrote:
> On 2014/03/07 21:27:29, Alpha wrote:
> > Should be VerifyTabCapture...
> 
> But we only verified MEDIA_LOOPBACK_AUDIO_CAPTURE from line 217...

Cast is really using tab capture. This enum is for desktop capture.

[miu, 2014-03-08 03:24:55]

IMHO, this is not the right approach.  You're adding a lot of plumbing to get
the MediaStreamType into MediaStreamAudioProcessor::Initialize() just to make an
override.  Furthermore, this is the wrong place to be making the constraints
overrides: The constraints the caller has passed to the
MediaStreamAudioProcessor ctor are not the ones actually being used, and that's
very confusing.

Instead, you should do all the overriding upstream in
MediaStreamDependencyFactory::InitializeMediaStreamAudioSource().  There's
already some "constraints override" code there, and it makes sense to keep it
all in the same place.

In other words, I think this entire CL should be:

diff --git a/content/renderer/media/media_stream_dependency_factory.cc
b/content/renderer/media/media_stream_dependency_factory.cc
index be33d64..4f513d4 100644
--- a/content/renderer/media/media_stream_dependency_factory.cc
+++ b/content/renderer/media/media_stream_dependency_factory.cc
@@ -193,19 +193,18 @@ bool
MediaStreamDependencyFactory::InitializeMediaStreamAudioSource(
     MediaStreamAudioSource* source_data) {
   DVLOG(1) << "InitializeMediaStreamAudioSources()";
 
-  // Do additional source initialization if the audio source is a valid
-  // microphone or tab audio.
-  RTCMediaConstraints native_audio_constraints(audio_constraints);
-  ApplyFixedAudioConstraints(&native_audio_constraints);
-
-  StreamDeviceInfo device_info = source_data->device_info();
-  RTCMediaConstraints constraints = native_audio_constraints;
-  // May modify both |constraints| and |effects|.
-  HarmonizeConstraintsAndEffects(&constraints,
-                                 &device_info.device.input.effects);
-
+  const StreamDeviceInfo& device_info = source_data->device_info();
+  blink::WebMediaConstraints constraints = audio_constraints;
+  if (device_info.type == MEDIA_DEVICE_AUDIO_CAPTURE) {
+    // Do additional source initialization if the audio source is a valid
+    // microphone or tab audio.
+    ApplyFixedAudioConstraints(&constraints);
+    // May modify both |constraints| and |effects|.
+    HarmonizeConstraintsAndEffects(&constraints,
+                                   &device_info.device.input.effects);
+  }
   scoped_refptr<WebRtcAudioCapturer> capturer(
-      CreateAudioCapturer(render_view_id, device_info, audio_constraints));
+      CreateAudioCapturer(render_view_id, device_info, constraints));
   if (!capturer.get()) {
     DLOG(WARNING) << "Failed to create the capturer for device "
         << device_info.device.id;

[no longer working on chromium, 2014-03-08 09:27:56]

Hi Yuri, I got your point. But there are some reasons why things were done in
MediaStreamDependencyFactory and they will be done in MediaStreamAudioProcessor.

The background is that the audio constraints are used to configured APM, but
since the APM is PeerConnection, so we have to override the constraints in
MediaStreamDependencyFactory and pass the new constraints to Libjingle.
But with the new path (APM in Chrome), the constraints are used only in
MediaStreamAudioProcessor, moving everything about the contraints to one place
makes things clearer.
So the code you pointed out will soon be removed after moving APM to chrome.

Also, based on the piranha plant design, MediaStreamDependencyFactory should not
do anything with the audio source, WebRtcAudioCapturer probably will be merged
with MediaStreamAudioSource, and InitializeMediaStreamAudioSource will be moved
inside the source object. Overall, We will remove a bunch of legacy code after
moving the APM to Chrome. 


On 2014/03/08 03:24:55, miu wrote:
> IMHO, this is not the right approach.  You're adding a lot of plumbing to get
> the MediaStreamType into MediaStreamAudioProcessor::Initialize() just to make
an
> override.  Furthermore, this is the wrong place to be making the constraints
> overrides: The constraints the caller has passed to the
> MediaStreamAudioProcessor ctor are not the ones actually being used, and
that's
> very confusing.
> 
> Instead, you should do all the overriding upstream in
> MediaStreamDependencyFactory::InitializeMediaStreamAudioSource().  There's
> already some "constraints override" code there, and it makes sense to keep it
> all in the same place.
> 
> In other words, I think this entire CL should be:
> 
> diff --git a/content/renderer/media/media_stream_dependency_factory.cc
> b/content/renderer/media/media_stream_dependency_factory.cc
> index be33d64..4f513d4 100644
> --- a/content/renderer/media/media_stream_dependency_factory.cc
> +++ b/content/renderer/media/media_stream_dependency_factory.cc
> @@ -193,19 +193,18 @@ bool
> MediaStreamDependencyFactory::InitializeMediaStreamAudioSource(
>      MediaStreamAudioSource* source_data) {
>    DVLOG(1) << "InitializeMediaStreamAudioSources()";
>  
> -  // Do additional source initialization if the audio source is a valid
> -  // microphone or tab audio.
> -  RTCMediaConstraints native_audio_constraints(audio_constraints);
> -  ApplyFixedAudioConstraints(&native_audio_constraints);
> -
> -  StreamDeviceInfo device_info = source_data->device_info();
> -  RTCMediaConstraints constraints = native_audio_constraints;
> -  // May modify both |constraints| and |effects|.
> -  HarmonizeConstraintsAndEffects(&constraints,
> -                                 &device_info.device.input.effects);
> -
> +  const StreamDeviceInfo& device_info = source_data->device_info();
> +  blink::WebMediaConstraints constraints = audio_constraints;
> +  if (device_info.type == MEDIA_DEVICE_AUDIO_CAPTURE) {
> +    // Do additional source initialization if the audio source is a valid
> +    // microphone or tab audio.
> +    ApplyFixedAudioConstraints(&constraints);
> +    // May modify both |constraints| and |effects|.
> +    HarmonizeConstraintsAndEffects(&constraints,
> +                                   &device_info.device.input.effects);
> +  }
>    scoped_refptr<WebRtcAudioCapturer> capturer(
> -      CreateAudioCapturer(render_view_id, device_info, audio_constraints));
> +      CreateAudioCapturer(render_view_id, device_info, constraints));
>    if (!capturer.get()) {
>      DLOG(WARNING) << "Failed to create the capturer for device "
>          << device_info.device.id;

[miu, 2014-03-08 18:51:36]

Ah, I see.  If this is meant to completely replace all the other overrides in
the near future, then lgtm.

[no longer working on chromium, 2014-03-08 18:54:47]

On 2014/03/08 18:51:36, miu wrote:
> Ah, I see.  If this is meant to completely replace all the other overrides in
> the near future, then lgtm.

Yes, all the audio relevant code in MediaStreamDependencyFactory will be moved
to other places, and MediaStreamDependencyFactory probably will be renamed and
only deal with libjingle things.

Thanks.

[no longer working on chromium, 2014-03-09 15:12:56]

Done with changing test name to VerifyTabCaptureWithoutAudioProcessing.

I will land the patch soon.

Thanks.

https://codereview.chromium.org/190713004/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_unittest.cc (right):

https://codereview.chromium.org/190713004/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_unittest.cc:196:
TEST_F(MediaStreamAudioProcessorTest, VerifyCastWithoutAudioProcessing) {
On 2014/03/07 21:27:29, Alpha wrote:
> Should be VerifyTabCapture...

Done.

[no longer working on chromium, 2014-03-09 15:17:54]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-09 15:18:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190713004/100001

[commit-bot: I haz the power, 2014-03-09 15:18:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-09 15:18:47]

Failed to apply patch for
content/renderer/media/media_stream_audio_processor.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/media_stream_audio_processor.cc
  Hunk #1 succeeded at 142 with fuzz 1 (offset -1 lines).
  Hunk #2 FAILED at 151.
  Hunk #3 succeeded at 261 (offset 27 lines).
  Hunk #4 succeeded at 270 (offset 27 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
content/renderer/media/media_stream_audio_processor.cc.rej

Patch:       content/renderer/media/media_stream_audio_processor.cc
Index: content/renderer/media/media_stream_audio_processor.cc
diff --git a/content/renderer/media/media_stream_audio_processor.cc
b/content/renderer/media/media_stream_audio_processor.cc
index
b055dcc6699c7a906650d5a7d8e26a1973a7d6a9..e19591998e330ef01466e1208e951277c892b3d8
100644
--- a/content/renderer/media/media_stream_audio_processor.cc
+++ b/content/renderer/media/media_stream_audio_processor.cc
@@ -143,6 +143,7 @@ MediaStreamAudioProcessor::MediaStreamAudioProcessor(
     const media::AudioParameters& source_params,
     const blink::WebMediaConstraints& constraints,
     int effects,
+    MediaStreamType type,
     WebRtcPlayoutDataSource* playout_data_source)
     : render_delay_ms_(0),
       playout_data_source_(playout_data_source),
@@ -150,7 +151,7 @@ MediaStreamAudioProcessor::MediaStreamAudioProcessor(
       typing_detected_(false) {
   capture_thread_checker_.DetachFromThread();
   render_thread_checker_.DetachFromThread();
-  InitializeAudioProcessingModule(constraints, effects);
+  InitializeAudioProcessingModule(constraints, effects, type);
   InitializeCaptureConverter(source_params);
 }
 
@@ -233,7 +234,8 @@ void
MediaStreamAudioProcessor::GetStats(AudioProcessorStats* stats) {
 }
 
 void MediaStreamAudioProcessor::InitializeAudioProcessingModule(
-    const blink::WebMediaConstraints& constraints, int effects) {
+    const blink::WebMediaConstraints& constraints, int effects,
+    MediaStreamType type) {
   DCHECK(!audio_processing_);
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableAudioTrackProcessing)) {
@@ -241,7 +243,12 @@ void
MediaStreamAudioProcessor::InitializeAudioProcessingModule(
   }
 
   RTCMediaConstraints native_constraints(constraints);
-  ApplyFixedAudioConstraints(&native_constraints);
+
+  // Only apply the fixed constraints for gUM of MEDIA_DEVICE_AUDIO_CAPTURE.
+  DCHECK(IsAudioMediaType(type));
+  if (type == MEDIA_DEVICE_AUDIO_CAPTURE)
+    ApplyFixedAudioConstraints(&native_constraints);
+
   if (effects & media::AudioParameters::ECHO_CANCELLER) {
     // If platform echo canceller is enabled, disable the software AEC.
     native_constraints.AddMandatory(

[no longer working on chromium, 2014-03-10 08:49:32]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-10 08:49:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190713004/50004

[commit-bot: I haz the power, 2014-03-10 09:09:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-10 09:09:31]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[no longer working on chromium, 2014-03-10 12:16:29]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-10 12:16:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190713004/50004

[commit-bot: I haz the power, 2014-03-10 13:05:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190713004/50004

[commit-bot: I haz the power, 2014-03-10 14:20:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190713004/50004

[commit-bot: I haz the power, 2014-03-10 15:37:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190713004/50004

[commit-bot: I haz the power, 2014-03-10 17:35:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190713004/50004

[commit-bot: I haz the power, 2014-03-10 18:10:52]

Change committed as 255975