From 1655e45d85be0ecbf172115031bdbbc88a65c1b2 Mon Sep 17 00:00:00 2001 From: aleloi Date: Mon, 24 Oct 2016 06:56:56 -0700 Subject: [PATCH] Elimiteted race condition in the AudioMixer. The mixer allocates an audio frame for each added data source. This audio frame was deallocated when a source was removed from the mixer. Source removal could happen during the mixing, and the existing locking scheme (and the Clang thread checker) was not sufficient to prevent a data race. After this change, the mixer doesn't release its lock until it is finished with the sources' Audio frames. Since multi-threaded access to the mixer only happens when a source is added or removed, we believe that this change wouldn't have any noticeable performance impact. NOTRY=True BUG=webrtc:6346 Review-Url: https://codereview.webrtc.org/2439283002 Cr-Commit-Position: refs/heads/master@{#14744} --- .../modules/audio_mixer/audio_mixer_impl.cc | 30 +++++++++---------- webrtc/modules/audio_mixer/audio_mixer_impl.h | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/webrtc/modules/audio_mixer/audio_mixer_impl.cc b/webrtc/modules/audio_mixer/audio_mixer_impl.cc index c646fcb56c..a7af8c33d7 100644 --- a/webrtc/modules/audio_mixer/audio_mixer_impl.cc +++ b/webrtc/modules/audio_mixer/audio_mixer_impl.cc @@ -200,23 +200,23 @@ void AudioMixerImpl::Mix(int sample_rate, { rtc::CritScope lock(&crit_); mix_list = GetAudioFromSources(); + + for (const auto& frame : mix_list) { + RemixFrame(number_of_channels, frame); + } + + audio_frame_for_mixing->UpdateFrame( + -1, time_stamp_, NULL, 0, OutputFrequency(), AudioFrame::kNormalSpeech, + AudioFrame::kVadPassive, number_of_channels); + + time_stamp_ += static_cast(sample_size_); + + use_limiter_ = mix_list.size() > 1; + + // We only use the limiter if we're actually mixing multiple streams. + MixFromList(audio_frame_for_mixing, mix_list, use_limiter_); } - for (const auto& frame : mix_list) { - RemixFrame(number_of_channels, frame); - } - - audio_frame_for_mixing->UpdateFrame( - -1, time_stamp_, NULL, 0, OutputFrequency(), AudioFrame::kNormalSpeech, - AudioFrame::kVadPassive, number_of_channels); - - time_stamp_ += static_cast(sample_size_); - - use_limiter_ = mix_list.size() > 1; - - // We only use the limiter if we're actually mixing multiple streams. - MixFromList(audio_frame_for_mixing, mix_list, use_limiter_); - if (audio_frame_for_mixing->samples_per_channel_ == 0) { // Nothing was mixed, set the audio samples to silence. audio_frame_for_mixing->samples_per_channel_ = sample_size_; diff --git a/webrtc/modules/audio_mixer/audio_mixer_impl.h b/webrtc/modules/audio_mixer/audio_mixer_impl.h index 876284fac6..e2f55fce40 100644 --- a/webrtc/modules/audio_mixer/audio_mixer_impl.h +++ b/webrtc/modules/audio_mixer/audio_mixer_impl.h @@ -57,7 +57,7 @@ class AudioMixerImpl : public AudioMixer { void Mix(int sample_rate_hz, size_t number_of_channels, - AudioFrame* audio_frame_for_mixing) override; + AudioFrame* audio_frame_for_mixing) override LOCKS_EXCLUDED(crit_); // Returns true if the source was mixed last round. Returns // false and logs an error if the source was never added to the