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}
diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc
index e91a567..1c88581 100644
--- a/modules/audio_processing/audio_processing_impl.cc
+++ b/modules/audio_processing/audio_processing_impl.cc
@@ -746,8 +746,7 @@
}
// 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 @@
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 @@
}
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 @@
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::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 4246594..ee3fb4d 100644
--- a/modules/audio_processing/audio_processing_impl.h
+++ b/modules/audio_processing/audio_processing_impl.h
@@ -211,14 +211,18 @@
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 @@
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_);
diff --git a/modules/audio_processing/audio_processing_impl_unittest.cc b/modules/audio_processing/audio_processing_impl_unittest.cc
index c7e25a9..180960a 100644
--- a/modules/audio_processing/audio_processing_impl_unittest.cc
+++ b/modules/audio_processing/audio_processing_impl_unittest.cc
@@ -378,6 +378,15 @@
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 9ba4ee7..8f9e535 100644
--- a/modules/audio_processing/audio_processing_unittest.cc
+++ b/modules/audio_processing/audio_processing_unittest.cc
@@ -832,13 +832,9 @@
}
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));
}
}