From 12e319aafe00129010ee4ad7231bebd57340d529 Mon Sep 17 00:00:00 2001 From: Sam Zackrisson Date: Fri, 3 Jan 2020 14:54:20 +0100 Subject: [PATCH] Merge the preambles of the ProcessStream implementations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Per Åhgren Cr-Commit-Position: refs/heads/master@{#30144} --- .../audio_processing/audio_processing_impl.cc | 106 +++++------------- .../audio_processing/audio_processing_impl.h | 19 ++-- .../audio_processing_impl_unittest.cc | 9 ++ .../audio_processing_unittest.cc | 10 +- 4 files changed, 51 insertions(+), 93 deletions(-) diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc index e91a567476..1c88581a03 100644 --- a/modules/audio_processing/audio_processing_impl.cc +++ b/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, - const StreamConfig& input_config, - const StreamConfig& output_config, - float* const* dest) { - TRACE_EVENT0("webrtc", "AudioProcessing::ProcessStream_StreamConfig"); +int AudioProcessingImpl::MaybeInitializeCapture( + const StreamConfig& input_config, + const StreamConfig& output_config) { 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. - + // Acquire the capture lock in order to access api_format. The lock is + // released immediately, as we may need to acquire the render lock as part + // of 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 - // constrained to be identical in the int16 interface. - processing_config = formats_.api_format; - - reinitialization_required = UpdateActiveSubmoduleStates(); + if (!frame) { + return kNullPointerError; } - 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)); - } + StreamConfig input_config(frame->sample_rate_hz_, frame->num_channels_, + /*has_keyboard=*/false); + StreamConfig output_config(frame->sample_rate_hz_, frame->num_channels_, + /*has_keyboard=*/false); + RTC_DCHECK_EQ(frame->samples_per_channel(), input_config.num_frames()); + RETURN_ON_ERR(MaybeInitializeCapture(input_config, output_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. diff --git a/modules/audio_processing/audio_processing_impl.h b/modules/audio_processing/audio_processing_impl.h index 424659419b..ee3fb4d659 100644 --- a/modules/audio_processing/audio_processing_impl.h +++ b/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 - // the render and capture threads. The check for whether modifications - // are needed is done while holding the render lock only, thereby avoiding - // that the capture thread blocks the render thread. - // The struct is modified in a single-threaded manner by holding both the - // render and capture locks. + // Methods for modifying the formats struct that is used by both + // the render and capture threads. The check for whether modifications are + // needed is done while holding a single lock only, thereby avoiding that the + // capture thread blocks the render thread. + // Called by render: Holds the render lock when reading the format struct and + // 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 stats_message_queue_; } stats_reporter_; - std::vector aec_render_queue_buffer_ RTC_GUARDED_BY(crit_render_); - std::vector aec_capture_queue_buffer_ RTC_GUARDED_BY(crit_capture_); - std::vector aecm_render_queue_buffer_ RTC_GUARDED_BY(crit_render_); std::vector aecm_capture_queue_buffer_ RTC_GUARDED_BY(crit_capture_); diff --git a/modules/audio_processing/audio_processing_impl_unittest.cc b/modules/audio_processing/audio_processing_impl_unittest.cc index c7e25a9635..180960a620 100644 --- a/modules/audio_processing/audio_processing_impl_unittest.cc +++ b/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); diff --git a/modules/audio_processing/audio_processing_unittest.cc b/modules/audio_processing/audio_processing_unittest.cc index 9ba4ee7dfa..8f9e53529f 100644 --- a/modules/audio_processing/audio_processing_unittest.cc +++ b/modules/audio_processing/audio_processing_unittest.cc @@ -832,13 +832,9 @@ TEST_F(ApmTest, Channels) { } TEST_F(ApmTest, SampleRatesInt) { - // Testing invalid sample rates - SetContainerFormat(10000, 2, &frame_, &float_cb_); - EXPECT_EQ(apm_->kBadSampleRateError, ProcessStreamChooser(kIntFormat)); - // 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_); + // Testing some valid sample rates. + for (int sample_rate : {8000, 12000, 16000, 32000, 44100, 48000, 96000}) { + SetContainerFormat(sample_rate, 2, &frame_, &float_cb_); EXPECT_NOERR(ProcessStreamChooser(kIntFormat)); } }