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}
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 2b62022..37a0a8c 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 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 @@
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<const int16_t> audio,
+ rtc::Buffer* encoded) override {
+ return enc_->Encode(rtp_timestamp, audio, encoded);
+ }
+ EncodedInfo EncodeInternal(uint32_t rtp_timestamp,
+ rtc::ArrayView<const int16_t> 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<AudioEncoder> 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<AudioEncoder>(
+ new RawAudioEncoderWrapper(ef->external_speech_encoder));
+ }
+ return true;
+}
+
} // namespace
void AudioCodingModuleImpl::ChangeLogger::MaybeLog(int value) {
@@ -200,15 +279,13 @@
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 @@
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 @@
if (ci) {
return rtc::Optional<CodecInst>(*ci);
}
- auto* enc = encoder_factory_->codec_manager.GetStackParams()->speech_encoder;
+ CreateSpeechEncoderIfNecessary(encoder_factory_.get());
+ const std::unique_ptr<AudioEncoder>& enc =
+ encoder_factory_->codec_manager.GetStackParams()->speech_encoder;
if (enc) {
- return rtc::Optional<CodecInst>(CodecManager::ForgeCodecInst(enc));
+ return rtc::Optional<CodecInst>(CodecManager::ForgeCodecInst(enc.get()));
}
return rtc::Optional<CodecInst>();
}
@@ -451,6 +533,7 @@
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 @@
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 @@
// 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 cdf4944..195ec28 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 @@
namespace acm2 {
+struct EncoderFactory;
+
class AudioCodingModuleImpl final : public AudioCodingModule {
public:
friend webrtc::AudioCodingImpl;
@@ -249,16 +251,12 @@
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<EncoderFactory> 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<AudioEncoder> 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 463ff7b..6e004f9 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 @@
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 ad67377..81adf81 100644
--- a/webrtc/modules/audio_coding/acm2/codec_manager.cc
+++ b/webrtc/modules/audio_coding/acm2/codec_manager.cc
@@ -113,7 +113,7 @@
}
send_codec_inst_ = rtc::Optional<CodecInst>(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 660a9c0..fbd3a18 100644
--- a/webrtc/modules/audio_coding/acm2/codec_manager.h
+++ b/webrtc/modules/audio_coding/acm2/codec_manager.h
@@ -41,6 +41,9 @@
const CodecInst* GetCodecInst() const {
return send_codec_inst_ ? &*send_codec_inst_ : nullptr;
}
+
+ void UnsetCodecInst() { send_codec_inst_ = rtc::Optional<CodecInst>(); }
+
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 8320614..e8fe144 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 @@
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 91c5e4d..f37b12c 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 @@
return std::unique_ptr<AudioEncoder>();
}
-std::unique_ptr<AudioEncoder> CreateRedEncoder(AudioEncoder* encoder,
- int red_payload_type) {
+std::unique_ptr<AudioEncoder> CreateRedEncoder(
+ std::unique_ptr<AudioEncoder> 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<AudioEncoder>(new AudioEncoderCopyRed(config));
+ config.speech_encoder = std::move(encoder);
+ return std::unique_ptr<AudioEncoder>(
+ new AudioEncoderCopyRed(std::move(config)));
#else
return std::unique_ptr<AudioEncoder>();
#endif
}
-std::unique_ptr<AudioEncoder> CreateCngEncoder(AudioEncoder* encoder,
- int payload_type,
- ACMVADMode vad_mode) {
+std::unique_ptr<AudioEncoder> CreateCngEncoder(
+ std::unique_ptr<AudioEncoder> 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 @@
default:
FATAL();
}
- return std::unique_ptr<AudioEncoder>(new AudioEncoderCng(config));
+ return std::unique_ptr<AudioEncoder>(new AudioEncoderCng(std::move(config)));
}
std::unique_ptr<AudioDecoder> CreateIsacDecoder(
@@ -234,13 +237,9 @@
RentACodec::RentACodec() = default;
RentACodec::~RentACodec() = default;
-AudioEncoder* RentACodec::RentEncoder(const CodecInst& codec_inst) {
- std::unique_ptr<AudioEncoder> enc =
- CreateEncoder(codec_inst, &isac_bandwidth_info_);
- if (!enc)
- return nullptr;
- speech_encoder_ = std::move(enc);
- return speech_encoder_.get();
+std::unique_ptr<AudioEncoder> RentACodec::RentEncoder(
+ const CodecInst& codec_inst) {
+ return CreateEncoder(codec_inst, &isac_bandwidth_info_);
}
RentACodec::StackParameters::StackParameters() {
@@ -253,7 +252,8 @@
RentACodec::StackParameters::~StackParameters() = default;
-AudioEncoder* RentACodec::RentEncoderStack(StackParameters* param) {
+std::unique_ptr<AudioEncoder> RentACodec::RentEncoderStack(
+ StackParameters* param) {
RTC_DCHECK(param->speech_encoder);
if (param->use_codec_fec) {
@@ -282,19 +282,14 @@
// reset the latter to ensure its buffer is empty.
param->speech_encoder->Reset();
}
- AudioEncoder* encoder_stack = param->speech_encoder;
+ std::unique_ptr<AudioEncoder> 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 dd2dece..d6f159a 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 @@
~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<AudioEncoder> RentEncoder(const CodecInst& codec_inst);
struct StackParameters {
StackParameters();
~StackParameters();
- AudioEncoder* speech_encoder = nullptr;
+ std::unique_ptr<AudioEncoder> speech_encoder;
+
bool use_codec_fec = false;
bool use_red = false;
bool use_cng = false;
@@ -218,10 +218,9 @@
// 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<AudioEncoder> 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 8cc59d6..cbdd5e9 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 @@
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 @@
}
RentACodec rent_a_codec_;
- AudioEncoder* speech_encoder_ = nullptr;
- AudioEncoder* encoder_ = nullptr;
+ std::unique_ptr<AudioEncoder> encoder_;
uint32_t timestamp_ = 0;
};
@@ -103,87 +109,91 @@
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<AudioEncoder>(external_encoder);
+ std::unique_ptr<AudioEncoder> 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<const int16_t>(audio),
- &encoded))
+ EXPECT_CALL(
+ *external_encoder,
+ EncodeImpl(0, rtc::ArrayView<const int16_t>(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<const int16_t>(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<AudioEncoder> 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<MockAudioEncoder>(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 5daf7be..3b48131 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 <algorithm>
#include <memory>
#include <limits>
+#include <utility>
namespace webrtc {
@@ -35,6 +36,20 @@
} // 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 @@
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<Vad>(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 b581e32..1384cd5 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 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<AudioEncoder> speech_encoder;
Vad::Aggressiveness vad_mode = Vad::kVadNormal;
int sid_frame_interval_ms = 100;
int num_cng_coefficients = 8;
@@ -47,7 +49,7 @@
Vad* vad = nullptr;
};
- explicit AudioEncoderCng(const Config& config);
+ explicit AudioEncoderCng(Config&& config);
~AudioEncoderCng() override;
size_t MaxEncodedBytes() const override;
@@ -75,7 +77,7 @@
rtc::Buffer* encoded);
size_t SamplesPer10msFrame() const;
- AudioEncoder* speech_encoder_;
+ std::unique_ptr<AudioEncoder> 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 a4416c9..8f30d78 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 @@
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<size_t>(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 @@
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 @@
if (active_speech) {
EXPECT_EQ(kMockReturnEncodedBytes, encoded_info_.encoded_bytes);
} else {
- EXPECT_EQ(static_cast<size_t>(config_.num_cng_coefficients + 1),
+ EXPECT_EQ(static_cast<size_t>(num_cng_coefficients + 1),
encoded_info_.encoded_bytes);
}
}
@@ -132,7 +142,7 @@
const size_t blocks_per_frame =
static_cast<size_t>(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 @@
// 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 @@
return encoded_info_.payload_type != kCngPayloadType;
}
- AudioEncoderCng::Config config_;
std::unique_ptr<AudioEncoderCng> cng_;
- MockAudioEncoder mock_encoder_;
+ std::unique_ptr<MockAudioEncoder> 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 @@
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, 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 @@
// |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<size_t>(config_.num_cng_coefficients) + 1,
+ EXPECT_EQ(static_cast<size_t>(num_cng_coefficients) + 1,
encoded_info_.encoded_bytes);
EXPECT_EQ(expected_timestamp, encoded_info_.encoded_timestamp);
}
@@ -286,7 +302,7 @@
// 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 @@
// 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 @@
// 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 @@
.WillRepeatedly(Return(Vad::kPassive));
Encode();
EXPECT_EQ(kCngPayloadType, encoded_info_.payload_type);
- EXPECT_EQ(static_cast<size_t>(config_.num_cng_coefficients) + 1,
+ EXPECT_EQ(static_cast<size_t>(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 @@
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 @@
.WillOnce(Return(Vad::kPassive));
Encode();
EXPECT_EQ(kCngPayloadType, encoded_info_.payload_type);
- EXPECT_EQ(static_cast<size_t>(config_.num_cng_coefficients) + 1,
+ EXPECT_EQ(static_cast<size_t>(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 @@
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 @@
// 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 b5a124e..4275f54 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 <string.h>
+#include <utility>
+
#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 2aa8edc..a67ae48 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 <memory>
#include <vector>
#include "webrtc/base/buffer.h"
@@ -25,13 +26,14 @@
class AudioEncoderCopyRed final : public AudioEncoder {
public:
struct Config {
- public:
+ Config();
+ Config(Config&&);
+ ~Config();
int payload_type;
- AudioEncoder* speech_encoder;
+ std::unique_ptr<AudioEncoder> speech_encoder;
};
- // Caller keeps ownership of the AudioEncoder object.
- explicit AudioEncoderCopyRed(const Config& config);
+ explicit AudioEncoderCopyRed(Config&& config);
~AudioEncoderCopyRed() override;
@@ -56,7 +58,7 @@
rtc::Buffer* encoded) override;
private:
- AudioEncoder* speech_encoder_;
+ std::unique_ptr<AudioEncoder> 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 fefcbe2..c73cb9f 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 @@
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<AudioEncoder>(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 @@
timestamp_ += num_audio_samples_10ms;
}
- MockAudioEncoder mock_encoder_;
+ MockAudioEncoder* mock_encoder_;
std::unique_ptr<AudioEncoderCopyRed> red_;
uint32_t timestamp_;
int16_t audio_[kMaxNumSamples];
@@ -82,32 +80,33 @@
}
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 @@
InSequence s;
MockFunction<void(int check_point_id)> 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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;