Wire up AGC to the MediaStreamAudioProcessor

content/renderer/media/media_stream_audio_processor.cc

 // Copyright 2013 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_audio_processor.h"
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "content/public/common/content_switches.h"
 #include "content/renderer/media/media_stream_audio_processor_options.h"
 #include "content/renderer/media/rtc_media_constraints.h"
 #include "media/audio/audio_parameters.h"
 #include "media/base/audio_converter.h"
 #include "media/base/audio_fifo.h"
 #include "media/base/channel_layout.h"
 #include "third_party/WebKit/public/platform/WebMediaConstraints.h"
 #include "third_party/libjingle/source/talk/app/webrtc/mediaconstraintsinterface.h"
 
 namespace content {
 
 namespace {
 
 using webrtc::AudioProcessing;
 using webrtc::MediaConstraintsInterface;
 
 #if defined(ANDROID)

[ajm, 2014/01/22 21:50:55]

What about iOS? I think it is actually appropriate to use 32 kHz for iOS though,
since AEC is disabled. Just wanted to make sure you'd thought about it.

[no longer working on chromium, 2014/01/23 12:46:08]

WebRtc in Chrome is supported on IOS at all, that is why we don't have any
special configuration for IOS. Actually the whole media stack in Chrome is not
used at all on IOS.

But for the future if webrtc in Chrome works on IOS, I think 32KHz is
appropriate.

[ajm, 2014/01/23 17:30:06]

Good.

 const int kAudioProcessingSampleRate = 16000;
 #else
 const int kAudioProcessingSampleRate = 32000;
 #endif
 const int kAudioProcessingNumberOfChannel = 1;
 
 const int kMaxNumberOfBuffersInFifo = 2;
 
 }  // namespace
 
@@ skipping 98 matching lines @@
   // Handles mixing and resampling between input and output parameters.
   media::AudioConverter audio_converter_;
   scoped_ptr<media::AudioBus> audio_wrapper_;
   scoped_ptr<media::AudioFifo> fifo_;
 };
 
 MediaStreamAudioProcessor::MediaStreamAudioProcessor(
     const media::AudioParameters& source_params,
     const blink::WebMediaConstraints& constraints,
     int effects)
-    : render_delay_ms_(0) {
+    : render_delay_ms_(0),
+      stereo_channels_swapping_(false) {
   capture_thread_checker_.DetachFromThread();
   render_thread_checker_.DetachFromThread();
   InitializeAudioProcessingModule(constraints, effects);
   InitializeCaptureConverter(source_params);
 }
 
 MediaStreamAudioProcessor::~MediaStreamAudioProcessor() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   StopAudioProcessing();
 }
@@ skipping 22 matching lines @@
   base::subtle::Release_Store(&render_delay_ms_, new_render_delay_ms);
 
   InitializeRenderConverterIfNeeded(sample_rate, number_of_channels,
                                     number_of_frames);
 
   // TODO(xians): Avoid this extra interleave/deinterleave.
   render_data_bus_->FromInterleaved(render_audio,
                                     render_data_bus_->frames(),
                                     sizeof(render_audio[0]));
   render_converter_->Push(render_data_bus_.get());
   while (render_converter_->Convert(&render_frame_))

[ajm, 2014/01/22 21:50:55]

FYI, we should be able to handle this conversion in a smarter way inside
AnalyzeReverseStream, so hopefully this can go away eventually.

[no longer working on chromium, 2014/01/23 12:46:08]

That will be awesome. I hope AnalyzeReverseStream can accept float* as data type
and any number as buffer size.

[ajm, 2014/01/23 17:30:06]

I unfortunately didn't mean handle everything ;) It should be able to handle
downmixing (which is knowledge that should belong to the AEC) and resampling
itself. It can do resampling more efficiently, because there are some cases in
which it can be avoided.

[no longer working on chromium, 2014/01/24 09:10:49]

