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/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());
+
+  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();
   }
 }
 
 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