audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/test/audio_processing_simulator.cc

Index: webrtc/modules/audio_processing/test/audio_processing_simulator.cc
diff --git a/webrtc/modules/audio_processing/test/audio_processing_simulator.cc b/webrtc/modules/audio_processing/test/audio_processing_simulator.cc
index d2c274f460010af7976fc2a2f1032c8bb72ecc8a..b049115a29e5ecd29e814722927d28a7fd440ef3 100644
--- a/webrtc/modules/audio_processing/test/audio_processing_simulator.cc
+++ b/webrtc/modules/audio_processing/test/audio_processing_simulator.cc
@@ -14,9 +14,11 @@
 #include <iostream>
 #include <sstream>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
 #include "webrtc/base/stringutils.h"
 #include "webrtc/common_audio/include/audio_util.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
@@ -25,6 +27,9 @@ namespace webrtc {
 namespace test {
 namespace {
 
+constexpr FakeRecordingDevice::MicrophoneKind kDefaultMicKind =
+    FakeRecordingDevice::MicrophoneKind::kIdentity;
+
 void CopyFromAudioFrame(const AudioFrame& src, ChannelBuffer<float>* dest) {
   RTC_CHECK_EQ(src.num_channels_, dest->num_channels());
   RTC_CHECK_EQ(src.samples_per_channel_, dest->num_frames());
@@ -78,7 +83,11 @@ void CopyToAudioFrame(const ChannelBuffer<float>& src, AudioFrame* dest) {
 
 AudioProcessingSimulator::AudioProcessingSimulator(
     const SimulationSettings& settings)
-    : settings_(settings) {
+        : settings_(settings),
+          new_analog_level_(settings.initial_mic_gain),

[peah-webrtc, 2017/05/16 12:19:35]

There is a naming confusion between level and gain which I think could be more
clear.Here new_analog_level_ is initialized to initial_mic_gain.

I think I prefer level over gain as that is the naming in stream_analog_level()
, WDYT? 

[AleBzk, 2017/05/17 11:52:23]

Right, i should have been consistent.
Yup!

+          fake_recording_device_(settings_.simulate_mic_gain ?
+              static_cast<FakeRecordingDevice::MicrophoneKind>(
+                  *settings.simulated_mic_kind) : kDefaultMicKind) {
   if (settings_.ed_graph_output_filename &&
       settings_.ed_graph_output_filename->size() > 0) {
     residual_echo_likelihood_graph_writer_.open(
@@ -103,6 +112,25 @@ AudioProcessingSimulator::ScopedTimer::~ScopedTimer() {
 }
 
 void AudioProcessingSimulator::ProcessStream(bool fixed_interface) {
+  // Optionally use the fake recording device to simulate analog gain.
+  if (settings_.simulate_mic_gain) {
+    if (fixed_interface) {
+      fake_recording_device_.SimulateAnalogGain(
+          new_analog_level_, real_recording_device_level_, &fwd_frame_);
+    } else {
+      // TODO(alessiob): Remove DCHECKs below once Per has reviewed.
+      RTC_DCHECK_EQ(in_config_.num_channels(), in_buf_->num_channels());

[peah-webrtc, 2017/05/16 12:19:35]

Why are these DCHECK-s needed here now? There should be nothing new that is
changed in this CL that requires that, right?

If the DCHECKS is something that missed to be checked I think that should
instead happen when in_config_ and in_buf_ are created.

[AleBzk, 2017/05/17 11:52:23]

Sorry, I should have added a comment for you here instead of a TODO.

If you see what's changed, you can notice that I call num_channels() and
num_frames() on in_buf_, whereas before they were called on in_config_. Since
in_buf_ is passed into SimulateAnalogGain(), I just use it to get the number of
channels and frames.

Anyways, I tested that part of the code, so I remove the DCHECKs.

+      RTC_DCHECK_EQ(in_config_.num_frames(), in_buf_->num_frames());
+      fake_recording_device_.SimulateAnalogGain(
+          new_analog_level_, real_recording_device_level_, in_buf_.get());
+    }
+  }
+
+  // Notify the mic gain level to AGC.

[peah-webrtc, 2017/05/16 12:19:35]

Both using  gain and level here sounds like duplicate terminology.

[AleBzk, 2017/05/17 11:52:23]

Done.

+  RTC_CHECK_EQ(AudioProcessing::kNoError,
+               ap_->gain_control()->set_stream_analog_level(new_analog_level_));

[peah-webrtc, 2017/05/16 12:19:35]

This is not correct I think. There is nothing in the usage of APM that ensures
that the actually applied analog level is what is reported was applied.

My suggestion is to make new_analog_level_ part of fake_recording device_ (but
with a new name) and let this method ask fake_recording device_ for what level
is applied.

E.g
ap_->gain_control()->set_stream_analog_level(settings_.simulate_mic_gain ?
fake_recording device_.GetLevel() : real_recording_device_level_);

ap_->ProcessStream(...);

int level = ap_->gain_control()->stream_analog_level();
if (settings_.simulate_mic_gain) {
  fake_recording device_.GetLevel(level);
}

[AleBzk, 2017/05/17 11:52:23]

I completely agree with this point.
In fact, a user could for instance manually change the input mic gain, or the
hardware/driver can override the set value.
Maybe I'm wrong, but what you suggest looks exactly like it was when I first
submitted this CL.
Look at
https://codereview.webrtc.org/2834643002/diff/100001/webrtc/modules/audio_pro...
around line 115. There was a fake mic class storing the mic value. Maybe at that
time, some details were missing or unclear.

I rewrote FakeRecordingDevice, which holds the current mic level replacing the
role of |new_analog_level_|.

[peah-webrtc, 2017/05/17 14:52:12]

What I stated was that "I'd prefer a decoupling between the stored stream analog
level and the fake
audio device for the case when there is no simulation of the analog gain..."

I think that still makes sense, and it is like that for the suggested construct 
ap_->gain_control()->set_stream_analog_level(settings_.simulate_mic_gain ?
fake_recording device_.GetLevel() : real_recording_device_level_);

I see, however, that I probably mixed up the real_recording_device_level_ and
new_analog_level variables but, done right, I actually think that this is
according to the referred statement.

Or maybe I missed something?

+
+  // Process the current audio frame.
   if (fixed_interface) {
     {
       const auto st = ScopedTimer(mutable_proc_time());
@@ -116,6 +144,9 @@ void AudioProcessingSimulator::ProcessStream(bool fixed_interface) {
                                     out_config_, out_buf_->channels()));
   }
 
+  // Store the mic gain level suggested by AGC if required.

[peah-webrtc, 2017/05/16 12:19:35]

Both using  gain and level here sounds like duplicate terminology.

[AleBzk, 2017/05/17 11:52:23]

Right, thanks for the comment.
I only left level.

+  new_analog_level_ = ap_->gain_control()->stream_analog_level();
+
   if (buffer_writer_) {
     buffer_writer_->Write(*out_buf_);
   }
@@ -193,6 +224,8 @@ void AudioProcessingSimulator::SetupBuffersConfigsOutputs(
   rev_frame_.num_channels_ = reverse_input_num_channels;
 
   if (settings_.use_verbose_logging) {
+    rtc::LogMessage::LogToDebug(rtc::LS_VERBOSE);
+
     std::cout << "Sample rates:" << std::endl;
     std::cout << " Forward input: " << input_sample_rate_hz << std::endl;
     std::cout << " Forward output: " << output_sample_rate_hz << std::endl;