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..e7be0fa7a5c80417c315abd0c1253c819b84b575 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"
 
@@ -68,8 +71,7 @@ AecDumpBasedSimulator::AecDumpBasedSimulator(const SimulationSettings& settings)
 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.
@@ -158,12 +160,24 @@ void AecDumpBasedSimulator::PrepareProcessStreamCall(
 
   // TODO(peah): Add support for controlling the analog level via the

[peah-webrtc, 2017/05/05 06:28:41]

You can remove this TODO in this CL :-)

[AleBzk, 2017/05/05 12:20:17]

Done.

   // 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;
+
+  // Level is always logged in AEC dumps.
+  RTC_CHECK(msg.has_level());
+
+  // When the analog gain is simulated, set the undo level to |msg.level()| to
+  // virtually restore the unmodified microphone signal level.
+  if (settings_.simulate_mic_gain) {
+    fake_recording_device_.NotifyAudioDeviceLevel(msg.level());

[peah-webrtc, 2017/05/05 06:28:41]

I'd prefer all the handling of the fake_recording_device_ to go into the
audio_processing_simulator code. That would simplify the usage of the device, as
at least to me the usage of Notification, setting of the analog level and
ProcessStream is very distributed and hard to overlook.

My suggestion would be to solve this by simply passing the msg.level() into the
ProcessStream call on line 577. Then you'd be able to do all of this inside
ProcessStream, and aec_dump_simulator would not need to use FakeAudioDevice.

Something that would conform even more to how the incoming audio streams are
used would maybe be to update a state variable with the new level, similarly to
how it is done for std::unique_ptr<ChannelBuffer<float>> in_buf_, etc. I.e., not
pass this information via the ProcessStream call. I think that would simplify
the use of this from the WavBasedSimulator as well (as otherwise you'd need to
have a different ProcessStream call for that (or use an optional in the call).

In code
class AudioProcessingSimulator {
  

 void ProcessStream() {
  ...
  if (settings_.simulate_mic_gain) {
   fake_audiodevice.ProcessStream(level_for_input_, samples);
  
ap_->gain_control()->set_stream_analog_level(fake_recording_device_.analog_level());
  }
  else {
   ap_->gain_control()->set_stream_analog_level(*level_for_input_);
  }
  ap_->ProcessStream(samples);

  ...
}


  rtc::Optional<int> level_for_input_;
};

class AecDumpBasedSimulator::PrepareProcessStreamCall() {
 ...
 level_for_input_ = rtc::Optional<int>(msg.level());
 ...




WDYT?

[AleBzk, 2017/05/05 12:20:17]

Right. I made fake_recording_device_ private instead of protected.
Plus, FakeRecordingDevice::SimulateAnalogGain() now also gets the mic level to
simulate and the level of the real mic device (if known).
Yup. I think the changes I've made to address other comments cover this.
Take a look and let me know.

+    LOG(LS_VERBOSE) << "setting mic gain undo level from AEC dump to "

[peah-webrtc, 2017/05/05 06:28:41]

setting->Setting?

[AleBzk, 2017/05/05 12:20:17]

Done.

+        << msg.level();

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

This would get logged once for every audio frame. Even for LS_VERBOSE it seems a
bit too much. I don't know how what typically use verbose logging for, though.
It seems to be used a bit in neteq_impl.cc

[peah-webrtc, 2017/05/05 06:28:41]

Yes, I agree. The logging here is only in test code, but it will overwhelm the
log with these log messages so I don't think we want that. For logging I think
it more make sense to signal certain events. This is something that we know
always happens so we don't need to log that.

[AleBzk, 2017/05/05 12:20:17]

I moved this in the ctor so it's shown once.

+  }
+
+  // When the analog gain is not simulated, the AEC dump level has to be used in
+  // AudioProcessingSimulator::ProcessStream().
+  if (!settings_.simulate_mic_gain) {

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

Suggest merge into the if statement above as an else branch.

[AleBzk, 2017/05/05 12:20:17]

Done.

[AleBzk, 2017/05/05 12:20:17]

Done.

+    fake_recording_device_.set_analog_level(msg.level());
+    LOG(LS_VERBOSE) << "AEC dump overriding AGC suggested level to "
+        << msg.level();
   }
 }
 
@@ -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);
 }