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}

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 @@ std::unique_ptr<CNG_enc_inst, CngInstDeleter> 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<Vad>(config.vad)
                       : CreateVad(config.vad_mode)) {
-  RTC_CHECK(config.IsOk()) << "Invalid configuration.";
   cng_inst_ = CreateCngInst(SampleRateHz(), sid_frame_interval_ms_,
                             num_cng_coefficients_);
 }

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<AudioEncoder> 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<AudioEncoder> speech_encoder_;
   const int cng_payload_type_;
   const int num_cng_coefficients_;
   const int sid_frame_interval_ms_;

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<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 @@ 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<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 @@ class AudioEncoderCngTest : public ::testing::Test {
     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 @@ 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<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 @@ 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<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 @@ 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<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 @@ 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<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 @@ 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();

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.";
 }

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 @@ namespace webrtc {
 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 @@ protected:
                          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_;

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<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 @@ class AudioEncoderCopyRedTest : public ::testing::Test {
     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, 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<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 @@ 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;