Extension and refactoring of the audioproc_f tool to be a fully fledged tool for audio processing m…

webrtc/modules/audio_processing/test/wav_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 "webrtc/modules/audio_processing/test/wav_based_simulator.h"
+
+#include "webrtc/base/checks.h"
+#include "webrtc/test/testsupport/trace_to_stderr.h"
+
+namespace webrtc {
+namespace test {
+
+std::vector<WavBasedSimulator::SimulationEventType>
+WavBasedSimulator::GetDefaultEventChain() const {
+  std::vector<WavBasedSimulator::SimulationEventType> call_chain(2);
+  call_chain[0] = SimulationEventType::kProcessStream;
+  call_chain[1] = SimulationEventType::kProcessReverseStream;
+  return call_chain;
+}
+
+void WavBasedSimulator::PrepareProcessStreamCall() {
+  if (settings_.fixed_interface) {
+    CopyToAudioFrame(*in_buf_, &fwd_frame_);
+  }
+  ap_->set_stream_key_pressed(true);

[aluebs-webrtc, 2016/04/30 02:08:05]

Isn't there a setting for this? For example if the TS is enabled?

[peah-webrtc, 2016/05/02 06:18:42]

True. Done.

+
+  RTC_CHECK_EQ(AudioProcessing::kNoError,
+               ap_->set_stream_delay_ms(
+                   settings_.stream_delay ? *settings_.stream_delay : 0));
+
+  ap_->echo_cancellation()->set_stream_drift_samples(
+      settings_.stream_drift_samples ? *settings_.stream_drift_samples : 0);
+
+  RTC_CHECK_EQ(AudioProcessing::kNoError,
+               ap_->gain_control()->set_stream_analog_level(
+                   last_specified_microphone_level_));

[aluebs-webrtc, 2016/04/30 02:08:05]

last_specified_microphone_level_ is never modified. Should it be a constant?

[peah-webrtc, 2016/05/02 06:18:42]

That is a mistake. The idea is to query the last level from the AGC but that
code got lost. I readded it. PTAL.

[aluebs-webrtc, 2016/05/03 22:30:40]

Maybe I am not familiar enough with this setting, but why do we need to query
it, to set it again here? Doesn't it maintain its value internally?

[peah-webrtc, 2016/05/09 11:37:31]

I'm not sure how it works in APM but this is how process_test.cc does it.

And I think it makes sense. analog_stream_level() reports what level APM thinks
the analog level should be at, and set_analog_stream_level() reports what the
analog level was actually set to. So by doing it like this we basically simulate
an analog microphone level that is set exactly to what APM wants.

[aluebs-webrtc, 2016/05/09 16:27:31]

Sounds good.

[peah-webrtc, 2016/05/11 12:19:27]

Acknowledged.

+}
+
+void WavBasedSimulator::PrepareReverseProcessStreamCall() {
+  if (settings_.fixed_interface) {
+    CopyToAudioFrame(*reverse_in_buf_, &rev_frame_);
+  }
+}
+
+void WavBasedSimulator::Process() {
+  std::unique_ptr<test::TraceToStderr> trace_to_stderr;
+  if (settings_.use_verbose_logging) {
+    trace_to_stderr.reset(new test::TraceToStderr(true));
+  }
+
+  call_chain_ = GetDefaultEventChain();
+  CreateAudioProcessor();
+
+  Initialize();
+
+  bool samples_left_to_process = true;
+  int call_chain_index = 0;
+  int num_forward_chunks_processed = 0;
+  while (samples_left_to_process) {
+    switch (call_chain_[call_chain_index]) {

[aluebs-webrtc, 2016/04/30 02:08:05]

Do we really need this enum-function-vector-switch complexity to just toggle
between ProcessStream and ProcessReverseStream? Can't we just call one after the
other in the loop directly?

[peah-webrtc, 2016/05/02 06:18:42]

The idea is to simplify changing the default call-order if needed. For instance,
it is sometimes important to evaluate call-orders like RFRF... or RRFRRFFF...
instead of the standard FRFR... (F=forward, R=reverse).
This is much simpler to alter if the order is stored in a vector rather than
hardcoded by toggling between the calls.

[aluebs-webrtc, 2016/05/03 22:30:40]

If you think this is a enough frequent use case to justify the higher code
complexity, that is ok with me.

[peah-webrtc, 2016/05/09 11:37:31]

Acknowledged.

+      case SimulationEventType::kProcessStream:
+        samples_left_to_process = HandleProcessStreamCall();
+        ++num_forward_chunks_processed;
+        break;
+      case SimulationEventType::kProcessReverseStream:
+        if (settings_.reverse_input_filename) {
+          samples_left_to_process = HandleProcessReverseStreamCall();
+        }
+        break;
+      default:
+        RTC_CHECK(false);
+    }
+
+    call_chain_index = (call_chain_index + 1) % call_chain_.size();
+
+    if (trace_to_stderr) {
+      trace_to_stderr->SetTimeSeconds(
+          num_forward_chunks_processed * 1.f /

[aluebs-webrtc, 2016/04/30 02:08:05]

You can cast to avoid a multiplication here.

[peah-webrtc, 2016/05/02 06:18:42]

Done.

+          AudioProcessingSimulator::kChunksPerSecond);
+    }
+  }
+
+  DestroyAudioProcessor();
+}
+
+bool WavBasedSimulator::HandleProcessStreamCall() {
+  if (buffer_reader_->Read(in_buf_.get())) {

[aluebs-webrtc, 2016/04/30 02:08:05]

I like functions with only one return statement:

bool samples_left_to_process = buffer_reader_->Read(in_buf_.get());
if (samples_left_to_process) {
  ...
}
return samples_left_to_process;

But anyway, you don't need the else clause.

[peah-webrtc, 2016/05/02 06:18:42]

Nice!
Done.

+    PrepareProcessStreamCall();
+    ProcessStream(settings_.fixed_interface);

[aluebs-webrtc, 2016/04/30 02:08:05]

Why do we need to pass in this setting? Can't AudioProcessingSimulator access
itself?

[peah-webrtc, 2016/05/02 06:18:42]

It could for the wav-based simulation but it cannot for the aecdump-based
simulation as this is stored in the aecdump and the reading of that is done
outside AudioProcessingSimulator.
Therefore, in order to make the operations in the wav-based and aecdump-based
processing as simular as possibly I implemented it like this.

I could add another ProcessStream call in AudioProcessingSimulator that takes no
arguments and which determines the interface based on the settings but I prefer
this variant since
1) I think it simplifies the code not to extende the AudioProcessingSimulator
api further.
2) The alternative will require more lines of code.

WDYT? 
As it is now

[aluebs-webrtc, 2016/05/03 22:30:40]

If that is the case, it makes sense to leave as is. I didn't know we stored the
interface used in the aecdump.

[peah-webrtc, 2016/05/09 11:37:31]

Acknowledged.

+    return true;
+  } else {
+    return false;
+  }
+}
+
+bool WavBasedSimulator::HandleProcessReverseStreamCall() {
+  if (reverse_buffer_reader_->Read(reverse_in_buf_.get())) {
+    PrepareReverseProcessStreamCall();
+    ProcessReverseStream(settings_.fixed_interface);
+    return true;
+  } else {
+    return false;
+  }
+}
+
+void WavBasedSimulator::Initialize() {
+  std::unique_ptr<WavReader> in_file(
+      new WavReader(settings_.input_filename->c_str()));
+  int input_sample_rate_hz = in_file->sample_rate();
+  int input_num_channels = in_file->num_channels();
+  buffer_reader_.reset(new ChannelBufferWavReader(std::move(in_file)));
+
+  int output_sample_rate_hz = settings_.output_sample_rate_hz
+                                  ? *settings_.output_sample_rate_hz
+                                  : input_sample_rate_hz;
+  int output_num_channels = settings_.output_num_channels
+                                ? *settings_.output_num_channels
+                                : input_num_channels;
+
+  int reverse_sample_rate_hz = 48000;
+  int reverse_num_channels = 1;
+  int reverse_output_sample_rate_hz = 48000;
+  int reverse_output_num_channels = 1;
+  if (settings_.reverse_input_filename) {
+    std::unique_ptr<WavReader> reverse_in_file(
+        new WavReader(settings_.reverse_input_filename->c_str()));
+    reverse_sample_rate_hz = reverse_in_file->sample_rate();
+    reverse_num_channels = reverse_in_file->num_channels();
+    reverse_buffer_reader_.reset(
+        new ChannelBufferWavReader(std::move(reverse_in_file)));
+
+    reverse_output_sample_rate_hz =
+        settings_.reverse_output_sample_rate_hz
+            ? *settings_.reverse_output_sample_rate_hz
+            : reverse_sample_rate_hz;
+    reverse_output_num_channels = settings_.reverse_output_num_channels
+                                      ? *settings_.reverse_output_num_channels
+                                      : reverse_num_channels;
+  }
+
+  SetupBuffersConfigsOutputs(
+      input_sample_rate_hz, output_sample_rate_hz, reverse_sample_rate_hz,
+      reverse_output_sample_rate_hz, input_num_channels, output_num_channels,
+      reverse_num_channels, reverse_output_num_channels);
+}
+
+}  // namespace test
+}  // namespace webrtc