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..bdc7a73cb4512c15998f0616bd140d47681eb759 100644
--- a/webrtc/modules/audio_processing/include/audio_processing.h
+++ b/webrtc/modules/audio_processing/include/audio_processing.h
@@ -91,14 +91,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;
-  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 +197,10 @@ struct Intelligibility {
 // Usage example, omitting error checking:
 // AudioProcessing* apm = AudioProcessing::Create(0);
 //
+// AudioProcessing::Config config;
+// config.level_controller.enable = true;
+// apm->ApplyConfig(config);

[the sun, 2016/09/01 13:48:00]

So I guess it is best if the example includes handling the return code, perhaps
with an RTC_CHECK().

[peah-webrtc, 2016/09/02 08:22:00]

I rewrote this again after feedback from the other reviewer.

[the sun, 2016/09/02 09:37:05]

I don't see that feedback in this CL, unless you're talking about Henrik's first
set of comments? I'm confused, because you already changed the implementation
after that.

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

That feedback is now in the comments from Henrik.

+//
 // apm->high_pass_filter()->Enable(true);
 //
 // apm->echo_cancellation()->enable_drift_compensation(false);
@@ -244,6 +240,22 @@ struct Intelligibility {
 //
 class AudioProcessing {
  public:
+  // 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 are controlled
+  // by changing the default values in the ApmConfig struct. The config is

[the sun, 2016/09/01 13:48:00]

ApmConfig -> AudioProcessing::Config

[peah-webrtc, 2016/09/02 08:22:00]

Done.

+  // applied by passing the struct to the ApplyConfig method in the audio
+  // processing interface. It is possible to verify the specified parameters by

[the sun, 2016/09/01 13:48:01]

"audio processing interface" is unnecessary in this context.

[peah-webrtc, 2016/09/02 08:22:00]

Done.

+  // calling the Validate() method.
+  struct Config {
+    struct LevelController {
+      bool enabled = false;
+    } level_controller;
+    bool Validate() const;

[the sun, 2016/09/01 13:48:00]

Please remove this method and keep it a plain struct.

[peah-webrtc, 2016/09/02 08:22:00]

The argument by the other reviewer is that this is good to have as it allows the
user to test a config without having to crash the application once we set these
settings during construction time.

[the sun, 2016/09/02 09:37:05]

Well, sure, but we could just as well handle that in the ctor then: either we
DCHECK the config is valid and revert to some sane settings if it isn't, or we
CHECK the config, if we're lazy or just don't believe we can recover. In the
unlikely even that the client actually does DCHECK(Validate) on the config
before giving it to the ctor, we still need to prepare for an invalid config.
And besides, what is the client to do with the information?

Additionally, none of the other places where we use construct-from-config
semantics, employ a Validate() method. I know, consistency is the hobgoblin of
little minds etc. :) But still, it makes sense to make the APIs work the same.

There's a better way than all of this, of course, which is to use types for the
config parameters that enforce these constraints; ranges etc.

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

We use this pattern for the AudioEncoders. Each implementation of the
AudioEncoder interface has a Config which can be passed to the ctor. The ctor
will crash on invalid configurations (silently modifying it to a valid config is
not tractable), and it is the responsibility of the caller to validate it. What
is the caller to do with a failed validation? Well, unlike the ctor, it can
probably flag an error, and maybe revert to a default config if that makes
sense.

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

Acknowledged.

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

Types would be a good step on the way, but it is extremely hard, and becomes
quite messy if generalized, to incorporate valid intra-parameter dependencies. 

An example of that (which can be eliminated in the long run using AEC modes, but
for the sake of discussion lets assume it cannot), is the selection of the
mobile aec and aec. As it is now, each ProcessStream call checks for whether
these are activated at the same time and return an error.
It would be much nicer if that error could instead be returned at the time of
the creation of APM/at the time when the parameters in APM are set. 
Letting the ctor crash for such a setup is not the right thing to do and in my
mind it would then be better to
1) Let the Validate() method flag that an improper setting is used.
2) Let the ctor default to the safest choice of only using the mobile aec in
this case.

+  };
+
   // TODO(mgraczyk): Remove once all methods that use ChannelLayout are gone.
   enum ChannelLayout {
     kMono,
@@ -262,9 +274,9 @@ class AudioProcessing {
   // 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);
+  static AudioProcessing* Create(const webrtc::Config& config);
   // Only for testing.
-  static AudioProcessing* Create(const Config& config,
+  static AudioProcessing* Create(const webrtc::Config& config,
                                  NonlinearBeamformer* beamformer);
   virtual ~AudioProcessing() {}
 
@@ -300,9 +312,13 @@ class AudioProcessing {
                          ChannelLayout output_layout,
                          ChannelLayout reverse_layout) = 0;
 
+  // TODO(peah): This method is a temporary solution used to take control
+  // over the parameters in the audio processing module and is likely to change.
+  virtual bool ApplyConfig(const Config& config) = 0;
+
   // Pass down additional options which don't have explicit setters. This
   // ensures the options are applied immediately.
-  virtual void SetExtraOptions(const Config& config) = 0;
+  virtual void SetExtraOptions(const webrtc::Config& config) = 0;
 
   // TODO(ajm): Only intended for internal use. Make private and friend the
   // necessary classes?