AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

(This is a re-land of https://codereview.webrtc.org/2339953002.)

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

[kwiberg-webrtc, 2016-09-19 11:20:05]

Description was changed from

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

(This is a re-land of https://codereview.webrtc.org/2339953002.)

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

to

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

(This is a re-land of https://codereview.webrtc.org/2339953002.)

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

[kwiberg-webrtc, 2016-09-19 11:20:05]

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

[hlundin-webrtc, 2016-09-19 11:37:36]

LG. Please, consider (at least a few of) my comments.

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:94: if (last_audio_decoder_ &&
last_audio_decoder_->channels > 1) {
This is functionality best kept inside NetEq. ACM should not have to bother.
Feel free to do that in a follow-up. :)

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:297: int
AcmReceiver::LastAudioCodec(CodecInst* codec) const {
I think this should (and can) provide a pointer to a constant object. Not that
the code is any better the way it was...

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:368: uint8_t payload_type)
const {
Rename to "first_payload_byte". This code is very confusing otherwise.

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/include/neteq.h (right):

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/include/neteq.h:255: virtual
rtc::Optional<CodecInst> GetDecoder(int payload_type) const = 0;
Short comment, please. What if not found?

[kwiberg-webrtc, 2016-09-19 12:18:34]

New patch set posted.

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:94: if (last_audio_decoder_ &&
last_audio_decoder_->channels > 1) {
On 2016/09/19 11:37:36, hlundin-webrtc wrote:
> This is functionality best kept inside NetEq. ACM should not have to bother.
> Feel free to do that in a follow-up. :)

Moving this into NetEq would mean that AcmReceiver would stop keeping track of
last decoder and last sample rate, right? Then yes, that would certainly be a
separate CL (one which would take AcmReceiver even closer to having no job left
to do).

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:297: int
AcmReceiver::LastAudioCodec(CodecInst* codec) const {
On 2016/09/19 11:37:36, hlundin-webrtc wrote:
> I think this should (and can) provide a pointer to a constant object. Not that
> the code is any better the way it was...

Changing this from filling in the caller's CodecInst to returning a const
pointer to last_audio_decoder_ would violate the locking scheme, since we'd be
returning a pointer to a variable protected by crit_sect_ to code that's not
protected by crit_sect_.

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:368: uint8_t payload_type)
const {
On 2016/09/19 11:37:36, hlundin-webrtc wrote:
> Rename to "first_payload_byte". This code is very confusing otherwise.

Good idea.

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/include/neteq.h (right):

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/include/neteq.h:255: virtual
rtc::Optional<CodecInst> GetDecoder(int payload_type) const = 0;
On 2016/09/19 11:37:36, hlundin-webrtc wrote:
> Short comment, please. What if not found?

Done.

[hlundin-webrtc, 2016-09-19 12:22:03]

LGTM with one nit.

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:94: if (last_audio_decoder_ &&
last_audio_decoder_->channels > 1) {
On 2016/09/19 12:18:34, kwiberg-webrtc wrote:
> On 2016/09/19 11:37:36, hlundin-webrtc wrote:
> > This is functionality best kept inside NetEq. ACM should not have to bother.
> > Feel free to do that in a follow-up. :)
> 
> Moving this into NetEq would mean that AcmReceiver would stop keeping track of
> last decoder and last sample rate, right? Then yes, that would certainly be a
> separate CL (one which would take AcmReceiver even closer to having no job
left
> to do).

Acknowledged.

https://codereview.webrtc.org/2354453003/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:297: int
AcmReceiver::LastAudioCodec(CodecInst* codec) const {
On 2016/09/19 12:18:34, kwiberg-webrtc wrote:
> On 2016/09/19 11:37:36, hlundin-webrtc wrote:
> > I think this should (and can) provide a pointer to a constant object. Not
that
> > the code is any better the way it was...
> 
> Changing this from filling in the caller's CodecInst to returning a const
> pointer to last_audio_decoder_ would violate the locking scheme, since we'd be
> returning a pointer to a variable protected by crit_sect_ to code that's not
> protected by crit_sect_.

Oh, sorry, I read the code wrong. Please, disregard...

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

https://codereview.webrtc.org/2354453003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/include/neteq.h:255: // Return info about the
decoder for the given payload type, or an empty
nit: Returns

[kwiberg-webrtc, 2016-09-20 08:15:02]

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

https://codereview.webrtc.org/2354453003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/include/neteq.h:255: // Return info about the
decoder for the given payload type, or an empty
On 2016/09/19 12:22:03, hlundin-webrtc wrote:
> nit: Returns

I wonder how long I'll have to work on this code base before I learn this
comment style rule...

Done.

[kwiberg-webrtc, 2016-09-20 08:15:54]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-09-20 08:15:55]

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/2354453003/#ps40001 (title:
"grammar")

[commit-bot: I haz the power, 2016-09-20 08:16:00]

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-20 10:07:48]

Description was changed from

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

(This is a re-land of https://codereview.webrtc.org/2339953002.)

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

to

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

(This is a re-land of https://codereview.webrtc.org/2339953002.)

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

[commit-bot: I haz the power, 2016-09-20 10:07:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-20 10:07:53]

Description was changed from

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

(This is a re-land of https://codereview.webrtc.org/2339953002.)

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

to

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

(This is a re-land of https://codereview.webrtc.org/2339953002.)

BUG=webrtc:5801

Committed: https://crrev.com/6f0f616b53c0a7b95f7616fdcafe67ec595b2b33
Cr-Commit-Position: refs/heads/master@{#14303}
==========

[commit-bot: I haz the power, 2016-09-20 10:07:54]

Patchset 3 (id:??) landed as
https://crrev.com/6f0f616b53c0a7b95f7616fdcafe67ec595b2b33
Cr-Commit-Position: refs/heads/master@{#14303}