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..0d5493d89ec5d0dc8e5ca1d4bef340ce550dadaf 100644
--- a/webrtc/modules/audio_processing/test/audio_processing_simulator.cc
+++ b/webrtc/modules/audio_processing/test/audio_processing_simulator.cc
@@ -14,6 +14,7 @@
 #include <iostream>
 #include <sstream>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "webrtc/base/checks.h"
@@ -102,19 +103,36 @@ AudioProcessingSimulator::ScopedTimer::~ScopedTimer() {
   proc_time_->min = std::min(proc_time_->min, interval);
 }
 
-void AudioProcessingSimulator::ProcessStream(bool fixed_interface) {
+void AudioProcessingSimulator::ProcessStream(bool fixed_interface,
+                                             bool update_analog_level) {

[peah-webrtc, 2017/04/26 12:54:44]

I think the naming of the update_analog_level could be done to be more
descriptive.

I interpret it as a verb meaning that the analog level should be updated which I
see as that the new leve reported by APM should be applied to update the analog
gain. But that is not what it means, right? What it rather means, is that the
reported analog level should be specified in the APM, which is quite different.

+  if (update_analog_level) {
+    RTC_CHECK_EQ(AudioProcessing::kNoError,
+                 ap_->gain_control()->set_stream_analog_level(
+                     last_specified_microphone_level_));
+  }
   if (fixed_interface) {
     {
       const auto st = ScopedTimer(mutable_proc_time());
+      // TODO(alessiob): Apply last_specified_microphone_level_ to fwd_frame_

[peah-webrtc, 2017/04/26 12:54:44]

I think the approach to apply the microphone level is not sufficiently general.
For instance, separating the gain with the gain application would need (at
least) two different models of one microphone to be used to properly simulate
soft-clipping. 

Have you considered bundling the simulation of the analog gain, and the gain
application, into a virtual microphone model?

+      // simulating a mic with analog gain.
       RTC_CHECK_EQ(AudioProcessing::kNoError, ap_->ProcessStream(&fwd_frame_));
     }
     CopyFromAudioFrame(fwd_frame_, out_buf_.get());
   } else {
     const auto st = ScopedTimer(mutable_proc_time());
+    // TODO(alessiob): Apply last_specified_microphone_level_ to
+    // in_buf_->channels() simulating a mic with analog gain.
     RTC_CHECK_EQ(AudioProcessing::kNoError,
                  ap_->ProcessStream(in_buf_->channels(), in_config_,
                                     out_config_, out_buf_->channels()));
   }
+  // Update last_specified_microphone_level_ using the value suggested by AGC

[peah-webrtc, 2017/04/26 12:54:44]

the AGC

+  // or the default if settings_.simulate_mic_gain is false.
+  if (update_analog_level) {
+    last_specified_microphone_level_ = settings_.simulate_mic_gain ?

[hlundin-webrtc, 2017/04/26 12:11:37]

This is confusing. If update_analog_level is true, but we have not set
simulate_mic_gain, then the effect of update_analog_level is essentially a
reset. To me it seems that we are overloading the meaning of
last_specified_microphone_level_ and update_analog_level.

update_analog_level seems to specify that we should set and get the gain from
gain_control(), but it also resets the level which was set from the AECdump
file.

[peah-webrtc, 2017/04/26 12:54:44]

This is not correct. If you don't simulate the mic gain, you should not reset
the gain. I think it works in practice though as you in the dump-simulator will
set the last_specified_microphone_level_  before the next call. The
else-statement in this expression is however fully redundant and misleading.

I think the separation that is introduced between where the simulated gain, and
the recorded gain are applied stored in order to be applied in the call to
ap_->gain_control()->set_stream_analog_level(..) makes the code harder to read.
I'd very much prefer for that to be done at the same place in an if-statement so
that it is easy to see where how that gain is chosen/computed.

+        ap_->gain_control()->stream_analog_level()
+            : kInitialMicrophoneGainLevel;
+  }
 
   if (buffer_writer_) {
     buffer_writer_->Write(*out_buf_);