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

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 512 matching lines @@
     capture_nonlocked_.split_rate = kSampleRate16kHz;
   } else {
     capture_nonlocked_.split_rate =
         capture_nonlocked_.fwd_proc_format.sample_rate_hz();
   }
 
   return InitializeLocked();
 }
 
 void AudioProcessingImpl::ApplyConfig(const AudioProcessing::Config& config) {
-  AudioProcessing::Config config_to_use = config;
-
-  bool config_ok = LevelController::Validate(config_to_use.level_controller);
-  if (!config_ok) {
-    LOG(LS_ERROR) << "AudioProcessing module config error" << std::endl
-                  << "level_controller: "
-                  << LevelController::ToString(config_to_use.level_controller)
-                  << std::endl
-                  << "Reverting to default parameter set";
-    config_to_use.level_controller = AudioProcessing::Config::LevelController();
-  }
-
   // 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.level_controller);
+  if (!config_ok) {
+    LOG(LS_ERROR) << "AudioProcessing module config error" << std::endl
+                  << "level_controller: "
+                  << LevelController::ToString(config.level_controller)
+                  << std::endl
+                  << "Reverting to default parameter set";
+    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.

+
   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;
   }
 }
 
@@ skipping 1055 matching lines @@
       fwd_proc_format(kSampleRate16kHz),
       split_rate(kSampleRate16kHz) {}
 
 AudioProcessingImpl::ApmCaptureState::~ApmCaptureState() = default;
 
 AudioProcessingImpl::ApmRenderState::ApmRenderState() = default;
 
 AudioProcessingImpl::ApmRenderState::~ApmRenderState() = default;
 
 }  // namespace webrtc