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..baa930632a1df22872aa12afcfb24c01e6cf16f5 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -116,6 +116,10 @@ int ClosestHigherNativeRate(int min_proc_rate) {
 
 }  // namespace
 
+bool AudioProcessing::Config::Validate() const {
+  return level_controller.Validate();
+}
+
 // Throughout webrtc, it's assumed that success is represented by zero.
 static_assert(AudioProcessing::kNoError == 0, "kNoError must be zero");
 
@@ -149,15 +153,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 +172,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 +193,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_);
@@ -238,6 +241,28 @@ int AudioProcessingImpl::Initialize() {
   return InitializeLocked();
 }
 
+bool AudioProcessingImpl::ApplyConfig(const AudioProcessing::Config& config) {
+  bool config_ok = config.Validate();
+  RTC_DCHECK(config_ok);
+

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

dd

[peah-webrtc, 2016/08/31 08:08:31]

Done.

+  if (!config_ok) {
+    return false;
+  }
+
+  // Run in a single-threaded manner when applying the settings.
+  rtc::CritScope cs_render(&crit_render_);
+  rtc::CritScope cs_capture(&crit_capture_);
+
+  capture_nonlocked_.level_controller_enabled = config.level_controller.enabled;
+  if (capture_nonlocked_.level_controller_enabled) {

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

You could drop the capture_nonlocked_.level_controller_enabled and just have the
pointer to the LevelController. Destroy it when not needed (pointer is null), or
create it when needed.

[peah-webrtc, 2016/08/31 08:08:31]

I'm not sure I follow that. Do you mean that I should drop it only here, or in
general? I think it would make sense to simplify this but note that the
LevelController does not have any enabled property, i.e., it does not know
whether it is enabled or not. That is tracked by
capture_nonlocked_.level_controller_enabled.

I think it would make sense to simplify this but I'd rather wait until the
parameter scheme has settled. The level controller is not the only one behaving
like this. The intelligibility enhancer and the beamformer does it similarly.

+    InitializeLevelController();
+  }
+  LOG(LS_INFO) << "Level controller activated: "
+               << capture_nonlocked_.level_controller_enabled;
+
+  return true;
+}
+
 int AudioProcessingImpl::Initialize(int input_sample_rate_hz,
                                     int output_sample_rate_hz,
                                     int reverse_sample_rate_hz,
@@ -413,7 +438,7 @@ int AudioProcessingImpl::InitializeLocked(const ProcessingConfig& config) {
   return InitializeLocked();
 }
 
-void AudioProcessingImpl::SetExtraOptions(const Config& config) {
+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) {

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

Note that the new code won't avoid initializing the LC if the enabled flag
doesn't change. Was that intentional?

[peah-webrtc, 2016/08/31 08:08:31]

No, that was not intended at all. Great catch!!!

Done.

-    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) {