audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/test/audio_processing_simulator.cc

Index: webrtc/modules/audio_processing/test/audio_processing_simulator.cc
diff --git a/webrtc/modules/audio_processing/test/audio_processing_simulator.cc b/webrtc/modules/audio_processing/test/audio_processing_simulator.cc
index 58b47e221316191fb6f5c19004ba25576c83719b..9a42152ad40718a65de265904cbaaaa4f0b237bd 100644
--- a/webrtc/modules/audio_processing/test/audio_processing_simulator.cc
+++ b/webrtc/modules/audio_processing/test/audio_processing_simulator.cc
@@ -14,17 +14,23 @@
 #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"
+#include "webrtc/modules/audio_processing/test/fake_recording_device.h"
 
 namespace webrtc {
 namespace test {
 namespace {
 
+constexpr FakeRecordingDevice::DeviceKind kDefaultFakeRecDeviceKind =
+    FakeRecordingDevice::DeviceKind::IDENTITY;
+
 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());
@@ -79,7 +85,13 @@ void CopyToAudioFrame(const ChannelBuffer<float>& src, AudioFrame* dest) {
 
 AudioProcessingSimulator::AudioProcessingSimulator(
     const SimulationSettings& settings)
-    : settings_(settings) {
+        : settings_(settings),
+          fake_recording_device_(new FakeRecordingDevice(

[peah-webrtc, 2017/06/29 05:45:27]

You don't need to create this dynamically. Since you have it in the initializer
list, you can instead just initialized it without dynamic allocation.

[AleBzk, 2017/06/29 11:43:35]

Thanks. I recalled why I added the member as a unique pointer. I wanted forward
declaration in the header file. But I guess the option you suggest is preferred
in the end.

+            settings.initial_mic_level,
+            settings_.simulate_mic_gain ? static_cast<
+                FakeRecordingDevice::DeviceKind>(*settings.simulated_mic_kind)
+                    : kDefaultFakeRecDeviceKind)) {
+  RTC_DCHECK(fake_recording_device_);
   if (settings_.ed_graph_output_filename &&
       settings_.ed_graph_output_filename->size() > 0) {
     residual_echo_likelihood_graph_writer_.open(
@@ -104,6 +116,37 @@ AudioProcessingSimulator::ScopedTimer::~ScopedTimer() {
 }
 
 void AudioProcessingSimulator::ProcessStream(bool fixed_interface) {
+  if (settings_.aec_dump_input_filename) {
+    RTC_DCHECK(aec_dump_mic_level_);
+    if (settings_.simulate_mic_gain) {
+      // When the analog gain is sumulated and an AEC dump is used as input, set
+      // the undo level to |aec_dump_mic_level_| to virtually restore the
+      // unmodified microphone signal level.
+      fake_recording_device_->set_undo_mic_level(aec_dump_mic_level_);
+    } else {
+      // When the analog gain is not simulated, the AEC dump level must to be
+      // used to override the value set from the gain controller in the
+      // previously analyzed audio frame.
+      fake_recording_device_->set_mic_level(*aec_dump_mic_level_);

[AleBzk, 2017/06/22 10:16:01]

Line 130 overrides what is set in lines 145-147.

[peah-webrtc, 2017/06/29 05:45:26]

As I commented on a previous patch, I definitely don't think that the
fake_recording device should be involved when the mic gain is not simulated. 

If you avoid that, you can skip the set_mic_level method. 

[AleBzk, 2017/06/29 11:43:35]

Thanks. Sorry for having kept this. It's up to us deciding if this could make
sense.
My view is that, when an AEC dump is used as input, AGC should behave as close
as possible as in the recorded call. This can be done by informing AGC on the
mic gain that was set during the call.

But I also understand that, since no analog mic gain is simulated, there's not
much to observe.

I'll remove as you suggest.

+    }
+  }

[AleBzk, 2017/06/22 10:16:01]

About lines 119-132: the AEC dump analog gain simulation logic has been moved
here following Per's request.

+
+  // Optionally use the fake recording device to simulate analog gain.
+  RTC_DCHECK(fake_recording_device_);
+  if (settings_.simulate_mic_gain) {
+    if (fixed_interface) {
+      fake_recording_device_->SimulateAnalogGain(&fwd_frame_);

[peah-webrtc, 2017/06/29 05:45:26]

Why not just add the optional aec_dump_mic_level_ as an input to
SimulateAnalogGain? That would eliminate the need for the lines 119-132.

[AleBzk, 2017/06/29 11:43:35]

I'd prefer as it is now, otherwise we have to add nested "if"s since we have 4
combinations for (fixed_interface, aec_dum_input && simulate_gain).

Also, from a design perspective, either we pass all the parameters (i.e., undo
mic level and mic level) to SimulateAnalogGain() or just the buffer to process.

WDYT?

[peah-webrtc, 2017/06/29 22:03:59]

I would personally prefer to have it as one single call as it gives shorter code
and I don't think that would complicate anything.. But lets see how it turns out
with the changes done in response to the comment above.

+    } else {
+      fake_recording_device_->SimulateAnalogGain(in_buf_.get());
+    }
+  }
+
+  // Notify the current mic level to AGC.
+  RTC_CHECK_EQ(AudioProcessing::kNoError,
+               ap_->gain_control()->set_stream_analog_level(
+                  fake_recording_device_->mic_level()));

[peah-webrtc, 2017/06/29 05:45:27]

This is something that I think complicates the code quite a bit. As I commented
on a previous patch, I don't think that the fake recording device should at all
be involved with the mic_level if the mic gain should not be simulated.

As it is now, it seems from this line as if the fake_recording_device is
choosing the mic level, instead of that the mic level in the aec_dump is passed.
Furthermore, verifying that that is not the case is not that straightforward.

Why not do as I proposed in a comment on a previous patch, and pass in 
*aec_dump_mic_level_ directly.

[AleBzk, 2017/06/29 11:43:35]

What happens during a call is that AGC suggests a gain but for many different
reasons, the suggestion can be overridden (e.g., the user manually sets the mic
gain via OS UI). I haven't verified if AGC makes use of the actual mic gain to
suggest a new one or if it uses the sequence of suggested gains blindly. That's
why I think we'd better to tell AGC the actual mic gain.

I understand that you find this confusing, but this is an AGC API issue - and in
fact we planned to eliminate the "set_stream_analog_level, ProcessStream,
stream_analog_level" sequence of invocations (error prone). My suggestion is
that until the AGC API remains as it is, we keep this call, maybe adding more
details in the comment.

WDYT?

[peah-webrtc, 2017/06/29 22:03:59]

You definitely need to pass the mic gain to the fake recording device when it is
used, as that is how the AGC operates. 

The concern I have is that you involve the fake recording device even when it is
not used, and to me that is confusing. What I meant with my comment is that I
want you to directly pass the gain that is stored in the recording (or rather
copied from the recording) . As it is now, the gain is copied into fakeaudio
device and then passed from that into the agc which gives the impression that
the fakeaudiodevice is involved, which it is not.

This change does not require any change in the AGC API.

+
+  // Process the current audio frame.
   if (fixed_interface) {
     {
       const auto st = ScopedTimer(mutable_proc_time());
@@ -117,6 +160,10 @@ void AudioProcessingSimulator::ProcessStream(bool fixed_interface) {
                                     out_config_, out_buf_->channels()));
   }
 
+  // Store the mic level suggested by AGC if required.

[peah-webrtc, 2017/06/29 05:45:27]

The comment "if required" seems out of place. Could you please clarify it, or
rephrase it?

[AleBzk, 2017/06/29 11:43:35]

Done.

+  fake_recording_device_->set_mic_level(
+      ap_->gain_control()->stream_analog_level());
+
   if (buffer_writer_) {
     buffer_writer_->Write(*out_buf_);
   }
@@ -194,6 +241,8 @@ void AudioProcessingSimulator::SetupBuffersConfigsOutputs(
   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;