Introduced new scheme for controlling the functionality inside the audio processing module

webrtc/modules/audio_processing/include/audio_processing.h

 /*
  *  Copyright (c) 2012 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.
  */
 
@@ skipping 26 matching lines @@
 class ProcessingConfig;
 
 class EchoCancellation;
 class EchoControlMobile;
 class GainControl;
 class HighPassFilter;
 class LevelEstimator;
 class NoiseSuppression;
 class VoiceDetection;
 
+// The struct below constitutes the new parameter scheme for the
+// audio processing functionality. It is being introduced gradually and
+// until it is fully introduced, it is prone to change.
+//
+// The parameters and behavior of the audio processing module is speficied

[hlundin-webrtc, 2016/08/30 11:26:26]

speficied -> specified

[hlundin-webrtc, 2016/08/30 11:26:26]

is -> are

[peah-webrtc, 2016/08/30 17:05:58]

Done.

[peah-webrtc, 2016/08/30 17:05:58]

Done.

+// by changing the default values in the ApmSettings struct. It is possible

[hlundin-webrtc, 2016/08/30 11:26:26]

The parameters and behavior are in fact specified by the struct regardless of
whether the default values are kept or changed. Consider rephrasing.

[peah-webrtc, 2016/08/30 17:05:57]

Done.

+// to verify the specified parameters by calling the IsOk method for each
+// substruct. The settings are applied by passing the struct to the

[hlundin-webrtc, 2016/08/30 11:26:26]

What happens if I call ApmSettings::IsOk()? Will it aggregate the results of all
substructs' IsOk functions?

[peah-webrtc, 2016/08/30 17:05:57]

Yes, I added a comment about that.

+// ApplySettings

[hlundin-webrtc, 2016/08/30 11:26:26]

Line wrapping is odd.

[peah-webrtc, 2016/08/30 17:05:58]

Done.

+// method in the audio processing interface.
+struct ApmSettings {

[the sun, 2016/08/30 10:32:39]

name this Config and put it inside the AudioProcessing interface.

[hlundin-webrtc, 2016/08/30 11:26:26]

+1

[peah-webrtc, 2016/08/30 17:05:57]

Done.

[peah-webrtc, 2016/08/30 17:05:57]

Done.

+  struct LevelControllerSettings {

[hlundin-webrtc, 2016/08/30 11:26:26]

Should the substructs' configs be declared here or in their respective classes?
In ACM, we have an AudioCodingModule::Config struct, which contains a
NetEq::Config struct. This does of course mean that audio_coding_module.h must
include neteq.h, which may not be desired in the APM.

[peah-webrtc, 2016/08/30 17:05:57]

I don't think that we should allow/require audio_processing.h to include the
header files of the submodules. I'll try with another variant and see what you
think of that.

+    bool enabled = false;
+    bool IsOk() const;
+  } level_controller;
+  bool IsOk() const;
+};
+
 // Use to enable the extended filter mode in the AEC, along with robustness
 // measures around the reported system delays. It comes with a significant
 // increase in AEC complexity, but is much more robust to unreliable reported
 // delays.
 //
 // Detailed changes to the algorithm:
 // - The filter length is changed from 48 to 128 ms. This comes with tuning of
 //   several parameters: i) filter adaptation stepsize and error threshold;
 //   ii) non-linear processing smoothing and overdrive.
 // - Option to ignore the reported delays on platforms which we deem
@@ skipping 27 matching lines @@
 // EchoControlMobile. It can be set in the constructor
 // or using AudioProcessing::SetExtraOptions().
 struct RefinedAdaptiveFilter {
   RefinedAdaptiveFilter() : enabled(false) {}
   explicit RefinedAdaptiveFilter(bool enabled) : enabled(enabled) {}
   static const ConfigOptionID identifier =
       ConfigOptionID::kAecRefinedAdaptiveFilter;
   bool enabled;
 };
 