That is good thing anyway, please let me know when I can remove the resampling
in chrome.

     audio_processing_->AnalyzeReverseStream(&render_frame_);
 }
 
 bool MediaStreamAudioProcessor::ProcessAndConsumeData(
     base::TimeDelta capture_delay, int volume, bool key_pressed,
-    int16** out) {
+    int* new_volume, int16** out) {
   DCHECK(capture_thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("audio",
                "MediaStreamAudioProcessor::ProcessAndConsumeData");
+  *new_volume = 0;
 
   if (!capture_converter_->Convert(&capture_frame_))
     return false;
 
-  ProcessData(&capture_frame_, capture_delay, volume, key_pressed);
+  *new_volume = ProcessData(&capture_frame_, capture_delay, volume,
+                            key_pressed);
   *out = capture_frame_.data_;
 
   return true;
 }
 
 const media::AudioParameters& MediaStreamAudioProcessor::InputFormat() const {
   return capture_converter_->source_parameters();
 }
 
 const media::AudioParameters& MediaStreamAudioProcessor::OutputFormat() const {
   return capture_converter_->sink_parameters();
 }
 
 void MediaStreamAudioProcessor::InitializeAudioProcessingModule(
     const blink::WebMediaConstraints& constraints, int effects) {
   DCHECK(!audio_processing_);
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableAudioTrackProcessing)) {
     return;
   }
 
   RTCMediaConstraints native_constraints(constraints);
   ApplyFixedAudioConstraints(&native_constraints);
   if (effects & media::AudioParameters::ECHO_CANCELLER) {

[ajm, 2014/01/22 21:50:55]

How does this interact with the code in media_stream_dependency_factory.cc? Do
we need this logic in more than one place?

And what if we add support for additional effects? MSDF handles this as soon as
the effect/constraint map is updated.

[no longer working on chromium, 2014/01/23 12:46:08]

Those code in media_stream_dependency_factory will be removed to a static method
in media_stream_audio_procesor_options.h.
And when WebRtcAudioCapturer gets the constraings and effect, it will call the
static method to update the effect, then WebRtcAudioCapturer opens the hardware.

[ajm, 2014/01/23 17:30:06]

Sounds good.

     // If platform echo cancellator is enabled, disable the software AEC.

[ajm, 2014/01/22 21:50:55]

cancellator -> canceller

[no longer working on chromium, 2014/01/23 12:46:08]

done

     native_constraints.AddMandatory(
         MediaConstraintsInterface::kEchoCancellation,
         MediaConstraintsInterface::kValueFalse, true);
   }
 
   const bool enable_aec = GetPropertyFromConstraints(
       &native_constraints, MediaConstraintsInterface::kEchoCancellation);
   const bool enable_ns = GetPropertyFromConstraints(
       &native_constraints, MediaConstraintsInterface::kNoiseSuppression);
   const bool enable_high_pass_filter = GetPropertyFromConstraints(
       &native_constraints, MediaConstraintsInterface::kHighpassFilter);
 #if defined(IOS) || defined(ANDROID)

[ajm, 2014/01/22 21:50:55]

Set enable_agc = false here as well.

[no longer working on chromium, 2014/01/23 12:46:08]

Done, thanks.

   const bool enable_experimental_aec = false;
   const bool enable_typing_detection = false;
 #else
   const bool enable_experimental_aec = GetPropertyFromConstraints(
       &native_constraints,
       MediaConstraintsInterface::kExperimentalEchoCancellation);
   const bool enable_typing_detection = GetPropertyFromConstraints(
       &native_constraints, MediaConstraintsInterface::kTypingNoiseDetection);
 #endif
+  const bool enable_agc = GetPropertyFromConstraints(
+      &native_constraints, webrtc::MediaConstraintsInterface::kAutoGainControl);
+  // TODO(xians): Comments in mediachannel.h claim that SetAgcConfig is only
+  // available for old AGC, check with ajm@ to see if we support
+  // set_target_level_dbfs(), set_compression_gain_db() and enable_limiter()
+  // from the constraints.

