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

webrtc/modules/audio_processing/test/audioproc_float.cc

 /*
- *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.

[aluebs-webrtc, 2016/04/26 04:33:39]

I don't think the year should be updated because of a refactoring.

[peah-webrtc, 2016/04/26 07:19:29]

Done.

[aluebs-webrtc, 2016/04/27 16:06:47]

It was 2014 though.

[peah-webrtc, 2016/04/28 07:41:11]

Sorry.
Done.

  *
  *  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 <stdio.h>
-
-#include <iostream>
 #include <memory>
-#include <sstream>
-#include <string>
-#include <utility>
 
 #include "gflags/gflags.h"
-#include "webrtc/base/checks.h"
-#include "webrtc/base/format_macros.h"
-#include "webrtc/common_audio/channel_buffer.h"
-#include "webrtc/common_audio/wav_file.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
-#include "webrtc/modules/audio_processing/test/audio_file_processor.h"
-#include "webrtc/modules/audio_processing/test/protobuf_utils.h"
-#include "webrtc/modules/audio_processing/test/test_utils.h"
-#include "webrtc/system_wrappers/include/tick_util.h"
-#include "webrtc/test/testsupport/trace_to_stderr.h"
-
+#include "webrtc/modules/audio_processing/test/aec_dump_based_simulator.h"
+#include "webrtc/modules/audio_processing/test/audio_processing_simulator.h"
+#include "webrtc/modules/audio_processing/test/wav_based_simulator.h"
+
+namespace webrtc {
+namespace test {
 namespace {
 
-bool ValidateOutChannels(const char* flagname, int32_t value) {
-  return value >= 0;
+const int kParameterNotSpecifiedValue = -10000;
+
+const char kUsageDescription[] =
+    "Usage: audioproc_f [options] -i <input.wav>\n"
+    "                   or\n"
+    "                    audioproc_f [options] -dump <aec_dump>\n"
+    "\n\n"
+    "Command-line tool to simulate a call using the audio "
+    "processing module, either based on wav files or "
+    "protobuf debug dump recordings and writes to an output";
+
+DEFINE_string(dump_input, "", "Aec dump input filename");
+DEFINE_string(dump_output, "", "Aec dump output filename");
+DEFINE_string(i, "", "Forward stream input wav filename");
+DEFINE_string(o, "", "Forward stream output wav filename");

[aluebs-webrtc, 2016/04/26 04:33:39]

Don't we ant some default value for the output file?

[peah-webrtc, 2016/04/26 07:19:29]

I don't think so. In many cases the output is not needed when running
audioproc(_f) and I don't see why we would need a default for the output. This
is also according to how it was done in process_test.cc.

[aluebs-webrtc, 2016/04/27 16:06:47]

I think in most of the cases the output is of interest and we avoid having to
specify this parameter, but I see your point.

[peah-webrtc, 2016/04/28 07:41:11]

Acknowledged.

+DEFINE_string(ri, "", "Reverse stream input wav filename");
+DEFINE_string(ro, "", "Reverse stream output wav filename");
+DEFINE_int32(num_output_channels,
+             kParameterNotSpecifiedValue,
+             "Number of forward stream output channels (1-2)");

[aluebs-webrtc, 2016/04/26 04:33:39]

Any number of channels is supported, right?

[peah-webrtc, 2016/04/26 07:19:29]

I'll change that to support any channels. 
Done.

+DEFINE_int32(num_reverse_output_channels,
+             kParameterNotSpecifiedValue,
+             "Number of Reverse stream output channels (1-2)");
+DEFINE_int32(output_sample_rate_hz,
+             kParameterNotSpecifiedValue,
+             "Forward stream output sample rate in Hz (1-48000)");
+DEFINE_int32(reverse_output_sample_rate_hz,
+             kParameterNotSpecifiedValue,
+             "Reverse stream output sample rate in Hz (1-48000)");
+DEFINE_string(mic_positions,
+              "",
+              "Space delimited cartesian coordinates of microphones in "
+              "meters. The coordinates of each point are contiguous. For a "
+              "two element array: \"x1 y1 z1 x2 y2 z2\"");
+DEFINE_int32(target_angle_degrees,
+             90,
+             "The azimuth of the target in degrees (0-90). Only applies to "
+             "beamforming.");
+DEFINE_bool(fixed_interface,
+            false,
+            "Use the fixed interface when operating on wav files");
+DEFINE_bool(aec, false, "Activate the canceller");

[aluebs-webrtc, 2016/04/26 04:33:39]

echo canceller?

[peah-webrtc, 2016/04/26 07:19:29]

Done.

+DEFINE_bool(no_aec, false, "Dectivate the echo canceller");
+DEFINE_bool(aecm, false, "Activate the mobile echo controller");
+DEFINE_bool(no_aecm, false, "Dectivate the mobile echo controller");
+DEFINE_bool(agc, false, "Activate the AGC");
+DEFINE_bool(no_agc, false, "Dectivate the AGC");
+DEFINE_bool(hpf, false, "Activate the high-pass filter");
+DEFINE_bool(no_hpf, false, "Dectivate the high-pass filter");
+DEFINE_bool(ns, false, "Activate the noise suppressor");
+DEFINE_bool(no_ns, false, "Dectivate the noise suppressor");
+DEFINE_bool(ts, false, "Activate the transient suppressor");
+DEFINE_bool(no_ts, false, "Dectivate the transient suppressor");
+DEFINE_bool(bf, false, "Activate the beamformer");
+DEFINE_bool(no_bf, false, "Dectivate the beamformer");
+DEFINE_bool(ie, false, "Activate the intelligibility enhancer");
+DEFINE_bool(no_ie, false, "Dectivate the intelligibility enhancer");
+DEFINE_bool(vad, false, "Activate the voice activity detector");
+DEFINE_bool(no_vad, false, "Dectivate the voice activity detector");
+DEFINE_bool(le, false, "Activate the level estimator");
+DEFINE_bool(no_le, false, "Dectivate the level estimator");

[aluebs-webrtc, 2016/04/26 04:33:39]

Why do we need 2 flags to specify if a component is enabled or disabled? (if I
remember correctly, each "flag" has its correspondent "noflag") Also, why are
both false? Isn't it easier to have only one flag with the right default value
that can be overwritten? Then you can also drop the all_default flag.

[peah-webrtc, 2016/04/26 07:19:29]

This was the most simple way (for the user of the audioproc_f) that I could come
up with but there are probably better alternatives. 

The underlying idea behind this is that each parameter basically should
constitute a tri-state value, where it 
either turns off the feature, turns on the feature, or keeps it as its default
value. 
This is important for this to work well with aecdump recordings where there are
default values for the settings stored in the recording that the simulator
should act upon. 
When reading the config event from the aecdump the simulator will then either
1) apply the setting in the config if no parameter value has been explicitly
specified on the command line.
2) apply the parameter setting specified if that has been explicitly specified
on the command line. 

For a on/off feature, for instance turning on the aec a tri-state variable is
needed for the above. And it is not possible to use a default value as that
would then overwrite the value in the config. For non on/off parameters I solved
this by using a default value kParameterNotSpecifiedValue represent that the
parameter had not been set, but for boolean flags that is not possible.

Afaics, this is not possible to achieve using the no-flag-feature of gflags, as
that enforces the options to be two-state (bundling the no_aec and aec only
gives two values for the parameter and therefore it cannot be used as a
tri-state to control the feature).

One alternative could be to use integer flags instead of specifying the on/off
flags using boolean flags, i.e., instead of using the command line --no_aec use
--aec 0. I did not choose that option as that was not how the process_test.cc
and audioproc_float.cc used it before, but if you think that is a better option
I'd be happy to use that.

That is some background to the choice of implementation. If you have a better
approach, I'd be happy to change this.

[aluebs-webrtc, 2016/04/27 16:06:47]

I personally prefer 1 int flag than 2 different bool flags that add more code
and is confusing for a user, since you could technically specify "flag" and
"no_flag" at the same time.
But what about this. One global flag that forces to use the aecdump values for
all flags and then just boolean flags with the correct default value?

[peah-webrtc, 2016/04/28 07:41:11]

That sounds like a good option. But I see the drawback of having to do
double-bookkeeping in the sense that the default options in the simulator needs
to be up2date with what the default options in the AECdump. Furthermore, there
are not really any globally default options that can be assumed I think.

I then prefer the int flag option. But lets ask @minyue what he prefers before
doing any changes here.

Would you be ok with using the int flag option instead of the "global flag"
alternative?

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

I prefer the global flag suggestion, but I am open to an int flag. But agreed on
getting minyue's opinion here.
I don't understand why the simulator would need to be up to date with the
aecdump defaults. How is this different in my proposal than in the one you
implemented?
Anyway, I think there could be some reasonable global defaults when the ones
from the aecdump are not used. Maybe something like the components that are
enabled in a standard WebRTC call, but I see how this is fuzzy.

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

Maybe I misunderstood you but my concern is what I would do if I'd want to run
an aecdump recording and customize some feature: for instance just turn off the
noise suppressor. Then I'd either have to rely on the all the other flags to
have default values that matches the values in the recording, or manually
specify all the values that were present in the recording.
Both ways to do that seems error prone to me and I prefer analysis of aecdump
recordings to something that should be possible to done quickly in a way that
can cause as few mistakes as possible.

There are definitely some reasonable global defaults that could be used but if
we can avoid adding a dependency on them I think we should, both to reduce the
risk of errors and also to avoid having to update the defaults in the simulator
each time those defaults are changed.

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

I see the point when you want to use the aecdump values for all except a few
parameters it can be painful. But in that case I still feel the int flags would
be less bug prone and confusing. But as said, let's get minyue's view on this.

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

Acknowledged.

+DEFINE_bool(all_default,
+            false,
+            "Activate all of the default components (will be overridden by any "
+            "other settings)");
+DEFINE_int32(aec_suppression_level,
+             kParameterNotSpecifiedValue,
+             "Set the aec suppression level (0-2)");
+DEFINE_bool(delay_agnostic, false, "Activate the level estimator");
+DEFINE_bool(no_delay_agnostic, false, "Dectivate the level estimator");
+DEFINE_bool(extended_filter, false, "Activate the level estimator");
+DEFINE_bool(no_extended_filter, false, "Dectivate the level estimator");
+DEFINE_bool(drift_compensation, false, "Activate the level estimator");
+DEFINE_bool(no_drift_compensation, false, "Dectivate the level estimator");
+DEFINE_bool(aec3, false, "Activate the level estimator");
+DEFINE_bool(no_aec3, false, "Dectivate the level estimator");
+DEFINE_bool(refined_adaptive_filter,
+            false,
+            "Activate the refined adaptive filter functionality");
+DEFINE_bool(no_refined_adaptive_filter,
+            false,
+            "Deactivate the refined adaptive filter functionality");
+DEFINE_int32(aecm_routing_mode,
+             kParameterNotSpecifiedValue,
+             "Specify the AECM routing mode (0-4)");
+DEFINE_bool(aecm_comfort_noise, false, "Activate the AECM comfort noise");
+DEFINE_bool(no_aecm_comfort_noise, false, "Dectivate the AECM comfort noise");
+DEFINE_int32(agc_mode,
+             kParameterNotSpecifiedValue,
+             "Specify the AGC mode (0-2)");
+DEFINE_int32(agc_target_level,
+             kParameterNotSpecifiedValue,
+             "Specify the AGC target level (0-31)");
+DEFINE_bool(agc_limiter, false, "Activate the level estimator");
+DEFINE_bool(no_agc_limiter, false, "Dectivate the level estimator");
+DEFINE_int32(agc_compression_gain,
+             kParameterNotSpecifiedValue,
+             "Specify the AGC compression gain (0-90)");
+DEFINE_int32(vad_likelihood,
+             kParameterNotSpecifiedValue,
+             "Specify the VAD likelihood (0-3)");
+DEFINE_int32(ns_level,
+             kParameterNotSpecifiedValue,
+             "Specify the NS level (0-3)");
+DEFINE_int32(stream_delay,
+             kParameterNotSpecifiedValue,
+             "Specify the stream delay in ms to use");
+DEFINE_int32(stream_drift_samples,
+             kParameterNotSpecifiedValue,
+             "Specify the number of stream drift samples to use");
+DEFINE_bool(performance_report, false, "Report the APM performance ");
+DEFINE_bool(verbose, false, "Produce verbose output");
+DEFINE_bool(bitexactness_report,
+            false,
+            "Report bitexactness for aec dump result reproduction");
+DEFINE_bool(discard_settings_in_aecdump,

[aluebs-webrtc, 2016/04/26 04:33:39]

Probably more intuitive to use the positive opposite use_aecdump_settings.

[peah-webrtc, 2016/04/26 07:19:29]

The absolutely most common thing to do when analyzing aecdumps is to reapply the
settings stored in the aecdump, so therefore I wanted that to be the default
option. If the option is called use_aecdump_settings, and the default value is
true, then one must negate that in the command line to achieve discarding the
settings. 
Would it be possible to do that nicely using the "no" flags option you
mentioned?

[aluebs-webrtc, 2016/04/27 16:06:47]

Yes, you just --use nouse_aecdump_settings

[peah-webrtc, 2016/04/28 07:41:11]

Yes, but that would then mean that "use_aecdump_settings" is never used (since
it is anyway the default) and the only thing the user would specify is the
"nouse_aecdump_settings" which at least to me is more awkward than to use
"discard_settings_in_aecdump".

Furthermore, use_aecdump_settings sounds more like the the settings stored in
the aecdump are used, which is not really true as they may be overridden by any
of the other command-line settings (e.g., agc_mode). Therefore, I think
discard_settings_in_aecdump is better in that it exactly specifies what is being
done.

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

Agreed. Let's leave it as is.

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

Acknowledged.

+            false,
+            "Discard any config settings specified in the aec dump");
+DEFINE_bool(store_intermediate_output,
+            false,
+            "Creates new output files after each init");
+
+void SetSettingIfSpecified(const std::string value,
+                           rtc::Optional<std::string>* parameter) {
+  if (value.compare("") != 0) {
+    *parameter = rtc::Optional<std::string>(value);
+  }
+}
+
+void SetSettingIfSpecified(int value, rtc::Optional<int>* parameter) {
+  if (value != kParameterNotSpecifiedValue) {
+    *parameter = rtc::Optional<int>(value);
+  }
+}
+
+void SetSettingIfFlagSet(bool condition,
+                         rtc::Optional<bool>* parameter,
+                         bool value_to_set) {
+  if (condition) {
+    *parameter = rtc::Optional<bool>(value_to_set);
+  }
+}
+
+SimulationSettings CreateSettings() {
+  SimulationSettings settings;
+  if (FLAGS_all_default) {
+    settings.use_le = rtc::Optional<bool>(true);
+    settings.use_vad = rtc::Optional<bool>(true);
+    settings.use_ie = rtc::Optional<bool>(false);
+    settings.use_bf = rtc::Optional<bool>(false);
+    settings.use_ts = rtc::Optional<bool>(true);
+    settings.use_ns = rtc::Optional<bool>(true);
+    settings.use_hpf = rtc::Optional<bool>(true);
+    settings.use_agc = rtc::Optional<bool>(true);
+    settings.use_aec = rtc::Optional<bool>(true);
+    settings.use_aecm = rtc::Optional<bool>(false);
+  }
+  SetSettingIfSpecified(FLAGS_dump_input, &settings.aec_dump_input_filename);
+  SetSettingIfSpecified(FLAGS_dump_output, &settings.aec_dump_output_filename);
+  SetSettingIfSpecified(FLAGS_i, &settings.input_filename);
+  SetSettingIfSpecified(FLAGS_o, &settings.output_filename);
+  SetSettingIfSpecified(FLAGS_ri, &settings.reverse_input_filename);
+  SetSettingIfSpecified(FLAGS_ro, &settings.reverse_output_filename);
+  SetSettingIfSpecified(FLAGS_num_output_channels,
+                        &settings.output_num_channels);
+  SetSettingIfSpecified(FLAGS_num_reverse_output_channels,
+                        &settings.reverse_output_num_channels);

[aluebs-webrtc, 2016/04/26 04:33:39]

For these parameters it would be great to ave the same name for the flag and the
settings field.

[peah-webrtc, 2016/04/26 07:19:29]

Good point!
Done.

+  SetSettingIfSpecified(FLAGS_output_sample_rate_hz,
+                        &settings.output_sample_rate_hz);
+  SetSettingIfSpecified(FLAGS_reverse_output_sample_rate_hz,
+                        &settings.reverse_output_sample_rate_hz);
+  SetSettingIfSpecified(FLAGS_mic_positions, &settings.microphone_positions);
+  settings.target_angle_degrees = FLAGS_target_angle_degrees;
+  SetSettingIfFlagSet(FLAGS_aec, &settings.use_aec, true);
+  SetSettingIfFlagSet(FLAGS_no_aec, &settings.use_aec, false);
+  SetSettingIfFlagSet(FLAGS_aecm, &settings.use_aecm, true);
+  SetSettingIfFlagSet(FLAGS_no_aecm, &settings.use_aecm, false);
+  SetSettingIfFlagSet(FLAGS_agc, &settings.use_agc, true);
+  SetSettingIfFlagSet(FLAGS_no_agc, &settings.use_agc, false);
+  SetSettingIfFlagSet(FLAGS_hpf, &settings.use_hpf, true);
+  SetSettingIfFlagSet(FLAGS_no_hpf, &settings.use_hpf, false);
+  SetSettingIfFlagSet(FLAGS_ns, &settings.use_ns, true);
+  SetSettingIfFlagSet(FLAGS_no_ns, &settings.use_ns, false);
+  SetSettingIfFlagSet(FLAGS_ts, &settings.use_ts, true);
+  SetSettingIfFlagSet(FLAGS_no_ts, &settings.use_ts, false);
+  SetSettingIfFlagSet(FLAGS_bf, &settings.use_bf, true);
+  SetSettingIfFlagSet(FLAGS_no_bf, &settings.use_bf, false);
+  SetSettingIfFlagSet(FLAGS_ie, &settings.use_ie, true);
+  SetSettingIfFlagSet(FLAGS_no_ie, &settings.use_ie, false);
+  SetSettingIfFlagSet(FLAGS_vad, &settings.use_vad, true);
+  SetSettingIfFlagSet(FLAGS_no_vad, &settings.use_vad, false);
+  SetSettingIfFlagSet(FLAGS_le, &settings.use_le, true);
+  SetSettingIfFlagSet(FLAGS_no_le, &settings.use_le, false);

[aluebs-webrtc, 2016/04/26 04:33:39]

This can be simplified so much with my suggestion above.

[peah-webrtc, 2016/04/26 07:19:29]

Yes, probably. But please explain more on how that would be used.

[aluebs-webrtc, 2016/04/27 16:06:46]

See above.

[peah-webrtc, 2016/04/28 07:41:11]

Acknowledged.

+  SetSettingIfSpecified(FLAGS_aec_suppression_level,
+                        &settings.aec_suppression_level);
+  SetSettingIfFlagSet(FLAGS_delay_agnostic, &settings.use_delay_agnostic, true);
+  SetSettingIfFlagSet(FLAGS_no_delay_agnostic, &settings.use_delay_agnostic,
+                      false);
+  SetSettingIfFlagSet(FLAGS_extended_filter, &settings.use_extended_filter,
+                      true);
+  SetSettingIfFlagSet(FLAGS_no_extended_filter, &settings.use_extended_filter,
+                      false);
+  SetSettingIfFlagSet(FLAGS_drift_compensation,
+                      &settings.use_drift_compensation, true);
+  SetSettingIfFlagSet(FLAGS_no_drift_compensation,
+                      &settings.use_drift_compensation, false);
+  SetSettingIfFlagSet(FLAGS_refined_adaptive_filter,
+                      &settings.use_refined_adaptive_filter, true);
+  SetSettingIfFlagSet(FLAGS_no_refined_adaptive_filter,
+                      &settings.use_refined_adaptive_filter, false);
+
+  SetSettingIfFlagSet(FLAGS_aec3, &settings.use_aec3, true);
+  SetSettingIfFlagSet(FLAGS_no_aec3, &settings.use_aec3, false);
+  SetSettingIfSpecified(FLAGS_aecm_routing_mode, &settings.aecm_routing_mode);
+  SetSettingIfFlagSet(FLAGS_aecm_comfort_noise,
+                      &settings.use_aecm_comfort_noise, true);
+  SetSettingIfFlagSet(FLAGS_no_aecm_comfort_noise,
+                      &settings.use_aecm_comfort_noise, false);
+  SetSettingIfSpecified(FLAGS_agc_mode, &settings.agc_mode);
+  SetSettingIfSpecified(FLAGS_agc_target_level, &settings.agc_target_level);
+  SetSettingIfFlagSet(FLAGS_agc_limiter, &settings.use_agc_limiter, true);
+  SetSettingIfFlagSet(FLAGS_no_agc_limiter, &settings.use_agc_limiter, false);
+  SetSettingIfSpecified(FLAGS_agc_compression_gain,
+                        &settings.agc_compression_gain);
+  SetSettingIfSpecified(FLAGS_vad_likelihood, &settings.vad_likelihood);
+  SetSettingIfSpecified(FLAGS_ns_level, &settings.ns_level);
+  SetSettingIfSpecified(FLAGS_stream_delay, &settings.stream_delay);
+  SetSettingIfSpecified(FLAGS_stream_drift_samples,
+                        &settings.stream_drift_samples);
+  settings.report_performance = FLAGS_performance_report;
+  settings.use_verbose_logging = FLAGS_verbose;
+  settings.report_bitexactness = FLAGS_bitexactness_report;
+  settings.discard_all_settings_in_aecdump = FLAGS_discard_settings_in_aecdump;
+  settings.fixed_interface = FLAGS_fixed_interface;
+  settings.store_intermediate_output = FLAGS_store_intermediate_output;
+
+  return settings;
+}
+
+void PerformBasicParameterSanityChecks(const SimulationSettings& settings) {
+  auto report_conditional_error_and_exit = [](bool condition,

[aluebs-webrtc, 2016/04/26 04:33:39]

Why not making this a function directly?

[peah-webrtc, 2016/04/26 07:19:29]

Done.

+                                              std::string message) {
+    if (condition) {
+      fprintf(stderr, "%s", message.c_str());
+      exit(1);
+    }
+  };
+
+  if (settings.input_filename || settings.reverse_input_filename) {
+    report_conditional_error_and_exit(!!settings.aec_dump_input_filename,

[aluebs-webrtc, 2016/04/26 04:33:39]

double negation?

[peah-webrtc, 2016/04/26 07:19:29]

aec_dump_input_filename is an optional, and I want to true to be passed if it is
set. The double negation is a way to achieve that in a simple manner.

[aluebs-webrtc, 2016/04/27 16:06:47]

Oh, I thought it would cast automatically to bool.

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

Acknowledged.

+                                      "Error: The aec dump cannot be specified "
+                                      "together with input wav files!\n");
+
+    report_conditional_error_and_exit(!settings.input_filename,
+                                      "Error: When operating at wav files, the "
+                                      "input wav filename must be "
+                                      "specified!\n");
+
+    report_conditional_error_and_exit(
+        settings.reverse_output_filename && !settings.reverse_input_filename,
+        "Error: When operating at wav files, the reverse input wav filename "
+        "must be specified if the reverse output wav filename is specified!\n");
+  } else {
+    report_conditional_error_and_exit(!settings.aec_dump_input_filename,
+                                      "Error: Either the aec dump or the wav "
+                                      "input files must be specified!\n");
+  }
+
+  report_conditional_error_and_exit(
+      settings.use_aec && *settings.use_aec && settings.use_aecm &&
+          *settings.use_aecm,
+      "Error: The AEC and the AECM cannot be activated at the same time!\n");
+
+  report_conditional_error_and_exit(
+      settings.output_sample_rate_hz &&
+          (*settings.output_sample_rate_hz <= 0 ||
+           *settings.output_sample_rate_hz > 48000),

[aluebs-webrtc, 2016/04/26 04:33:39]

Why do we impose this limit? The APM can handle an arbitrary sample rate.

[peah-webrtc, 2016/04/28 07:41:11]

Done.

+      "Error: --output_sample_rate_hz must be betweem 0 and 48000!\n");
+
+  report_conditional_error_and_exit(

[aluebs-webrtc, 2016/04/26 04:33:39]

All these checks for a single parameter can be implemented in the gflags
framework.

[peah-webrtc, 2016/04/26 07:19:29]

No, that does not work if you want to have tri-state variables. The problem then
is that the validator enforces the default value to be valid, and therefore the
default value that is used as an indicator that the parameter is not set must be
changed to be in the valid range. For some of the options that is possible but
not for some of the others.

[aluebs-webrtc, 2016/04/27 16:06:47]

Good point.

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

Acknowledged.

+      settings.reverse_output_sample_rate_hz &&
+          (*settings.reverse_output_sample_rate_hz <= 0 ||
+           *settings.reverse_output_sample_rate_hz > 48000),
+      "Error: --reverse_output_sample_rate_hz must be betweem 0 and 48000!\n");
+
+  report_conditional_error_and_exit(
+      settings.output_num_channels && (*settings.output_num_channels <= 0 ||
+                                       *settings.output_num_channels > 2),

[aluebs-webrtc, 2016/04/26 04:33:39]

Why do we impose this limit? The APM can handle an arbitrary number of channels,
right?

[peah-webrtc, 2016/04/26 07:19:29]

Done.

+      "Error: --output_num_channels must be 1 or 2!\n");
+
+  report_conditional_error_and_exit(
+      settings.reverse_output_num_channels &&
+          (*settings.reverse_output_num_channels <= 0 ||
+           *settings.reverse_output_num_channels > 2),
+      "Error: --reverse_output_num_channels must be 1 or 2!\n");
+
+  report_conditional_error_and_exit(
+      settings.use_bf && *settings.use_bf && !settings.microphone_positions,
+      "Error: --mic_positions must be specified when the beamformer is "
+      "activated.\n");
+
+  report_conditional_error_and_exit(
+      settings.target_angle_degrees < 0 || settings.target_angle_degrees > 90,

[aluebs-webrtc, 2016/04/26 04:33:39]

The angle can definitively go between 0 and 359 degrees. And I think that if it
isn't in that range it would wrap to this range anyways.

[peah-webrtc, 2016/04/26 07:19:29]

My mistake. I think it is anyway nice to impose a sane angle  between 0 and 359.
Or do you instead prefer no limit and rely on wrapping behavior?

Done.

[aluebs-webrtc, 2016/04/27 16:06:47]

I don't mind imposing this limit.

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

Acknowledged.

+      "Error: -target_angle_degrees must be specified between 0 and 90.\n");
+
+  report_conditional_error_and_exit(
+      settings.aec_suppression_level &&
+          ((*settings.aec_suppression_level) < 0 ||
+           (*settings.aec_suppression_level) > 2),
+      "Error: --aec_suppression_level must be specified between 0 and 2.\n");
+
+  report_conditional_error_and_exit(
+      settings.aecm_routing_mode && ((*settings.aecm_routing_mode) < 0 ||
+                                     (*settings.aecm_routing_mode) > 4),
+      "Error: --aecm_routing_mode must be specified between 0 and 4.\n");
+
+  report_conditional_error_and_exit(
+      settings.agc_target_level && ((*settings.agc_target_level) < 0 ||
+                                    (*settings.agc_target_level) > 31),
+      "Error: --agc_target_level must be specified between 0 and 31.\n");
+
+  report_conditional_error_and_exit(
+      settings.agc_compression_gain && ((*settings.agc_compression_gain) < 0 ||
+                                        (*settings.agc_compression_gain) > 90),
+      "Error: --agc_compression_gain must be specified between 0 and 90.\n");
+
+  report_conditional_error_and_exit(
+      settings.vad_likelihood &&
+          ((*settings.vad_likelihood) < 0 || (*settings.vad_likelihood) > 3),
+      "Error: --vad_likelihood must be specified between 0 and 3.\n");
+
+  report_conditional_error_and_exit(
+      settings.ns_level &&
+          ((*settings.ns_level) < 0 || (*settings.ns_level) > 3),
+      "Error: --ns_level must be specified between 0 and 3.\n");
+
+  auto valid_wav_name = [](const std::string& wav_file_name) {

[aluebs-webrtc, 2016/04/26 04:33:39]

Why isn't this a normal function?

[peah-webrtc, 2016/04/26 07:19:29]

The lambda/function is only used in the lines below and nowhere else. If the
functionality in the function/lambda is very light-weight, I typically try to
include it as a lambda and place it close to where it is used in order to
simplify the code readability. 

In this case it is just used to simplify the if-statements by removing repeated
code, and if it would be implemented as a function it would need to be placed
~100 lines above where it is used.

[aluebs-webrtc, 2016/04/27 16:06:46]

Personally I find interleaved lambdas to be less readable than a function.
Specially because finding a lambda makes at least me assume it will be passed as
a function pointer or it will grab some environment variables. And I don't think
it matters too much where it is defined for such a simple method as long as the
name is intuitive. But if you think it is better, I don't have such a strong
opinion about it.

[peah-webrtc, 2016/04/28 07:41:11]

Acknowledged.

+    if (wav_file_name.size() < 5) {
+      return false;
+    }
+    if ((wav_file_name.compare(wav_file_name.size() - 4, 4, ".wav") == 0) ||
+        (wav_file_name.compare(wav_file_name.size() - 4, 4, ".WAV") == 0)) {
+      return true;
+    }
+    return false;
+  };
+
+  report_conditional_error_and_exit(
+      settings.input_filename && (!valid_wav_name(*settings.input_filename)),
+      "Error: --i must be a valid .wav file name.\n");
+
+  report_conditional_error_and_exit(
+      settings.output_filename && (!valid_wav_name(*settings.output_filename)),
+      "Error: --o must be a valid .wav file name.\n");
+
+  report_conditional_error_and_exit(
+      settings.reverse_input_filename &&
+          (!valid_wav_name(*settings.reverse_input_filename)),
+      "Error: --ri must be a valid .wav file name.\n");
+
+  report_conditional_error_and_exit(
+      settings.reverse_output_filename &&
+          (!valid_wav_name(*settings.reverse_output_filename)),
+      "Error: --ro must be a valid .wav file name.\n");
 }
 
 }  // namespace
 
-DEFINE_string(dump, "", "Name of the aecdump debug file to read from.");
-DEFINE_string(i, "", "Name of the capture input stream file to read from.");
-DEFINE_string(
-    o,
-    "out.wav",
-    "Name of the output file to write the processed capture stream to.");
-DEFINE_string(ri, "", "Name of the render input stream file to read from.");
-DEFINE_string(
-    ro,
-    "out_reverse.wav",
-    "Name of the output file to write the processed render stream to.");
-DEFINE_int32(out_channels, 1, "Number of output channels.");
-const bool out_channels_dummy =
-    google::RegisterFlagValidator(&FLAGS_out_channels, &ValidateOutChannels);
-DEFINE_int32(rev_out_channels, 1, "Number of reverse output channels.");
-const bool rev_out_channels_dummy =
-    google::RegisterFlagValidator(&FLAGS_rev_out_channels,
-                                  &ValidateOutChannels);
-DEFINE_int32(out_sample_rate, 48000, "Output sample rate in Hz.");
-DEFINE_int32(rev_out_sample_rate, 48000, "Reverse output sample rate in Hz.");
-DEFINE_string(mic_positions, "",
-    "Space delimited cartesian coordinates of microphones in meters. "
-    "The coordinates of each point are contiguous. "
-    "For a two element array: \"x1 y1 z1 x2 y2 z2\"");
-DEFINE_double(
-    target_angle_degrees,
-    90,
-    "The azimuth of the target in degrees. Only applies to beamforming.");
-
-DEFINE_bool(aec, false, "Enable echo cancellation.");
-DEFINE_bool(agc, false, "Enable automatic gain control.");
-DEFINE_bool(hpf, false, "Enable high-pass filtering.");
-DEFINE_bool(ns, false, "Enable noise suppression.");
-DEFINE_bool(ts, false, "Enable transient suppression.");
-DEFINE_bool(bf, false, "Enable beamforming.");
-DEFINE_bool(ie, false, "Enable intelligibility enhancer.");
-DEFINE_bool(all, false, "Enable all components.");
-
-DEFINE_int32(ns_level, -1, "Noise suppression level [0 - 3].");
-
-DEFINE_bool(perf, false, "Enable performance tests.");
-
-namespace webrtc {
-namespace {
-
-const int kChunksPerSecond = 100;
-const char kUsage[] =
-    "Command-line tool to run audio processing on WAV files. Accepts either\n"
-    "an input capture WAV file or protobuf debug dump and writes to an output\n"
-    "WAV file.\n"
-    "\n"
-    "All components are disabled by default.";
-
-}  // namespace
-
 int main(int argc, char* argv[]) {
-  google::SetUsageMessage(kUsage);
+  google::SetUsageMessage(kUsageDescription);
   google::ParseCommandLineFlags(&argc, &argv, true);
 
-  if (!((FLAGS_i.empty()) ^ (FLAGS_dump.empty()))) {
-    fprintf(stderr,
-            "An input file must be specified with either -i or -dump.\n");
-    return 1;
-  }
-
-  test::TraceToStderr trace_to_stderr(true);
-  Config config;
-  if (FLAGS_bf || FLAGS_all) {
-    if (FLAGS_mic_positions.empty()) {
-      fprintf(stderr, "-mic_positions must be specified when -bf is used.\n");
-      return 1;
-    }
-    config.Set<Beamforming>(new Beamforming(
-        true, ParseArrayGeometry(FLAGS_mic_positions),
-        SphericalPointf(DegreesToRadians(FLAGS_target_angle_degrees), 0.f,
-                        1.f)));
-  }
-  config.Set<ExperimentalNs>(new ExperimentalNs(FLAGS_ts || FLAGS_all));
-  config.Set<Intelligibility>(new Intelligibility(FLAGS_ie || FLAGS_all));
-
-  std::unique_ptr<AudioProcessing> ap(AudioProcessing::Create(config));
-  RTC_CHECK_EQ(kNoErr, ap->echo_cancellation()->Enable(FLAGS_aec || FLAGS_all));
-  RTC_CHECK_EQ(kNoErr, ap->gain_control()->Enable(FLAGS_agc || FLAGS_all));
-  RTC_CHECK_EQ(kNoErr, ap->high_pass_filter()->Enable(FLAGS_hpf || FLAGS_all));
-  RTC_CHECK_EQ(kNoErr, ap->noise_suppression()->Enable(FLAGS_ns || FLAGS_all));
-  if (FLAGS_ns_level != -1) {
-    RTC_CHECK_EQ(kNoErr,
-                 ap->noise_suppression()->set_level(
-                     static_cast<NoiseSuppression::Level>(FLAGS_ns_level)));
-  }
-  ap->set_stream_key_pressed(FLAGS_ts);
-
-  std::unique_ptr<AudioFileProcessor> processor;
-  auto out_file = std::unique_ptr<WavWriter>(new WavWriter(
-      FLAGS_o, FLAGS_out_sample_rate, static_cast<size_t>(FLAGS_out_channels)));
-  std::cout << FLAGS_o << ": " << out_file->FormatAsString() << std::endl;
-  if (FLAGS_dump.empty()) {
-    auto in_file = std::unique_ptr<WavReader>(new WavReader(FLAGS_i));
-    std::cout << FLAGS_i << ": " << in_file->FormatAsString() << std::endl;
-    std::unique_ptr<WavReader> reverse_in_file;
-    std::unique_ptr<WavWriter> reverse_out_file;
-    if (!FLAGS_ri.empty()) {
-      reverse_in_file.reset(new WavReader(FLAGS_ri));
-      reverse_out_file.reset(new WavWriter(
-          FLAGS_ro,
-          FLAGS_rev_out_sample_rate,
-          static_cast<size_t>(FLAGS_rev_out_channels)));
-      std::cout << FLAGS_ri << ": "
-                << reverse_in_file->FormatAsString() << std::endl;
-      std::cout << FLAGS_ro << ": "
-                << reverse_out_file->FormatAsString() << std::endl;
-    }
-    processor.reset(new WavFileProcessor(std::move(ap),
-                                         std::move(in_file),
-                                         std::move(out_file),
-                                         std::move(reverse_in_file),
-                                         std::move(reverse_out_file)));
-
+  SimulationSettings settings = CreateSettings();
+  PerformBasicParameterSanityChecks(settings);
+  std::unique_ptr<AudioProcessingSimulator> processor;
+
+  if (settings.aec_dump_input_filename) {
+    processor.reset(new AecDumpBasedSimulator(settings));
   } else {
-    processor.reset(new AecDumpFileProcessor(
-        std::move(ap), fopen(FLAGS_dump.c_str(), "rb"), std::move(out_file)));
-  }
-
-  int num_chunks = 0;
-  while (processor->ProcessChunk()) {
-    trace_to_stderr.SetTimeSeconds(num_chunks * 1.f / kChunksPerSecond);
-    ++num_chunks;
-  }
-
-  if (FLAGS_perf) {
+    processor.reset(new WavBasedSimulator(settings));
+  }
+
+  processor->Process();
+
+  int num_fwd_chunks_processed = processor->get_num_process_stream_calls();

[aluebs-webrtc, 2016/04/26 04:33:39]

I don't think this temp variable is necessary, but in any case it should be
const and should be moved inside the if scope.

[peah-webrtc, 2016/04/26 07:19:29]

Done.

+
+  if (settings.report_performance) {
     const auto& proc_time = processor->proc_time();
     int64_t exec_time_us = proc_time.sum.Microseconds();
     printf(
         "\nExecution time: %.3f s, File time: %.2f s\n"
-        "Time per chunk (mean, max, min):\n%.0f us, %.0f us, %.0f us\n",
-        exec_time_us * 1e-6, num_chunks * 1.f / kChunksPerSecond,
-        exec_time_us * 1.f / num_chunks, 1.f * proc_time.max.Microseconds(),
-        1.f * proc_time.min.Microseconds());
+        "Time per fwd stream chunk (mean, max, min):\n%.0f us, %.0f us, %.0f "
+        "us\n",
+        exec_time_us * 1e-6, num_fwd_chunks_processed * 1.f /
+                                 AudioProcessingSimulator::kChunksPerSecond,
+        exec_time_us * 1.f / num_fwd_chunks_processed,
+        1.f * proc_time.max.Microseconds(), 1.f * proc_time.min.Microseconds());
+  }
+
+  if (settings.report_bitexactness && settings.aec_dump_input_filename) {

[aluebs-webrtc, 2016/04/26 04:33:39]

It should probably be checked in your sanity function that it has an aecdump
when asking for bit exactness testing.

[peah-webrtc, 2016/04/26 07:19:29]

Good point!
Done.

+    if (processor->OutputWasBitexact()) {
+      printf("The processing was bitexact.\n");
+    } else {
+      printf("The processing was not bitexact.\n");
+    }
   }
 
   return 0;
 }
 
+}  // namespace test
 }  // namespace webrtc
 
 int main(int argc, char* argv[]) {
-  return webrtc::main(argc, argv);
-}
+  return webrtc::test::main(argc, argv);
+}