Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG=webrtc:6020, chromium:622647

Review-Url: https://codereview.webrtc.org/2089183002
Cr-Commit-Position: refs/heads/master@{#13273}

webrtc/modules/audio_coding/acm2/codec_manager.cc

@@ -113,7 +113,7 @@ bool CodecManager::RegisterEncoder(const CodecInst& send_codec) {
   }
 
   send_codec_inst_ = rtc::Optional<CodecInst>(send_codec);
-  codec_stack_params_.speech_encoder.reset();  // Caller must recreate it.
+  recreate_encoder_ = true;  // Caller must recreate it.
   return true;
 }
 
@@ -190,5 +190,67 @@ bool CodecManager::SetCodecFEC(bool enable_codec_fec) {
   return true;
 }
 
+bool CodecManager::MakeEncoder(RentACodec* rac, AudioCodingModule* acm) {
+  RTC_DCHECK(rac);
+  RTC_DCHECK(acm);
+
+  if (!recreate_encoder_) {
+    bool error = false;
+    // Try to re-use the speech encoder we've given to the ACM.
+    acm->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) {
+      if (!*encoder) {
+        // There is no existing encoder.
+        recreate_encoder_ = true;
+        return;
+      }
+
+      // Extract the speech encoder from the ACM.
+      std::unique_ptr<AudioEncoder> enc = std::move(*encoder);
+      while (true) {
+        auto sub_enc = enc->ReclaimContainedEncoders();
+        if (sub_enc.empty()) {
+          break;
+        }
+        RTC_CHECK_EQ(1u, sub_enc.size());
+
+        // Replace enc with its sub encoder. We need to put the sub encoder in
+        // a temporary first, since otherwise the old value of enc would be
+        // destroyed before the new value got assigned, which would be bad
+        // since the new value is a part of the old value.
+        auto tmp_enc = std::move(sub_enc[0]);
+        enc = std::move(tmp_enc);
+      }
+
+      // Wrap it in a new encoder stack and put it back.
+      codec_stack_params_.speech_encoder = std::move(enc);
+      *encoder = rac->RentEncoderStack(&codec_stack_params_);
+      if (!*encoder) {
+        error = true;
+      }
+    });
+    if (error) {
+      return false;
+    }
+    if (!recreate_encoder_) {
+      return true;
+    }
+  }
+
+  if (!send_codec_inst_) {
+    // We don't have the information we need to create a new speech encoder.
+    // (This is not an error.)
+    return true;
+  }
+
+  codec_stack_params_.speech_encoder = rac->RentEncoder(*send_codec_inst_);
+  auto stack = rac->RentEncoderStack(&codec_stack_params_);
+  if (!stack) {
+    return false;
+  }
+  acm->SetEncoder(std::move(stack));
+  recreate_encoder_ = false;
+  return true;
+}
+
 }  // namespace acm2
 }  // namespace webrtc

webrtc/modules/audio_coding/acm2/codec_manager.h

@@ -59,31 +59,13 @@ class CodecManager final {
   // Uses the provided Rent-A-Codec to create a new encoder stack, if we have a
   // complete specification; if so, it is then passed to set_encoder. On error,
   // returns false.
-  bool MakeEncoder(RentACodec* rac, AudioCodingModule* acm) {
-    RTC_DCHECK(rac);
-    RTC_DCHECK(acm);
-    if (!codec_stack_params_.speech_encoder && send_codec_inst_) {
-      // We have no speech encoder, but we have a specification for making one.
-      auto enc = rac->RentEncoder(*send_codec_inst_);
-      if (!enc)
-        return false;
-      codec_stack_params_.speech_encoder = std::move(enc);
-    }
-    auto stack = rac->RentEncoderStack(&codec_stack_params_);
-    if (stack) {
-      // Give new encoder stack to the ACM.
-      acm->SetEncoder(std::move(stack));
-    } else {
-      // The specification was good but incomplete, so we have no encoder stack
-      // to give to the ACM.
-    }
-    return true;
-  }
+  bool MakeEncoder(RentACodec* rac, AudioCodingModule* acm);
 
  private:
   rtc::ThreadChecker thread_checker_;
   rtc::Optional<CodecInst> send_codec_inst_;
   RentACodec::StackParameters codec_stack_params_;
+  bool recreate_encoder_ = true;  // Need to recreate encoder?
 
   RTC_DISALLOW_COPY_AND_ASSIGN(CodecManager);
 };

