From 311525e71559227e0cc762236450cd0cbdbde636 Mon Sep 17 00:00:00 2001 From: aleloi Date: Wed, 7 Sep 2016 06:13:12 -0700 Subject: [PATCH] Several lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added. NOTRY=True Review-Url: https://codereview.webrtc.org/2286343002 Cr-Commit-Position: refs/heads/master@{#14106} --- .../modules/audio_mixer/audio_mixer_impl.cc | 157 +++++++----------- webrtc/modules/audio_mixer/audio_mixer_impl.h | 36 ++-- .../audio_mixer/test/audio_mixer_unittest.cc | 95 ++++++++++- 3 files changed, 175 insertions(+), 113 deletions(-) diff --git a/webrtc/modules/audio_mixer/audio_mixer_impl.cc b/webrtc/modules/audio_mixer/audio_mixer_impl.cc index 5a8d1eb764..93b78bd4c3 100644 --- a/webrtc/modules/audio_mixer/audio_mixer_impl.cc +++ b/webrtc/modules/audio_mixer/audio_mixer_impl.cc @@ -12,7 +12,9 @@ #include #include +#include +#include "webrtc/base/thread_annotations.h" #include "webrtc/modules/audio_mixer/audio_frame_manipulator.h" #include "webrtc/modules/audio_mixer/audio_mixer_defines.h" #include "webrtc/modules/audio_processing/include/audio_processing.h" @@ -120,105 +122,80 @@ void NewMixHistory::ResetMixedStatus() { } std::unique_ptr AudioMixer::Create(int id) { - AudioMixerImpl* mixer = new AudioMixerImpl(id); - if (!mixer->Init()) { - delete mixer; - return NULL; - } - return std::unique_ptr(mixer); + return AudioMixerImpl::Create(id); } -AudioMixerImpl::AudioMixerImpl(int id) +AudioMixerImpl::AudioMixerImpl(int id, std::unique_ptr limiter) : id_(id), - output_frequency_(kDefaultFrequency), - sample_size_(0), audio_source_list_(), additional_audio_source_list_(), num_mixed_audio_sources_(0), use_limiter_(true), - time_stamp_(0) { + time_stamp_(0), + limiter_(std::move(limiter)) { + SetOutputFrequency(kDefaultFrequency); thread_checker_.DetachFromThread(); } AudioMixerImpl::~AudioMixerImpl() {} -bool AudioMixerImpl::Init() { - crit_.reset(CriticalSectionWrapper::CreateCriticalSection()); - if (crit_.get() == NULL) - return false; - - cb_crit_.reset(CriticalSectionWrapper::CreateCriticalSection()); - if (cb_crit_.get() == NULL) - return false; - +std::unique_ptr AudioMixerImpl::Create(int id) { Config config; config.Set(new ExperimentalAgc(false)); - limiter_.reset(AudioProcessing::Create(config)); - if (!limiter_.get()) - return false; + std::unique_ptr limiter(AudioProcessing::Create(config)); + if (!limiter.get()) + return nullptr; - if (SetOutputFrequency(kDefaultFrequency) == -1) - return false; - - if (limiter_->gain_control()->set_mode(GainControl::kFixedDigital) != - limiter_->kNoError) - return false; + if (limiter->gain_control()->set_mode(GainControl::kFixedDigital) != + limiter->kNoError) + return nullptr; // We smoothly limit the mixed frame to -7 dbFS. -6 would correspond to the // divide-by-2 but -7 is used instead to give a bit of headroom since the // AGC is not a hard limiter. - if (limiter_->gain_control()->set_target_level_dbfs(7) != limiter_->kNoError) - return false; + if (limiter->gain_control()->set_target_level_dbfs(7) != limiter->kNoError) + return nullptr; - if (limiter_->gain_control()->set_compression_gain_db(0) != - limiter_->kNoError) - return false; + if (limiter->gain_control()->set_compression_gain_db(0) != limiter->kNoError) + return nullptr; - if (limiter_->gain_control()->enable_limiter(true) != limiter_->kNoError) - return false; + if (limiter->gain_control()->enable_limiter(true) != limiter->kNoError) + return nullptr; - if (limiter_->gain_control()->Enable(true) != limiter_->kNoError) - return false; + if (limiter->gain_control()->Enable(true) != limiter->kNoError) + return nullptr; - return true; + return std::unique_ptr( + new AudioMixerImpl(id, std::move(limiter))); } void AudioMixerImpl::Mix(int sample_rate, size_t number_of_channels, AudioFrame* audio_frame_for_mixing) { RTC_DCHECK(number_of_channels == 1 || number_of_channels == 2); - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK_RUN_ON(&thread_checker_); + std::map mixedAudioSourcesMap; + + if (sample_rate != kNbInHz && sample_rate != kWbInHz && + sample_rate != kSwbInHz && sample_rate != kFbInHz) { + WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, id_, + "Invalid frequency: %d", sample_rate); + RTC_NOTREACHED(); + return; + } + + if (OutputFrequency() != sample_rate) { + SetOutputFrequency(static_cast(sample_rate)); + } + AudioFrameList mixList; AudioFrameList additionalFramesList; - std::map mixedAudioSourcesMap; + int num_mixed_audio_sources; { - CriticalSectionScoped cs(cb_crit_.get()); - Frequency mixing_frequency; - - switch (sample_rate) { - case 8000: - mixing_frequency = kNbInHz; - break; - case 16000: - mixing_frequency = kWbInHz; - break; - case 32000: - mixing_frequency = kSwbInHz; - break; - case 48000: - mixing_frequency = kFbInHz; - break; - default: - RTC_NOTREACHED(); - return; - } - - if (OutputFrequency() != mixing_frequency) { - SetOutputFrequency(mixing_frequency); - } - + rtc::CritScope lock(&crit_); mixList = UpdateToMix(kMaximumAmountOfMixedAudioSources); GetAdditionalAudio(&additionalFramesList); + num_mixed_audio_sources = static_cast(num_mixed_audio_sources_); } for (FrameAndMuteInfo& frame_and_mute : mixList) { @@ -234,24 +211,19 @@ void AudioMixerImpl::Mix(int sample_rate, time_stamp_ += static_cast(sample_size_); - use_limiter_ = num_mixed_audio_sources_ > 1; + use_limiter_ = num_mixed_audio_sources > 1; // We only use the limiter if it supports the output sample rate and // we're actually mixing multiple streams. MixFromList(audio_frame_for_mixing, mixList, id_, use_limiter_); - - { - CriticalSectionScoped cs(crit_.get()); - MixAnonomouslyFromList(audio_frame_for_mixing, additionalFramesList); - - 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_; - audio_frame_for_mixing->Mute(); - } else { - // Only call the limiter if we have something to mix. - LimitMixedAudio(audio_frame_for_mixing); - } + MixAnonomouslyFromList(audio_frame_for_mixing, additionalFramesList); + 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_; + audio_frame_for_mixing->Mute(); + } else { + // Only call the limiter if we have something to mix. + LimitMixedAudio(audio_frame_for_mixing); } // Pass the final result to the level indicator. @@ -261,8 +233,7 @@ void AudioMixerImpl::Mix(int sample_rate, } int32_t AudioMixerImpl::SetOutputFrequency(const Frequency& frequency) { - CriticalSectionScoped cs(crit_.get()); - + RTC_DCHECK_RUN_ON(&thread_checker_); output_frequency_ = frequency; sample_size_ = static_cast((output_frequency_ * kFrameDurationInMs) / 1000); @@ -271,7 +242,7 @@ int32_t AudioMixerImpl::SetOutputFrequency(const Frequency& frequency) { } AudioMixer::Frequency AudioMixerImpl::OutputFrequency() const { - CriticalSectionScoped cs(crit_.get()); + RTC_DCHECK_RUN_ON(&thread_checker_); return output_frequency_; } @@ -282,9 +253,8 @@ int32_t AudioMixerImpl::SetMixabilityStatus(MixerAudioSource* audio_source, // audio source is in the _audioSourceList if it is being mixed. SetAnonymousMixabilityStatus(audio_source, false); } - size_t numMixedAudioSources; { - CriticalSectionScoped cs(cb_crit_.get()); + rtc::CritScope lock(&crit_); const bool isMixed = IsAudioSourceInList(*audio_source, audio_source_list_); // API must be called with a new state. if (!(mixable ^ isMixed)) { @@ -309,27 +279,22 @@ int32_t AudioMixerImpl::SetMixabilityStatus(MixerAudioSource* audio_source, if (numMixedNonAnonymous > kMaximumAmountOfMixedAudioSources) { numMixedNonAnonymous = kMaximumAmountOfMixedAudioSources; } - numMixedAudioSources = + num_mixed_audio_sources_ = numMixedNonAnonymous + additional_audio_source_list_.size(); } - // A MixerAudioSource was added or removed. Make sure the scratch - // buffer is updated if necessary. - // Note: The scratch buffer may only be updated in Process(). - CriticalSectionScoped cs(crit_.get()); - num_mixed_audio_sources_ = numMixedAudioSources; return 0; } bool AudioMixerImpl::MixabilityStatus( const MixerAudioSource& audio_source) const { - CriticalSectionScoped cs(cb_crit_.get()); + rtc::CritScope lock(&crit_); return IsAudioSourceInList(audio_source, audio_source_list_); } int32_t AudioMixerImpl::SetAnonymousMixabilityStatus( MixerAudioSource* audio_source, bool anonymous) { - CriticalSectionScoped cs(cb_crit_.get()); + rtc::CritScope lock(&crit_); if (IsAudioSourceInList(*audio_source, additional_audio_source_list_)) { if (anonymous) { return 0; @@ -363,11 +328,12 @@ int32_t AudioMixerImpl::SetAnonymousMixabilityStatus( bool AudioMixerImpl::AnonymousMixabilityStatus( const MixerAudioSource& audio_source) const { - CriticalSectionScoped cs(cb_crit_.get()); + rtc::CritScope lock(&crit_); return IsAudioSourceInList(audio_source, additional_audio_source_list_); } AudioFrameList AudioMixerImpl::UpdateToMix(size_t maxAudioFrameCounter) const { + RTC_DCHECK_RUN_ON(&thread_checker_); AudioFrameList result; std::vector audioSourceMixingDataList; @@ -426,6 +392,7 @@ AudioFrameList AudioMixerImpl::UpdateToMix(size_t maxAudioFrameCounter) const { void AudioMixerImpl::GetAdditionalAudio( AudioFrameList* additionalFramesList) const { + RTC_DCHECK_RUN_ON(&thread_checker_); WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_, "GetAdditionalAudio(additionalFramesList)"); // The GetAudioFrameWithMuted() callback may result in the audio source being @@ -533,6 +500,7 @@ int32_t AudioMixerImpl::MixFromList(AudioFrame* mixedAudio, int32_t AudioMixerImpl::MixAnonomouslyFromList( AudioFrame* mixedAudio, const AudioFrameList& audioFrameList) const { + RTC_DCHECK_RUN_ON(&thread_checker_); WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_, "MixAnonomouslyFromList(mixedAudio, audioFrameList)"); @@ -549,6 +517,7 @@ int32_t AudioMixerImpl::MixAnonomouslyFromList( } bool AudioMixerImpl::LimitMixedAudio(AudioFrame* mixedAudio) const { + RTC_DCHECK_RUN_ON(&thread_checker_); if (!use_limiter_) { return true; } @@ -578,6 +547,7 @@ bool AudioMixerImpl::LimitMixedAudio(AudioFrame* mixedAudio) const { } int AudioMixerImpl::GetOutputAudioLevel() { + RTC_DCHECK_RUN_ON(&thread_checker_); const int level = audio_level_.Level(); WEBRTC_TRACE(kTraceStateInfo, kTraceAudioMixerServer, id_, "GetAudioOutputLevel() => level=%d", level); @@ -585,6 +555,7 @@ int AudioMixerImpl::GetOutputAudioLevel() { } int AudioMixerImpl::GetOutputAudioLevelFullRange() { + RTC_DCHECK_RUN_ON(&thread_checker_); const int level = audio_level_.LevelFullRange(); WEBRTC_TRACE(kTraceStateInfo, kTraceAudioMixerServer, id_, "GetAudioOutputLevelFullRange() => level=%d", level); diff --git a/webrtc/modules/audio_mixer/audio_mixer_impl.h b/webrtc/modules/audio_mixer/audio_mixer_impl.h index 6ddf9ca5e8..66cf982242 100644 --- a/webrtc/modules/audio_mixer/audio_mixer_impl.h +++ b/webrtc/modules/audio_mixer/audio_mixer_impl.h @@ -62,13 +62,10 @@ class AudioMixerImpl : public AudioMixer { // AudioProcessing only accepts 10 ms frames. static const int kFrameDurationInMs = 10; - explicit AudioMixerImpl(int id); + static std::unique_ptr Create(int id); ~AudioMixerImpl() override; - // Must be called after ctor. - bool Init(); - // AudioMixer functions int32_t SetMixabilityStatus(MixerAudioSource* audio_source, bool mixable) override; @@ -82,6 +79,8 @@ class AudioMixerImpl : public AudioMixer { const MixerAudioSource& audio_source) const override; private: + AudioMixerImpl(int id, std::unique_ptr limiter); + // Set/get mix frequency int32_t SetOutputFrequency(const Frequency& frequency); Frequency OutputFrequency() const; @@ -89,7 +88,8 @@ class AudioMixerImpl : public AudioMixer { // Compute what audio sources to mix from audio_source_list_. Ramp in // and out. Update mixed status. maxAudioFrameCounter specifies how // many participants are allowed to be mixed. - AudioFrameList UpdateToMix(size_t maxAudioFrameCounter) const; + AudioFrameList UpdateToMix(size_t maxAudioFrameCounter) const + EXCLUSIVE_LOCKS_REQUIRED(crit_); // Return the lowest mixing frequency that can be used without having to // downsample any audio. @@ -98,7 +98,8 @@ class AudioMixerImpl : public AudioMixer { const MixerAudioSourceList& mixList) const; // Return the AudioFrames that should be mixed anonymously. - void GetAdditionalAudio(AudioFrameList* additionalFramesList) const; + void GetAdditionalAudio(AudioFrameList* additionalFramesList) const + EXCLUSIVE_LOCKS_REQUIRED(crit_); // This function returns true if it finds the MixerAudioSource in the // specified list of MixerAudioSources. @@ -131,36 +132,35 @@ class AudioMixerImpl : public AudioMixer { int GetOutputAudioLevelFullRange() override; - std::unique_ptr crit_; - std::unique_ptr cb_crit_; + rtc::CriticalSection crit_; - int32_t id_; + const int32_t id_; // The current sample frequency and sample size when mixing. - Frequency output_frequency_; - size_t sample_size_; + Frequency output_frequency_ ACCESS_ON(&thread_checker_); + size_t sample_size_ ACCESS_ON(&thread_checker_); // List of all audio sources. Note all lists are disjunct - MixerAudioSourceList audio_source_list_; // May be mixed. + MixerAudioSourceList audio_source_list_ GUARDED_BY(crit_); // May be mixed. // Always mixed, anonomously. - MixerAudioSourceList additional_audio_source_list_; + MixerAudioSourceList additional_audio_source_list_ GUARDED_BY(crit_); - size_t num_mixed_audio_sources_; + size_t num_mixed_audio_sources_ GUARDED_BY(crit_); // Determines if we will use a limiter for clipping protection during // mixing. - bool use_limiter_; + bool use_limiter_ ACCESS_ON(&thread_checker_); - uint32_t time_stamp_; + uint32_t time_stamp_ ACCESS_ON(&thread_checker_); // Ensures that Mix is called from the same thread. rtc::ThreadChecker thread_checker_; // Used for inhibiting saturation in mixing. - std::unique_ptr limiter_; + std::unique_ptr limiter_ ACCESS_ON(&thread_checker_); // Measures audio level for the combined signal. - voe::AudioLevel audio_level_; + voe::AudioLevel audio_level_ ACCESS_ON(&thread_checker_); }; } // namespace webrtc diff --git a/webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc b/webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc index 36bd7c4211..895535fdb2 100644 --- a/webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc +++ b/webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc @@ -8,10 +8,14 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include + #include #include #include "testing/gmock/include/gmock/gmock.h" +#include "webrtc/base/bind.h" +#include "webrtc/base/thread.h" #include "webrtc/modules/audio_mixer/audio_mixer_defines.h" #include "webrtc/modules/audio_mixer/audio_mixer.h" @@ -103,7 +107,7 @@ void MixAndCompare( mixer->Mix(kDefaultSampleRateHz, 1, &frame_for_mixing); for (int i = 0; i < num_audio_sources; i++) { - EXPECT_EQ(participants[i].IsMixed(), expected_status[i]) + EXPECT_EQ(expected_status[i], participants[i].IsMixed()) << "Mixed status of AudioSource #" << i << " wrong."; } } @@ -202,6 +206,63 @@ TEST(AudioMixer, LargestEnergyVadActiveMixed) { } } +TEST(AudioMixer, FrameNotModifiedForSingleParticipant) { + const std::unique_ptr mixer(AudioMixer::Create(kId)); + + MockMixerAudioSource participant; + + ResetFrame(participant.fake_frame()); + const int n_samples = participant.fake_frame()->samples_per_channel_; + + // Modify the frame so that it's not zero. + for (int j = 0; j < n_samples; j++) { + participant.fake_frame()->data_[j] = j; + } + + EXPECT_EQ(0, mixer->SetMixabilityStatus(&participant, true)); + EXPECT_CALL(participant, GetAudioFrameWithMuted(_, _)).Times(Exactly(2)); + + AudioFrame audio_frame; + // Two mix iteration to compare after the ramp-up step. + for (int i = 0; i < 2; i++) { + mixer->Mix(kDefaultSampleRateHz, + 1, // number of channels + &audio_frame); + } + + EXPECT_EQ( + 0, memcmp(participant.fake_frame()->data_, audio_frame.data_, n_samples)); +} + +TEST(AudioMixer, FrameNotModifiedForSingleAnonymousParticipant) { + const std::unique_ptr mixer(AudioMixer::Create(kId)); + + MockMixerAudioSource participant; + + ResetFrame(participant.fake_frame()); + const int n_samples = participant.fake_frame()->samples_per_channel_; + + // Modify the frame so that it's not zero. + for (int j = 0; j < n_samples; j++) { + participant.fake_frame()->data_[j] = j; + } + + EXPECT_EQ(0, mixer->SetMixabilityStatus(&participant, true)); + EXPECT_EQ(0, mixer->SetAnonymousMixabilityStatus(&participant, true)); + EXPECT_CALL(participant, GetAudioFrameWithMuted(_, _)).Times(Exactly(2)); + + AudioFrame audio_frame; + // Two mix iteration to compare after the ramp-up step. + for (int i = 0; i < 2; i++) { + mixer->Mix(kDefaultSampleRateHz, + 1, // number of channels + &audio_frame); + } + + EXPECT_EQ( + 0, memcmp(participant.fake_frame()->data_, audio_frame.data_, n_samples)); +} + TEST(AudioMixer, ParticipantSampleRate) { const std::unique_ptr mixer(AudioMixer::Create(kId)); @@ -332,11 +393,41 @@ TEST(AudioMixer, RampedOutSourcesShouldNotBeMarkedMixed) { // The loudest participants should have been mixed. for (int i = 1; i < kAudioSources; i++) { - EXPECT_EQ(participants[i].IsMixed(), true) + EXPECT_EQ(true, participants[i].IsMixed()) << "Mixed status of AudioSource #" << i << " wrong."; } } +// This test checks that the initialization and participant addition +// can be done on a different thread. +TEST(AudioMixer, ConstructFromOtherThread) { + std::unique_ptr init_thread = rtc::Thread::Create(); + std::unique_ptr participant_thread = rtc::Thread::Create(); + init_thread->Start(); + std::unique_ptr mixer( + init_thread->Invoke>( + RTC_FROM_HERE, std::bind(&AudioMixer::Create, kId))); + MockMixerAudioSource participant; + + ResetFrame(participant.fake_frame()); + + participant_thread->Start(); + EXPECT_EQ(0, participant_thread->Invoke( + RTC_FROM_HERE, rtc::Bind(&AudioMixer::SetMixabilityStatus, + mixer.get(), &participant, true))); + + EXPECT_EQ( + 0, participant_thread->Invoke( + RTC_FROM_HERE, rtc::Bind(&AudioMixer::SetAnonymousMixabilityStatus, + mixer.get(), &participant, true))); + + EXPECT_CALL(participant, GetAudioFrameWithMuted(_, kDefaultSampleRateHz)) + .Times(Exactly(1)); + + // Do one mixer iteration + mixer->Mix(kDefaultSampleRateHz, 1, &frame_for_mixing); +} + TEST(AudioMixer, MutedShouldMixAfterUnmuted) { constexpr int kAudioSources = AudioMixer::kMaximumAmountOfMixedAudioSources + 1;