Hide ACMCodecDB::codec_settings_ behind an accessor

webrtc/modules/audio_coding/main/acm2/codec_manager.cc

Index: webrtc/modules/audio_coding/main/acm2/codec_manager.cc
diff --git a/webrtc/modules/audio_coding/main/acm2/codec_manager.cc b/webrtc/modules/audio_coding/main/acm2/codec_manager.cc
index f9b77e898511ff2dd4d682ff085ba30c3d3e9d3b..fce6bd7a086d31d171e5274453e72a899c4cf063 100644
--- a/webrtc/modules/audio_coding/main/acm2/codec_manager.cc
+++ b/webrtc/modules/audio_coding/main/acm2/codec_manager.cc
@@ -63,8 +63,14 @@ int IsValidSendCodec(const CodecInst& send_codec, bool is_primary_encoder) {
     return -1;
   }
 
-  if (ACMCodecDB::codec_settings_[codec_id].channel_support <
-      send_codec.channels) {
+  const rtc::Maybe<bool> supported_num_channels = [codec_id, &send_codec] {
+    auto cid = RentACodec::CodecIdFromIndex(codec_id);
+    return cid ? RentACodec::IsSupportedNumChannels(*cid, send_codec.channels)
+               : rtc::Maybe<bool>();
+  }();
+  if (!supported_num_channels)
+    return -1;
+  if (!*supported_num_channels) {

[tommi, 2015/11/03 14:20:19]

Are these 8 lines, basically doing the same as the below 4 lines?

auto cid = RentACodec::CodecIdFromIndex(codec_id);
if (!cid)
  return -1;
if (!RentACodec::IsSupportedNumChannels(*cid, send_codec.channels)) {

If so, then I don't think that's really a good reason for using lambdas and/or
rtc::Maybe.  As is, the code is a lot harder to read then the above 4 lines.
As is, the reader needs to know about operator overloading in rtc::Maybe as well
as figure out why we need a lambda when there's nothing async or callback-ish
going on.  I'm also a little on the fence about the point at which the lambda
gets evaluated and how that is clear from the code.

[kwiberg-webrtc, 2015/11/03 15:13:27]

Almost. Without the lambda, it looks like this if I'm not mistaken:

  auto cid = RentACodec::CodecIdFromIndex(codec_id);
  if (!cid)
    return -1;
  auto supported_num_channels = RentACodec::IsSupportedNumChannels(
                                    *cid, send_codec.channels);
  if (!supported_num_channels)
    return -1;
  if (!*supported_num_channels) {

But one may be tempted to argue that it's not worth distinguishing
between the cases where we fail because the codec_id is invalid and
the one where the requested number of channels wasn't supported. In
that case, something like this ought to work:

  auto cid = RentACodec::CodecIdFromIndex(codec_id);
  if (!cid || !RentACodec::IsSupportedNumChannels(
                  *cid, send_codec.channels).value_or(false)) {

Note how this conflates three error sources: CodecIdFromIndex failing,
IsSupportedNumChannels failing, and IsSupportedNumChannels returning
false.

(The seemingly excessive number of calls that can fail here is the
result of an in-progress refactoring. This is what it looks like after
I've used Maybe to expose all the places where things could possibly
go wrong, but before I've done anything about it.)
That's true anyway, since we call functions that return Maybe.
To limit the scope of cid. I could possibly be made to admit that
avoiding leaking cid into the surrounding scope might not be worth the
extra complexity of the lambda here.
That's easy: it's evaluated by the parenthesis pair on line 70,
without ever being bound to a name. Only its return value is bound.
Think of it as a way to return a value out of a block. (It looks a lot
prettier if you allow default captures for lambdas that can't escape
the scope in which they're defined, which the Chromium style guide
still doesn't.)

[tommi, 2015/11/03 16:50:01]

Somehow I missed that |auto| was rtc::Maybe too.  So, we are creating one
temporary rtc::Maybe and returning another inside the lambda. Should the
variable name have been maybe_cid?  I ask since it's not guaranteed to be a cid.
 This is a nit, but I think it would improve readability.  (Fine to disregard
since the CL is already committed)
Ah sorry, I did notice that actually the first time I glanced over the code, but
the second time I must have read }(); as }; and then was wondering if the
evaluation could actually occur inside the overloaded operators.

     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                  "%d number of channels not supportedn for %s.",
                  send_codec.channels, send_codec.plname);