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/logging.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 {
 
+constexpr FakeRecordingDevice::MicrophoneKind kDefaultMicKind =
+    FakeRecordingDevice::MicrophoneKind::kIdentity;
+
 void CopyFromAudioFrame(const AudioFrame& src, ChannelBuffer<float>* dest) {
   RTC_CHECK_EQ(src.num_channels_, dest->num_channels());
   RTC_CHECK_EQ(src.samples_per_channel_, dest->num_frames());
   // Copy the data from the input buffer.
   std::vector<float> tmp(src.samples_per_channel_ * src.num_channels_);
   S16ToFloat(src.data_, tmp.size(), tmp.data());
   Deinterleave(tmp.data(), src.samples_per_channel_, src.num_channels_,
                dest->channels());
 }
 
@@ skipping 33 matching lines @@
   for (size_t ch = 0; ch < dest->num_channels_; ++ch) {
     for (size_t sample = 0; sample < dest->samples_per_channel_; ++sample) {
       dest->data_[sample * dest->num_channels_ + ch] =
           src.channels()[ch][sample] * 32767;
     }
   }
 }
 
 AudioProcessingSimulator::AudioProcessingSimulator(
     const SimulationSettings& settings)
-    : settings_(settings) {
+        : settings_(settings),
+          new_analog_level_(settings.initial_mic_gain),

[peah-webrtc, 2017/05/16 12:19:35]

There is a naming confusion between level and gain which I think could be more
clear.Here new_analog_level_ is initialized to initial_mic_gain.

I think I prefer level over gain as that is the naming in stream_analog_level()
, WDYT? 

[AleBzk, 2017/05/17 11:52:23]

Right, i should have been consistent.
Yup!

+          fake_recording_device_(settings_.simulate_mic_gain ?
+              static_cast<FakeRecordingDevice::MicrophoneKind>(
+                  *settings.simulated_mic_kind) : kDefaultMicKind) {
   if (settings_.ed_graph_output_filename &&
       settings_.ed_graph_output_filename->size() > 0) {
     residual_echo_likelihood_graph_writer_.open(
         *settings_.ed_graph_output_filename);
     RTC_CHECK(residual_echo_likelihood_graph_writer_.is_open());
     WriteEchoLikelihoodGraphFileHeader(&residual_echo_likelihood_graph_writer_);
   }
 }
 
 AudioProcessingSimulator::~AudioProcessingSimulator() {
   if (residual_echo_likelihood_graph_writer_.is_open()) {
     WriteEchoLikelihoodGraphFileFooter(&residual_echo_likelihood_graph_writer_);
     residual_echo_likelihood_graph_writer_.close();
   }
 }
 
 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) {
+  // Optionally use the fake recording device to simulate analog gain.
+  if (settings_.simulate_mic_gain) {
+    if (fixed_interface) {
+      fake_recording_device_.SimulateAnalogGain(
+          new_analog_level_, real_recording_device_level_, &fwd_frame_);
+    } else {
+      // TODO(alessiob): Remove DCHECKs below once Per has reviewed.
+      RTC_DCHECK_EQ(in_config_.num_channels(), in_buf_->num_channels());

[peah-webrtc, 2017/05/16 12:19:35]

Why are these DCHECK-s needed here now? There should be nothing new that is
changed in this CL that requires that, right?

If the DCHECKS is something that missed to be checked I think that should
instead happen when in_config_ and in_buf_ are created.

[AleBzk, 2017/05/17 11:52:23]

Sorry, I should have added a comment for you here instead of a TODO.

If you see what's changed, you can notice that I call num_channels() and
num_frames() on in_buf_, whereas before they were called on in_config_. Since
in_buf_ is passed into SimulateAnalogGain(), I just use it to get the number of
channels and frames.

Anyways, I tested that part of the code, so I remove the DCHECKs.

+      RTC_DCHECK_EQ(in_config_.num_frames(), in_buf_->num_frames());
+      fake_recording_device_.SimulateAnalogGain(
+          new_analog_level_, real_recording_device_level_, in_buf_.get());
+    }
+  }
+
+  // Notify the mic gain level to AGC.

[peah-webrtc, 2017/05/16 12:19:35]

Both using  gain and level here sounds like duplicate terminology.

[AleBzk, 2017/05/17 11:52:23]

Done.

+  RTC_CHECK_EQ(AudioProcessing::kNoError,
+               ap_->gain_control()->set_stream_analog_level(new_analog_level_));

[peah-webrtc, 2017/05/16 12:19:35]

This is not correct I think. There is nothing in the usage of APM that ensures
that the actually applied analog level is what is reported was applied.

My suggestion is to make new_analog_level_ part of fake_recording device_ (but
with a new name) and let this method ask fake_recording device_ for what level
is applied.

E.g
ap_->gain_control()->set_stream_analog_level(settings_.simulate_mic_gain ?
fake_recording device_.GetLevel() : real_recording_device_level_);

ap_->ProcessStream(...);

int level = ap_->gain_control()->stream_analog_level();
if (settings_.simulate_mic_gain) {
  fake_recording device_.GetLevel(level);
}

[AleBzk, 2017/05/17 11:52:23]

I completely agree with this point.
In fact, a user could for instance manually change the input mic gain, or the
hardware/driver can override the set value.
Maybe I'm wrong, but what you suggest looks exactly like it was when I first
submitted this CL.
Look at
https://codereview.webrtc.org/2834643002/diff/100001/webrtc/modules/audio_pro...
around line 115. There was a fake mic class storing the mic value. Maybe at that
time, some details were missing or unclear.

I rewrote FakeRecordingDevice, which holds the current mic level replacing the
role of |new_analog_level_|.

[peah-webrtc, 2017/05/17 14:52:12]

What I stated was that "I'd prefer a decoupling between the stored stream analog
level and the fake
audio device for the case when there is no simulation of the analog gain..."

I think that still makes sense, and it is like that for the suggested construct 
ap_->gain_control()->set_stream_analog_level(settings_.simulate_mic_gain ?
fake_recording device_.GetLevel() : real_recording_device_level_);

I see, however, that I probably mixed up the real_recording_device_level_ and
new_analog_level variables but, done right, I actually think that this is
according to the referred statement.

Or maybe I missed something?

+
+  // Process the current audio frame.
   if (fixed_interface) {
     {
       const auto st = ScopedTimer(mutable_proc_time());
       RTC_CHECK_EQ(AudioProcessing::kNoError, ap_->ProcessStream(&fwd_frame_));
     }
     CopyFromAudioFrame(fwd_frame_, out_buf_.get());
   } else {
     const auto st = ScopedTimer(mutable_proc_time());
     RTC_CHECK_EQ(AudioProcessing::kNoError,
                  ap_->ProcessStream(in_buf_->channels(), in_config_,
                                     out_config_, out_buf_->channels()));
   }
 
+  // Store the mic gain level suggested by AGC if required.

[peah-webrtc, 2017/05/16 12:19:35]

Both using  gain and level here sounds like duplicate terminology.

[AleBzk, 2017/05/17 11:52:23]

Right, thanks for the comment.
I only left level.

+  new_analog_level_ = ap_->gain_control()->stream_analog_level();
+
   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 57 matching lines @@
   fwd_frame_.samples_per_channel_ =
       rtc::CheckedDivExact(fwd_frame_.sample_rate_hz_, kChunksPerSecond);
   fwd_frame_.num_channels_ = input_num_channels;
 
   rev_frame_.sample_rate_hz_ = reverse_input_sample_rate_hz;
   rev_frame_.samples_per_channel_ =
       rtc::CheckedDivExact(rev_frame_.sample_rate_hz_, kChunksPerSecond);
   rev_frame_.num_channels_ = reverse_input_num_channels;
 
   if (settings_.use_verbose_logging) {
+    rtc::LogMessage::LogToDebug(rtc::LS_VERBOSE);
+
     std::cout << "Sample rates:" << std::endl;
     std::cout << " Forward input: " << input_sample_rate_hz << std::endl;
     std::cout << " Forward output: " << output_sample_rate_hz << std::endl;
     std::cout << " Reverse input: " << reverse_input_sample_rate_hz
               << std::endl;
     std::cout << " Reverse output: " << reverse_output_sample_rate_hz
               << std::endl;
     std::cout << "Number of channels: " << std::endl;
     std::cout << " Forward input: " << input_num_channels << std::endl;
     std::cout << " Forward output: " << output_num_channels << std::endl;
@@ skipping 182 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