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 280 matching lines @@
     public_submodules_->level_estimator.reset(
         new LevelEstimatorImpl(&crit_capture_));
     public_submodules_->noise_suppression.reset(
         new NoiseSuppressionImpl(&crit_capture_));
     public_submodules_->voice_detection.reset(
         new VoiceDetectionImpl(&crit_capture_));
     public_submodules_->gain_control_for_experimental_agc.reset(
         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());
   }
 
   SetExtraOptions(config);
 }
 
 AudioProcessingImpl::~AudioProcessingImpl() {
   // Depends on gain_control_ and
   // public_submodules_->gain_control_for_experimental_agc.
   private_submodules_->agc_manager.reset();
@@ skipping 206 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;
+  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.

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