audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/test/audio_processing_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 "webrtc/modules/audio_processing/test/audio_processing_simulator.h"
 
 #include <algorithm>
 #include <iostream>
 #include <sstream>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/stringutils.h"
 #include "webrtc/common_audio/include/audio_util.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 
 namespace webrtc {
 namespace test {
 namespace {
@@ skipping 68 matching lines @@
   }
 }
 
 AudioProcessingSimulator::ScopedTimer::~ScopedTimer() {
   int64_t interval = rtc::TimeNanos() - start_time_;
   proc_time_->sum += interval;
   proc_time_->max = std::max(proc_time_->max, interval);
   proc_time_->min = std::min(proc_time_->min, interval);
 }
 
-void AudioProcessingSimulator::ProcessStream(bool fixed_interface) {
+void AudioProcessingSimulator::ProcessStream(bool fixed_interface,
+                                             bool update_analog_level) {

[peah-webrtc, 2017/04/26 12:54:44]

I think the naming of the update_analog_level could be done to be more
descriptive.

I interpret it as a verb meaning that the analog level should be updated which I
see as that the new leve reported by APM should be applied to update the analog
gain. But that is not what it means, right? What it rather means, is that the
reported analog level should be specified in the APM, which is quite different.

+  if (update_analog_level) {
+    RTC_CHECK_EQ(AudioProcessing::kNoError,
+                 ap_->gain_control()->set_stream_analog_level(
+                     last_specified_microphone_level_));
+  }
   if (fixed_interface) {
     {
       const auto st = ScopedTimer(mutable_proc_time());
+      // TODO(alessiob): Apply last_specified_microphone_level_ to fwd_frame_

[peah-webrtc, 2017/04/26 12:54:44]

I think the approach to apply the microphone level is not sufficiently general.
For instance, separating the gain with the gain application would need (at
least) two different models of one microphone to be used to properly simulate
soft-clipping. 

Have you considered bundling the simulation of the analog gain, and the gain
application, into a virtual microphone model?

+      // simulating a mic with analog gain.
       RTC_CHECK_EQ(AudioProcessing::kNoError, ap_->ProcessStream(&fwd_frame_));
     }
     CopyFromAudioFrame(fwd_frame_, out_buf_.get());
   } else {
     const auto st = ScopedTimer(mutable_proc_time());
+    // TODO(alessiob): Apply last_specified_microphone_level_ to
+    // in_buf_->channels() simulating a mic with analog gain.
     RTC_CHECK_EQ(AudioProcessing::kNoError,
                  ap_->ProcessStream(in_buf_->channels(), in_config_,
                                     out_config_, out_buf_->channels()));
   }
+  // Update last_specified_microphone_level_ using the value suggested by AGC

[peah-webrtc, 2017/04/26 12:54:44]

the AGC

+  // or the default if settings_.simulate_mic_gain is false.
+  if (update_analog_level) {
+    last_specified_microphone_level_ = settings_.simulate_mic_gain ?

[hlundin-webrtc, 2017/04/26 12:11:37]

This is confusing. If update_analog_level is true, but we have not set
simulate_mic_gain, then the effect of update_analog_level is essentially a
reset. To me it seems that we are overloading the meaning of
last_specified_microphone_level_ and update_analog_level.

update_analog_level seems to specify that we should set and get the gain from
gain_control(), but it also resets the level which was set from the AECdump
file.

[peah-webrtc, 2017/04/26 12:54:44]

This is not correct. If you don't simulate the mic gain, you should not reset
the gain. I think it works in practice though as you in the dump-simulator will
set the last_specified_microphone_level_  before the next call. The
else-statement in this expression is however fully redundant and misleading.

I think the separation that is introduced between where the simulated gain, and
the recorded gain are applied stored in order to be applied in the call to
ap_->gain_control()->set_stream_analog_level(..) makes the code harder to read.
I'd very much prefer for that to be done at the same place in an if-statement so
that it is easy to see where how that gain is chosen/computed.

+        ap_->gain_control()->stream_analog_level()
+            : kInitialMicrophoneGainLevel;
+  }
 
   if (buffer_writer_) {
     buffer_writer_->Write(*out_buf_);
   }
 
   if (residual_echo_likelihood_graph_writer_.is_open()) {
     auto stats = ap_->GetStatistics();
     residual_echo_likelihood_graph_writer_ << stats.residual_echo_likelihood
                                            << ", ";
   }
@@ skipping 260 matching lines @@
     size_t kMaxFilenameSize = AudioProcessing::kMaxFilenameSize;
     RTC_CHECK_LE(settings_.aec_dump_output_filename->size(), kMaxFilenameSize);
     RTC_CHECK_EQ(AudioProcessing::kNoError,
                  ap_->StartDebugRecording(
                      settings_.aec_dump_output_filename->c_str(), -1));
   }
 }
 
 }  // namespace test
 }  // namespace webrtc