Fix race in AudioCodingModuleImpl::Add10MsData()
AudioCodingModuleImpl::Add10MsData() calls two private methods that
together do all the work: Add10MsDataInternal() and Encode(). They
each took locks internally in order to protect access to, among other
things, codec_manager_.
This turned out to be inadequate. Add10MsDataInternal() calls
codec_manager_.CurrentEncoder()->SampleRateHz() in order to be able to
resample the audio data to what the current encoder wants. When the
resampled data is fed to the encoder deep inside the Encode() call,
that sample rate must still be correct, but occasionally it wasn't,
which triggered a CHECK. (The specific test that failed was the
voe_auto_test subtest
CodecTest.OpusMaxPlaybackRateCannotBeSetForNonOpus, which changes the
current encoder while encoding is in progress.)
This CL solves the problem by covering all of
AudioCodingModuleImpl::Add10MsData() in a single critical section, so
that the sample rate obtained in Add10MsDataInternal() is guaranteed
to still be valid during the Encode() call.
BUG=4644
R=henrik.lundin@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/52459004
Cr-Commit-Position: refs/heads/master@{#9174}
diff --git a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
index 20e194f..18ff8b3 100644
--- a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
+++ b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
@@ -161,37 +161,32 @@
AudioEncoder::EncodedInfo encoded_info;
uint8_t previous_pltype;
- // Keep the scope of the ACM critical section limited.
- {
- CriticalSectionScoped lock(acm_crit_sect_);
- // Check if there is an encoder before.
- if (!HaveValidEncoder("Process")) {
- return -1;
- }
+ // Check if there is an encoder before.
+ if (!HaveValidEncoder("Process"))
+ return -1;
- AudioEncoder* audio_encoder = codec_manager_.CurrentEncoder();
- // Scale the timestamp to the codec's RTP timestamp rate.
- uint32_t rtp_timestamp =
- first_frame_ ? input_data.input_timestamp
- : last_rtp_timestamp_ +
- rtc::CheckedDivExact(
- input_data.input_timestamp - last_timestamp_,
- static_cast<uint32_t>(rtc::CheckedDivExact(
- audio_encoder->SampleRateHz(),
- audio_encoder->RtpTimestampRateHz())));
- last_timestamp_ = input_data.input_timestamp;
- last_rtp_timestamp_ = rtp_timestamp;
- first_frame_ = false;
+ AudioEncoder* audio_encoder = codec_manager_.CurrentEncoder();
+ // Scale the timestamp to the codec's RTP timestamp rate.
+ uint32_t rtp_timestamp =
+ first_frame_ ? input_data.input_timestamp
+ : last_rtp_timestamp_ +
+ rtc::CheckedDivExact(
+ input_data.input_timestamp - last_timestamp_,
+ static_cast<uint32_t>(rtc::CheckedDivExact(
+ audio_encoder->SampleRateHz(),
+ audio_encoder->RtpTimestampRateHz())));
+ last_timestamp_ = input_data.input_timestamp;
+ last_rtp_timestamp_ = rtp_timestamp;
+ first_frame_ = false;
- encoded_info = audio_encoder->Encode(rtp_timestamp, input_data.audio,
- input_data.length_per_channel,
- sizeof(stream), stream);
- if (encoded_info.encoded_bytes == 0 && !encoded_info.send_even_if_empty) {
- // Not enough data.
- return 0;
- }
- previous_pltype = previous_pltype_; // Read it while we have the critsect.
+ encoded_info = audio_encoder->Encode(rtp_timestamp, input_data.audio,
+ input_data.length_per_channel,
+ sizeof(stream), stream);
+ if (encoded_info.encoded_bytes == 0 && !encoded_info.send_even_if_empty) {
+ // Not enough data.
+ return 0;
}
+ previous_pltype = previous_pltype_; // Read it while we have the critsect.
RTPFragmentationHeader my_fragmentation;
ConvertEncodedInfoToFragmentationHeader(encoded_info, &my_fragmentation);
@@ -219,10 +214,7 @@
vad_callback_->InFrameType(frame_type);
}
}
- {
- CriticalSectionScoped lock(acm_crit_sect_);
- previous_pltype_ = encoded_info.payload_type;
- }
+ previous_pltype_ = encoded_info.payload_type;
return static_cast<int32_t>(encoded_info.encoded_bytes);
}
@@ -316,6 +308,7 @@
// Add 10MS of raw (PCM) audio data to the encoder.
int AudioCodingModuleImpl::Add10MsData(const AudioFrame& audio_frame) {
InputData input_data;
+ CriticalSectionScoped lock(acm_crit_sect_);
int r = Add10MsDataInternal(audio_frame, &input_data);
return r < 0 ? r : Encode(input_data);
}
@@ -352,7 +345,6 @@
return -1;
}
- CriticalSectionScoped lock(acm_crit_sect_);
// Do we have a codec registered?
if (!HaveValidEncoder("Add10MsData")) {
return -1;
@@ -1009,6 +1001,7 @@
int AudioCodingImpl::Add10MsAudio(const AudioFrame& audio_frame) {
acm2::AudioCodingModuleImpl::InputData input_data;
+ CriticalSectionScoped lock(acm_old_->acm_crit_sect_);
if (acm_old_->Add10MsDataInternal(audio_frame, &input_data) != 0)
return -1;
return acm_old_->Encode(input_data);
diff --git a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h
index ceed650..a657f4f 100644
--- a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h
+++ b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h
@@ -252,8 +252,10 @@
int16_t buffer[WEBRTC_10MS_PCM_AUDIO];
};
- int Add10MsDataInternal(const AudioFrame& audio_frame, InputData* input_data);
- int Encode(const InputData& input_data);
+ int Add10MsDataInternal(const AudioFrame& audio_frame, InputData* input_data)
+ EXCLUSIVE_LOCKS_REQUIRED(acm_crit_sect_);
+ int Encode(const InputData& input_data)
+ EXCLUSIVE_LOCKS_REQUIRED(acm_crit_sect_);
int InitializeReceiverSafe() EXCLUSIVE_LOCKS_REQUIRED(acm_crit_sect_);
