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

webrtc/modules/audio_coding/acm2/codec_manager.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 95 matching lines @@
     case RentACodec::RegistrationResult::kSkip:
       break;
   }
 
   if (IsOpus(send_codec)) {
     // VAD/DTX not supported.
     codec_stack_params_.use_cng = false;
   }
 
   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;
 }
 
 CodecInst CodecManager::ForgeCodecInst(
     const AudioEncoder* external_speech_encoder) {
   CodecInst ci;
   ci.channels = external_speech_encoder->NumChannels();
   ci.plfreq = external_speech_encoder->SampleRateHz();
   ci.pacsize = rtc::CheckedDivExact(
       static_cast<int>(external_speech_encoder->Max10MsFramesInAPacket() *
@@ skipping 56 matching lines @@
   if (enable_codec_fec && codec_stack_params_.use_red) {
     WEBRTC_TRACE(webrtc::kTraceWarning, webrtc::kTraceAudioCoding, 0,
                  "Codec internal FEC and RED cannot be co-enabled.");
     return false;
   }
 
   codec_stack_params_.use_codec_fec = 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