Hide ACMCodecDB::codec_settings_ behind an accessor

webrtc/modules/audio_coding/main/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 45 matching lines @@
     return -1;
   }
 
   // Telephone-event cannot be a send codec.
   if (!STR_CASE_CMP(send_codec.plname, "telephone-event")) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                  "telephone-event cannot be a send codec");
     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);
     return -1;
   }
 
   if (!is_primary_encoder) {
     // If registering the secondary encoder, then RED and CN are not valid
     // choices as encoder.
     if (IsCodecRED(send_codec)) {
@@ skipping 378 matching lines @@
     case 48000:
       return -1;
     default:
       FATAL() << sample_rate_hz << " Hz is not supported";
       return -1;
   }
 }
 
 }  // namespace acm2
 }  // namespace webrtc