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

webrtc/modules/audio_coding/acm2/codec_manager.cc

Index: webrtc/modules/audio_coding/acm2/codec_manager.cc
diff --git a/webrtc/modules/audio_coding/acm2/codec_manager.cc b/webrtc/modules/audio_coding/acm2/codec_manager.cc
index 81adf81a83cf46c9862fce7c371d28edb5085bf0..f028c45f99130eb205d3818ac691b348ca9946e0 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<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_) {

[ossu, 2016/06/23 08:32:07]

The flow of this whole function is pretty difficult to follow.
I realize this is a "temporary" bug fix until, but I'm a bit afraid it will keep
for longer than we expect. Would it be possible to hold the speech encoder
separate from the rest of the stack (through, say, a scoped_refptr in the ACM
and then in whatever encoder stack that uses it) so that we could just reuse it
directly, rather than having to peel back the layers of the encoder stack onion
to get to it?

[kwiberg-webrtc, 2016/06/23 10:31:51]

Hmm. Yes, that's possible. We change the cng and red encoders to hold either a
raw (non-owning) pointer to the speech encoder, or some sort of shared pointer,
and have the ACM (co-)own the speech encoder; we can then easily make an
accessor for pulling it out again. That's arguably less bad than the solution I
have right now.

However, this means nontrivial modifications to the cng and red encoders, and to
this code. That isn't compatible with getting this bug fixed quickly enough.

[hlundin-webrtc, 2016/06/23 10:38:46]

Speed is of importance here.

+    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;

[hlundin-webrtc, 2016/06/23 07:46:36]

Should recreate_encoder_ be modified here?

[kwiberg-webrtc, 2016/06/23 10:31:51]

No. It's currently true, and should be true because we should recreate the
encoder as soon as possible. (Alternatively, it doesn't matter what value it
has, because we never set send_codec_inst_ without simultaneously setting
recreate_encoder_ to true.)

[hlundin-webrtc, 2016/06/23 10:38:46]

Acknowledged.

+  }
+
+  codec_stack_params_.speech_encoder = rac->RentEncoder(*send_codec_inst_);
+  auto stack = rac->RentEncoderStack(&codec_stack_params_);
+  if (!stack) {
+    return false;

[hlundin-webrtc, 2016/06/23 07:46:36]

.. and here?

[kwiberg-webrtc, 2016/06/23 10:31:51]

Same reasoning as above. We should only set it on line 251, which is the first
point where we know that we successfully created an encoder and injected it into
the ACM.

[hlundin-webrtc, 2016/06/23 10:38:47]

Acknowledged.

+  }
+  acm->SetEncoder(std::move(stack));
+  recreate_encoder_ = false;
+  return true;
+}
+
 }  // namespace acm2
 }  // namespace webrtc