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

webrtc/modules/audio_processing/audio_processing_impl.cc

 /*
  *  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 131 matching lines @@
 struct AudioProcessingImpl::ApmPrivateSubmodules {
   explicit ApmPrivateSubmodules(NonlinearBeamformer* beamformer)
       : beamformer(beamformer) {}
   // Accessed internally from capture or during initialization
   std::unique_ptr<NonlinearBeamformer> beamformer;
   std::unique_ptr<AgcManagerDirect> agc_manager;
   std::unique_ptr<LevelController> level_controller;
 };
 
 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) {
     delete apm;
     apm = nullptr;
   }
 
   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)),
       constants_(config.Get<ExperimentalAgc>().startup_min_volume,
 #if defined(WEBRTC_ANDROID) || defined(WEBRTC_IOS)
                  false),
 #else
                  config.Get<ExperimentalAgc>().enabled),
 #endif
 #if defined(WEBRTC_ANDROID) || defined(WEBRTC_IOS)
       capture_(false,
 #else
       capture_(config.Get<ExperimentalNs>().enabled,
 #endif
                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_);
 
     public_submodules_->echo_cancellation.reset(
         new EchoCancellationImpl(&crit_render_, &crit_capture_));
     public_submodules_->echo_control_mobile.reset(
         new EchoControlMobileImpl(&crit_render_, &crit_capture_));
     public_submodules_->gain_control.reset(
         new GainControlImpl(&crit_capture_, &crit_capture_));
@@ skipping 202 matching lines @@
       capture_nonlocked_.fwd_proc_format.sample_rate_hz() == kSampleRate48kHz) {
     capture_nonlocked_.split_rate = kSampleRate16kHz;
   } else {
     capture_nonlocked_.split_rate =
         capture_nonlocked_.fwd_proc_format.sample_rate_hz();
   }
 
   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_);
 
   public_submodules_->echo_cancellation->SetExtraOptions(config);
 
   if (capture_.transient_suppressor_enabled !=
       config.Get<ExperimentalNs>().enabled) {
     capture_.transient_suppressor_enabled =
         config.Get<ExperimentalNs>().enabled;
     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) {
     capture_nonlocked_.intelligibility_enabled =
         config.Get<Intelligibility>().enabled;
     InitializeIntelligibility();
   }
 #endif
 
 #ifdef WEBRTC_ANDROID_PLATFORM_BUILD
@@ skipping 1068 matching lines @@
   debug_dump_.capture.event_msg->mutable_config()->CopyFrom(config);
 
   RETURN_ON_ERR(WriteMessageToDebugFile(debug_dump_.debug_file.get(),
                                         &debug_dump_.num_bytes_left_for_log_,
                                         &crit_debug_, &debug_dump_.capture));
   return kNoError;
 }
 #endif  // WEBRTC_AUDIOPROC_DEBUG_DUMP
 
 }  // namespace webrtc