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..45e32f097402d23f19466ceb92adf9117eb1be9d 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,15 @@
  *  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/fake_recording_device.h"
 #include "webrtc/modules/audio_processing/test/protobuf_utils.h"
 #include "webrtc/test/testsupport/trace_to_stderr.h"
 
@@ -63,13 +67,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 +164,20 @@ 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());
+
+  RTC_DCHECK(fake_recording_device_);
+  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.
+    fake_recording_device_->set_undo_mic_level(rtc::Optional<int>(msg.level()));
   } 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() - i.e., overriding any value
+    // set from a gain controller once the previous audio frame has been
+    // analyzed.
+    fake_recording_device_->set_mic_level(msg.level());

[AleBzk, 2017/05/23 13:56:41]

@Per: you asked to move this to the parent class and store the level from the
AEC dump into a local variable.

First of all, note that I'm not using local variables anymore but directly the
fake recording device (which is a protected member of the parent class). Last
time you reviewed this CL, you commented on an old PS using variables.

Back to your main point. If we move this logic to the parent class, then the
latter needs sth like "if (aec dump case)", which I don't like much because for
the AEC dump case we have a dedicated implementation of the abstract class.

In this way instead, the parent class just does the stuff common to WAV and
AECdump, whereas the additional features specific of each case are done in
WavBasedSimulator and AecDumpBasedSimulator respectively.

Finally, note that msg.level() has two different applications depending on
whether the analog mic is simulated. To me, it has to be AecDumpBasedSimulator
that knows what to change in the simulation.

However, I understand that to me everything looks clear, but it could be
different for the reader. So, if you still see this confusing I propose the
following changes:
- in the parent class AudioProcessingSimulator, I add "rtc::Optional<int>
aec_dump_mic_level_" as protected member
- AecDumpBasedSimulator always sets its value in there
- AudioProcessingSimulator uses aec_dump_mic_level_ if available
- since WavBasedSimulator doesn't set it, there's no need of "if (aec dump)"

WDYT?

[peah-webrtc, 2017/05/23 22:13:20]

What I don't like with this approach is that it gives the impression that the
fake_recording_device actually modifies the level in
audio_processing_simulator.cc even though it is specified not to simulate the
mic gain. Furthermore, in order to see this that this really is not the case I'd
need to dig fairly deep into fake_recording_device.

I see your point about that the AudioProcessingSimulator code should not need to
know whether you are using a wav or an aecdump based simulation. It is
definitely valid but I don't think there is anyway around that in this case. The
latest patch actually also has such a dependency but in a hidden way as the
fake_recording_device knows what kind of simulation is running. I'd rather make
that more explicit.

What about something similar to the following code:

void AecDumpBasedSimulator::PrepareProcessStreamCall() {
...

level_ = rtc::Optional<int>(msg.level());
...
}


class AudioProcessingSimulator {
...
protected:
rtc::Optional<int> level_;
...
};

void AudioProcessingSimulator::ProcessStream(bool fixed_interface) {
    // Optionally use the fake recording device to simulate analog gain.
    RTC_DCHECK(fake_recording_device_);
    if (settings_.simulate_mic_gain) {
      if (fixed_interface) {
        fake_recording_device_->SimulateAnalogGain(level_, &fwd_frame_);
      } else {
        fake_recording_device_->SimulateAnalogGain(level_, in_buf_.get());
      }
    
ap_->gain_control()->set_stream_analog_level(fake_recording_device_->mic_level()
: level_));
   }
  else {
   // Use the level 100 as default if no other level has been externally
specified.
   ap_->gain_control()->set_stream_analog_level(level_ ? * level_ : 100));
 }
  ...
    // Process the current audio frame.
...
    // Store the mic level suggested by AGC if required.
   int new_level = ap_->gain_control()->stream_analog_level();
   if (settings_.simulate_mic_gain) {
     fake_recording_device_->set_mic_level(new_level);
   }
..
}

[AleBzk, 2017/06/22 10:16:00]

Done.

   }
 }
 
@@ -562,14 +576,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);
 }