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;