webrtc/modules/audio_coding/codecs/audio_encoder.cc

@@ -60,4 +60,7 @@ void AudioEncoder::SetProjectedPacketLossRate(double fraction) {}
 
 void AudioEncoder::SetTargetBitrate(int target_bps) {}
 
+rtc::ArrayView<std::unique_ptr<AudioEncoder>>
+AudioEncoder::ReclaimContainedEncoders() { return nullptr; }
+
 }  // namespace webrtc

webrtc/modules/audio_coding/codecs/audio_encoder.h

@@ -149,6 +149,15 @@ class AudioEncoder {
   // implementation does the latter).
   virtual void SetTargetBitrate(int target_bps);
 
+  // Causes this encoder to let go of any other encoders it contains, and
+  // returns a pointer to an array where they are stored (which is required to
+  // live as long as this encoder). Unless the returned array is empty, you may
+  // not call any methods on this encoder afterwards, except for the
+  // destructor. The default implementation just returns an empty array.
+  // NOTE: This method is subject to change. Do not call or override it.
+  virtual rtc::ArrayView<std::unique_ptr<AudioEncoder>>
+  ReclaimContainedEncoders();
+
  protected:
   // Subclasses implement this to perform the actual encoding. Called by
   // Encode().

webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc

@@ -187,6 +187,11 @@ void AudioEncoderCng::SetTargetBitrate(int bits_per_second) {
   speech_encoder_->SetTargetBitrate(bits_per_second);
 }
 
+rtc::ArrayView<std::unique_ptr<AudioEncoder>>
+AudioEncoderCng::ReclaimContainedEncoders() {
+  return rtc::ArrayView<std::unique_ptr<AudioEncoder>>(&speech_encoder_, 1);
+}
+
 AudioEncoder::EncodedInfo AudioEncoderCng::EncodePassive(
     size_t frames_to_encode,
     rtc::Buffer* encoded) {

webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h

@@ -63,6 +63,8 @@ class AudioEncoderCng final : public AudioEncoder {
   void SetMaxPlaybackRate(int frequency_hz) override;
   void SetProjectedPacketLossRate(double fraction) override;
   void SetTargetBitrate(int target_bps) override;
+  rtc::ArrayView<std::unique_ptr<AudioEncoder>> ReclaimContainedEncoders()
+      override;
 
  private:
   EncodedInfo EncodePassive(size_t frames_to_encode,

webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc

@@ -123,4 +123,9 @@ void AudioEncoderCopyRed::SetTargetBitrate(int bits_per_second) {
   speech_encoder_->SetTargetBitrate(bits_per_second);
 }
 
+rtc::ArrayView<std::unique_ptr<AudioEncoder>>
+AudioEncoderCopyRed::ReclaimContainedEncoders() {
+  return rtc::ArrayView<std::unique_ptr<AudioEncoder>>(&speech_encoder_, 1);
+}
+
 }  // namespace webrtc

webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h

@@ -51,6 +51,8 @@ class AudioEncoderCopyRed final : public AudioEncoder {
   void SetMaxPlaybackRate(int frequency_hz) override;
   void SetProjectedPacketLossRate(double fraction) override;
   void SetTargetBitrate(int target_bps) override;
+  rtc::ArrayView<std::unique_ptr<AudioEncoder>> ReclaimContainedEncoders()
+      override;
 
  protected:
   EncodedInfo EncodeImpl(uint32_t rtp_timestamp,