audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc

Index: webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc
diff --git a/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc b/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc
index d1cd48424a1cfcb3708c170f2a1a91d866f35887..c16f1c771a33a6c922afd653377c938e4baa7b89 100644
--- a/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc
+++ b/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc
@@ -8,11 +8,14 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include <algorithm>
 #include <iostream>
+#include <utility>
 
 #include "webrtc/modules/audio_processing/test/aec_dump_based_simulator.h"
 
 #include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
 #include "webrtc/modules/audio_processing/test/protobuf_utils.h"
 #include "webrtc/test/testsupport/trace_to_stderr.h"
 
@@ -63,13 +66,17 @@ bool VerifyFloatBitExactness(const webrtc::audioproc::Stream& msg,
 }  // namespace
 
 AecDumpBasedSimulator::AecDumpBasedSimulator(const SimulationSettings& settings)
-    : AudioProcessingSimulator(settings) {}
+    : AudioProcessingSimulator(settings) {
+  if (settings_.simulate_mic_gain) {
+    LOG(LS_VERBOSE) << "Simulating analog mic gain using AEC dump as input "
+        << "(the unmodified mic gain level will be virtually restored)";
+  }
+}
 
 AecDumpBasedSimulator::~AecDumpBasedSimulator() = default;
 
 void AecDumpBasedSimulator::PrepareProcessStreamCall(
-    const webrtc::audioproc::Stream& msg,
-    bool* set_stream_analog_level_called) {
+    const webrtc::audioproc::Stream& msg) {
   if (msg.has_input_data()) {
     // Fixed interface processing.
     // Verify interface invariance.
@@ -156,14 +163,17 @@ void AecDumpBasedSimulator::PrepareProcessStreamCall(
     ap_->set_stream_key_pressed(*settings_.use_ts);
   }
 
-  // TODO(peah): Add support for controlling the analog level via the
-  // command-line.
-  if (msg.has_level()) {
-    RTC_CHECK_EQ(AudioProcessing::kNoError,
-                 ap_->gain_control()->set_stream_analog_level(msg.level()));
-    *set_stream_analog_level_called = true;
+  // Level is always logged in AEC dumps.
+  RTC_CHECK(msg.has_level());
+
+  if (settings_.simulate_mic_gain) {
+    // When the analog gain is simulated, set the undo level to |msg.level()| to
+    // virtually restore the unmodified microphone signal level.
+    *real_recording_device_level_ = msg.level();

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

Why do you need to store msg.level(); in different values when
settings_.simulate_mic_gain is, and is not, set? 
I'd prefer that you store it in one value, and then have a corresponding
if-statement (if needed) in audio_processing_simulator. As it is now, I think it
is harder than necessary to follow the code.

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

Because the level is used for different purposes.

Case 1.
When simulating the microphone gain and an AEC dump is provided, we first need
to virtually restore the unmodified mic level - i.e., a fixed level that would
have been observed, if AGC had not modified the mic gain. Note that we are
undoing assuming that the user does not change the mic level and that the
unmodified mic gain is the maximum.
Then the AGC instance in APM will notify a suggested analog level, which will be
used by the gain simulator.

Case 2.
When the mic gain simulation is disabled, then we should inform AGC with the
level that has been used during the recorded call. It's done to call
gain_control.set_stream_analog_level(), ProcessStream(),
gain_control.stream_analog_level() in the correct order to reproduce the
pipeline side effects.
This would cause confusion and in the simulated mic gain case using one variable
is not even possible (otherwise AGC will overwrite the undo level).

Let's discuss this offline if needed.

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

I agree that the usage is different, what I propose is to put that logic into
audio_processing_simulator.cc instead. This is the only place where msg.level()
is read, and now it is, based on settings_.simulate_mic_gain, stored in either
of two variables. 
Then in the method ProcessStream in audio_processing_simulator.cc we again have
a conditional settings_.simulate_mic_gain and do specific processing in which
both of the above variables are used.

What about storing the msg.level() into one variable, and then move the
if-statement here to ProcessStream? Then all of this logic is moved there. As it
is now, the roles of new_analog_level_ and real_recording_device_level is quite
confusing to me, but that will definitely be easier to follow if they are set in
the same place.  

Note that both new_analog_level_ and real_recording_device_level_ are only used
here and inside ProcessStream. The same holds for settings_.simulate_mic_gain. 

Or do you have a specific reason why this assignment needs to be in this method?

   } else {
-    *set_stream_analog_level_called = false;
+    // When the analog gain is not simulated, the AEC dump level has to be used
+    // in AudioProcessingSimulator::ProcessStream().
+    new_analog_level_ = msg.level();
   }
 }
 
@@ -562,14 +572,8 @@ void AecDumpBasedSimulator::HandleMessage(const webrtc::audioproc::Init& msg) {
 
 void AecDumpBasedSimulator::HandleMessage(
     const webrtc::audioproc::Stream& msg) {
-  bool set_stream_analog_level_called = false;
-  PrepareProcessStreamCall(msg, &set_stream_analog_level_called);
+  PrepareProcessStreamCall(msg);
   ProcessStream(interface_used_ == InterfaceType::kFixedInterface);
-  if (set_stream_analog_level_called) {
-    // Call stream analog level to ensure that any side-effects are triggered.
-    (void)ap_->gain_control()->stream_analog_level();
-  }
-
   VerifyProcessStreamBitExactness(msg);
 }