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 72f154a66d94c84034e078897ccf40e39a4d1a2b..127c7c3afa482a6858b3d6af3728018d6cfc2048 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -530,22 +530,23 @@ int AudioProcessingImpl::InitializeLocked(const ProcessingConfig& config) {
 }
 
 void AudioProcessingImpl::ApplyConfig(const AudioProcessing::Config& config) {
-  AudioProcessing::Config config_to_use = config;
+  // Run in a single-threaded manner when applying the settings.
+  rtc::CritScope cs_render(&crit_render_);
+  rtc::CritScope cs_capture(&crit_capture_);
 
-  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;
+    private_submodules_->level_controller->ApplyConfig(default_config);
+  } else {
+    private_submodules_->level_controller->ApplyConfig(config.level_controller);
   }

[the sun, 2016/09/14 10:00:59]

Why change the previous code? You could just have put the call to:
private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller);
beneath the CritScope:s.

Also, it looks like that code needs to go after the below initialization of the
LC, otherwise the initial level may be ignored?

Note further, that we tripped up the logic below in the previous CL to introduce
the Config scheme: you are ignoring whatever LC::Validate() has to say about the
config and instead using the enabled flag from the config we've been given as
argument (even though it might be set differently in the default state of
LC::Config. The below code should use "config_to_use", not "config".

[the sun, 2016/09/14 17:02:15]

If I'm not mistaken about that, it seems there's one or two test cases
missing...

[hlundin-webrtc, 2016/09/15 07:50:17]

+1 on all of the above.

[peah-webrtc, 2016/09/16 07:11:06]

Acknowledged.

[peah-webrtc, 2016/09/16 07:11:06]

Very, very likely. What I guess you mean is to check whether falsy config input
gives nonproper APM initialization behavior? Right? 

[peah-webrtc, 2016/09/16 07:11:06]

The reason for moving the critscopes is that 
1) private_submodules_ can only be accessed while holding crit_capture_
2) crit_render_ must always be aquired before crit_capture_.
3) Since the AudioProcessing::Config is no longer a POD, and I don't want to add
a copy/assignment constructor if not strictly neccessary, I cannot simply copy
the config.

Regarding the enable flag, good find. Will fix that once we decide on the
copy/assignment constructor issue.

I have a feeling that we need to add a copy/assignment constructor for the
::Config struct since in order to
1) simplify this method which will grow in complexity as we add more methods.
2) allow caching a copy of the config in the APM state.

WDYT?

[the sun, 2016/09/16 08:00:40]

Yes, so figure out the config state before taking the locks, and only call
ApplyConfig later on (i.e. after enable/disable of the module).
Right - so if we can instead set the initial level as a default in the config,
we wouldn't need the Optional which forces us have explicit
copy/assignment/ctor/dtor. Plus, the code wouldn't have to deal with the (tiny
bit of) extra ambiguity. Sounds like a win to me. :)
I think the longer we can stick to only primitive types in the config, the
better. You may be right that eventually we'll need to add copy/assignment, but
let's pretend the world is all fluff and rainbows and cross that bridge once we
need to.

[the sun, 2016/09/16 08:00:40]

Yes, something like that - figure out a test to trigger the error I spotted,
with using the wrong config object when deciding whether to enable the LC.

[hlundin-webrtc, 2016/09/16 08:17:16]

Acknowledged.

[peah-webrtc, 2016/09/16 11:36:04]

I really like rainbows and fluffy clouds! I'll include that level into the
state.

[peah-webrtc, 2016/09/16 11:36:04]

Acknowledged.

 
-  // 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 !=
       capture_nonlocked_.level_controller_enabled) {
     InitializeLevelController();