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 35 matching lines @@
 }
 
 }  // namespace
 
 AecDumpBasedSimulator::AecDumpBasedSimulator(const SimulationSettings& settings)
     : AudioProcessingSimulator(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.
     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

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