[no longer working on chromium, 2014/01/21 09:05:47]

Andrew, do you know if SetAgcConfig is needed or not? I don't how it is hooked
up with the constraints.

[ajm, 2014/01/22 21:50:55]

No, we don't support these currently. You can remove this TODO.

[no longer working on chromium, 2014/01/23 12:46:08]

Done.

+
+  stereo_channels_swapping_ = GetPropertyFromConstraints(
+      &native_constraints, webrtc::MediaConstraintsInterface::kAudioMirroring);
 
   // Return immediately if no audio processing component is enabled.
   if (!enable_aec && !enable_experimental_aec && !enable_ns &&
-      !enable_high_pass_filter && !enable_typing_detection) {
+      !enable_high_pass_filter && !enable_typing_detection && !enable_agc) {
     return;
   }
 
   // Create and configure the webrtc::AudioProcessing.
   audio_processing_.reset(webrtc::AudioProcessing::Create(0));
 
   // Enable the audio processing components.
   if (enable_aec) {
     EnableEchoCancellation(audio_processing_.get());
     if (enable_experimental_aec)
       EnableExperimentalEchoCancellation(audio_processing_.get());
   }
 
   if (enable_ns)
     EnableNoiseSuppression(audio_processing_.get());
 
   if (enable_high_pass_filter)
     EnableHighPassFilter(audio_processing_.get());
 
   if (enable_typing_detection)
     EnableTypingDetection(audio_processing_.get());
 
+  if (enable_agc)
+    EnableAutomaticGainControl(audio_processing_.get());
 
   // Configure the audio format the audio processing is running on. This
   // has to be done after all the needed components are enabled.

[ajm, 2014/01/22 21:50:55]

Why is that? Did you hit an error? Typically it would be better to do it first.

And just FYI:
Although until recently it was necessary to explicitly reinitialize,
ProcessStream is now able to handle changes on the fly (it initializes
internally).

set_sample_rate_hz and friends are basically deprecated; instead I'd like to add
an overloaded Initialize() to AudioProcessing which will take these parameters,
which can optionally be called if the user knows the parameters ahead of time.

In your case here it doesn't really matter since you're hardcoding the sample
rates.

[no longer working on chromium, 2014/01/23 12:46:08]

I never hit any problem here. I think the reason why I call set_sample_rate_hz()
after the configuration is because we call set_sample_rate_hz() and
set_num_channels() for each packet. So I assume it is better to do it after the
configuration.

I moved it up after the creation.
Do you mean that I can remove these two setters instead? It will be great if I
can.
How about passing those values via the constructor instead of Initialize() ?

   CHECK_EQ(audio_processing_->set_sample_rate_hz(kAudioProcessingSampleRate),
            0);
   CHECK_EQ(audio_processing_->set_num_channels(kAudioProcessingNumberOfChannel,
                                                kAudioProcessingNumberOfChannel),
            0);
 }
 
 void MediaStreamAudioProcessor::InitializeCaptureConverter(
     const media::AudioParameters& source_params) {
   DCHECK(source_params.IsValid());
@@ skipping 46 matching lines @@
   media::AudioParameters sink_params(
       media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
       media::CHANNEL_LAYOUT_MONO, kAudioProcessingSampleRate, 16,
       kAudioProcessingSampleRate / 100);
   render_converter_.reset(
       new MediaStreamAudioConverter(source_params, sink_params));
   render_data_bus_ = media::AudioBus::Create(number_of_channels,
                                              frames_per_buffer);
 }
 
