Hide ACMCodecDB::codec_settings_ behind an accessor

Hide ACMCodecDB::codec_settings_ behind an accessor

BUG=webrtc:5028
Committed: https://crrev.com/de94d08d57820fb1b67a31354e867a26aebb17d3
Cr-Commit-Position: refs/heads/master@{#10494}

[kwiberg-webrtc, 2015-11-02 06:22:30]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2015-11-02 07:11:25]

Description was changed from

==========
Hide ACMCodecDB::codec_settings_ behind an accessor

BUG=webrtc:5028
==========

to

==========
Hide ACMCodecDB::codec_settings_ behind an accessor

BUG=webrtc:5028
==========

[kwiberg-webrtc, 2015-11-02 07:11:25]

kwiberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[kwiberg-webrtc, 2015-11-02 07:11:38]

PTAL

[hlundin-webrtc, 2015-11-03 08:48:56]

LGTM with a comment.

https://codereview.webrtc.org/1419013005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/1419013005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:68: return cid ?
RentACodec::SupportedNumChannels(*cid, send_codec.channels)
While I think this code is correct, I got fooled at first by the method name
SupportedNumChannels. I was expecting a number to compare with, not a boolean.
Maybe change it to ValidNumChannels?

[kwiberg-webrtc, 2015-11-03 09:32:03]

https://codereview.webrtc.org/1419013005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/1419013005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:68: return cid ?
RentACodec::SupportedNumChannels(*cid, send_codec.channels)
On 2015/11/03 08:48:55, hlundin-webrtc wrote:
> While I think this code is correct, I got fooled at first by the method name
> SupportedNumChannels. I was expecting a number to compare with, not a boolean.
> Maybe change it to ValidNumChannels?

IsSupportedNumChannels?

[hlundin-webrtc, 2015-11-03 10:00:47]

https://codereview.webrtc.org/1419013005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/1419013005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:68: return cid ?
RentACodec::SupportedNumChannels(*cid, send_codec.channels)
On 2015/11/03 09:32:03, kwiberg-webrtc wrote:
> On 2015/11/03 08:48:55, hlundin-webrtc wrote:
> > While I think this code is correct, I got fooled at first by the method name
> > SupportedNumChannels. I was expecting a number to compare with, not a
boolean.
> > Maybe change it to ValidNumChannels?
> 
> IsSupportedNumChannels?

IsABellNecessaryOnABike? Sounds good. Make it happen.

[kwiberg-webrtc, 2015-11-03 12:52:32]

Patchset #3 (id:60001) has been deleted

[kwiberg-webrtc, 2015-11-03 12:53:05]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2015-11-03 12:53:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1419013005/#ps40001 (title:
"ICanHasNumChannels?")

[commit-bot: I haz the power, 2015-11-03 12:53:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419013005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419013005/40001

[commit-bot: I haz the power, 2015-11-03 13:46:14]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-11-03 13:46:25]

Patchset 2 (id:??) landed as
https://crrev.com/de94d08d57820fb1b67a31354e867a26aebb17d3
Cr-Commit-Position: refs/heads/master@{#10494}

[tommi, 2015-11-03 14:20:18]

tommi@webrtc.org changed reviewers:
+ tommi@webrtc.org

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

https://codereview.webrtc.org/1419013005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/1419013005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:73: if
(!*supported_num_channels) {
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]

https://codereview.webrtc.org/1419013005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/1419013005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:73: if
(!*supported_num_channels) {
On 2015/11/03 14:20:19, tommi (webrtc) wrote:
> 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.

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.)

> As is, the reader needs to know about operator overloading in
> rtc::Maybe

That's true anyway, since we call functions that return Maybe.

> as well as figure out why we need a lambda when there's nothing
> async or callback-ish going on.

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.

> 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.

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]

https://codereview.webrtc.org/1419013005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/1419013005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:73: if
(!*supported_num_channels) {
On 2015/11/03 15:13:27, kwiberg-webrtc wrote:
> On 2015/11/03 14:20:19, tommi (webrtc) wrote:
> > 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.
> 
> 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.)
> 
> > As is, the reader needs to know about operator overloading in
> > rtc::Maybe
> 
> That's true anyway, since we call functions that return Maybe.

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)

> > as well as figure out why we need a lambda when there's nothing
> > async or callback-ish going on.
> 
> 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.
> 
> > 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.
> 
> 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.)

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.