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

webrtc/modules/audio_processing/audio_processing_impl.cc

Index: webrtc/modules/audio_processing/audio_processing_impl.cc
diff --git a/webrtc/modules/audio_processing/audio_processing_impl.cc b/webrtc/modules/audio_processing/audio_processing_impl.cc
index 011325f8ca1cc00a525d01e11ef296db4ecddf52..d86f6c36ebf2abdf46171dd4dcbec9f962a8e970 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -149,15 +149,15 @@ struct AudioProcessingImpl::ApmPrivateSubmodules {
 };
 
 AudioProcessing* AudioProcessing::Create() {
-  Config config;
+  webrtc::Config config;
   return Create(config, nullptr);
 }
 
-AudioProcessing* AudioProcessing::Create(const Config& config) {
+AudioProcessing* AudioProcessing::Create(const webrtc::Config& config) {
   return Create(config, nullptr);
 }
 
-AudioProcessing* AudioProcessing::Create(const Config& config,
+AudioProcessing* AudioProcessing::Create(const webrtc::Config& config,
                                          NonlinearBeamformer* beamformer) {
   AudioProcessingImpl* apm = new AudioProcessingImpl(config, beamformer);
   if (apm->Initialize() != kNoError) {
@@ -168,10 +168,10 @@ AudioProcessing* AudioProcessing::Create(const Config& config,
   return apm;
 }
 
-AudioProcessingImpl::AudioProcessingImpl(const Config& config)
+AudioProcessingImpl::AudioProcessingImpl(const webrtc::Config& config)
     : AudioProcessingImpl(config, nullptr) {}
 
-AudioProcessingImpl::AudioProcessingImpl(const Config& config,
+AudioProcessingImpl::AudioProcessingImpl(const webrtc::Config& config,
                                          NonlinearBeamformer* beamformer)
     : public_submodules_(new ApmPublicSubmodules()),
       private_submodules_(new ApmPrivateSubmodules(beamformer)),
@@ -189,8 +189,7 @@ AudioProcessingImpl::AudioProcessingImpl(const Config& config,
                config.Get<Beamforming>().array_geometry,
                config.Get<Beamforming>().target_direction),
       capture_nonlocked_(config.Get<Beamforming>().enabled,
-                         config.Get<Intelligibility>().enabled,
-                         config.Get<LevelControl>().enabled) {
+                         config.Get<Intelligibility>().enabled) {
   {
     rtc::CritScope cs_render(&crit_render_);
     rtc::CritScope cs_capture(&crit_capture_);
@@ -413,7 +412,33 @@ int AudioProcessingImpl::InitializeLocked(const ProcessingConfig& config) {
   return InitializeLocked();
 }
 
-void AudioProcessingImpl::SetExtraOptions(const Config& config) {
+void AudioProcessingImpl::ApplyConfig(const AudioProcessing::Config& config) {
+  AudioProcessing::Config config_to_use = config;
+
+  bool config_ok = LevelController::Validate(config_to_use.level_controller);

[the sun, 2016/09/07 15:49:39]

I really like this; nice and clear.

[hlundin-webrtc, 2016/09/07 21:13:05]

Agree. But wouldn't it be nice if ApplyConfig returned a bool to signal if the
config was valid or not? At least tests can be made to die a screaming death if
they do not test what was intended.

[peah-webrtc, 2016/09/08 09:13:48]

I agree but my concern with that is that we would then use a scheme that we know
we will not be using/do not want to use in the future once we can do this
initialization in the constructor.

I'm open to both variants though.

WDYT?

[peah-webrtc, 2016/09/08 09:13:48]

Acknowledged.

+  if (!config_ok) {
+    LOG(LS_ERROR) << "AudioProcessing module config error" << std::endl
+                  << LevelController::ToString(config_to_use.level_controller)
+                  << std::endl
+                  << "Reverting to default parameter set";
+    config_to_use.level_controller = LevelController::DefaultConfig();

[hlundin-webrtc, 2016/09/07 21:13:05]

I don't think you need the DefaultConfig() function. Why can't you simply ask
for a new level controller config struct? The ctor should give you the default
config.

[peah-webrtc, 2016/09/08 09:13:48]

The Config is a struct that is defined inside audio_processing.h and I'm not
convinced the constructor of that can be able to properly configure the
submodules of APM. That would require logic in the submodules to be put into the
Config functionality which I don't think we want to have.

The best approach I could of think that does not require any settings logic
oufside of the submodules is to ask the LevelController for what it thinks a
properly configured default config should be.

An option could be to send the faulty Config as an input to the DefaultConfig()
method so that the submodule can do it's best to fix the parameters that are
set. (Probably the method should then be renamed to FixConfig as you previously
proposed).

WDYT?

 
How would we accomp

[the sun, 2016/09/08 09:45:55]

Going forward, I'd like to see the Config definitions move into their respective
sub modules.

Also, in this case we're talking about a single bool. Let's stick to just using
the default ctor for now and introduce specialized methods to generate other
default configs, when we actually need that.

[peah-webrtc, 2016/09/08 19:34:24]

That makes sense.
Done.

+  }
+
+  // Run in a single-threaded manner when applying the settings.
+  rtc::CritScope cs_render(&crit_render_);

[the sun, 2016/09/07 15:49:39]

Nice that you're making the locked section as small as possible!

[peah-webrtc, 2016/09/08 09:13:48]

:-)

+  rtc::CritScope cs_capture(&crit_capture_);
+
+  if (config.level_controller.enabled !=
+      capture_nonlocked_.level_controller_enabled) {
+    InitializeLevelController();
+    LOG(LS_INFO) << "Level controller activated: "
+                 << capture_nonlocked_.level_controller_enabled;
+    capture_nonlocked_.level_controller_enabled =
+        config.level_controller.enabled;
+  }
+}
+
+void AudioProcessingImpl::SetExtraOptions(const webrtc::Config& config) {
   // Run in a single-threaded manner when setting the extra options.
   rtc::CritScope cs_render(&crit_render_);
   rtc::CritScope cs_capture(&crit_capture_);
@@ -427,16 +452,6 @@ void AudioProcessingImpl::SetExtraOptions(const Config& config) {
     InitializeTransient();
   }
 
-  if (capture_nonlocked_.level_controller_enabled !=
-      config.Get<LevelControl>().enabled) {
-    capture_nonlocked_.level_controller_enabled =
-        config.Get<LevelControl>().enabled;
-    LOG(LS_INFO) << "Level controller activated: "
-                 << config.Get<LevelControl>().enabled;
-
-    InitializeLevelController();
-  }
-
 #if WEBRTC_INTELLIGIBILITY_ENHANCER
   if(capture_nonlocked_.intelligibility_enabled !=
      config.Get<Intelligibility>().enabled) {