-void MediaStreamAudioProcessor::ProcessData(webrtc::AudioFrame* audio_frame,
-                                            base::TimeDelta capture_delay,
-                                            int volume,
-                                            bool key_pressed) {
+int MediaStreamAudioProcessor::ProcessData(webrtc::AudioFrame* audio_frame,
+                                           base::TimeDelta capture_delay,
+                                           int volume,
+                                           bool key_pressed) {
   DCHECK(capture_thread_checker_.CalledOnValidThread());
   if (!audio_processing_)
-    return;
+    return 0;
 
-  TRACE_EVENT0("audio", "MediaStreamAudioProcessor::Process10MsData");
+  TRACE_EVENT0("audio", "MediaStreamAudioProcessor::ProcessData");
   DCHECK_EQ(audio_processing_->sample_rate_hz(),
             capture_converter_->sink_parameters().sample_rate());
   DCHECK_EQ(audio_processing_->num_input_channels(),
             capture_converter_->sink_parameters().channels());
   DCHECK_EQ(audio_processing_->num_output_channels(),
             capture_converter_->sink_parameters().channels());
 
   base::subtle::Atomic32 render_delay_ms =
       base::subtle::Acquire_Load(&render_delay_ms_);
   int64 capture_delay_ms = capture_delay.InMilliseconds();
   DCHECK_LT(capture_delay_ms,
             std::numeric_limits<base::subtle::Atomic32>::max());
   int total_delay_ms =  capture_delay_ms + render_delay_ms;
   if (total_delay_ms > 1000) {

[ajm, 2014/01/22 21:50:55]

In voice engine, this was a lower value (300 ms I believe) and the AEC is
actually capped at 500 ms. Any reason for this change?

[no longer working on chromium, 2014/01/23 12:46:08]

This is from old code in WebRtcAudioDeviceImpl, but I agree that we should
report a warning bigger than 300.

     LOG(WARNING) << "Large audio delay, capture delay: " << capture_delay_ms
                  << "ms; render delay: " << render_delay_ms << "ms";
   }
 
   audio_processing_->set_stream_delay_ms(total_delay_ms);
   webrtc::GainControl* agc = audio_processing_->gain_control();
+  // TODO(xians): We used to have a problem with the truncation of the volume.
+  // For example, if the OS has 25 volume steps, and the current volume is 7,
+  // which will be scaled to 70 in [0, 255]. When the AGC tries to adjust to
+  // volume to 76, SetVolume will fail updating the volume due to truncating
+  // the new volume back to 7. WebRtc works around this problem by keeping
+  // track values and forcing AGC to continue its trend.
+  // Check with ajm@ on if we still need the workaround.
   int err = agc->set_stream_analog_level(volume);

[no longer working on chromium, 2014/01/21 09:05:47]

Andrew, could you please take a look at this comment. The question is about a
old issue that the OS volume's precision is too small to let AGC change the
volume, we made a workaround in voe_base_impl but I was told by bjorn that you
guys would fix the problem inside AGC.

[ajm, 2014/01/23 17:30:06]

As discussed off review, you can remove this TODO.

[no longer working on chromium, 2014/01/24 09:10:49]

Done. Thanks.

   DCHECK_EQ(err, 0) << "set_stream_analog_level() error: " << err;
   err = audio_processing_->ProcessStream(audio_frame);
   DCHECK_EQ(err, 0) << "ProcessStream() error: " << err;
 
-  // TODO(xians): Add support for AGC, typing detection, audio level
-  // calculation, stereo swapping.
+  // TODO(xians): Add support for typing detection, audio level calculation.
+
+  if (stereo_channels_swapping_ && audio_frame->num_channels_ == 2) {
+    // TODO(xians): Swap the stereo channels after switching to media::AudioBus.
+  }
+
+  // Return 0 if the volume has not been changed, otherwise return the new
+  // volume.
+  return (agc->stream_analog_level() == volume) ?
+      0 : agc->stream_analog_level();
 }
 
 void MediaStreamAudioProcessor::StopAudioProcessing() {
   if (!audio_processing_.get())
     return;
 
   audio_processing_.reset();
 }
 
 }  // namespace content