Merge the preambles of the ProcessStream implementations

The two functions have a lot of shared logic and locking. This CL consolidates that into a single function.

Bug: webrtc:111235
Change-Id: Ib1c32165dbf0e212c7d4b0753bcbb5ffd05eb6fe
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/163022
Commit-Queue: Sam Zackrisson <saza@webrtc.org>
Reviewed-by: Per Åhgren <peah@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30144}

modules/audio_processing/audio_processing_impl.cc

@@ -746,8 +746,7 @@ void AudioProcessingImpl::ApplyAgc1Config(
 }
 
 // TODO(webrtc:5298): Remove.
-void AudioProcessingImpl::SetExtraOptions(const webrtc::Config& config) {
+void AudioProcessingImpl::SetExtraOptions(const webrtc::Config& config) {}
-}
 
 int AudioProcessingImpl::proc_sample_rate_hz() const {
   // Used as callback from submodules, hence locking is not allowed.
@@ -844,28 +843,16 @@ void AudioProcessingImpl::RuntimeSettingEnqueuer::Enqueue(
     RTC_LOG(LS_ERROR) << "Cannot enqueue a new runtime setting.";
 }
 
-int AudioProcessingImpl::ProcessStream(const float* const* src,
+int AudioProcessingImpl::MaybeInitializeCapture(
     const StreamConfig& input_config,
-                                       const StreamConfig& output_config,
+    const StreamConfig& output_config) {
-                                       float* const* dest) {
-  TRACE_EVENT0("webrtc", "AudioProcessing::ProcessStream_StreamConfig");
   ProcessingConfig processing_config;
   bool reinitialization_required = false;
   {
-    // Acquire the capture lock in order to:
+    // Acquire the capture lock in order to access api_format. The lock is
-    // - Safely call the function that retrieves the render side data. This
+    // released immediately, as we may need to acquire the render lock as part
-    //   function accesses APM getters that need the capture lock held when
+    // of the conditional reinitialization.
-    //   being called.
-    // - Access api_format. The lock is released immediately due to the
-    //   conditional reinitialization.
-
     rtc::CritScope cs_capture(&crit_capture_);
-    EmptyQueuedRenderAudio();
-
-    if (!src || !dest) {
-      return kNullPointerError;
-    }
-
     processing_config = formats_.api_format;
     reinitialization_required = UpdateActiveSubmoduleStates();
   }
@@ -881,15 +868,25 @@ int AudioProcessingImpl::ProcessStream(const float* const* src,
   }
 
   if (reinitialization_required) {
-    // Reinitialize.
     rtc::CritScope cs_render(&crit_render_);
     rtc::CritScope cs_capture(&crit_capture_);
     RETURN_ON_ERR(InitializeLocked(processing_config));
   }
+  return kNoError;
+}
+
+int AudioProcessingImpl::ProcessStream(const float* const* src,
+                                       const StreamConfig& input_config,
+                                       const StreamConfig& output_config,
+                                       float* const* dest) {
+  TRACE_EVENT0("webrtc", "AudioProcessing::ProcessStream_StreamConfig");
+  if (!src || !dest) {
+    return kNullPointerError;
+  }
+
+  RETURN_ON_ERR(MaybeInitializeCapture(input_config, output_config));
 
   rtc::CritScope cs_capture(&crit_capture_);
-  RTC_DCHECK_EQ(processing_config.input_stream().num_frames(),
-                formats_.api_format.input_stream().num_frames());
 
   if (aec_dump_) {
     RecordUnprocessedCaptureStream(src);
@@ -1114,64 +1111,18 @@ void AudioProcessingImpl::EmptyQueuedRenderAudio() {
 
 int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
   TRACE_EVENT0("webrtc", "AudioProcessing::ProcessStream_AudioFrame");
-
-  ProcessingConfig processing_config;
-  bool reinitialization_required = false;
-  {
-    // 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.
-    // - Access api_format. The lock is released immediately due to the
-    //   conditional reinitialization.
-    rtc::CritScope cs_capture(&crit_capture_);
-    EmptyQueuedRenderAudio();
-
   if (!frame) {
     return kNullPointerError;
   }
-    // Must be a native rate.
-    if (frame->sample_rate_hz_ != kSampleRate8kHz &&
-        frame->sample_rate_hz_ != kSampleRate16kHz &&
-        frame->sample_rate_hz_ != kSampleRate32kHz &&
-        frame->sample_rate_hz_ != kSampleRate48kHz) {
-      return kBadSampleRateError;
-    }
 
-    // TODO(ajm): The input and output rates and channels are currently
+  StreamConfig input_config(frame->sample_rate_hz_, frame->num_channels_,
-    // constrained to be identical in the int16 interface.
+                            /*has_keyboard=*/false);
-    processing_config = formats_.api_format;
+  StreamConfig output_config(frame->sample_rate_hz_, frame->num_channels_,
-
+                             /*has_keyboard=*/false);
-    reinitialization_required = UpdateActiveSubmoduleStates();
+  RTC_DCHECK_EQ(frame->samples_per_channel(), input_config.num_frames());
-  }
+  RETURN_ON_ERR(MaybeInitializeCapture(input_config, output_config));
-
-  reinitialization_required =
-      reinitialization_required ||
-      processing_config.input_stream().sample_rate_hz() !=
-          frame->sample_rate_hz_ ||
-      processing_config.input_stream().num_channels() != frame->num_channels_ ||
-      processing_config.output_stream().sample_rate_hz() !=
-          frame->sample_rate_hz_ ||
-      processing_config.output_stream().num_channels() != frame->num_channels_;
-
-  if (reinitialization_required) {
-    processing_config.input_stream().set_sample_rate_hz(frame->sample_rate_hz_);
-    processing_config.input_stream().set_num_channels(frame->num_channels_);
-    processing_config.output_stream().set_sample_rate_hz(
-        frame->sample_rate_hz_);
-    processing_config.output_stream().set_num_channels(frame->num_channels_);
-
-    // Reinitialize.
-    rtc::CritScope cs_render(&crit_render_);
-    rtc::CritScope cs_capture(&crit_capture_);
-    RETURN_ON_ERR(InitializeLocked(processing_config));
-  }
 
   rtc::CritScope cs_capture(&crit_capture_);
-  if (frame->samples_per_channel_ !=
-      formats_.api_format.input_stream().num_frames()) {
-    return kBadDataLengthError;
-  }
 
   if (aec_dump_) {
     RecordUnprocessedCaptureStream(*frame);
@@ -1204,6 +1155,7 @@ int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
 }
 
 int AudioProcessingImpl::ProcessCaptureStreamLocked() {
+  EmptyQueuedRenderAudio();
   HandleCaptureRuntimeSettings();
 
   // Ensure that not both the AEC and AECM are active at the same time.

modules/audio_processing/audio_processing_impl.h

@@ -211,14 +211,18 @@ class AudioProcessingImpl : public AudioProcessing {
     bool first_update_ = true;
   };
 
-  // Method for modifying the formats struct that are called from both
+  // Methods for modifying the formats struct that is used by both
-  // the render and capture threads. The check for whether modifications
+  // the render and capture threads. The check for whether modifications are
-  // are needed is done while holding the render lock only, thereby avoiding
+  // needed is done while holding a single lock only, thereby avoiding that the
-  // that the capture thread blocks the render thread.
+  // capture thread blocks the render thread.
-  // The struct is modified in a single-threaded manner by holding both the
+  // Called by render: Holds the render lock when reading the format struct and
-  // render and capture locks.
+  // acquires both locks if reinitialization is required.
   int MaybeInitializeRender(const ProcessingConfig& processing_config)
       RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_render_);
+  // Called by capture: Holds the capture lock when reading the format struct
+  // and acquires both locks if reinitialization is needed.
+  int MaybeInitializeCapture(const StreamConfig& input_config,
+                             const StreamConfig& output_config);
 
   // Method for updating the state keeping track of the active submodules.
   // Returns a bool indicating whether the state has changed.
@@ -473,9 +477,6 @@ class AudioProcessingImpl : public AudioProcessing {
     SwapQueue<AudioProcessingStats> stats_message_queue_;
   } stats_reporter_;
 
-  std::vector<float> aec_render_queue_buffer_ RTC_GUARDED_BY(crit_render_);
-  std::vector<float> aec_capture_queue_buffer_ RTC_GUARDED_BY(crit_capture_);
-
   std::vector<int16_t> aecm_render_queue_buffer_ RTC_GUARDED_BY(crit_render_);
   std::vector<int16_t> aecm_capture_queue_buffer_ RTC_GUARDED_BY(crit_capture_);
 

modules/audio_processing/audio_processing_impl_unittest.cc

@@ -378,6 +378,15 @@ TEST(AudioProcessingImplTest, RenderPreProcessorBeforeEchoDetector) {
   constexpr int16_t kAudioLevel = 1000;
   constexpr int kSampleRateHz = 16000;
   constexpr size_t kNumChannels = 1;
+  // Explicitly initialize APM to ensure no render frames are discarded.
+  const ProcessingConfig processing_config = {{
+      {kSampleRateHz, kNumChannels, /*has_keyboard=*/false},
+      {kSampleRateHz, kNumChannels, /*has_keyboard=*/false},
+      {kSampleRateHz, kNumChannels, /*has_keyboard=*/false},
+      {kSampleRateHz, kNumChannels, /*has_keyboard=*/false},
+  }};
+  apm->Initialize(processing_config);
+
   AudioFrame frame;
   InitializeAudioFrame(kSampleRateHz, kNumChannels, &frame);
 

modules/audio_processing/audio_processing_unittest.cc

@@ -832,13 +832,9 @@ TEST_F(ApmTest, Channels) {
 }
 
 TEST_F(ApmTest, SampleRatesInt) {
-  // Testing invalid sample rates
+  // Testing some valid sample rates.
-  SetContainerFormat(10000, 2, &frame_, &float_cb_);
+  for (int sample_rate : {8000, 12000, 16000, 32000, 44100, 48000, 96000}) {
-  EXPECT_EQ(apm_->kBadSampleRateError, ProcessStreamChooser(kIntFormat));
+    SetContainerFormat(sample_rate, 2, &frame_, &float_cb_);
-  // Testing valid sample rates
-  int fs[] = {8000, 16000, 32000, 48000};
-  for (size_t i = 0; i < arraysize(fs); i++) {
-    SetContainerFormat(fs[i], 2, &frame_, &float_cb_);
     EXPECT_NOERR(ProcessStreamChooser(kIntFormat));
   }
 }