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

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.
  */
 
 #include "webrtc/modules/audio_processing/audio_processing_impl.h"
 
 #include <algorithm>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/platform_file.h"
 #include "webrtc/base/trace_event.h"
 #include "webrtc/common_audio/audio_converter.h"
 #include "webrtc/common_audio/channel_buffer.h"
 #include "webrtc/common_audio/include/audio_util.h"
 #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h"
 #include "webrtc/modules/audio_processing/aec/aec_core.h"
 #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"
 #include "webrtc/modules/audio_processing/gain_control_for_experimental_agc.h"
 #include "webrtc/modules/audio_processing/gain_control_impl.h"
 #include "webrtc/modules/audio_processing/high_pass_filter_impl.h"
 #if WEBRTC_INTELLIGIBILITY_ENHANCER
 #include "webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h"
 #endif
 #include "webrtc/modules/audio_processing/level_controller/level_controller.h"
@@ skipping 175 matching lines @@
   return false;
 #endif
 }
 
 struct AudioProcessingImpl::ApmPublicSubmodules {
   ApmPublicSubmodules() {}
   // Accessed externally of APM without any lock acquired.
   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;
   std::unique_ptr<GainControlForExperimentalAgc>
       gain_control_for_experimental_agc;
 
   // Accessed internally from both render and capture.
   std::unique_ptr<TransientSuppressor> transient_suppressor;
 #if WEBRTC_INTELLIGIBILITY_ENHANCER
   std::unique_ptr<IntelligibilityEnhancer> intelligibility_enhancer;
 #endif
 };
 
 struct AudioProcessingImpl::ApmPrivateSubmodules {
   explicit ApmPrivateSubmodules(NonlinearBeamformer* beamformer)
       : beamformer(beamformer) {}
   // 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;
 };
 
 AudioProcessing* AudioProcessing::Create() {
   webrtc::Config config;
   return Create(config, nullptr);
 }
 
 AudioProcessing* AudioProcessing::Create(const webrtc::Config& config) {
   return Create(config, nullptr);
@@ skipping 35 matching lines @@
   {
     rtc::CritScope cs_render(&crit_render_);
     rtc::CritScope cs_capture(&crit_capture_);
 
     public_submodules_->echo_cancellation.reset(
         new EchoCancellationImpl(&crit_render_, &crit_capture_));
     public_submodules_->echo_control_mobile.reset(
         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(
         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_));
 
@@ skipping 147 matching lines @@
     }
     private_submodules_->agc_manager->Initialize();
     private_submodules_->agc_manager->SetCaptureMuted(
         capture_.output_will_be_muted);
   }
   InitializeTransient();
   InitializeBeamformer();
 #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());
   public_submodules_->level_estimator->Initialize();
   InitializeLevelController();
 
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   if (debug_dump_.debug_file->is_open()) {
     int err = WriteInitMessage();
     if (err != kNoError) {
@@ skipping 101 matching lines @@
     capture_nonlocked_.level_controller_enabled =
         config_.level_controller.enabled;
     // TODO(peah): Remove the conditional initialization to always initialize
     // the level controller regardless of whether it is enabled or not.
     InitializeLevelController();
   }
   LOG(LS_INFO) << "Level controller activated: "
                << 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) {
   // 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 344 matching lines @@
     capture_buffer->SplitIntoFrequencyBands();
   }
 
   if (capture_nonlocked_.beamformer_enabled) {
     private_submodules_->beamformer->AnalyzeChunk(
         *capture_buffer->split_data_f());
     // Discards all channels by the leftmost one.
     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);
 
   // Ensure that the stream delay was set before the call to the
   // AEC ProcessCaptureAudio function.
   if (public_submodules_->echo_cancellation->is_enabled() &&
       !was_stream_delay_set()) {
     return AudioProcessing::kStreamParameterNotSetError;
   }
@@ skipping 385 matching lines @@
 }
 
 GainControl* AudioProcessingImpl::gain_control() const {
   if (constants_.use_experimental_agc) {
     return public_submodules_->gain_control_for_experimental_agc.get();
   }
   return public_submodules_->gain_control.get();
 }
 
 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 {
   return public_submodules_->level_estimator.get();
 }
 
 NoiseSuppression* AudioProcessingImpl::noise_suppression() const {
   return public_submodules_->noise_suppression.get();
 }
 
 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(),
       capture_nonlocked_.intelligibility_enabled,
       capture_nonlocked_.beamformer_enabled,
       public_submodules_->gain_control->is_enabled(),
       capture_nonlocked_.level_controller_enabled,
       public_submodules_->voice_detection->is_enabled(),
       public_submodules_->level_estimator->is_enabled(),
       capture_.transient_suppressor_enabled);
@@ skipping 27 matching lines @@
   if (capture_nonlocked_.intelligibility_enabled) {
     public_submodules_->intelligibility_enhancer.reset(
         new IntelligibilityEnhancer(capture_nonlocked_.split_rate,
                                     render_.render_audio->num_channels(),
                                     render_.render_audio->num_bands(),
                                     NoiseSuppressionImpl::num_noise_bins()));
   }
 #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());
 }
 
 void AudioProcessingImpl::MaybeUpdateHistograms() {
   static const int kMinDiffDelayMs = 60;
 
   if (echo_cancellation()->is_enabled()) {
     // Activate delay_jumps_ counters if we know echo_cancellation is runnning.
     // If a stream has echo we know that the echo_cancellation is in process.
@@ skipping 159 matching lines @@
   config.set_aecm_routing_mode(static_cast<int>(
       public_submodules_->echo_control_mobile->routing_mode()));
 
   config.set_agc_enabled(public_submodules_->gain_control->is_enabled());
   config.set_agc_mode(
       static_cast<int>(public_submodules_->gain_control->mode()));
   config.set_agc_limiter_enabled(
       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(
       static_cast<int>(public_submodules_->noise_suppression->level()));
 
   config.set_transient_suppression_enabled(
       capture_.transient_suppressor_enabled);
   config.set_intelligibility_enhancer_enabled(
       capture_nonlocked_.intelligibility_enabled);
 
@@ skipping 42 matching lines @@
       capture_processing_format(kSampleRate16kHz),
       split_rate(kSampleRate16kHz) {}
 
 AudioProcessingImpl::ApmCaptureState::~ApmCaptureState() = default;
 
 AudioProcessingImpl::ApmRenderState::ApmRenderState() = default;
 
 AudioProcessingImpl::ApmRenderState::~ApmRenderState() = default;
 
 }  // namespace webrtc