-// Enables the adaptive level controller.
-struct LevelControl {
-  LevelControl() : enabled(false) {}
-  explicit LevelControl(bool enabled) : enabled(enabled) {}
-  static const ConfigOptionID identifier = ConfigOptionID::kLevelControl;

[the sun, 2016/08/30 10:32:39]

Don't forget to remove ConfigOptionID::kLevelControl

[peah-webrtc, 2016/08/30 17:05:57]

Can we do that? There is a comment in common.h that specifically states that
those should never be removed, but rather deprecated.

-  bool enabled;
-};
-
 // Enables delay-agnostic echo cancellation. This feature relies on internally
 // estimated delays between the process and reverse streams, thus not relying
 // on reported system delays. This configuration only applies to
 // EchoCancellation and not EchoControlMobile. It can be set in the constructor
 // or using AudioProcessing::SetExtraOptions().
 struct DelayAgnostic {
   DelayAgnostic() : enabled(false) {}
   explicit DelayAgnostic(bool enabled) : enabled(enabled) {}
   static const ConfigOptionID identifier = ConfigOptionID::kDelayAgnostic;
   bool enabled;
@@ skipping 86 matching lines @@
 //   2. Parameter getters are never called concurrently with the corresponding
 //      setter.
 //
 // APM accepts only linear PCM audio data in chunks of 10 ms. The int16
 // interfaces use interleaved data, while the float interfaces use deinterleaved
 // data.
 //
 // Usage example, omitting error checking:
 // AudioProcessing* apm = AudioProcessing::Create(0);
 //
+// ApmSettings settings;
+// settings.level_controller_settings.enable = true;
+// RTC_DCHECK(settings.IsOk());

[hlundin-webrtc, 2016/08/30 11:26:26]

The "user" should always verify the settings, not only in debug mode. That is
if (!settings.IsOk()) {
  // Failure. Don't use this config.
}

[peah-webrtc, 2016/08/30 17:05:57]

Good point! I edited it a bit. PTAL

[the sun, 2016/08/30 18:05:35]

I disagree. We need to validate the config anyway, so why force that on the
user? Make the API simple instead.

[peah-webrtc, 2016/09/02 08:21:59]

The argument by the other reviewer is that there anyway needs to be a validation
method once the config is used at the creation of apm in order to allow any
other action that to crash for falsy parameters.

[hlundin-webrtc, 2016/09/06 08:56:03]

(Same comment as in later patch set, for completeness and increased confusion.)
I agree that it isn't necessary to do that before calling ApplyConfig. That
method could instead return an error, as it does now.

My thinking was a bit ahead of the current implementation. I suppose we want to
be able to provide a Config to the APM ctor in the future. And in that case, the
user must validate the config before passing it, since the ctor itself has no
option but to crash if the config is invalid.

[peah-webrtc, 2016/09/07 05:42:59]

Acknowledged.

+// apm->ApplySettings(settings);
+//
 // apm->high_pass_filter()->Enable(true);
 //
 // apm->echo_cancellation()->enable_drift_compensation(false);
 // apm->echo_cancellation()->Enable(true);
 //
 // apm->noise_reduction()->set_level(kHighSuppression);
 // apm->noise_reduction()->Enable(true);
 //
 // apm->gain_control()->set_analog_level_limits(0, 255);
 // apm->gain_control()->set_mode(kAdaptiveAnalog);
@@ skipping 43 matching lines @@
   // stream which requires processing. On the server-side, this would typically
   // be one instance for every incoming stream.
   static AudioProcessing* Create();
   // Allows passing in an optional configuration at create-time.
   static AudioProcessing* Create(const Config& config);
   // Only for testing.
   static AudioProcessing* Create(const Config& config,
                                  NonlinearBeamformer* beamformer);
   virtual ~AudioProcessing() {}
 
+  virtual int ApplySettings(const ApmSettings& settings) = 0;

[the sun, 2016/08/30 10:32:38]

- ApplyConfig()
- Add TODO(peah) that this is temporary?

[peah-webrtc, 2016/08/30 17:05:57]

Done.

+
   // Initializes internal states, while retaining all user settings. This
   // should be called before beginning to process a new audio stream. However,
   // it is not necessary to call before processing the first stream after
   // creation.
   //
   // It is also not necessary to call if the audio parameters (sample
   // rate and number of channels) have changed. Passing updated parameters
   // directly to |ProcessStream()| and |ProcessReverseStream()| is permissible.
   // If the parameters are known at init-time though, they may be provided.
   virtual int Initialize() = 0;
@@ skipping 693 matching lines @@
   // This does not impact the size of frames passed to |ProcessStream()|.
   virtual int set_frame_size_ms(int size) = 0;
   virtual int frame_size_ms() const = 0;
 
  protected:
   virtual ~VoiceDetection() {}
 };
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_PROCESSING_INCLUDE_AUDIO_PROCESSING_H_