Introduced the new parameter setting scheme for activating the high-pass filter in APM

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 93e43f7add126592ddf940480d53292b5280ce45..18546b2560e4231754acaaf6faba5999163b6b6e 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -23,6 +23,7 @@
 #include "webrtc/modules/audio_processing/agc/agc_manager_direct.h"
 #include "webrtc/modules/audio_processing/audio_buffer.h"
 #include "webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h"
+#include "webrtc/modules/audio_processing/biquad_high_pass_filter.h"
 #include "webrtc/modules/audio_processing/common.h"
 #include "webrtc/modules/audio_processing/echo_cancellation_impl.h"
 #include "webrtc/modules/audio_processing/echo_control_mobile_impl.h"
@@ -218,7 +219,7 @@ struct AudioProcessingImpl::ApmPublicSubmodules {
   std::unique_ptr<EchoCancellationImpl> echo_cancellation;
   std::unique_ptr<EchoControlMobileImpl> echo_control_mobile;
   std::unique_ptr<GainControlImpl> gain_control;
-  std::unique_ptr<HighPassFilterImpl> high_pass_filter;
+  std::unique_ptr<HighPassFilterImpl> high_pass_filter_impl;

[the sun, 2016/10/26 09:06:43]

Since the HPFImpl is now just redirecting calls onto APM, I think it is a bit
confusing that it sits among the other submodules here. IIRC the reason for this
sub grouping was to make locking semantics explicit.

How about you instead add it directly in the APM class?

You could even keep the redirecting HPFImpl in an anonymous namespace right here
in the .cc.

[peah-webrtc, 2016/10/28 05:50:28]

Great suggestion!
I added it to the APM class. I did, however, need to put it as a unique pointer
as I otherwise gets issues with the constness of the high_pass_filter()
interface function.

Done.

   std::unique_ptr<LevelEstimatorImpl> level_estimator;
   std::unique_ptr<NoiseSuppressionImpl> noise_suppression;
   std::unique_ptr<VoiceDetectionImpl> voice_detection;
@@ -238,6 +239,7 @@ struct AudioProcessingImpl::ApmPrivateSubmodules {
   // Accessed internally from capture or during initialization
   std::unique_ptr<NonlinearBeamformer> beamformer;
   std::unique_ptr<AgcManagerDirect> agc_manager;
+  std::unique_ptr<BiquadHighPassFilter> high_pass_filter;
   std::unique_ptr<LevelController> level_controller;
 };
 
@@ -293,8 +295,8 @@ AudioProcessingImpl::AudioProcessingImpl(const webrtc::Config& config,
         new EchoControlMobileImpl(&crit_render_, &crit_capture_));
     public_submodules_->gain_control.reset(
         new GainControlImpl(&crit_capture_, &crit_capture_));
-    public_submodules_->high_pass_filter.reset(
-        new HighPassFilterImpl(&crit_capture_));
+    public_submodules_->high_pass_filter_impl.reset(
+        new HighPassFilterImpl(this));
     public_submodules_->level_estimator.reset(
         new LevelEstimatorImpl(&crit_capture_));
     public_submodules_->noise_suppression.reset(
@@ -462,8 +464,7 @@ int AudioProcessingImpl::InitializeLocked() {
 #if WEBRTC_INTELLIGIBILITY_ENHANCER
   InitializeIntelligibility();
 #endif
-  public_submodules_->high_pass_filter->Initialize(num_proc_channels(),
-                                                   proc_sample_rate_hz());
+  InitializeHighPassFilter();
   public_submodules_->noise_suppression->Initialize(num_proc_channels(),
                                                     proc_sample_rate_hz());
   public_submodules_->voice_detection->Initialize(proc_split_sample_rate_hz());
@@ -585,6 +586,11 @@ void AudioProcessingImpl::ApplyConfig(const AudioProcessing::Config& config) {
                << capture_nonlocked_.level_controller_enabled;
 
   private_submodules_->level_controller->ApplyConfig(config_.level_controller);
+
+  InitializeHighPassFilter();
+
+  LOG(LS_INFO) << "Highpass filter activated: "

[the sun, 2016/10/26 09:06:43]

Move log line into InitializeHighPassFilter?

[peah-webrtc, 2016/10/28 05:50:28]

That would make sense, but currently InitializeHighPassFilter may be called
multiple times during a call and is not really tied to the activation of the
highpassfilter. I'd rather have it such that there is one logline when the
highpassfilter is activated and not one for each time it is initialized.

Therefore, I keep it as it is. WDYT?

+               << config_.high_pass_filter.enabled;
 }
 
 void AudioProcessingImpl::SetExtraOptions(const webrtc::Config& config) {
@@ -949,7 +955,9 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() {
     capture_buffer->set_num_channels(1);
   }
 
-  public_submodules_->high_pass_filter->ProcessCaptureAudio(capture_buffer);
+  if (private_submodules_->high_pass_filter) {
+    private_submodules_->high_pass_filter->Process(capture_buffer);
+  }
   RETURN_ON_ERR(
       public_submodules_->gain_control->AnalyzeCaptureAudio(capture_buffer));
   public_submodules_->noise_suppression->AnalyzeCaptureAudio(capture_buffer);
@@ -1355,7 +1363,7 @@ GainControl* AudioProcessingImpl::gain_control() const {
 }
 
 HighPassFilter* AudioProcessingImpl::high_pass_filter() const {
-  return public_submodules_->high_pass_filter.get();
+  return public_submodules_->high_pass_filter_impl.get();
 }
 
 LevelEstimator* AudioProcessingImpl::level_estimator() const {
@@ -1370,9 +1378,18 @@ VoiceDetection* AudioProcessingImpl::voice_detection() const {
   return public_submodules_->voice_detection.get();
 }
 
+void AudioProcessingImpl::EnableHighPassFilter(bool enable) {
+  config_.high_pass_filter.enabled = enable;

[the sun, 2016/10/26 09:06:43]

Thread checks?

[peah-webrtc, 2016/10/28 05:50:28]

I see now that there is a lock missing here. I fixed that.

Regarding thread checkers, that would make sense. But I won't include it since
1) It does not change the way that this method is called. Either it works, or
not. We could try it to see whether we are called in a consistent manner and
change that scheme if it is not how we want it but I think that should be a
separate CL.
2) The code is designed to work  without any restrictions on what thread is
calling this method. Adding a thread check that passes would merely lock the
current calling scheme so that it cannot be changed. I don't think that we
should do that until we see a need for it from an APM perspective or until we
have explicitly designed our APIs to enforce a certain scheme for being called.
3) There is nothing in our APIs that states that the HighPassFilter must be used
from a single thread. Therefore I don't think we can/should impose that with
thread checkers.

WDYT?

+  ApplyConfig(config_);
+}
+
+bool AudioProcessingImpl::IsHighPassFilterEnabled() {
+  return config_.high_pass_filter.enabled;
+}
+
 bool AudioProcessingImpl::UpdateActiveSubmoduleStates() {
   return submodule_states_.Update(
-      public_submodules_->high_pass_filter->is_enabled(),
+      config_.high_pass_filter.enabled,
       public_submodules_->echo_cancellation->is_enabled(),
       public_submodules_->echo_control_mobile->is_enabled(),
       public_submodules_->noise_suppression->is_enabled(),
@@ -1420,6 +1437,15 @@ void AudioProcessingImpl::InitializeIntelligibility() {
 #endif
 }
 
+void AudioProcessingImpl::InitializeHighPassFilter() {
+  if (config_.high_pass_filter.enabled) {
+    private_submodules_->high_pass_filter.reset(
+        new BiquadHighPassFilter(num_proc_channels(), proc_sample_rate_hz()));
+  } else {
+    private_submodules_->high_pass_filter.reset();
+  }
+}
+
 void AudioProcessingImpl::InitializeLevelController() {
   private_submodules_->level_controller->Initialize(proc_sample_rate_hz());
 }
@@ -1599,7 +1625,7 @@ int AudioProcessingImpl::WriteConfigMessage(bool forced) {
       public_submodules_->gain_control->is_limiter_enabled());
   config.set_noise_robust_agc_enabled(constants_.use_experimental_agc);
 
-  config.set_hpf_enabled(public_submodules_->high_pass_filter->is_enabled());
+  config.set_hpf_enabled(config_.high_pass_filter.enabled);
 
   config.set_ns_enabled(public_submodules_->noise_suppression->is_enabled());
   config.set_ns_level(