Reland of added functionality for specifying the initial signal level to use for the gain estimation

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 9b7b95334542f70241458488324d614a4ec19554..92d7fc0cf0d7cf4ab9058298ea127083c85f9944 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -298,6 +298,8 @@ AudioProcessingImpl::AudioProcessingImpl(const webrtc::Config& config,
         new GainControlForExperimentalAgc(
             public_submodules_->gain_control.get(), &crit_capture_));
 
+    // TODO(peah): Move this creation to happen only when the level controller
+    // is enabled.
     private_submodules_->level_controller.reset(new LevelController());
   }
 
@@ -524,29 +526,35 @@ int AudioProcessingImpl::InitializeLocked(const ProcessingConfig& config) {
 }
 
 void AudioProcessingImpl::ApplyConfig(const AudioProcessing::Config& config) {
-  AudioProcessing::Config config_to_use = config;
+  config_ = config;
 
-  bool config_ok = LevelController::Validate(config_to_use.level_controller);
+  bool config_ok = LevelController::Validate(config_.level_controller);
   if (!config_ok) {
     LOG(LS_ERROR) << "AudioProcessing module config error" << std::endl
                   << "level_controller: "
-                  << LevelController::ToString(config_to_use.level_controller)
+                  << LevelController::ToString(config.level_controller)
                   << std::endl
                   << "Reverting to default parameter set";
-    config_to_use.level_controller = AudioProcessing::Config::LevelController();
+    AudioProcessing::Config::LevelController default_config;
+    config_.level_controller = default_config;

[the sun, 2016/09/16 12:40:24]

nit: Do this in one line. No need to name temporary variables, if not required.

[peah-webrtc, 2016/09/19 16:37:10]

Done.

   }
 
   // Run in a single-threaded manner when applying the settings.
   rtc::CritScope cs_render(&crit_render_);
   rtc::CritScope cs_capture(&crit_capture_);
 
-  if (config.level_controller.enabled !=
+  // TODO(peah): Replace the use of capture_nonlocked_.level_controller_enabled
+  // with the value in config_ everywhere in the code.
+  if (config_.level_controller.enabled !=
       capture_nonlocked_.level_controller_enabled) {
+    capture_nonlocked_.level_controller_enabled =
+        config.level_controller.enabled;

[the sun, 2016/09/16 12:40:24]

config_
where is the test case?

nit: it is nice if the conditional and the assignment is ordered the same way,
for these types of constructs.

[peah-webrtc, 2016/09/19 16:37:10]

The test case is in audio_processing_unittests.cc. But maybe some variant of the
 test is missing there? Please let me know then!

Changed the ordering and changed to using config_ instead of config.

[the sun, 2016/09/19 18:53:56]

Ah, the problem with the test is it checks the flag in config_.level_controller.
Hm. You're right, it does become a mess with all the friend declarations, plus
the white box nature of the test will become a burden. Ideally, we'd have had a
factory method for the LC given to the APM on c-tion - then we could test out
the logic without really creating an LC.

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

I fully agree. We should definitely have that. For the level controller, we
could probably achieve that today but for the other submodules it is not yet
possible so until we can for all submodules we should have a way of doing this
that works for all submodules.

Let's continue the discussion offline.

+    // TODO(peah): Remove the conditional initialization to always initialize
+    // the level controller regardless of whether it is enabled or not.
     InitializeLevelController();
+    // TODO(peah): Add logline regardless of whether this is enabled or not.
     LOG(LS_INFO) << "Level controller activated: "
                  << capture_nonlocked_.level_controller_enabled;
-    capture_nonlocked_.level_controller_enabled =
-        config.level_controller.enabled;
   }

[the sun, 2016/09/16 12:40:24]

Did you forget lc->ApplyConfig()?

[peah-webrtc, 2016/09/19 16:37:10]

Oh no! Great find! I for sure did miss that. 

I'm not sure if we should add a unittest to avoid that though. It becomes quite
messy in the long run with all the friend-class statements. The best thing would
be to add tests that verify the effect of the level controller.

WDYT, is it ok to leave it without this without any further testing to avoid
this? 

[the sun, 2016/09/19 18:53:56]

Let's discuss offline.

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

Acknowledged.

 }