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..fe1327b28a7f2677dd10d74f765ebf5c1c23a169 100644
--- a/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc
+++ b/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc
@@ -8,7 +8,9 @@
  *  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"
 
@@ -69,7 +71,7 @@ AecDumpBasedSimulator::~AecDumpBasedSimulator() = default;
 
 void AecDumpBasedSimulator::PrepareProcessStreamCall(
     const webrtc::audioproc::Stream& msg,
-    bool* set_stream_analog_level_called) {
+    bool* update_analog_level) {
   if (msg.has_input_data()) {
     // Fixed interface processing.
     // Verify interface invariance.
@@ -158,12 +160,16 @@ void AecDumpBasedSimulator::PrepareProcessStreamCall(
 
   // 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;
-  } else {
-    *set_stream_analog_level_called = false;
+  // If the AECdump does not include a level, AGC was disabled during the call.

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

No, I don't think it works like this. msg.has_level() is not added only when the
AGC is on. Afaics, the level is always stored in the aecdump for newer
recordings. (please correct me if I'm wrong though).

[aleloi, 2017/05/04 12:47:13]

I just checked, msg->set_level(gain_control()->stream_analog_level()); is always
called. Maybe we should to a simplification and not consider the case
!msg.has_level() ?

+  // If so and if the analog gain simulation is disabled, inform
+  // AudioProcessingSimulator::ProcessStream in order not to call
+  // GainControl::set_stream_analog_level() and
+  // GainControl::stream_analog_level() before and after
+  // AudioProcessing::ProcessStream respectively.
+  *update_analog_level = msg.has_level() || settings_.simulate_mic_gain;
+  if (!settings_.simulate_mic_gain && msg.has_level()) {
+    // The analog gain simulation is off, use the level stored in the AECdump.
+    last_specified_microphone_level_ = msg.level();

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

I think this CL would be easier to review with an explanation of how you plan to
add the simulation of the analog gain. 

To me it would make sense to have it in the same place as where the msg.level()
is used, so that it is clear where the selection is done, but you have not added
that part yet, right? 

Could you please explain how that is to be added? As it is now, the simulation
causes the reported analog gain to be bypassed, right?

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

I think the name last_specified_microphone_level_ is a bit misleading here.

It was before this CL present only in the wav-based simulator as a crude way of
mocking an analog gain via allowing the previously set level to be reported as
the current mic gain level.

Here, it is used in a different manner and I think the code would benefit from
it being renamed. For instance, on 172, last_specified_microphone_level is set
to the level in the aecdump which may not at all be the microphone level that
was specified by the AGC (which is how it was before used).

[aleloi, 2017/05/04 12:47:13]

Acknowledged.

   }
 }
 
@@ -562,14 +568,10 @@ 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);
-  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();
-  }
-
+  bool update_analog_level = true;
+  PrepareProcessStreamCall(msg, &update_analog_level);

[aleloi, 2017/05/04 12:47:13]

I think update_analog_level should be renamed in the method as well.

+  ProcessStream(interface_used_ == InterfaceType::kFixedInterface,
+                update_analog_level);
   VerifyProcessStreamBitExactness(msg);
 }