From 116ec6da50c7c82b2341b97e2d6d512ade3f6e00 Mon Sep 17 00:00:00 2001 From: aleloi Date: Wed, 12 Oct 2016 06:07:07 -0700 Subject: [PATCH] Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted. BUG=webrtc:6346 NOTRY=True Review-Url: https://codereview.webrtc.org/2408683002 Cr-Commit-Position: refs/heads/master@{#14612} --- webrtc/modules/BUILD.gn | 2 +- webrtc/modules/audio_mixer/audio_mixer.h | 21 +++-- .../modules/audio_mixer/audio_mixer_impl.cc | 82 +++++-------------- webrtc/modules/audio_mixer/audio_mixer_impl.h | 17 ++-- ...ittest.cc => audio_mixer_impl_unittest.cc} | 56 +++++++------ 5 files changed, 76 insertions(+), 102 deletions(-) rename webrtc/modules/audio_mixer/{test/audio_mixer_unittest.cc => audio_mixer_impl_unittest.cc} (86%) diff --git a/webrtc/modules/BUILD.gn b/webrtc/modules/BUILD.gn index a44f006ded..ff0918e6d4 100644 --- a/webrtc/modules/BUILD.gn +++ b/webrtc/modules/BUILD.gn @@ -337,7 +337,7 @@ if (rtc_include_tests) { "audio_conference_mixer/test/audio_conference_mixer_unittest.cc", "audio_device/fine_audio_buffer_unittest.cc", "audio_mixer/audio_frame_manipulator_unittest.cc", - "audio_mixer/test/audio_mixer_unittest.cc", + "audio_mixer/audio_mixer_impl_unittest.cc", "audio_processing/aec/echo_cancellation_unittest.cc", "audio_processing/aec/system_delay_unittest.cc", "audio_processing/agc/agc_manager_direct_unittest.cc", diff --git a/webrtc/modules/audio_mixer/audio_mixer.h b/webrtc/modules/audio_mixer/audio_mixer.h index 0c71469238..7e58a8d6a0 100644 --- a/webrtc/modules/audio_mixer/audio_mixer.h +++ b/webrtc/modules/audio_mixer/audio_mixer.h @@ -13,13 +13,13 @@ #include +#include "webrtc/base/refcount.h" #include "webrtc/modules/include/module_common_types.h" namespace webrtc { -class AudioMixer { +class AudioMixer : public rtc::RefCountInterface { public: - static const int kMaximumAmountOfMixedAudioSources = 3; // A callback class that all mixer participants must inherit from/implement. class Source { public: @@ -47,16 +47,23 @@ class AudioMixer { // mixer. virtual AudioFrameWithInfo GetAudioFrameWithInfo(int sample_rate_hz) = 0; + // A way for a mixer implementation do distinguish participants. + virtual int ssrc() = 0; + protected: virtual ~Source() {} }; - // Factory method. Constructor disabled. - static std::unique_ptr Create(); - virtual ~AudioMixer() {} + // Since the mixer is reference counted, the destructor may be + // called from any thread. + ~AudioMixer() override {} - // Add/remove audio sources as candidates for mixing. - virtual int32_t SetMixabilityStatus(Source* audio_source, bool mixable) = 0; + // Returns true if adding/removing was successful. A source is never + // added twice and removal is never attempted if a source has not + // been successfully added to the mixer. Addition and removal can + // happen on different threads. + virtual bool AddSource(Source* audio_source) = 0; + virtual bool RemoveSource(Source* audio_source) = 0; // Performs mixing by asking registered audio sources for audio. The // mixed result is placed in the provided AudioFrame. Will only be diff --git a/webrtc/modules/audio_mixer/audio_mixer_impl.cc b/webrtc/modules/audio_mixer/audio_mixer_impl.cc index e570e22499..57fae8cc0d 100644 --- a/webrtc/modules/audio_mixer/audio_mixer_impl.cc +++ b/webrtc/modules/audio_mixer/audio_mixer_impl.cc @@ -137,13 +137,8 @@ AudioMixerImpl::SourceStatusList::iterator FindSourceInList( } // namespace -std::unique_ptr AudioMixer::Create() { - return AudioMixerImpl::Create(); -} - AudioMixerImpl::AudioMixerImpl(std::unique_ptr limiter) : audio_source_list_(), - num_mixed_audio_sources_(0), use_limiter_(true), time_stamp_(0), limiter_(std::move(limiter)) { @@ -153,7 +148,7 @@ AudioMixerImpl::AudioMixerImpl(std::unique_ptr limiter) AudioMixerImpl::~AudioMixerImpl() {} -std::unique_ptr AudioMixerImpl::Create() { +rtc::scoped_refptr AudioMixerImpl::Create() { Config config; config.Set(new ExperimentalAgc(false)); std::unique_ptr limiter(AudioProcessing::Create(config)); @@ -186,8 +181,8 @@ std::unique_ptr AudioMixerImpl::Create() { return nullptr; } - return std::unique_ptr( - new AudioMixerImpl(std::move(limiter))); + return rtc::scoped_refptr( + new rtc::RefCountedObject(std::move(limiter))); } void AudioMixerImpl::Mix(int sample_rate, @@ -201,11 +196,9 @@ void AudioMixerImpl::Mix(int sample_rate, } AudioFrameList mix_list; - size_t num_mixed_audio_sources; { rtc::CritScope lock(&crit_); - mix_list = GetNonAnonymousAudio(); - num_mixed_audio_sources = num_mixed_audio_sources_; + mix_list = GetAudioFromSources(); } for (const auto& frame : mix_list) { @@ -218,7 +211,7 @@ void AudioMixerImpl::Mix(int sample_rate, time_stamp_ += static_cast(sample_size_); - use_limiter_ = num_mixed_audio_sources > 1; + 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_); @@ -246,39 +239,26 @@ int AudioMixerImpl::OutputFrequency() const { return output_frequency_; } -int32_t AudioMixerImpl::SetMixabilityStatus(Source* audio_source, - bool mixable) { - { - rtc::CritScope lock(&crit_); - const bool is_mixed = FindSourceInList(audio_source, &audio_source_list_) != - audio_source_list_.end(); - // API must be called with a new state. - if (!(mixable ^ is_mixed)) { - return -1; - } - bool success = false; - if (mixable) { - success = AddAudioSourceToList(audio_source, &audio_source_list_); - } else { - success = RemoveAudioSourceFromList(audio_source, &audio_source_list_); - } - if (!success) { - RTC_NOTREACHED(); - return -1; - } - - size_t num_mixed_non_anonymous = audio_source_list_.size(); - if (num_mixed_non_anonymous > kMaximumAmountOfMixedAudioSources) { - num_mixed_non_anonymous = kMaximumAmountOfMixedAudioSources; - } - num_mixed_audio_sources_ = num_mixed_non_anonymous; - } - return 0; +bool AudioMixerImpl::AddSource(Source* audio_source) { + RTC_DCHECK(audio_source); + rtc::CritScope lock(&crit_); + RTC_DCHECK(FindSourceInList(audio_source, &audio_source_list_) == + audio_source_list_.end()) + << "Source already added to mixer"; + audio_source_list_.emplace_back(audio_source, false, 0); + return true; } +bool AudioMixerImpl::RemoveSource(Source* audio_source) { + RTC_DCHECK(audio_source); + rtc::CritScope lock(&crit_); + const auto iter = FindSourceInList(audio_source, &audio_source_list_); + RTC_DCHECK(iter != audio_source_list_.end()) << "Source not present in mixer"; + audio_source_list_.erase(iter); + return true; +} - -AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() { +AudioFrameList AudioMixerImpl::GetAudioFromSources() { RTC_DCHECK_RUN_ON(&thread_checker_); AudioFrameList result; std::vector audio_source_mixing_data_list; @@ -330,24 +310,6 @@ AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() { return result; } -bool AudioMixerImpl::AddAudioSourceToList( - Source* audio_source, - SourceStatusList* audio_source_list) const { - audio_source_list->emplace_back(audio_source, false, 0); - return true; -} - -bool AudioMixerImpl::RemoveAudioSourceFromList( - Source* audio_source, - SourceStatusList* audio_source_list) const { - const auto iter = FindSourceInList(audio_source, audio_source_list); - if (iter != audio_source_list->end()) { - audio_source_list->erase(iter); - return true; - } else { - return false; - } -} bool AudioMixerImpl::LimitMixedAudio(AudioFrame* mixed_audio) const { RTC_DCHECK_RUN_ON(&thread_checker_); diff --git a/webrtc/modules/audio_mixer/audio_mixer_impl.h b/webrtc/modules/audio_mixer/audio_mixer_impl.h index 9048775460..87541ee642 100644 --- a/webrtc/modules/audio_mixer/audio_mixer_impl.h +++ b/webrtc/modules/audio_mixer/audio_mixer_impl.h @@ -14,6 +14,7 @@ #include #include +#include "webrtc/base/scoped_ref_ptr.h" #include "webrtc/base/thread_annotations.h" #include "webrtc/base/thread_checker.h" #include "webrtc/modules/audio_mixer/audio_mixer.h" @@ -41,15 +42,18 @@ class AudioMixerImpl : public AudioMixer { // AudioProcessing only accepts 10 ms frames. static const int kFrameDurationInMs = 10; + static const int kMaximumAmountOfMixedAudioSources = 3; static const int kDefaultFrequency = 48000; - static std::unique_ptr Create(); + static rtc::scoped_refptr Create(); ~AudioMixerImpl() override; // AudioMixer functions - int32_t SetMixabilityStatus(Source* audio_source, bool mixable) override; - void Mix(int sample_rate, + bool AddSource(Source* audio_source) override; + bool RemoveSource(Source* audio_source) override; + + void Mix(int sample_rate_hz, size_t number_of_channels, AudioFrame* audio_frame_for_mixing) override; @@ -58,9 +62,10 @@ class AudioMixerImpl : public AudioMixer { // mixer. bool GetAudioSourceMixabilityStatusForTest(Source* audio_source) const; - private: + protected: explicit AudioMixerImpl(std::unique_ptr limiter); + private: // Set/get mix frequency void SetOutputFrequency(int frequency); int OutputFrequency() const; @@ -68,8 +73,7 @@ class AudioMixerImpl : public AudioMixer { // Compute what audio sources to mix from audio_source_list_. Ramp // in and out. Update mixed status. Mixes up to // kMaximumAmountOfMixedAudioSources audio sources. - AudioFrameList GetNonAnonymousAudio() EXCLUSIVE_LOCKS_REQUIRED(crit_); - + AudioFrameList GetAudioFromSources() EXCLUSIVE_LOCKS_REQUIRED(crit_); // Add/remove the MixerAudioSource to the specified // MixerAudioSource list. @@ -90,7 +94,6 @@ class AudioMixerImpl : public AudioMixer { // List of all audio sources. Note all lists are disjunct SourceStatusList audio_source_list_ GUARDED_BY(crit_); // May be mixed. - size_t num_mixed_audio_sources_ GUARDED_BY(crit_); // Determines if we will use a limiter for clipping protection during // mixing. bool use_limiter_ ACCESS_ON(&thread_checker_); diff --git a/webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc b/webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc similarity index 86% rename from webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc rename to webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc index 2226fe9c18..1e5f1fee95 100644 --- a/webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc +++ b/webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc @@ -10,6 +10,7 @@ #include +#include #include #include @@ -59,6 +60,8 @@ class MockMixerAudioSource : public AudioMixer::Source { MOCK_METHOD1(GetAudioFrameWithInfo, AudioFrameWithInfo(int sample_rate_hz)); + MOCK_METHOD0(ssrc, int()); + AudioFrame* fake_frame() { return &fake_frame_; } AudioFrameInfo fake_info() { return fake_audio_frame_info_; } void set_fake_info(const AudioFrameInfo audio_frame_info) { @@ -87,7 +90,7 @@ void MixAndCompare( RTC_DCHECK(frames.size() == frame_info.size()); RTC_DCHECK(frame_info.size() == expected_status.size()); - const std::unique_ptr mixer(AudioMixerImpl::Create()); + const auto mixer = AudioMixerImpl::Create(); std::vector participants(num_audio_sources); for (int i = 0; i < num_audio_sources; i++) { @@ -96,7 +99,7 @@ void MixAndCompare( } for (int i = 0; i < num_audio_sources; i++) { - EXPECT_EQ(0, mixer->SetMixabilityStatus(&participants[i], true)); + EXPECT_TRUE(mixer->AddSource(&participants[i])); EXPECT_CALL(participants[i], GetAudioFrameWithInfo(kDefaultSampleRateHz)) .Times(Exactly(1)); } @@ -112,9 +115,9 @@ void MixAndCompare( TEST(AudioMixer, LargestEnergyVadActiveMixed) { constexpr int kAudioSources = - AudioMixer::kMaximumAmountOfMixedAudioSources + 3; + AudioMixerImpl::kMaximumAmountOfMixedAudioSources + 3; - const std::unique_ptr mixer(AudioMixerImpl::Create()); + const auto mixer = AudioMixerImpl::Create(); MockMixerAudioSource participants[kAudioSources]; @@ -125,7 +128,7 @@ TEST(AudioMixer, LargestEnergyVadActiveMixed) { // modified by a ramped-in window. participants[i].fake_frame()->data_[80] = i; - EXPECT_EQ(0, mixer->SetMixabilityStatus(&participants[i], true)); + EXPECT_TRUE(mixer->AddSource(&participants[i])); EXPECT_CALL(participants[i], GetAudioFrameWithInfo(_)).Times(Exactly(1)); } @@ -143,7 +146,8 @@ TEST(AudioMixer, LargestEnergyVadActiveMixed) { bool is_mixed = mixer->GetAudioSourceMixabilityStatusForTest(&participants[i]); if (i == kAudioSources - 1 || - i < kAudioSources - 1 - AudioMixer::kMaximumAmountOfMixedAudioSources) { + i < kAudioSources - 1 - + AudioMixerImpl::kMaximumAmountOfMixedAudioSources) { EXPECT_FALSE(is_mixed) << "Mixing status of AudioSource #" << i << " wrong."; } else { @@ -154,7 +158,7 @@ TEST(AudioMixer, LargestEnergyVadActiveMixed) { } TEST(AudioMixer, FrameNotModifiedForSingleParticipant) { - const std::unique_ptr mixer(AudioMixer::Create()); + const auto mixer = AudioMixerImpl::Create(); MockMixerAudioSource participant; @@ -166,7 +170,7 @@ TEST(AudioMixer, FrameNotModifiedForSingleParticipant) { participant.fake_frame()->data_[j] = j; } - EXPECT_EQ(0, mixer->SetMixabilityStatus(&participant, true)); + EXPECT_TRUE(mixer->AddSource(&participant)); EXPECT_CALL(participant, GetAudioFrameWithInfo(_)).Times(Exactly(2)); AudioFrame audio_frame; @@ -182,12 +186,12 @@ TEST(AudioMixer, FrameNotModifiedForSingleParticipant) { } TEST(AudioMixer, ParticipantSampleRate) { - const std::unique_ptr mixer(AudioMixer::Create()); + const auto mixer = AudioMixerImpl::Create(); MockMixerAudioSource participant; ResetFrame(participant.fake_frame()); - EXPECT_EQ(0, mixer->SetMixabilityStatus(&participant, true)); + EXPECT_TRUE(mixer->AddSource(&participant)); for (auto frequency : {8000, 16000, 32000, 48000}) { EXPECT_CALL(participant, GetAudioFrameWithInfo(frequency)) .Times(Exactly(1)); @@ -199,12 +203,12 @@ TEST(AudioMixer, ParticipantSampleRate) { } TEST(AudioMixer, ParticipantNumberOfChannels) { - const std::unique_ptr mixer(AudioMixer::Create()); + const auto mixer = AudioMixerImpl::Create(); MockMixerAudioSource participant; ResetFrame(participant.fake_frame()); - EXPECT_EQ(0, mixer->SetMixabilityStatus(&participant, true)); + EXPECT_TRUE(mixer->AddSource(&participant)); for (size_t number_of_channels : {1, 2}) { EXPECT_CALL(participant, GetAudioFrameWithInfo(kDefaultSampleRateHz)) .Times(Exactly(1)); @@ -217,9 +221,9 @@ TEST(AudioMixer, ParticipantNumberOfChannels) { // another participant with higher energy is added. TEST(AudioMixer, RampedOutSourcesShouldNotBeMarkedMixed) { constexpr int kAudioSources = - AudioMixer::kMaximumAmountOfMixedAudioSources + 1; + AudioMixerImpl::kMaximumAmountOfMixedAudioSources + 1; - const std::unique_ptr mixer(AudioMixerImpl::Create()); + const auto mixer = AudioMixerImpl::Create(); MockMixerAudioSource participants[kAudioSources]; for (int i = 0; i < kAudioSources; i++) { @@ -231,7 +235,7 @@ TEST(AudioMixer, RampedOutSourcesShouldNotBeMarkedMixed) { // Add all participants but the loudest for mixing. for (int i = 0; i < kAudioSources - 1; i++) { - EXPECT_EQ(0, mixer->SetMixabilityStatus(&participants[i], true)); + EXPECT_TRUE(mixer->AddSource(&participants[i])); EXPECT_CALL(participants[i], GetAudioFrameWithInfo(kDefaultSampleRateHz)) .Times(Exactly(1)); } @@ -246,8 +250,7 @@ TEST(AudioMixer, RampedOutSourcesShouldNotBeMarkedMixed) { } // Add new participant with higher energy. - EXPECT_EQ(0, - mixer->SetMixabilityStatus(&participants[kAudioSources - 1], true)); + EXPECT_TRUE(mixer->AddSource(&participants[kAudioSources - 1])); for (int i = 0; i < kAudioSources; i++) { EXPECT_CALL(participants[i], GetAudioFrameWithInfo(kDefaultSampleRateHz)) .Times(Exactly(1)); @@ -273,17 +276,16 @@ 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, &AudioMixer::Create)); + const auto mixer = init_thread->Invoke>( + RTC_FROM_HERE, &AudioMixerImpl::Create); 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_TRUE(participant_thread->Invoke( + RTC_FROM_HERE, + rtc::Bind(&AudioMixer::AddSource, mixer.get(), &participant))); EXPECT_CALL(participant, GetAudioFrameWithInfo(kDefaultSampleRateHz)) .Times(Exactly(1)); @@ -294,7 +296,7 @@ TEST(AudioMixer, ConstructFromOtherThread) { TEST(AudioMixer, MutedShouldMixAfterUnmuted) { constexpr int kAudioSources = - AudioMixer::kMaximumAmountOfMixedAudioSources + 1; + AudioMixerImpl::kMaximumAmountOfMixedAudioSources + 1; std::vector frames(kAudioSources); for (auto& frame : frames) { @@ -312,7 +314,7 @@ TEST(AudioMixer, MutedShouldMixAfterUnmuted) { TEST(AudioMixer, PassiveShouldMixAfterNormal) { constexpr int kAudioSources = - AudioMixer::kMaximumAmountOfMixedAudioSources + 1; + AudioMixerImpl::kMaximumAmountOfMixedAudioSources + 1; std::vector frames(kAudioSources); for (auto& frame : frames) { @@ -330,7 +332,7 @@ TEST(AudioMixer, PassiveShouldMixAfterNormal) { TEST(AudioMixer, ActiveShouldMixBeforeLoud) { constexpr int kAudioSources = - AudioMixer::kMaximumAmountOfMixedAudioSources + 1; + AudioMixerImpl::kMaximumAmountOfMixedAudioSources + 1; std::vector frames(kAudioSources); for (auto& frame : frames) { @@ -350,7 +352,7 @@ TEST(AudioMixer, ActiveShouldMixBeforeLoud) { TEST(AudioMixer, UnmutedShouldMixBeforeLoud) { constexpr int kAudioSources = - AudioMixer::kMaximumAmountOfMixedAudioSources + 1; + AudioMixerImpl::kMaximumAmountOfMixedAudioSources + 1; std::vector frames(kAudioSources); for (auto& frame : frames) {