Remove locks from AECM and move it into private_submodules_

This drops the locks and annotations in EchoControlMobileImpl,
now that the interface is no longer externally accessible.

Additionally, SetEchoPath and GetEchoPath (with surrounding code) is
removed. They are unused.

Bug: webrtc:9929
Change-Id: Ibc6751754614ed39836f6ee6835d7b53dedd828c
Reviewed-on: https://webrtc-review.googlesource.com/c/109025
Reviewed-by: Per Åhgren <peah@webrtc.org>
Commit-Queue: Sam Zackrisson <saza@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25504}

modules/audio_processing/audio_processing_impl.cc

@@ -227,7 +227,6 @@ bool AudioProcessingImpl::ApmSubmoduleStates::LowCutFilteringRequired() const {
 struct AudioProcessingImpl::ApmPublicSubmodules {
   ApmPublicSubmodules() {}
   // Accessed externally of APM without any lock acquired.
-  std::unique_ptr<EchoControlMobileImpl> echo_control_mobile;
   std::unique_ptr<GainControlImpl> gain_control;
   std::unique_ptr<LevelEstimatorImpl> level_estimator;
   std::unique_ptr<NoiseSuppressionImpl> noise_suppression;
@@ -253,8 +252,9 @@ struct AudioProcessingImpl::ApmPrivateSubmodules {
   std::unique_ptr<GainController2> gain_controller2;
   std::unique_ptr<LowCutFilter> low_cut_filter;
   rtc::scoped_refptr<EchoDetector> echo_detector;
-  std::unique_ptr<EchoControl> echo_controller;
   std::unique_ptr<EchoCancellationImpl> echo_cancellation;
+  std::unique_ptr<EchoControl> echo_controller;
+  std::unique_ptr<EchoControlMobileImpl> echo_control_mobile;
   std::unique_ptr<CustomProcessing> capture_post_processor;
   std::unique_ptr<CustomProcessing> render_pre_processor;
   std::unique_ptr<GainApplier> pre_amplifier;
@@ -367,9 +367,6 @@ AudioProcessingImpl::AudioProcessingImpl(
     capture_nonlocked_.echo_controller_enabled =
         static_cast<bool>(echo_control_factory_);
 
-    private_submodules_->echo_cancellation.reset(new EchoCancellationImpl());
-    public_submodules_->echo_control_mobile.reset(
-        new EchoControlMobileImpl(&crit_render_, &crit_capture_));
     public_submodules_->gain_control.reset(
         new GainControlImpl(&crit_render_, &crit_capture_));
     public_submodules_->level_estimator.reset(
@@ -388,6 +385,8 @@ AudioProcessingImpl::AudioProcessingImpl(
           new rtc::RefCountedObject<ResidualEchoDetector>();
     }
 
+    private_submodules_->echo_cancellation.reset(new EchoCancellationImpl());
+    private_submodules_->echo_control_mobile.reset(new EchoControlMobileImpl());
     // TODO(alessiob): Move the injected gain controller once injection is
     // implemented.
     private_submodules_->gain_controller2.reset(new GainController2());
@@ -515,7 +514,7 @@ int AudioProcessingImpl::InitializeLocked() {
   RTC_DCHECK_EQ(0, success);
   success = private_submodules_->echo_cancellation->enable_delay_logging(true);
   RTC_DCHECK_EQ(0, success);
-  public_submodules_->echo_control_mobile->Initialize(
+  private_submodules_->echo_control_mobile->Initialize(
       proc_split_sample_rate_hz(), num_reverse_channels(),
       num_output_channels());
 
@@ -646,7 +645,7 @@ void AudioProcessingImpl::ApplyConfig(const AudioProcessing::Config& config) {
 
   private_submodules_->echo_cancellation->Enable(
       config_.echo_canceller.enabled && !config_.echo_canceller.mobile_mode);
-  public_submodules_->echo_control_mobile->Enable(
+  private_submodules_->echo_control_mobile->Enable(
       config_.echo_canceller.enabled && config_.echo_canceller.mobile_mode);
 
   private_submodules_->echo_cancellation->set_suppression_level(
@@ -1063,7 +1062,7 @@ void AudioProcessingImpl::EmptyQueuedRenderAudio() {
   }
 
   while (aecm_render_signal_queue_->Remove(&aecm_capture_queue_buffer_)) {
-    public_submodules_->echo_control_mobile->ProcessRenderAudio(
+    private_submodules_->echo_control_mobile->ProcessRenderAudio(
         aecm_capture_queue_buffer_);
   }
 
@@ -1085,9 +1084,6 @@ int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
     // Acquire the capture lock in order to safely call the function
     // that retrieves the render side data. This function accesses APM
     // getters that need the capture lock held when being called.
-    // The lock needs to be released as
-    // public_submodules_->echo_control_mobile->is_enabled() acquires this lock
-    // as well.
     rtc::CritScope cs_capture(&crit_capture_);
     EmptyQueuedRenderAudio();
   }
@@ -1157,7 +1153,7 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() {
   // TODO(peah): Simplify once the public API Enable functions for these
   // are moved to APM.
   RTC_DCHECK(!(private_submodules_->echo_cancellation->is_enabled() &&
-               public_submodules_->echo_control_mobile->is_enabled()));
+               private_submodules_->echo_control_mobile->is_enabled()));
 
   MaybeUpdateHistograms();
 
@@ -1261,7 +1257,7 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() {
         capture_buffer, stream_delay_ms()));
   }
 
-  if (public_submodules_->echo_control_mobile->is_enabled() &&
+  if (private_submodules_->echo_control_mobile->is_enabled() &&
       public_submodules_->noise_suppression->is_enabled()) {
     capture_buffer->CopyLowPassToReference();
   }
@@ -1269,14 +1265,14 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() {
 
   // Ensure that the stream delay was set before the call to the
   // AECM ProcessCaptureAudio function.
-  if (public_submodules_->echo_control_mobile->is_enabled() &&
+  if (private_submodules_->echo_control_mobile->is_enabled() &&
       !was_stream_delay_set()) {
     return AudioProcessing::kStreamParameterNotSetError;
   }
 
   if (!(private_submodules_->echo_controller ||
         private_submodules_->echo_cancellation->is_enabled())) {
-    RETURN_ON_ERR(public_submodules_->echo_control_mobile->ProcessCaptureAudio(
+    RETURN_ON_ERR(private_submodules_->echo_control_mobile->ProcessCaptureAudio(
         capture_buffer, stream_delay_ms()));
   }
 
@@ -1676,7 +1672,7 @@ bool AudioProcessingImpl::UpdateActiveSubmoduleStates() {
   return submodule_states_.Update(
       config_.high_pass_filter.enabled,
       private_submodules_->echo_cancellation->is_enabled(),
-      public_submodules_->echo_control_mobile->is_enabled(),
+      private_submodules_->echo_control_mobile->is_enabled(),
       config_.residual_echo_detector.enabled,
       public_submodules_->noise_suppression->is_enabled(),
       public_submodules_->gain_control->is_enabled(),
@@ -1864,11 +1860,11 @@ void AudioProcessingImpl::WriteAecDumpConfigMessage(bool forced) {
       private_submodules_->echo_cancellation->suppression_level());
 
   apm_config.aecm_enabled =
-      public_submodules_->echo_control_mobile->is_enabled();
+      private_submodules_->echo_control_mobile->is_enabled();
   apm_config.aecm_comfort_noise_enabled =
-      public_submodules_->echo_control_mobile->is_comfort_noise_enabled();
+      private_submodules_->echo_control_mobile->is_comfort_noise_enabled();
-  apm_config.aecm_routing_mode =
+  apm_config.aecm_routing_mode = static_cast<int>(
-      static_cast<int>(public_submodules_->echo_control_mobile->routing_mode());
+      private_submodules_->echo_control_mobile->routing_mode());
 
   apm_config.agc_enabled = public_submodules_->gain_control->is_enabled();
   apm_config.agc_mode =

modules/audio_processing/echo_control_mobile_bit_exact_unittest.cc

@@ -64,9 +64,7 @@ void RunBitexactnessTest(int sample_rate_hz,
                          EchoControlMobileImpl::RoutingMode routing_mode,
                          bool comfort_noise_enabled,
                          const rtc::ArrayView<const float>& output_reference) {
-  rtc::CriticalSection crit_render;
+  EchoControlMobileImpl echo_control_mobile;
-  rtc::CriticalSection crit_capture;
-  EchoControlMobileImpl echo_control_mobile(&crit_render, &crit_capture);
   SetupComponent(sample_rate_hz, routing_mode, comfort_noise_enabled,
                  &echo_control_mobile);
 

modules/audio_processing/echo_control_mobile_impl.cc

@@ -57,10 +57,6 @@ AudioProcessing::Error MapError(int err) {
 }
 }  // namespace
 
-size_t EchoControlMobileImpl::echo_path_size_bytes() {
-  return WebRtcAecm_echo_path_size_bytes();
-}
-
 struct EchoControlMobileImpl::StreamProperties {
   StreamProperties() = delete;
   StreamProperties(int sample_rate_hz,
@@ -92,17 +88,10 @@ class EchoControlMobileImpl::Canceller {
     return state_;
   }
 
-  void Initialize(int sample_rate_hz,
+  void Initialize(int sample_rate_hz) {
-                  unsigned char* external_echo_path,
-                  size_t echo_path_size_bytes) {
     RTC_DCHECK(state_);
     int error = WebRtcAecm_Init(state_, sample_rate_hz);
     RTC_DCHECK_EQ(AudioProcessing::kNoError, error);
-    if (external_echo_path != NULL) {
-      error = WebRtcAecm_InitEchoPath(state_, external_echo_path,
-                                      echo_path_size_bytes);
-      RTC_DCHECK_EQ(AudioProcessing::kNoError, error);
-    }
   }
 
  private:
@@ -110,27 +99,13 @@ class EchoControlMobileImpl::Canceller {
   RTC_DISALLOW_COPY_AND_ASSIGN(Canceller);
 };
 
-EchoControlMobileImpl::EchoControlMobileImpl(rtc::CriticalSection* crit_render,
+EchoControlMobileImpl::EchoControlMobileImpl()
-                                             rtc::CriticalSection* crit_capture)
+    : routing_mode_(kSpeakerphone), comfort_noise_enabled_(false) {}
-    : crit_render_(crit_render),
-      crit_capture_(crit_capture),
-      routing_mode_(kSpeakerphone),
-      comfort_noise_enabled_(false),
-      external_echo_path_(NULL) {
-  RTC_DCHECK(crit_render);
-  RTC_DCHECK(crit_capture);
-}
 
-EchoControlMobileImpl::~EchoControlMobileImpl() {
+EchoControlMobileImpl::~EchoControlMobileImpl() {}
-  if (external_echo_path_ != NULL) {
-    delete[] external_echo_path_;
-    external_echo_path_ = NULL;
-  }
-}
 
 void EchoControlMobileImpl::ProcessRenderAudio(
     rtc::ArrayView<const int16_t> packed_render_audio) {
-  rtc::CritScope cs_capture(crit_capture_);
   if (!enabled_) {
     return;
   }
@@ -183,7 +158,6 @@ size_t EchoControlMobileImpl::NumCancellersRequired(
 
 int EchoControlMobileImpl::ProcessCaptureAudio(AudioBuffer* audio,
                                                int stream_delay_ms) {
-  rtc::CritScope cs_capture(crit_capture_);
   if (!enabled_) {
     return AudioProcessing::kNoError;
   }
@@ -230,8 +204,6 @@ int EchoControlMobileImpl::ProcessCaptureAudio(AudioBuffer* audio,
 
 int EchoControlMobileImpl::Enable(bool enable) {
   // Ensure AEC and AECM are not both enabled.
-  rtc::CritScope cs_render(crit_render_);
-  rtc::CritScope cs_capture(crit_capture_);
   RTC_DCHECK(stream_properties_);
 
   if (enable &&
@@ -254,7 +226,6 @@ int EchoControlMobileImpl::Enable(bool enable) {
 }
 
 bool EchoControlMobileImpl::is_enabled() const {
-  rtc::CritScope cs(crit_capture_);
   return enabled_;
 }
 
@@ -262,90 +233,26 @@ int EchoControlMobileImpl::set_routing_mode(RoutingMode mode) {
   if (MapSetting(mode) == -1) {
     return AudioProcessing::kBadParameterError;
   }
-
-  {
-    rtc::CritScope cs(crit_capture_);
     routing_mode_ = mode;
-  }
   return Configure();
 }
 
 EchoControlMobileImpl::RoutingMode EchoControlMobileImpl::routing_mode() const {
-  rtc::CritScope cs(crit_capture_);
   return routing_mode_;
 }
 
 int EchoControlMobileImpl::enable_comfort_noise(bool enable) {
-  {
-    rtc::CritScope cs(crit_capture_);
     comfort_noise_enabled_ = enable;
-  }
   return Configure();
 }
 
 bool EchoControlMobileImpl::is_comfort_noise_enabled() const {
-  rtc::CritScope cs(crit_capture_);
   return comfort_noise_enabled_;
 }
 
-int EchoControlMobileImpl::SetEchoPath(const void* echo_path,
-                                       size_t size_bytes) {
-  {
-    rtc::CritScope cs_render(crit_render_);
-    rtc::CritScope cs_capture(crit_capture_);
-    if (echo_path == NULL) {
-      return AudioProcessing::kNullPointerError;
-    }
-    if (size_bytes != echo_path_size_bytes()) {
-      // Size mismatch
-      return AudioProcessing::kBadParameterError;
-    }
-
-    if (external_echo_path_ == NULL) {
-      external_echo_path_ = new unsigned char[size_bytes];
-    }
-    memcpy(external_echo_path_, echo_path, size_bytes);
-  }
-
-  // TODO(peah): Simplify once the Enable function has been removed from
-  // the public APM API.
-  RTC_DCHECK(stream_properties_);
-  Initialize(stream_properties_->sample_rate_hz,
-             stream_properties_->num_reverse_channels,
-             stream_properties_->num_output_channels);
-  return AudioProcessing::kNoError;
-}
-
-int EchoControlMobileImpl::GetEchoPath(void* echo_path,
-                                       size_t size_bytes) const {
-  rtc::CritScope cs(crit_capture_);
-  if (echo_path == NULL) {
-    return AudioProcessing::kNullPointerError;
-  }
-  if (size_bytes != echo_path_size_bytes()) {
-    // Size mismatch
-    return AudioProcessing::kBadParameterError;
-  }
-  if (!enabled_) {
-    return AudioProcessing::kNotEnabledError;
-  }
-
-  // Get the echo path from the first channel
-  int32_t err =
-      WebRtcAecm_GetEchoPath(cancellers_[0]->state(), echo_path, size_bytes);
-  if (err != 0) {
-    return MapError(err);
-  }
-
-  return AudioProcessing::kNoError;
-}
-
 void EchoControlMobileImpl::Initialize(int sample_rate_hz,
                                        size_t num_reverse_channels,
                                        size_t num_output_channels) {
-  rtc::CritScope cs_render(crit_render_);
-  rtc::CritScope cs_capture(crit_capture_);
-
   stream_properties_.reset(new StreamProperties(
       sample_rate_hz, num_reverse_channels, num_output_channels));
 
@@ -365,16 +272,12 @@ void EchoControlMobileImpl::Initialize(int sample_rate_hz,
     if (!canceller) {
       canceller.reset(new Canceller());
     }
-    canceller->Initialize(sample_rate_hz, external_echo_path_,
+    canceller->Initialize(sample_rate_hz);
-                          echo_path_size_bytes());
   }
-
   Configure();
 }
 
 int EchoControlMobileImpl::Configure() {
-  rtc::CritScope cs_render(crit_render_);
-  rtc::CritScope cs_capture(crit_capture_);
   AecmConfig config;
   config.cngMode = comfort_noise_enabled_;
   config.echoMode = MapSetting(routing_mode_);

modules/audio_processing/echo_control_mobile_impl.h

@@ -17,9 +17,6 @@
 #include <vector>
 
 #include "api/array_view.h"
-#include "rtc_base/constructormagic.h"
-#include "rtc_base/criticalsection.h"
-#include "rtc_base/thread_annotations.h"
 
 namespace webrtc {
 
@@ -29,8 +26,7 @@ class AudioBuffer;
 // robust option intended for use on mobile devices.
 class EchoControlMobileImpl {
  public:
-  EchoControlMobileImpl(rtc::CriticalSection* crit_render,
+  EchoControlMobileImpl();
-                        rtc::CriticalSection* crit_capture);
 
   ~EchoControlMobileImpl();
 
@@ -58,26 +54,6 @@ class EchoControlMobileImpl {
   int enable_comfort_noise(bool enable);
   bool is_comfort_noise_enabled() const;
 
-  // A typical use case is to initialize the component with an echo path from a
-  // previous call. The echo path is retrieved using |GetEchoPath()|, typically
-  // at the end of a call. The data can then be stored for later use as an
-  // initializer before the next call, using |SetEchoPath()|.
-  //
-  // Controlling the echo path this way requires the data |size_bytes| to match
-  // the internal echo path size. This size can be acquired using
-  // |echo_path_size_bytes()|. |SetEchoPath()| causes an entire reset, worth
-  // noting if it is to be called during an ongoing call.
-  //
-  // It is possible that version incompatibilities may result in a stored echo
-  // path of the incorrect size. In this case, the stored path should be
-  // discarded.
-  int SetEchoPath(const void* echo_path, size_t size_bytes);
-  int GetEchoPath(void* echo_path, size_t size_bytes) const;
-
-  // The returned path size is guaranteed not to change for the lifetime of
-  // the application.
-  static size_t echo_path_size_bytes();
-
   void ProcessRenderAudio(rtc::ArrayView<const int16_t> packed_render_audio);
   int ProcessCaptureAudio(AudioBuffer* audio, int stream_delay_ms);
 
@@ -99,20 +75,13 @@ class EchoControlMobileImpl {
 
   int Configure();
 
-  rtc::CriticalSection* const crit_render_ RTC_ACQUIRED_BEFORE(crit_capture_);
-  rtc::CriticalSection* const crit_capture_;
-
   bool enabled_ = false;
 
-  RoutingMode routing_mode_ RTC_GUARDED_BY(crit_capture_);
+  RoutingMode routing_mode_;
-  bool comfort_noise_enabled_ RTC_GUARDED_BY(crit_capture_);
+  bool comfort_noise_enabled_;
-  unsigned char* external_echo_path_ RTC_GUARDED_BY(crit_render_)
-      RTC_GUARDED_BY(crit_capture_);
 
   std::vector<std::unique_ptr<Canceller>> cancellers_;
   std::unique_ptr<StreamProperties> stream_properties_;
-
-  RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(EchoControlMobileImpl);
 };
 }  // namespace webrtc
 

modules/audio_processing/echo_control_mobile_unittest.cc

@@ -18,9 +18,7 @@
 
 namespace webrtc {
 TEST(EchoControlMobileTest, InterfaceConfiguration) {
-  rtc::CriticalSection crit_render;
+  EchoControlMobileImpl aecm;
-  rtc::CriticalSection crit_capture;
-  EchoControlMobileImpl aecm(&crit_render, &crit_capture);
   aecm.Initialize(AudioProcessing::kSampleRate16kHz, 2, 2);
 
   // Turn AECM on
@@ -46,28 +44,6 @@ TEST(EchoControlMobileTest, InterfaceConfiguration) {
   EXPECT_EQ(0, aecm.enable_comfort_noise(true));
   EXPECT_TRUE(aecm.is_comfort_noise_enabled());
 
-  // Set and get echo path
-  const size_t echo_path_size = aecm.echo_path_size_bytes();
-  std::vector<uint8_t> echo_path_in(echo_path_size);
-  std::vector<uint8_t> echo_path_out(echo_path_size);
-  EXPECT_EQ(AudioProcessing::kNullPointerError,
-            aecm.SetEchoPath(nullptr, echo_path_size));
-  EXPECT_EQ(AudioProcessing::kNullPointerError,
-            aecm.GetEchoPath(nullptr, echo_path_size));
-  EXPECT_EQ(AudioProcessing::kBadParameterError,
-            aecm.GetEchoPath(echo_path_out.data(), 1));
-  EXPECT_EQ(0, aecm.GetEchoPath(echo_path_out.data(), echo_path_size));
-  for (size_t i = 0; i < echo_path_size; i++) {
-    echo_path_in[i] = echo_path_out[i] + 1;
-  }
-  EXPECT_EQ(AudioProcessing::kBadParameterError,
-            aecm.SetEchoPath(echo_path_in.data(), 1));
-  EXPECT_EQ(0, aecm.SetEchoPath(echo_path_in.data(), echo_path_size));
-  EXPECT_EQ(0, aecm.GetEchoPath(echo_path_out.data(), echo_path_size));
-  for (size_t i = 0; i < echo_path_size; i++) {
-    EXPECT_EQ(echo_path_in[i], echo_path_out[i]);
-  }
-
   // Turn AECM off
   EXPECT_EQ(0, aecm.Enable(false));
   EXPECT_FALSE(aecm.is_enabled());