Switch to using new ACM methods for encoder management

webrtc/modules/audio_coding/acm2/codec_manager.h

Index: webrtc/modules/audio_coding/acm2/codec_manager.h
diff --git a/webrtc/modules/audio_coding/acm2/codec_manager.h b/webrtc/modules/audio_coding/acm2/codec_manager.h
index fbd3a18b188efa203949159bf0e0b8c9bde353b3..1c3be33d2d2742ca794dc09898c815d9d1e26a75 100644
--- a/webrtc/modules/audio_coding/acm2/codec_manager.h
+++ b/webrtc/modules/audio_coding/acm2/codec_manager.h
@@ -17,6 +17,7 @@
 #include "webrtc/base/optional.h"
 #include "webrtc/base/thread_checker.h"
 #include "webrtc/modules/audio_coding/acm2/rent_a_codec.h"
+#include "webrtc/modules/audio_coding/include/audio_coding_module.h"
 #include "webrtc/modules/audio_coding/include/audio_coding_module_typedefs.h"
 #include "webrtc/common_types.h"
 
@@ -55,6 +56,28 @@ class CodecManager final {
 
   bool SetCodecFEC(bool enable_codec_fec);
 
+  // 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) {

[the sun, 2016/04/06 11:58:25]

In a big fan of DCHECKing pointers before I use them. In this case it looks like
you should do it at the beginning of this function.

[kwiberg-webrtc, 2016/04/06 12:15:29]

I'm on the other side in this debate: since dereferencing a null pointer and
trying to use the result is guaranteed to cause a nice and clean crash, I feel
it isn't worth cluttering the code with [D]CHECKs for pointer nullness most of
the time. If a function has a lot of callers or if the place where dereferencing
would cause a crash is far away it might still be worth it, but neither of those
is the case here.

Ideally, we'd use mutable references here to make it difficult for callers to
pass null pointers, but style guide.

[the sun, 2016/04/06 13:05:59]

The real reason for the DCHECKs is to document expectations.

If you don't do that, and there's a crash at a later point, the developer fixing
the issue may look at the code, going "oh, someone forgot to check for the
pointer being null, I'll add an early out in that case", instead of "oh, this
function expects the pointer argument to always be valid, I'll look at what the
caller is doing wrong".

+    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;
+  }
+
  private:
   rtc::ThreadChecker thread_checker_;
   rtc::Optional<CodecInst> send_codec_inst_;