From 6030a129c03fdeb0f47e59ca72f14df856ba7ff9 Mon Sep 17 00:00:00 2001 From: kwiberg Date: Tue, 8 Mar 2016 06:01:31 -0800 Subject: [PATCH] Pass ownership of external encoders to the ACM We want this because otherwise the ACM uses its mutex to protect an encoder that's owned by someone else. That someone else may easily slip up and delete or otherwise touch the encoder before making sure that the ACM has stopped using it, bypassing the lock. BUG=webrtc:5028 Review URL: https://codereview.webrtc.org/1702943002 Cr-Commit-Position: refs/heads/master@{#11909} --- .../acm2/audio_coding_module_impl.cc | 107 ++++++++- .../acm2/audio_coding_module_impl.h | 8 +- .../audio_coding_module_unittest_oldapi.cc | 1 + .../audio_coding/acm2/codec_manager.cc | 2 +- .../modules/audio_coding/acm2/codec_manager.h | 3 + .../acm2/codec_manager_unittest.cc | 24 +- .../modules/audio_coding/acm2/rent_a_codec.cc | 49 ++-- .../modules/audio_coding/acm2/rent_a_codec.h | 15 +- .../acm2/rent_a_codec_unittest.cc | 104 ++++---- .../codecs/cng/audio_encoder_cng.cc | 22 +- .../codecs/cng/audio_encoder_cng.h | 10 +- .../codecs/cng/audio_encoder_cng_unittest.cc | 224 +++++++++++------- .../codecs/red/audio_encoder_copy_red.cc | 15 +- .../codecs/red/audio_encoder_copy_red.h | 12 +- .../red/audio_encoder_copy_red_unittest.cc | 51 ++-- 15 files changed, 403 insertions(+), 244 deletions(-) diff --git a/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc b/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc index 2b62022489..37a0a8cc9a 100644 --- a/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc +++ b/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc @@ -31,6 +31,12 @@ namespace webrtc { namespace acm2 { +struct EncoderFactory { + AudioEncoder* external_speech_encoder = nullptr; + CodecManager codec_manager; + RentACodec rent_a_codec; +}; + namespace { // TODO(turajs): the same functionality is used in NetEq. If both classes @@ -90,6 +96,79 @@ void ConvertEncodedInfoToFragmentationHeader( frag->fragmentationPlType[i] = info.redundant[i].payload_type; } } + +// Wraps a raw AudioEncoder pointer. The idea is that you can put one of these +// in a unique_ptr, to protect the contained raw pointer from being deleted +// when the unique_ptr expires. (This is of course a bad idea in general, but +// backwards compatibility.) +class RawAudioEncoderWrapper final : public AudioEncoder { + public: + RawAudioEncoderWrapper(AudioEncoder* enc) : enc_(enc) {} + size_t MaxEncodedBytes() const override { return enc_->MaxEncodedBytes(); } + int SampleRateHz() const override { return enc_->SampleRateHz(); } + size_t NumChannels() const override { return enc_->NumChannels(); } + int RtpTimestampRateHz() const override { return enc_->RtpTimestampRateHz(); } + size_t Num10MsFramesInNextPacket() const override { + return enc_->Num10MsFramesInNextPacket(); + } + size_t Max10MsFramesInAPacket() const override { + return enc_->Max10MsFramesInAPacket(); + } + int GetTargetBitrate() const override { return enc_->GetTargetBitrate(); } + EncodedInfo EncodeImpl(uint32_t rtp_timestamp, + rtc::ArrayView audio, + rtc::Buffer* encoded) override { + return enc_->Encode(rtp_timestamp, audio, encoded); + } + EncodedInfo EncodeInternal(uint32_t rtp_timestamp, + rtc::ArrayView audio, + size_t max_encoded_bytes, + uint8_t* encoded) override { + return enc_->EncodeInternal(rtp_timestamp, audio, max_encoded_bytes, + encoded); + } + void Reset() override { return enc_->Reset(); } + bool SetFec(bool enable) override { return enc_->SetFec(enable); } + bool SetDtx(bool enable) override { return enc_->SetDtx(enable); } + bool SetApplication(Application application) override { + return enc_->SetApplication(application); + } + void SetMaxPlaybackRate(int frequency_hz) override { + return enc_->SetMaxPlaybackRate(frequency_hz); + } + void SetProjectedPacketLossRate(double fraction) override { + return enc_->SetProjectedPacketLossRate(fraction); + } + void SetTargetBitrate(int target_bps) override { + return enc_->SetTargetBitrate(target_bps); + } + + private: + AudioEncoder* enc_; +}; + +// Return false on error. +bool CreateSpeechEncoderIfNecessary(EncoderFactory* ef) { + auto* sp = ef->codec_manager.GetStackParams(); + if (sp->speech_encoder) { + // Do nothing; we already have a speech encoder. + } else if (ef->codec_manager.GetCodecInst()) { + RTC_DCHECK(!ef->external_speech_encoder); + // We have no speech encoder, but we have a specification for making one. + std::unique_ptr enc = + ef->rent_a_codec.RentEncoder(*ef->codec_manager.GetCodecInst()); + if (!enc) + return false; // Encoder spec was bad. + sp->speech_encoder = std::move(enc); + } else if (ef->external_speech_encoder) { + RTC_DCHECK(!ef->codec_manager.GetCodecInst()); + // We have an external speech encoder. + sp->speech_encoder = std::unique_ptr( + new RawAudioEncoderWrapper(ef->external_speech_encoder)); + } + return true; +} + } // namespace void AudioCodingModuleImpl::ChangeLogger::MaybeLog(int value) { @@ -200,15 +279,13 @@ int AudioCodingModuleImpl::RegisterSendCodec(const CodecInst& send_codec) { if (!encoder_factory_->codec_manager.RegisterEncoder(send_codec)) { return -1; } - auto* sp = encoder_factory_->codec_manager.GetStackParams(); - if (!sp->speech_encoder && encoder_factory_->codec_manager.GetCodecInst()) { - // We have no speech encoder, but we have a specification for making one. - AudioEncoder* enc = encoder_factory_->rent_a_codec.RentEncoder( - *encoder_factory_->codec_manager.GetCodecInst()); - if (!enc) - return -1; - sp->speech_encoder = enc; + if (encoder_factory_->codec_manager.GetCodecInst()) { + encoder_factory_->external_speech_encoder = nullptr; } + if (!CreateSpeechEncoderIfNecessary(encoder_factory_.get())) { + return -1; + } + auto* sp = encoder_factory_->codec_manager.GetStackParams(); if (sp->speech_encoder) encoder_stack_ = encoder_factory_->rent_a_codec.RentEncoderStack(sp); return 0; @@ -217,8 +294,11 @@ int AudioCodingModuleImpl::RegisterSendCodec(const CodecInst& send_codec) { void AudioCodingModuleImpl::RegisterExternalSendCodec( AudioEncoder* external_speech_encoder) { rtc::CritScope lock(&acm_crit_sect_); + encoder_factory_->codec_manager.UnsetCodecInst(); + encoder_factory_->external_speech_encoder = external_speech_encoder; + RTC_CHECK(CreateSpeechEncoderIfNecessary(encoder_factory_.get())); auto* sp = encoder_factory_->codec_manager.GetStackParams(); - sp->speech_encoder = external_speech_encoder; + RTC_CHECK(sp->speech_encoder); encoder_stack_ = encoder_factory_->rent_a_codec.RentEncoderStack(sp); } @@ -229,9 +309,11 @@ rtc::Optional AudioCodingModuleImpl::SendCodec() const { if (ci) { return rtc::Optional(*ci); } - auto* enc = encoder_factory_->codec_manager.GetStackParams()->speech_encoder; + CreateSpeechEncoderIfNecessary(encoder_factory_.get()); + const std::unique_ptr& enc = + encoder_factory_->codec_manager.GetStackParams()->speech_encoder; if (enc) { - return rtc::Optional(CodecManager::ForgeCodecInst(enc)); + return rtc::Optional(CodecManager::ForgeCodecInst(enc.get())); } return rtc::Optional(); } @@ -451,6 +533,7 @@ bool AudioCodingModuleImpl::REDStatus() const { int AudioCodingModuleImpl::SetREDStatus(bool enable_red) { #ifdef WEBRTC_CODEC_RED rtc::CritScope lock(&acm_crit_sect_); + CreateSpeechEncoderIfNecessary(encoder_factory_.get()); if (!encoder_factory_->codec_manager.SetCopyRed(enable_red)) { return -1; } @@ -476,6 +559,7 @@ bool AudioCodingModuleImpl::CodecFEC() const { int AudioCodingModuleImpl::SetCodecFEC(bool enable_codec_fec) { rtc::CritScope lock(&acm_crit_sect_); + CreateSpeechEncoderIfNecessary(encoder_factory_.get()); if (!encoder_factory_->codec_manager.SetCodecFEC(enable_codec_fec)) { return -1; } @@ -507,6 +591,7 @@ int AudioCodingModuleImpl::SetVAD(bool enable_dtx, // Note: |enable_vad| is not used; VAD is enabled based on the DTX setting. RTC_DCHECK_EQ(enable_dtx, enable_vad); rtc::CritScope lock(&acm_crit_sect_); + CreateSpeechEncoderIfNecessary(encoder_factory_.get()); if (!encoder_factory_->codec_manager.SetVAD(enable_dtx, mode)) { return -1; } diff --git a/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h b/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h index cdf49443c0..195ec28dea 100644 --- a/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h +++ b/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h @@ -30,6 +30,8 @@ class AudioCodingImpl; namespace acm2 { +struct EncoderFactory; + class AudioCodingModuleImpl final : public AudioCodingModule { public: friend webrtc::AudioCodingImpl; @@ -249,16 +251,12 @@ class AudioCodingModuleImpl final : public AudioCodingModule { AcmReceiver receiver_; // AcmReceiver has it's own internal lock. ChangeLogger bitrate_logger_ GUARDED_BY(acm_crit_sect_); - struct EncoderFactory { - CodecManager codec_manager; - RentACodec rent_a_codec; - }; std::unique_ptr encoder_factory_ GUARDED_BY(acm_crit_sect_); // Current encoder stack, either obtained from // encoder_factory_->rent_a_codec.RentEncoderStack or provided by a call to // RegisterEncoder. - AudioEncoder* encoder_stack_ GUARDED_BY(acm_crit_sect_); + std::unique_ptr encoder_stack_ GUARDED_BY(acm_crit_sect_); // This is to keep track of CN instances where we can send DTMFs. uint8_t previous_pltype_ GUARDED_BY(acm_crit_sect_); diff --git a/webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc b/webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc index 463ff7b2ed..6e004f9e28 100644 --- a/webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc +++ b/webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc @@ -1632,6 +1632,7 @@ TEST_F(AcmSenderBitExactnessOldApi, External_Pcmu_20ms) { MockAudioEncoder mock_encoder; // Set expectations on the mock encoder and also delegate the calls to the // real encoder. + EXPECT_CALL(mock_encoder, Die()); EXPECT_CALL(mock_encoder, SampleRateHz()) .Times(AtLeast(1)) .WillRepeatedly(Invoke(&encoder, &AudioEncoderPcmU::SampleRateHz)); diff --git a/webrtc/modules/audio_coding/acm2/codec_manager.cc b/webrtc/modules/audio_coding/acm2/codec_manager.cc index ad67377d42..81adf81a83 100644 --- a/webrtc/modules/audio_coding/acm2/codec_manager.cc +++ b/webrtc/modules/audio_coding/acm2/codec_manager.cc @@ -113,7 +113,7 @@ bool CodecManager::RegisterEncoder(const CodecInst& send_codec) { } send_codec_inst_ = rtc::Optional(send_codec); - codec_stack_params_.speech_encoder = nullptr; // Caller must recreate it. + codec_stack_params_.speech_encoder.reset(); // Caller must recreate it. return true; } diff --git a/webrtc/modules/audio_coding/acm2/codec_manager.h b/webrtc/modules/audio_coding/acm2/codec_manager.h index 660a9c0fa0..fbd3a18b18 100644 --- a/webrtc/modules/audio_coding/acm2/codec_manager.h +++ b/webrtc/modules/audio_coding/acm2/codec_manager.h @@ -41,6 +41,9 @@ class CodecManager final { const CodecInst* GetCodecInst() const { return send_codec_inst_ ? &*send_codec_inst_ : nullptr; } + + void UnsetCodecInst() { send_codec_inst_ = rtc::Optional(); } + const RentACodec::StackParameters* GetStackParams() const { return &codec_stack_params_; } diff --git a/webrtc/modules/audio_coding/acm2/codec_manager_unittest.cc b/webrtc/modules/audio_coding/acm2/codec_manager_unittest.cc index 832061433f..e8fe14466b 100644 --- a/webrtc/modules/audio_coding/acm2/codec_manager_unittest.cc +++ b/webrtc/modules/audio_coding/acm2/codec_manager_unittest.cc @@ -37,36 +37,32 @@ std::unique_ptr CreateMockEncoder() { TEST(CodecManagerTest, ExternalEncoderFec) { auto enc0 = CreateMockEncoder(); auto enc1 = CreateMockEncoder(); + auto enc2 = CreateMockEncoder(); { ::testing::InSequence s; EXPECT_CALL(*enc0, SetFec(false)).WillOnce(Return(true)); - EXPECT_CALL(*enc0, Mark("A")); - EXPECT_CALL(*enc0, SetFec(true)).WillOnce(Return(true)); EXPECT_CALL(*enc1, SetFec(true)).WillOnce(Return(true)); - EXPECT_CALL(*enc1, SetFec(false)).WillOnce(Return(true)); - EXPECT_CALL(*enc0, Mark("B")); - EXPECT_CALL(*enc0, SetFec(false)).WillOnce(Return(true)); + EXPECT_CALL(*enc2, SetFec(true)).WillOnce(Return(false)); } CodecManager cm; RentACodec rac; + + // use_codec_fec starts out false. EXPECT_FALSE(cm.GetStackParams()->use_codec_fec); - cm.GetStackParams()->speech_encoder = enc0.get(); + cm.GetStackParams()->speech_encoder = std::move(enc0); EXPECT_TRUE(rac.RentEncoderStack(cm.GetStackParams())); EXPECT_FALSE(cm.GetStackParams()->use_codec_fec); - enc0->Mark("A"); + + // Set it to true. EXPECT_EQ(true, cm.SetCodecFEC(true)); - EXPECT_TRUE(rac.RentEncoderStack(cm.GetStackParams())); EXPECT_TRUE(cm.GetStackParams()->use_codec_fec); - cm.GetStackParams()->speech_encoder = enc1.get(); + cm.GetStackParams()->speech_encoder = std::move(enc1); EXPECT_TRUE(rac.RentEncoderStack(cm.GetStackParams())); EXPECT_TRUE(cm.GetStackParams()->use_codec_fec); - EXPECT_EQ(true, cm.SetCodecFEC(false)); - EXPECT_TRUE(rac.RentEncoderStack(cm.GetStackParams())); - enc0->Mark("B"); - EXPECT_FALSE(cm.GetStackParams()->use_codec_fec); - cm.GetStackParams()->speech_encoder = enc0.get(); + // Switch to a codec that doesn't support it. + cm.GetStackParams()->speech_encoder = std::move(enc2); EXPECT_TRUE(rac.RentEncoderStack(cm.GetStackParams())); EXPECT_FALSE(cm.GetStackParams()->use_codec_fec); } diff --git a/webrtc/modules/audio_coding/acm2/rent_a_codec.cc b/webrtc/modules/audio_coding/acm2/rent_a_codec.cc index 91c5e4d2fc..f37b12c181 100644 --- a/webrtc/modules/audio_coding/acm2/rent_a_codec.cc +++ b/webrtc/modules/audio_coding/acm2/rent_a_codec.cc @@ -179,25 +179,28 @@ std::unique_ptr CreateEncoder(const CodecInst& speech_inst, return std::unique_ptr(); } -std::unique_ptr CreateRedEncoder(AudioEncoder* encoder, - int red_payload_type) { +std::unique_ptr CreateRedEncoder( + std::unique_ptr encoder, + int red_payload_type) { #ifdef WEBRTC_CODEC_RED AudioEncoderCopyRed::Config config; config.payload_type = red_payload_type; - config.speech_encoder = encoder; - return std::unique_ptr(new AudioEncoderCopyRed(config)); + config.speech_encoder = std::move(encoder); + return std::unique_ptr( + new AudioEncoderCopyRed(std::move(config))); #else return std::unique_ptr(); #endif } -std::unique_ptr CreateCngEncoder(AudioEncoder* encoder, - int payload_type, - ACMVADMode vad_mode) { +std::unique_ptr CreateCngEncoder( + std::unique_ptr encoder, + int payload_type, + ACMVADMode vad_mode) { AudioEncoderCng::Config config; config.num_channels = encoder->NumChannels(); config.payload_type = payload_type; - config.speech_encoder = encoder; + config.speech_encoder = std::move(encoder); switch (vad_mode) { case VADNormal: config.vad_mode = Vad::kVadNormal; @@ -214,7 +217,7 @@ std::unique_ptr CreateCngEncoder(AudioEncoder* encoder, default: FATAL(); } - return std::unique_ptr(new AudioEncoderCng(config)); + return std::unique_ptr(new AudioEncoderCng(std::move(config))); } std::unique_ptr CreateIsacDecoder( @@ -234,13 +237,9 @@ std::unique_ptr CreateIsacDecoder( RentACodec::RentACodec() = default; RentACodec::~RentACodec() = default; -AudioEncoder* RentACodec::RentEncoder(const CodecInst& codec_inst) { - std::unique_ptr enc = - CreateEncoder(codec_inst, &isac_bandwidth_info_); - if (!enc) - return nullptr; - speech_encoder_ = std::move(enc); - return speech_encoder_.get(); +std::unique_ptr RentACodec::RentEncoder( + const CodecInst& codec_inst) { + return CreateEncoder(codec_inst, &isac_bandwidth_info_); } RentACodec::StackParameters::StackParameters() { @@ -253,7 +252,8 @@ RentACodec::StackParameters::StackParameters() { RentACodec::StackParameters::~StackParameters() = default; -AudioEncoder* RentACodec::RentEncoderStack(StackParameters* param) { +std::unique_ptr RentACodec::RentEncoderStack( + StackParameters* param) { RTC_DCHECK(param->speech_encoder); if (param->use_codec_fec) { @@ -282,19 +282,14 @@ AudioEncoder* RentACodec::RentEncoderStack(StackParameters* param) { // reset the latter to ensure its buffer is empty. param->speech_encoder->Reset(); } - AudioEncoder* encoder_stack = param->speech_encoder; + std::unique_ptr encoder_stack = + std::move(param->speech_encoder); if (param->use_red) { - red_encoder_ = CreateRedEncoder(encoder_stack, *red_pt); - if (red_encoder_) - encoder_stack = red_encoder_.get(); - } else { - red_encoder_.reset(); + encoder_stack = CreateRedEncoder(std::move(encoder_stack), *red_pt); } if (param->use_cng) { - cng_encoder_ = CreateCngEncoder(encoder_stack, *cng_pt, param->vad_mode); - encoder_stack = cng_encoder_.get(); - } else { - cng_encoder_.reset(); + encoder_stack = + CreateCngEncoder(std::move(encoder_stack), *cng_pt, param->vad_mode); } return encoder_stack; } diff --git a/webrtc/modules/audio_coding/acm2/rent_a_codec.h b/webrtc/modules/audio_coding/acm2/rent_a_codec.h index dd2deceb8d..d6f159a5b0 100644 --- a/webrtc/modules/audio_coding/acm2/rent_a_codec.h +++ b/webrtc/modules/audio_coding/acm2/rent_a_codec.h @@ -197,15 +197,15 @@ class RentACodec { ~RentACodec(); // Creates and returns an audio encoder built to the given specification. - // Returns null in case of error. The returned encoder is live until the next - // successful call to this function, or until the Rent-A-Codec is destroyed. - AudioEncoder* RentEncoder(const CodecInst& codec_inst); + // Returns null in case of error. + std::unique_ptr RentEncoder(const CodecInst& codec_inst); struct StackParameters { StackParameters(); ~StackParameters(); - AudioEncoder* speech_encoder = nullptr; + std::unique_ptr speech_encoder; + bool use_codec_fec = false; bool use_red = false; bool use_cng = false; @@ -218,10 +218,9 @@ class RentACodec { // Creates and returns an audio encoder stack constructed to the given // specification. If the specification isn't compatible with the encoder, it - // will be changed to match (things will be switched off). The returned - // encoder is live until the next successful call to this function, or until - // the Rent-A-Codec is destroyed. - AudioEncoder* RentEncoderStack(StackParameters* param); + // will be changed to match (things will be switched off). The speech encoder + // will be stolen. + std::unique_ptr RentEncoderStack(StackParameters* param); // Creates and returns an iSAC decoder, which will remain live until the // Rent-A-Codec is destroyed. Subsequent calls will simply return the same diff --git a/webrtc/modules/audio_coding/acm2/rent_a_codec_unittest.cc b/webrtc/modules/audio_coding/acm2/rent_a_codec_unittest.cc index 8cc59d6151..cbdd5e9728 100644 --- a/webrtc/modules/audio_coding/acm2/rent_a_codec_unittest.cc +++ b/webrtc/modules/audio_coding/acm2/rent_a_codec_unittest.cc @@ -19,22 +19,29 @@ namespace acm2 { using ::testing::Return; namespace { + const int kDataLengthSamples = 80; const int kPacketSizeSamples = 2 * kDataLengthSamples; const int16_t kZeroData[kDataLengthSamples] = {0}; const CodecInst kDefaultCodecInst = {0, "pcmu", 8000, kPacketSizeSamples, 1, 64000}; const int kCngPt = 13; + +class Marker final { + public: + MOCK_METHOD1(Mark, void(std::string desc)); +}; + } // namespace class RentACodecTestF : public ::testing::Test { protected: void CreateCodec() { - speech_encoder_ = rent_a_codec_.RentEncoder(kDefaultCodecInst); - ASSERT_TRUE(speech_encoder_); + auto speech_encoder = rent_a_codec_.RentEncoder(kDefaultCodecInst); + ASSERT_TRUE(speech_encoder); RentACodec::StackParameters param; param.use_cng = true; - param.speech_encoder = speech_encoder_; + param.speech_encoder = std::move(speech_encoder); encoder_ = rent_a_codec_.RentEncoderStack(¶m); } @@ -58,8 +65,7 @@ class RentACodecTestF : public ::testing::Test { } RentACodec rent_a_codec_; - AudioEncoder* speech_encoder_ = nullptr; - AudioEncoder* encoder_ = nullptr; + std::unique_ptr encoder_; uint32_t timestamp_ = 0; }; @@ -103,87 +109,91 @@ TEST_F(RentACodecTestF, VerifyCngFrames) { TEST(RentACodecTest, ExternalEncoder) { const int kSampleRateHz = 8000; - MockAudioEncoder external_encoder; - EXPECT_CALL(external_encoder, SampleRateHz()) + auto* external_encoder = new MockAudioEncoder; + EXPECT_CALL(*external_encoder, SampleRateHz()) .WillRepeatedly(Return(kSampleRateHz)); - EXPECT_CALL(external_encoder, NumChannels()).WillRepeatedly(Return(1)); - EXPECT_CALL(external_encoder, SetFec(false)).WillRepeatedly(Return(true)); + EXPECT_CALL(*external_encoder, NumChannels()).WillRepeatedly(Return(1)); + EXPECT_CALL(*external_encoder, SetFec(false)).WillRepeatedly(Return(true)); RentACodec rac; RentACodec::StackParameters param; - param.speech_encoder = &external_encoder; - EXPECT_EQ(&external_encoder, rac.RentEncoderStack(¶m)); + param.speech_encoder = std::unique_ptr(external_encoder); + std::unique_ptr encoder_stack = rac.RentEncoderStack(¶m); + EXPECT_EQ(external_encoder, encoder_stack.get()); const int kPacketSizeSamples = kSampleRateHz / 100; int16_t audio[kPacketSizeSamples] = {0}; rtc::Buffer encoded; AudioEncoder::EncodedInfo info; + Marker marker; { ::testing::InSequence s; info.encoded_timestamp = 0; - EXPECT_CALL(external_encoder, - EncodeImpl(0, rtc::ArrayView(audio), - &encoded)) + EXPECT_CALL( + *external_encoder, + EncodeImpl(0, rtc::ArrayView(audio), &encoded)) .WillOnce(Return(info)); - EXPECT_CALL(external_encoder, Mark("A")); - EXPECT_CALL(external_encoder, Mark("B")); - info.encoded_timestamp = 2; - EXPECT_CALL(external_encoder, - EncodeImpl(2, rtc::ArrayView(audio), - &encoded)) - .WillOnce(Return(info)); - EXPECT_CALL(external_encoder, Die()); + EXPECT_CALL(marker, Mark("A")); + EXPECT_CALL(marker, Mark("B")); + EXPECT_CALL(*external_encoder, Die()); + EXPECT_CALL(marker, Mark("C")); } - info = external_encoder.Encode(0, audio, &encoded); + info = encoder_stack->Encode(0, audio, &encoded); EXPECT_EQ(0u, info.encoded_timestamp); - external_encoder.Mark("A"); + marker.Mark("A"); // Change to internal encoder. CodecInst codec_inst = kDefaultCodecInst; codec_inst.pacsize = kPacketSizeSamples; param.speech_encoder = rac.RentEncoder(codec_inst); ASSERT_TRUE(param.speech_encoder); - EXPECT_EQ(param.speech_encoder, rac.RentEncoderStack(¶m)); + AudioEncoder* enc = param.speech_encoder.get(); + std::unique_ptr stack = rac.RentEncoderStack(¶m); + EXPECT_EQ(enc, stack.get()); // Don't expect any more calls to the external encoder. - info = param.speech_encoder->Encode(1, audio, &encoded); - external_encoder.Mark("B"); - - // Change back to external encoder again. - param.speech_encoder = &external_encoder; - EXPECT_EQ(&external_encoder, rac.RentEncoderStack(¶m)); - info = external_encoder.Encode(2, audio, &encoded); - EXPECT_EQ(2u, info.encoded_timestamp); + info = stack->Encode(1, audio, &encoded); + marker.Mark("B"); + encoder_stack.reset(); + marker.Mark("C"); } // Verify that the speech encoder's Reset method is called when CNG or RED // (or both) are switched on, but not when they're switched off. void TestCngAndRedResetSpeechEncoder(bool use_cng, bool use_red) { - MockAudioEncoder speech_encoder; - EXPECT_CALL(speech_encoder, NumChannels()).WillRepeatedly(Return(1)); - EXPECT_CALL(speech_encoder, Max10MsFramesInAPacket()) - .WillRepeatedly(Return(2)); - EXPECT_CALL(speech_encoder, SampleRateHz()).WillRepeatedly(Return(8000)); - EXPECT_CALL(speech_encoder, SetFec(false)).WillRepeatedly(Return(true)); + auto make_enc = [] { + auto speech_encoder = + std::unique_ptr(new MockAudioEncoder); + EXPECT_CALL(*speech_encoder, NumChannels()).WillRepeatedly(Return(1)); + EXPECT_CALL(*speech_encoder, Max10MsFramesInAPacket()) + .WillRepeatedly(Return(2)); + EXPECT_CALL(*speech_encoder, SampleRateHz()).WillRepeatedly(Return(8000)); + EXPECT_CALL(*speech_encoder, SetFec(false)).WillRepeatedly(Return(true)); + return speech_encoder; + }; + auto speech_encoder1 = make_enc(); + auto speech_encoder2 = make_enc(); + Marker marker; { ::testing::InSequence s; - EXPECT_CALL(speech_encoder, Mark("disabled")); - EXPECT_CALL(speech_encoder, Mark("enabled")); + EXPECT_CALL(marker, Mark("disabled")); + EXPECT_CALL(*speech_encoder1, Die()); + EXPECT_CALL(marker, Mark("enabled")); if (use_cng || use_red) - EXPECT_CALL(speech_encoder, Reset()); - EXPECT_CALL(speech_encoder, Die()); + EXPECT_CALL(*speech_encoder2, Reset()); + EXPECT_CALL(*speech_encoder2, Die()); } RentACodec::StackParameters param1, param2; - param1.speech_encoder = &speech_encoder; - param2.speech_encoder = &speech_encoder; + param1.speech_encoder = std::move(speech_encoder1); + param2.speech_encoder = std::move(speech_encoder2); param2.use_cng = use_cng; param2.use_red = use_red; - speech_encoder.Mark("disabled"); + marker.Mark("disabled"); RentACodec rac; rac.RentEncoderStack(¶m1); - speech_encoder.Mark("enabled"); + marker.Mark("enabled"); rac.RentEncoderStack(¶m2); } diff --git a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc index 5daf7be304..3b48131a75 100644 --- a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc +++ b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc @@ -13,6 +13,7 @@ #include #include #include +#include namespace webrtc { @@ -35,6 +36,20 @@ std::unique_ptr CreateCngInst( } // namespace +AudioEncoderCng::Config::Config() = default; + +// TODO(kwiberg): =default this when Visual Studio learns to handle it. +AudioEncoderCng::Config::Config(Config&& c) + : num_channels(c.num_channels), + payload_type(c.payload_type), + speech_encoder(std::move(c.speech_encoder)), + vad_mode(c.vad_mode), + sid_frame_interval_ms(c.sid_frame_interval_ms), + num_cng_coefficients(c.num_cng_coefficients), + vad(c.vad) {} + +AudioEncoderCng::Config::~Config() = default; + bool AudioEncoderCng::Config::IsOk() const { if (num_channels != 1) return false; @@ -51,15 +66,16 @@ bool AudioEncoderCng::Config::IsOk() const { return true; } -AudioEncoderCng::AudioEncoderCng(const Config& config) - : speech_encoder_(config.speech_encoder), +AudioEncoderCng::AudioEncoderCng(Config&& config) + : speech_encoder_( + ([&] { RTC_CHECK(config.IsOk()) << "Invalid configuration."; }(), + std::move(config.speech_encoder))), cng_payload_type_(config.payload_type), num_cng_coefficients_(config.num_cng_coefficients), sid_frame_interval_ms_(config.sid_frame_interval_ms), last_frame_active_(true), vad_(config.vad ? std::unique_ptr(config.vad) : CreateVad(config.vad_mode)) { - RTC_CHECK(config.IsOk()) << "Invalid configuration."; cng_inst_ = CreateCngInst(SampleRateHz(), sid_frame_interval_ms_, num_cng_coefficients_); } diff --git a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h index b581e32082..1384cd511e 100644 --- a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h +++ b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h @@ -31,12 +31,14 @@ class Vad; class AudioEncoderCng final : public AudioEncoder { public: struct Config { + Config(); + Config(Config&&); + ~Config(); bool IsOk() const; size_t num_channels = 1; int payload_type = 13; - // Caller keeps ownership of the AudioEncoder object. - AudioEncoder* speech_encoder = nullptr; + std::unique_ptr speech_encoder; Vad::Aggressiveness vad_mode = Vad::kVadNormal; int sid_frame_interval_ms = 100; int num_cng_coefficients = 8; @@ -47,7 +49,7 @@ class AudioEncoderCng final : public AudioEncoder { Vad* vad = nullptr; }; - explicit AudioEncoderCng(const Config& config); + explicit AudioEncoderCng(Config&& config); ~AudioEncoderCng() override; size_t MaxEncodedBytes() const override; @@ -75,7 +77,7 @@ class AudioEncoderCng final : public AudioEncoder { rtc::Buffer* encoded); size_t SamplesPer10msFrame() const; - AudioEncoder* speech_encoder_; + std::unique_ptr speech_encoder_; const int cng_payload_type_; const int num_cng_coefficients_; const int sid_frame_interval_ms_; diff --git a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc index a4416c955a..8f30d783ae 100644 --- a/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc +++ b/webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc @@ -34,42 +34,50 @@ static const int kCngPayloadType = 18; class AudioEncoderCngTest : public ::testing::Test { protected: AudioEncoderCngTest() - : mock_vad_(new MockVad), + : mock_encoder_owner_(new MockAudioEncoder), + mock_encoder_(mock_encoder_owner_.get()), + mock_vad_(new MockVad), timestamp_(4711), num_audio_samples_10ms_(0), sample_rate_hz_(8000) { memset(audio_, 0, kMaxNumSamples * 2); - config_.speech_encoder = &mock_encoder_; - EXPECT_CALL(mock_encoder_, NumChannels()).WillRepeatedly(Return(1)); - // Let the AudioEncoderCng object use a MockVad instead of its internally - // created Vad object. - config_.vad = mock_vad_; - config_.payload_type = kCngPayloadType; + EXPECT_CALL(*mock_encoder_, NumChannels()).WillRepeatedly(Return(1)); + EXPECT_CALL(*mock_encoder_, Die()).Times(1); } void TearDown() override { EXPECT_CALL(*mock_vad_, Die()).Times(1); cng_.reset(); - // Don't expect the cng_ object to delete the AudioEncoder object. But it - // will be deleted with the test fixture. This is why we explicitly delete - // the cng_ object above, and set expectations on mock_encoder_ afterwards. - EXPECT_CALL(mock_encoder_, Die()).Times(1); } - void CreateCng() { - // The config_ parameters may be changed by the TEST_Fs up until CreateCng() - // is called, thus we cannot use the values until now. + AudioEncoderCng::Config MakeCngConfig() { + AudioEncoderCng::Config config; + config.speech_encoder = std::move(mock_encoder_owner_); + EXPECT_TRUE(config.speech_encoder); + + // Let the AudioEncoderCng object use a MockVad instead of its internally + // created Vad object. + config.vad = mock_vad_; + config.payload_type = kCngPayloadType; + + return config; + } + + void CreateCng(AudioEncoderCng::Config&& config) { num_audio_samples_10ms_ = static_cast(10 * sample_rate_hz_ / 1000); ASSERT_LE(num_audio_samples_10ms_, kMaxNumSamples); - EXPECT_CALL(mock_encoder_, SampleRateHz()) - .WillRepeatedly(Return(sample_rate_hz_)); - // Max10MsFramesInAPacket() is just used to verify that the SID frame period - // is not too small. The return value does not matter that much, as long as - // it is smaller than 10. - EXPECT_CALL(mock_encoder_, Max10MsFramesInAPacket()).WillOnce(Return(1u)); - EXPECT_CALL(mock_encoder_, MaxEncodedBytes()) - .WillRepeatedly(Return(kMockMaxEncodedBytes)); - cng_.reset(new AudioEncoderCng(config_)); + if (config.speech_encoder) { + EXPECT_CALL(*mock_encoder_, SampleRateHz()) + .WillRepeatedly(Return(sample_rate_hz_)); + // Max10MsFramesInAPacket() is just used to verify that the SID frame + // period is not too small. The return value does not matter that much, + // as long as it is smaller than 10. + EXPECT_CALL(*mock_encoder_, Max10MsFramesInAPacket()) + .WillOnce(Return(1u)); + EXPECT_CALL(*mock_encoder_, MaxEncodedBytes()) + .WillRepeatedly(Return(kMockMaxEncodedBytes)); + } + cng_.reset(new AudioEncoderCng(std::move(config))); } void Encode() { @@ -88,27 +96,29 @@ class AudioEncoderCngTest : public ::testing::Test { InSequence s; AudioEncoder::EncodedInfo info; for (size_t j = 0; j < num_calls - 1; ++j) { - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillOnce(Return(info)); } info.encoded_bytes = kMockReturnEncodedBytes; - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) - .WillOnce(Invoke( - MockAudioEncoder::FakeEncoding(kMockReturnEncodedBytes))); + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) + .WillOnce( + Invoke(MockAudioEncoder::FakeEncoding(kMockReturnEncodedBytes))); } // Verifies that the cng_ object waits until it has collected // |blocks_per_frame| blocks of audio, and then dispatches all of them to // the underlying codec (speech or cng). void CheckBlockGrouping(size_t blocks_per_frame, bool active_speech) { - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()) + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) .WillRepeatedly(Return(blocks_per_frame)); - CreateCng(); + auto config = MakeCngConfig(); + const int num_cng_coefficients = config.num_cng_coefficients; + CreateCng(std::move(config)); EXPECT_CALL(*mock_vad_, VoiceActivity(_, _, _)) .WillRepeatedly(Return(active_speech ? Vad::kActive : Vad::kPassive)); // Don't expect any calls to the encoder yet. - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)).Times(0); + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)).Times(0); for (size_t i = 0; i < blocks_per_frame - 1; ++i) { Encode(); EXPECT_EQ(0u, encoded_info_.encoded_bytes); @@ -119,7 +129,7 @@ class AudioEncoderCngTest : public ::testing::Test { if (active_speech) { EXPECT_EQ(kMockReturnEncodedBytes, encoded_info_.encoded_bytes); } else { - EXPECT_EQ(static_cast(config_.num_cng_coefficients + 1), + EXPECT_EQ(static_cast(num_cng_coefficients + 1), encoded_info_.encoded_bytes); } } @@ -132,7 +142,7 @@ class AudioEncoderCngTest : public ::testing::Test { const size_t blocks_per_frame = static_cast(input_frame_size_ms / 10); - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()) + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) .WillRepeatedly(Return(blocks_per_frame)); // Expect nothing to happen before the last block is sent to cng_. @@ -167,7 +177,7 @@ class AudioEncoderCngTest : public ::testing::Test { // Set the speech encoder frame size to 60 ms, to ensure that the VAD will // be called twice. const size_t blocks_per_frame = 6; - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()) + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) .WillRepeatedly(Return(blocks_per_frame)); InSequence s; EXPECT_CALL(*mock_vad_, VoiceActivity(_, _, _)) @@ -184,9 +194,9 @@ class AudioEncoderCngTest : public ::testing::Test { return encoded_info_.payload_type != kCngPayloadType; } - AudioEncoderCng::Config config_; std::unique_ptr cng_; - MockAudioEncoder mock_encoder_; + std::unique_ptr mock_encoder_owner_; + MockAudioEncoder* mock_encoder_; MockVad* mock_vad_; // Ownership is transferred to |cng_|. uint32_t timestamp_; int16_t audio_[kMaxNumSamples]; @@ -194,34 +204,37 @@ class AudioEncoderCngTest : public ::testing::Test { rtc::Buffer encoded_; AudioEncoder::EncodedInfo encoded_info_; int sample_rate_hz_; + + RTC_DISALLOW_COPY_AND_ASSIGN(AudioEncoderCngTest); }; TEST_F(AudioEncoderCngTest, CreateAndDestroy) { - CreateCng(); + CreateCng(MakeCngConfig()); } TEST_F(AudioEncoderCngTest, CheckFrameSizePropagation) { - CreateCng(); - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()).WillOnce(Return(17U)); + CreateCng(MakeCngConfig()); + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) + .WillOnce(Return(17U)); EXPECT_EQ(17U, cng_->Num10MsFramesInNextPacket()); } TEST_F(AudioEncoderCngTest, CheckChangeBitratePropagation) { - CreateCng(); - EXPECT_CALL(mock_encoder_, SetTargetBitrate(4711)); + CreateCng(MakeCngConfig()); + EXPECT_CALL(*mock_encoder_, SetTargetBitrate(4711)); cng_->SetTargetBitrate(4711); } TEST_F(AudioEncoderCngTest, CheckProjectedPacketLossRatePropagation) { - CreateCng(); - EXPECT_CALL(mock_encoder_, SetProjectedPacketLossRate(0.5)); + CreateCng(MakeCngConfig()); + EXPECT_CALL(*mock_encoder_, SetProjectedPacketLossRate(0.5)); cng_->SetProjectedPacketLossRate(0.5); } TEST_F(AudioEncoderCngTest, EncodeCallsVad) { - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()) + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) .WillRepeatedly(Return(1U)); - CreateCng(); + CreateCng(MakeCngConfig()); EXPECT_CALL(*mock_vad_, VoiceActivity(_, _, _)) .WillOnce(Return(Vad::kPassive)); Encode(); @@ -253,13 +266,16 @@ TEST_F(AudioEncoderCngTest, EncodeCollects3BlocksActiveSpeech) { TEST_F(AudioEncoderCngTest, EncodePassive) { const size_t kBlocksPerFrame = 3; - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()) + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) .WillRepeatedly(Return(kBlocksPerFrame)); - CreateCng(); + auto config = MakeCngConfig(); + const auto sid_frame_interval_ms = config.sid_frame_interval_ms; + const auto num_cng_coefficients = config.num_cng_coefficients; + CreateCng(std::move(config)); EXPECT_CALL(*mock_vad_, VoiceActivity(_, _, _)) .WillRepeatedly(Return(Vad::kPassive)); // Expect no calls at all to the speech encoder mock. - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)).Times(0); + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)).Times(0); uint32_t expected_timestamp = timestamp_; for (size_t i = 0; i < 100; ++i) { Encode(); @@ -267,11 +283,11 @@ TEST_F(AudioEncoderCngTest, EncodePassive) { // |kBlocksPerFrame| calls. if ((i + 1) % kBlocksPerFrame == 0) { // Now check if a SID interval has elapsed. - if ((i % (config_.sid_frame_interval_ms / 10)) < kBlocksPerFrame) { + if ((i % (sid_frame_interval_ms / 10)) < kBlocksPerFrame) { // If so, verify that we got a CNG encoding. EXPECT_EQ(kCngPayloadType, encoded_info_.payload_type); EXPECT_FALSE(encoded_info_.speech); - EXPECT_EQ(static_cast(config_.num_cng_coefficients) + 1, + EXPECT_EQ(static_cast(num_cng_coefficients) + 1, encoded_info_.encoded_bytes); EXPECT_EQ(expected_timestamp, encoded_info_.encoded_timestamp); } @@ -286,7 +302,7 @@ TEST_F(AudioEncoderCngTest, EncodePassive) { // Verifies that the correct action is taken for frames with both active and // passive speech. TEST_F(AudioEncoderCngTest, MixedActivePassive) { - CreateCng(); + CreateCng(MakeCngConfig()); // All of the frame is active speech. ExpectEncodeCalls(6); @@ -314,35 +330,35 @@ TEST_F(AudioEncoderCngTest, MixedActivePassive) { // CheckVadInputSize(frame_size, expected_first_block_size, // expected_second_block_size); TEST_F(AudioEncoderCngTest, VadInputSize10Ms) { - CreateCng(); + CreateCng(MakeCngConfig()); CheckVadInputSize(10, 10, 0); } TEST_F(AudioEncoderCngTest, VadInputSize20Ms) { - CreateCng(); + CreateCng(MakeCngConfig()); CheckVadInputSize(20, 20, 0); } TEST_F(AudioEncoderCngTest, VadInputSize30Ms) { - CreateCng(); + CreateCng(MakeCngConfig()); CheckVadInputSize(30, 30, 0); } TEST_F(AudioEncoderCngTest, VadInputSize40Ms) { - CreateCng(); + CreateCng(MakeCngConfig()); CheckVadInputSize(40, 20, 20); } TEST_F(AudioEncoderCngTest, VadInputSize50Ms) { - CreateCng(); + CreateCng(MakeCngConfig()); CheckVadInputSize(50, 30, 20); } TEST_F(AudioEncoderCngTest, VadInputSize60Ms) { - CreateCng(); + CreateCng(MakeCngConfig()); CheckVadInputSize(60, 30, 30); } // Verifies that the correct payload type is set when CNG is encoded. TEST_F(AudioEncoderCngTest, VerifyCngPayloadType) { - CreateCng(); - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)).Times(0); - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()).WillOnce(Return(1U)); + CreateCng(MakeCngConfig()); + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)).Times(0); + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()).WillOnce(Return(1U)); EXPECT_CALL(*mock_vad_, VoiceActivity(_, _, _)) .WillOnce(Return(Vad::kPassive)); encoded_info_.payload_type = 0; @@ -353,8 +369,10 @@ TEST_F(AudioEncoderCngTest, VerifyCngPayloadType) { // Verifies that a SID frame is encoded immediately as the signal changes from // active speech to passive. TEST_F(AudioEncoderCngTest, VerifySidFrameAfterSpeech) { - CreateCng(); - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()) + auto config = MakeCngConfig(); + const auto num_cng_coefficients = config.num_cng_coefficients; + CreateCng(std::move(config)); + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) .WillRepeatedly(Return(1U)); // Start with encoding noise. EXPECT_CALL(*mock_vad_, VoiceActivity(_, _, _)) @@ -362,7 +380,7 @@ TEST_F(AudioEncoderCngTest, VerifySidFrameAfterSpeech) { .WillRepeatedly(Return(Vad::kPassive)); Encode(); EXPECT_EQ(kCngPayloadType, encoded_info_.payload_type); - EXPECT_EQ(static_cast(config_.num_cng_coefficients) + 1, + EXPECT_EQ(static_cast(num_cng_coefficients) + 1, encoded_info_.encoded_bytes); // Encode again, and make sure we got no frame at all (since the SID frame // period is 100 ms by default). @@ -373,8 +391,9 @@ TEST_F(AudioEncoderCngTest, VerifySidFrameAfterSpeech) { encoded_info_.payload_type = 0; EXPECT_CALL(*mock_vad_, VoiceActivity(_, _, _)) .WillOnce(Return(Vad::kActive)); - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)).WillOnce( - Invoke(MockAudioEncoder::FakeEncoding(kMockReturnEncodedBytes))); + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) + .WillOnce( + Invoke(MockAudioEncoder::FakeEncoding(kMockReturnEncodedBytes))); Encode(); EXPECT_EQ(kMockReturnEncodedBytes, encoded_info_.encoded_bytes); @@ -383,14 +402,14 @@ TEST_F(AudioEncoderCngTest, VerifySidFrameAfterSpeech) { .WillOnce(Return(Vad::kPassive)); Encode(); EXPECT_EQ(kCngPayloadType, encoded_info_.payload_type); - EXPECT_EQ(static_cast(config_.num_cng_coefficients) + 1, + EXPECT_EQ(static_cast(num_cng_coefficients) + 1, encoded_info_.encoded_bytes); } // Resetting the CNG should reset both the VAD and the encoder. TEST_F(AudioEncoderCngTest, Reset) { - CreateCng(); - EXPECT_CALL(mock_encoder_, Reset()).Times(1); + CreateCng(MakeCngConfig()); + EXPECT_CALL(*mock_encoder_, Reset()).Times(1); EXPECT_CALL(*mock_vad_, Reset()).Times(1); cng_->Reset(); } @@ -402,11 +421,9 @@ TEST_F(AudioEncoderCngTest, Reset) { class AudioEncoderCngDeathTest : public AudioEncoderCngTest { protected: AudioEncoderCngDeathTest() : AudioEncoderCngTest() { - // Don't provide a Vad mock object, since it will leak when the test dies. - config_.vad = NULL; EXPECT_CALL(*mock_vad_, Die()).Times(1); delete mock_vad_; - mock_vad_ = NULL; + mock_vad_ = nullptr; } // Override AudioEncoderCngTest::TearDown, since that one expects a call to @@ -414,45 +431,70 @@ class AudioEncoderCngDeathTest : public AudioEncoderCngTest { // deleted. void TearDown() override { cng_.reset(); - // Don't expect the cng_ object to delete the AudioEncoder object. But it - // will be deleted with the test fixture. This is why we explicitly delete - // the cng_ object above, and set expectations on mock_encoder_ afterwards. - EXPECT_CALL(mock_encoder_, Die()).Times(1); + } + + AudioEncoderCng::Config MakeCngConfig() { + // Don't provide a Vad mock object, since it would leak when the test dies. + auto config = AudioEncoderCngTest::MakeCngConfig(); + config.vad = nullptr; + return config; + } + + void TryWrongNumCoefficients(int num) { + EXPECT_DEATH( + [&] { + auto config = MakeCngConfig(); + config.num_cng_coefficients = num; + CreateCng(std::move(config)); + }(), + "Invalid configuration"); } }; TEST_F(AudioEncoderCngDeathTest, WrongFrameSize) { - CreateCng(); + CreateCng(MakeCngConfig()); num_audio_samples_10ms_ *= 2; // 20 ms frame. EXPECT_DEATH(Encode(), ""); num_audio_samples_10ms_ = 0; // Zero samples. EXPECT_DEATH(Encode(), ""); } -TEST_F(AudioEncoderCngDeathTest, WrongNumCoefficients) { - config_.num_cng_coefficients = -1; - EXPECT_DEATH(CreateCng(), "Invalid configuration"); - config_.num_cng_coefficients = 0; - EXPECT_DEATH(CreateCng(), "Invalid configuration"); - config_.num_cng_coefficients = 13; - EXPECT_DEATH(CreateCng(), "Invalid configuration"); +TEST_F(AudioEncoderCngDeathTest, WrongNumCoefficientsA) { + TryWrongNumCoefficients(-1); +} + +TEST_F(AudioEncoderCngDeathTest, WrongNumCoefficientsB) { + TryWrongNumCoefficients(0); +} + +TEST_F(AudioEncoderCngDeathTest, WrongNumCoefficientsC) { + TryWrongNumCoefficients(13); } TEST_F(AudioEncoderCngDeathTest, NullSpeechEncoder) { - config_.speech_encoder = NULL; - EXPECT_DEATH(CreateCng(), "Invalid configuration"); + auto config = MakeCngConfig(); + config.speech_encoder = nullptr; + EXPECT_DEATH(CreateCng(std::move(config)), ""); } -TEST_F(AudioEncoderCngDeathTest, Stereo) { - EXPECT_CALL(mock_encoder_, NumChannels()).WillRepeatedly(Return(2)); - EXPECT_DEATH(CreateCng(), "Invalid configuration"); - config_.num_channels = 2; - EXPECT_DEATH(CreateCng(), "Invalid configuration"); +TEST_F(AudioEncoderCngDeathTest, StereoEncoder) { + EXPECT_CALL(*mock_encoder_, NumChannels()).WillRepeatedly(Return(2)); + EXPECT_DEATH(CreateCng(MakeCngConfig()), "Invalid configuration"); +} + +TEST_F(AudioEncoderCngDeathTest, StereoConfig) { + EXPECT_DEATH( + [&] { + auto config = MakeCngConfig(); + config.num_channels = 2; + CreateCng(std::move(config)); + }(), + "Invalid configuration"); } TEST_F(AudioEncoderCngDeathTest, EncoderFrameSizeTooLarge) { - CreateCng(); - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()) + CreateCng(MakeCngConfig()); + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) .WillRepeatedly(Return(7U)); for (int i = 0; i < 6; ++i) Encode(); diff --git a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc index b5a124e43d..4275f54103 100644 --- a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc +++ b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc @@ -12,12 +12,23 @@ #include +#include + #include "webrtc/base/checks.h" namespace webrtc { -AudioEncoderCopyRed::AudioEncoderCopyRed(const Config& config) - : speech_encoder_(config.speech_encoder), +AudioEncoderCopyRed::Config::Config() = default; + +// TODO(kwiberg): =default this when Visual Studio learns to handle it. +AudioEncoderCopyRed::Config::Config(Config&& c) + : payload_type(c.payload_type), + speech_encoder(std::move(c.speech_encoder)) {} + +AudioEncoderCopyRed::Config::~Config() = default; + +AudioEncoderCopyRed::AudioEncoderCopyRed(Config&& config) + : speech_encoder_(std::move(config.speech_encoder)), red_payload_type_(config.payload_type) { RTC_CHECK(speech_encoder_) << "Speech encoder not provided."; } diff --git a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h index 2aa8edcfcd..a67ae486bb 100644 --- a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h +++ b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h @@ -11,6 +11,7 @@ #ifndef WEBRTC_MODULES_AUDIO_CODING_CODECS_RED_AUDIO_ENCODER_COPY_RED_H_ #define WEBRTC_MODULES_AUDIO_CODING_CODECS_RED_AUDIO_ENCODER_COPY_RED_H_ +#include #include #include "webrtc/base/buffer.h" @@ -25,13 +26,14 @@ namespace webrtc { class AudioEncoderCopyRed final : public AudioEncoder { public: struct Config { - public: + Config(); + Config(Config&&); + ~Config(); int payload_type; - AudioEncoder* speech_encoder; + std::unique_ptr speech_encoder; }; - // Caller keeps ownership of the AudioEncoder object. - explicit AudioEncoderCopyRed(const Config& config); + explicit AudioEncoderCopyRed(Config&& config); ~AudioEncoderCopyRed() override; @@ -56,7 +58,7 @@ protected: rtc::Buffer* encoded) override; private: - AudioEncoder* speech_encoder_; + std::unique_ptr speech_encoder_; int red_payload_type_; rtc::Buffer secondary_encoded_; EncodedInfoLeaf secondary_info_; diff --git a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc index fefcbe22f8..c73cb9f209 100644 --- a/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc +++ b/webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc @@ -33,28 +33,26 @@ static const size_t kMaxNumSamples = 48 * 10 * 2; // 10 ms @ 48 kHz stereo. class AudioEncoderCopyRedTest : public ::testing::Test { protected: AudioEncoderCopyRedTest() - : timestamp_(4711), + : mock_encoder_(new MockAudioEncoder), + timestamp_(4711), sample_rate_hz_(16000), num_audio_samples_10ms(sample_rate_hz_ / 100), red_payload_type_(200) { AudioEncoderCopyRed::Config config; config.payload_type = red_payload_type_; - config.speech_encoder = &mock_encoder_; - red_.reset(new AudioEncoderCopyRed(config)); + config.speech_encoder = std::unique_ptr(mock_encoder_); + red_.reset(new AudioEncoderCopyRed(std::move(config))); memset(audio_, 0, sizeof(audio_)); - EXPECT_CALL(mock_encoder_, NumChannels()).WillRepeatedly(Return(1U)); - EXPECT_CALL(mock_encoder_, SampleRateHz()) + EXPECT_CALL(*mock_encoder_, NumChannels()).WillRepeatedly(Return(1U)); + EXPECT_CALL(*mock_encoder_, SampleRateHz()) .WillRepeatedly(Return(sample_rate_hz_)); - EXPECT_CALL(mock_encoder_, MaxEncodedBytes()) + EXPECT_CALL(*mock_encoder_, MaxEncodedBytes()) .WillRepeatedly(Return(kMockMaxEncodedBytes)); } void TearDown() override { + EXPECT_CALL(*mock_encoder_, Die()).Times(1); red_.reset(); - // Don't expect the red_ object to delete the AudioEncoder object. But it - // will be deleted with the test fixture. This is why we explicitly delete - // the red_ object above, and set expectations on mock_encoder_ afterwards. - EXPECT_CALL(mock_encoder_, Die()).Times(1); } void Encode() { @@ -67,7 +65,7 @@ class AudioEncoderCopyRedTest : public ::testing::Test { timestamp_ += num_audio_samples_10ms; } - MockAudioEncoder mock_encoder_; + MockAudioEncoder* mock_encoder_; std::unique_ptr red_; uint32_t timestamp_; int16_t audio_[kMaxNumSamples]; @@ -82,32 +80,33 @@ TEST_F(AudioEncoderCopyRedTest, CreateAndDestroy) { } TEST_F(AudioEncoderCopyRedTest, CheckSampleRatePropagation) { - EXPECT_CALL(mock_encoder_, SampleRateHz()).WillOnce(Return(17)); + EXPECT_CALL(*mock_encoder_, SampleRateHz()).WillOnce(Return(17)); EXPECT_EQ(17, red_->SampleRateHz()); } TEST_F(AudioEncoderCopyRedTest, CheckNumChannelsPropagation) { - EXPECT_CALL(mock_encoder_, NumChannels()).WillOnce(Return(17U)); + EXPECT_CALL(*mock_encoder_, NumChannels()).WillOnce(Return(17U)); EXPECT_EQ(17U, red_->NumChannels()); } TEST_F(AudioEncoderCopyRedTest, CheckFrameSizePropagation) { - EXPECT_CALL(mock_encoder_, Num10MsFramesInNextPacket()).WillOnce(Return(17U)); + EXPECT_CALL(*mock_encoder_, Num10MsFramesInNextPacket()) + .WillOnce(Return(17U)); EXPECT_EQ(17U, red_->Num10MsFramesInNextPacket()); } TEST_F(AudioEncoderCopyRedTest, CheckMaxFrameSizePropagation) { - EXPECT_CALL(mock_encoder_, Max10MsFramesInAPacket()).WillOnce(Return(17U)); + EXPECT_CALL(*mock_encoder_, Max10MsFramesInAPacket()).WillOnce(Return(17U)); EXPECT_EQ(17U, red_->Max10MsFramesInAPacket()); } TEST_F(AudioEncoderCopyRedTest, CheckSetBitratePropagation) { - EXPECT_CALL(mock_encoder_, SetTargetBitrate(4711)); + EXPECT_CALL(*mock_encoder_, SetTargetBitrate(4711)); red_->SetTargetBitrate(4711); } TEST_F(AudioEncoderCopyRedTest, CheckProjectedPacketLossRatePropagation) { - EXPECT_CALL(mock_encoder_, SetProjectedPacketLossRate(0.5)); + EXPECT_CALL(*mock_encoder_, SetProjectedPacketLossRate(0.5)); red_->SetProjectedPacketLossRate(0.5); } @@ -120,7 +119,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckImmediateEncode) { InSequence s; MockFunction check; for (int i = 1; i <= 6; ++i) { - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillRepeatedly(Return(AudioEncoder::EncodedInfo())); EXPECT_CALL(check, Call(i)); Encode(); @@ -134,7 +133,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckNoOutput) { static const size_t kEncodedSize = 17; { InSequence s; - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(kEncodedSize))) .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(0))) .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(kEncodedSize))); @@ -165,7 +164,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckPayloadSizes) { static const int kNumPackets = 10; InSequence s; for (int encode_size = 1; encode_size <= kNumPackets; ++encode_size) { - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(encode_size))); } @@ -191,7 +190,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckTimestamps) { info.encoded_bytes = 17; info.encoded_timestamp = timestamp_; - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(info))); // First call is a special case, since it does not include a secondary @@ -202,7 +201,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckTimestamps) { uint32_t secondary_timestamp = primary_timestamp; primary_timestamp = timestamp_; info.encoded_timestamp = timestamp_; - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(info))); Encode(); @@ -221,7 +220,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckPayloads) { for (uint8_t i = 0; i < kPayloadLenBytes; ++i) { payload[i] = i; } - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillRepeatedly(Invoke(MockAudioEncoder::CopyEncoding(payload))); // First call is a special case, since it does not include a secondary @@ -257,7 +256,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckPayloadType) { AudioEncoder::EncodedInfo info; info.encoded_bytes = 17; info.payload_type = primary_payload_type; - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(info))); // First call is a special case, since it does not include a secondary @@ -269,7 +268,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckPayloadType) { const int secondary_payload_type = red_payload_type_ + 2; info.payload_type = secondary_payload_type; - EXPECT_CALL(mock_encoder_, EncodeImpl(_, _, _)) + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(info))); Encode(); @@ -299,7 +298,7 @@ TEST_F(AudioEncoderCopyRedDeathTest, NullSpeechEncoder) { AudioEncoderCopyRed* red = NULL; AudioEncoderCopyRed::Config config; config.speech_encoder = NULL; - EXPECT_DEATH(red = new AudioEncoderCopyRed(config), + EXPECT_DEATH(red = new AudioEncoderCopyRed(std::move(config)), "Speech encoder not provided."); // The delete operation is needed to avoid leak reports from memcheck. delete red;