From 075bb8d1256de8f7f3469eed9fc9b89bef74f824 Mon Sep 17 00:00:00 2001 From: Karl Wiberg Date: Tue, 12 May 2015 10:10:00 +0200 Subject: [PATCH] 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} --- .../main/acm2/audio_coding_module_impl.cc | 59 ++++++++----------- .../main/acm2/audio_coding_module_impl.h | 6 +- 2 files changed, 30 insertions(+), 35 deletions(-) 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 20e194f8f8..18ff8b326e 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 @@ int32_t AudioCodingModuleImpl::Encode(const InputData& input_data) { 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(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(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 @@ int32_t AudioCodingModuleImpl::Encode(const InputData& input_data) { 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(encoded_info.encoded_bytes); } @@ -316,6 +308,7 @@ int AudioCodingModuleImpl::RegisterTransportCallback( // 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 @@ int AudioCodingModuleImpl::Add10MsDataInternal(const AudioFrame& audio_frame, return -1; } - CriticalSectionScoped lock(acm_crit_sect_); // Do we have a codec registered? if (!HaveValidEncoder("Add10MsData")) { return -1; @@ -1009,6 +1001,7 @@ const CodecInst* AudioCodingImpl::GetSenderCodecInst() { 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 ceed65062d..a657f4f974 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 @@ class AudioCodingModuleImpl : public AudioCodingModule { 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_);