AcmReceiver: Eliminate AcmReceiver::decoders_

AcmReceiver: Eliminate AcmReceiver::decoders_

BUG=webrtc:5801
Committed: https://crrev.com/c4ccd4d61c5c061e445db126efaa4923c82ea9be
Cr-Commit-Position: refs/heads/master@{#14335}

[kwiberg-webrtc, 2016-09-20 15:03:13]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-09-20 15:04:01]

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

[kwiberg-webrtc, 2016-09-20 15:21:00]

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_format.cc (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_format.cc:38: a.num_channels ==
b.num_channels && a.parameters == b.parameters;
Should the .name comparison be case-insensitive?

[ossu, 2016-09-20 15:30:56]

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:185: // TODO(kwiberg): This
function has been ignoring the |sample_rate_hz|
Do it! I dare you!

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_format.cc (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_format.cc:38: a.num_channels ==
b.num_channels && a.parameters == b.parameters;
On 2016/09/20 15:21:00, kwiberg-webrtc wrote:
> Should the .name comparison be case-insensitive?

I think it should. We're doing case-insensitive matching of these names
everywhere else in the code, so it makes sense that this would be done here as
well.

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database.h:68: return audio_format_ ?
&*audio_format_ : nullptr;
I don't think we should be able to keep anything in this database that doesn't
have a valid format. My change in: https://codereview.webrtc.org/2355503002/
makes that so, so maybe put this one in after that one?

[kwiberg-webrtc, 2016-09-20 16:30:29]

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_format.cc (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_format.cc:38: a.num_channels ==
b.num_channels && a.parameters == b.parameters;
On 2016/09/20 15:30:55, ossu wrote:
> On 2016/09/20 15:21:00, kwiberg-webrtc wrote:
> > Should the .name comparison be case-insensitive?
> 
> I think it should. We're doing case-insensitive matching of these names
> everywhere else in the code, so it makes sense that this would be done here as
> well.

Done.

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database.h:68: return audio_format_ ?
&*audio_format_ : nullptr;
On 2016/09/20 15:30:56, ossu wrote:
> I don't think we should be able to keep anything in this database that doesn't
> have a valid format.

I agree. We currently do, though, and it isn't a problem for the stuff I change
in this CL (since only external decoders lack SdpAudioFormat, and I don't need
SdpAudioFormat for them).

> My change in: https://codereview.webrtc.org/2355503002/
> makes that so, so maybe put this one in after that one?

That's a good change, but I don't think it needs to be a prerequisite for this
CL. You also touch these lines, so whoever lands his CL last will get an
easy-to-fix conflict and can sort it out so that GetFormat returns a const ref
again.

[hlundin-webrtc, 2016-09-21 10:38:16]

lgtm

[ossu, 2016-09-21 10:46:10]

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database.h:68: return audio_format_ ?
&*audio_format_ : nullptr;
On 2016/09/20 16:30:29, kwiberg-webrtc wrote:
> On 2016/09/20 15:30:56, ossu wrote:
> > I don't think we should be able to keep anything in this database that
doesn't
> > have a valid format.
> 
> I agree. We currently do, though, and it isn't a problem for the stuff I
change
> in this CL (since only external decoders lack SdpAudioFormat, and I don't need
> SdpAudioFormat for them).
> 
> > My change in: https://codereview.webrtc.org/2355503002/
> > makes that so, so maybe put this one in after that one?
> 
> That's a good change, but I don't think it needs to be a prerequisite for this
> CL. You also touch these lines, so whoever lands his CL last will get an
> easy-to-fix conflict and can sort it out so that GetFormat returns a const ref
> again.

Not that it matters enormously, but if so, why don't you just return a reference
to optional here? That's essentially what you're replicating with making it a
pointer. And we've already got the optional just lying around.

[kwiberg-webrtc, 2016-09-21 11:51:58]

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database.h:68: return audio_format_ ?
&*audio_format_ : nullptr;
On 2016/09/21 10:46:10, ossu wrote:
> On 2016/09/20 16:30:29, kwiberg-webrtc wrote:
> > On 2016/09/20 15:30:56, ossu wrote:
> > > I don't think we should be able to keep anything in this database that
> doesn't
> > > have a valid format.
> > 
> > I agree. We currently do, though, and it isn't a problem for the stuff I
> change
> > in this CL (since only external decoders lack SdpAudioFormat, and I don't
need
> > SdpAudioFormat for them).
> > 
> > > My change in: https://codereview.webrtc.org/2355503002/
> > > makes that so, so maybe put this one in after that one?
> > 
> > That's a good change, but I don't think it needs to be a prerequisite for
this
> > CL. You also touch these lines, so whoever lands his CL last will get an
> > easy-to-fix conflict and can sort it out so that GetFormat returns a const
ref
> > again.
> 
> Not that it matters enormously, but if so, why don't you just return a
reference
> to optional here? That's essentially what you're replicating with making it a
> pointer. And we've already got the optional just lying around.

Because of the two equivalent return types const Optional<T>& and const T*, the
latter is (1) more well-known, (2) more general, (3) simpler, and (4) shorter.
Yes, returning const Optional<T>& here would be easier to implement and possibly
a tiny bit faster, but I didn't think it was worth it.

[ossu, 2016-09-21 12:00:56]

lgtm

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2351183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database.h:68: return audio_format_ ?
&*audio_format_ : nullptr;
On 2016/09/21 11:51:57, kwiberg-webrtc wrote:
> On 2016/09/21 10:46:10, ossu wrote:
> > On 2016/09/20 16:30:29, kwiberg-webrtc wrote:
> > > On 2016/09/20 15:30:56, ossu wrote:
> > > > I don't think we should be able to keep anything in this database that
> > doesn't
> > > > have a valid format.
> > > 
> > > I agree. We currently do, though, and it isn't a problem for the stuff I
> > change
> > > in this CL (since only external decoders lack SdpAudioFormat, and I don't
> need
> > > SdpAudioFormat for them).
> > > 
> > > > My change in: https://codereview.webrtc.org/2355503002/
> > > > makes that so, so maybe put this one in after that one?
> > > 
> > > That's a good change, but I don't think it needs to be a prerequisite for
> this
> > > CL. You also touch these lines, so whoever lands his CL last will get an
> > > easy-to-fix conflict and can sort it out so that GetFormat returns a const
> ref
> > > again.
> > 
> > Not that it matters enormously, but if so, why don't you just return a
> reference
> > to optional here? That's essentially what you're replicating with making it
a
> > pointer. And we've already got the optional just lying around.
> 
> Because of the two equivalent return types const Optional<T>& and const T*,
the
> latter is (1) more well-known, (2) more general, (3) simpler, and (4) shorter.
> Yes, returning const Optional<T>& here would be easier to implement and
possibly
> a tiny bit faster, but I didn't think it was worth it.

I'm not sure I agree with you, but I'm also not sure it's worth another pass
through code review for a bit of code it looks like I'll be removing shortly
anyways.

[kwiberg-webrtc, 2016-09-21 12:04:34]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-09-21 12:04:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 14:05:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 14:05:07]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-09-21 17:37:11]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-09-21 17:37:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 17:55:20]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-21 17:55:25]

Description was changed from

==========
AcmReceiver: Eliminate AcmReceiver::decoders_

BUG=webrtc:5801
==========

to

==========
AcmReceiver: Eliminate AcmReceiver::decoders_

BUG=webrtc:5801

Committed: https://crrev.com/c4ccd4d61c5c061e445db126efaa4923c82ea9be
Cr-Commit-Position: refs/heads/master@{#14335}
==========

[commit-bot: I haz the power, 2016-09-21 17:55:26]

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