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

webrtc/modules/audio_processing/include/audio_processing.h

Index: webrtc/modules/audio_processing/include/audio_processing.h
diff --git a/webrtc/modules/audio_processing/include/audio_processing.h b/webrtc/modules/audio_processing/include/audio_processing.h
index 09e5d5be1cba9b20a8b8210c690c3c23e1dc7709..7b15b14333d39d67e11d361d32b5075bdaf01e94 100644
--- a/webrtc/modules/audio_processing/include/audio_processing.h
+++ b/webrtc/modules/audio_processing/include/audio_processing.h
@@ -44,6 +44,24 @@ 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
@@ -91,14 +109,6 @@ struct RefinedAdaptiveFilter {
   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
@@ -205,6 +215,11 @@ struct Intelligibility {
 // 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);
@@ -268,6 +283,8 @@ class AudioProcessing {
                                  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