audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  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"
 
 namespace webrtc {
 namespace test {
 namespace {
 
 // Verify output bitexactness for the fixed interface.
 // TODO(peah): Check whether it would make sense to add a threshold
 // to use for checking the bitexactness in a soft manner.
@@ skipping 30 matching lines @@
         }
       }
     }
   }
   return true;
 }
 
 }  // 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.
     RTC_CHECK(interface_used_ == InterfaceType::kFixedInterface ||
               interface_used_ == InterfaceType::kNotSpecified);
     interface_used_ = InterfaceType::kFixedInterface;
 
     // Populate input buffer.
     RTC_CHECK_EQ(sizeof(fwd_frame_.data_[0]) * fwd_frame_.samples_per_channel_ *
                      fwd_frame_.num_channels_,
@@ skipping 66 matching lines @@
   }
 
   if (!settings_.use_ts) {
     if (msg.has_keypress()) {
       ap_->set_stream_key_pressed(msg.keypress());
     }
   } else {
     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.

   }
 }
 
 void AecDumpBasedSimulator::VerifyProcessStreamBitExactness(
     const webrtc::audioproc::Stream& msg) {
   if (bitexact_output_) {
     if (interface_used_ == InterfaceType::kFixedInterface) {
       bitexact_output_ = VerifyFixedBitExactness(msg, fwd_frame_);
     } else {
       bitexact_output_ = VerifyFloatBitExactness(msg, out_config_, *out_buf_);
@@ skipping 378 matching lines @@
   }
 
   SetupBuffersConfigsOutputs(
       msg.sample_rate(), output_sample_rate, msg.reverse_sample_rate(),
       reverse_output_sample_rate, msg.num_input_channels(), num_output_channels,
       msg.num_reverse_channels(), num_reverse_output_channels);
 }
 
 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);
 }
 
 void AecDumpBasedSimulator::HandleMessage(
     const webrtc::audioproc::ReverseStream& msg) {
   PrepareReverseProcessStreamCall(msg);
   ProcessReverseStream(interface_used_ == InterfaceType::kFixedInterface);
 }
 
 }  // namespace test
 }  // namespace webrtc