Switch to using new ACM methods for encoder management

webrtc/modules/audio_coding/acm2/codec_manager.h

 /*
  *  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.
  */
 
 #ifndef WEBRTC_MODULES_AUDIO_CODING_ACM2_CODEC_MANAGER_H_
 #define WEBRTC_MODULES_AUDIO_CODING_ACM2_CODEC_MANAGER_H_
 
 #include <map>
 
 #include "webrtc/base/constructormagic.h"
 #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"
 
 namespace webrtc {
 
 class AudioDecoder;
 class AudioEncoder;
 
 namespace acm2 {
 
@@ skipping 18 matching lines @@
     return &codec_stack_params_;
   }
   RentACodec::StackParameters* GetStackParams() { return &codec_stack_params_; }
 
   bool SetCopyRed(bool enable);
 
   bool SetVAD(bool enable, ACMVADMode mode);
 
   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_;
   RentACodec::StackParameters codec_stack_params_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(CodecManager);
 };
 
 }  // namespace acm2
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_AUDIO_CODING_ACM2_CODEC_MANAGER_H_