Move Rent-A-Codec out of CodecManager

Move Rent-A-Codec out of CodecManager

So that the two of them sit next to each other at the top level of
AudioCodingModuleImpl. CodecManager now manages the specifications for
Rent-A-Codec, rather than managing encoders directly.

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

[kwiberg-webrtc, 2015-12-15 11:53:47]

Description was changed from

==========
Move Rent-A-Codec out of CodecManager

So that the two of them sit next to each other at the top level of
AudioCodingModuleImpl. CodecManager now manages the specifications for
Rent-A-Codec, rather than managing encoders directly.

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

to

==========
Move Rent-A-Codec out of CodecManager

So that the two of them sit next to each other at the top level of
AudioCodingModuleImpl. CodecManager now manages the specifications for
Rent-A-Codec, rather than managing encoders directly.

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

[kwiberg-webrtc, 2015-12-15 11:53:47]

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

[hlundin-webrtc, 2015-12-15 13:42:03]

LG, but see comments.

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

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:202: return -1;
I'd like to see some logging, so that it is possible to understand what happened
and why.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:205: if
(codec_manager_.GetCodecInst() && !sp->speech_encoder) {
Please, add a comment to explain this condition. In particular, what does the
value of codec_manager_.GetCodecInst() mean, and what if it does not evaluate to
true?

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:209: return -1;
Log here too, please.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:245: auto* enc =
rent_a_codec_.GetEncoderStack();
const auto*
(Because GetEncoderStack() violates the style guide and returns a non-const
pointer to a member through a const-qualified method.)

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:345: if
(current_num_channels != ptr_frame->num_channels_)
Did ptr_frame->num_channels_ change since remix was set? Otherwise, you can just
condition on remix here. (Maybe change the name and make it const, too.)

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:364: bool resample
= in_frame.sample_rate_hz_ != enc->SampleRateHz();
const

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:368: bool down_mix
= in_frame.num_channels_ == 2 && enc->NumChannels() == 1;
const
Add a TODO to generalize the condition; it should probably be
"in_frame.num_channels_ > enc->NumChannels()"

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:449: int
AudioCodingModuleImpl::SetREDStatus(bool enable_red) {
Does this compile when WEBRTC_CODEC_RED is not defined?

[kwiberg-webrtc, 2015-12-16 10:37:37]

Patchset #2 (id:20001) has been deleted

[kwiberg-webrtc, 2015-12-16 10:57:49]

New patch set uploaded!

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

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:202: return -1;
On 2015/12/15 13:42:03, hlundin-webrtc wrote:
> I'd like to see some logging, so that it is possible to understand what
happened
> and why.

codec_manager_.RegisterEncoder already logs in all cases where it returns false,
providing more detail than what we could produce at this point.

It'd be nice if logging was visible in the function signature... that'd be
another advantage of passing a logger object to functions that want to log.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:205: if
(codec_manager_.GetCodecInst() && !sp->speech_encoder) {
On 2015/12/15 13:42:03, hlundin-webrtc wrote:
> Please, add a comment to explain this condition. In particular, what does the
> value of codec_manager_.GetCodecInst() mean, and what if it does not evaluate
to
> true?

Done.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:209: return -1;
On 2015/12/15 13:42:03, hlundin-webrtc wrote:
> Log here too, please.

Same answer: rent_a_codec_.RentEncoder already logs.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:245: auto* enc =
rent_a_codec_.GetEncoderStack();
On 2015/12/15 13:42:03, hlundin-webrtc wrote:
> const auto*
> (Because GetEncoderStack() violates the style guide and returns a non-const
> pointer to a member through a const-qualified method.)

Done.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:345: if
(current_num_channels != ptr_frame->num_channels_)
On 2015/12/15 13:42:02, hlundin-webrtc wrote:
> Did ptr_frame->num_channels_ change since remix was set? Otherwise, you can
just
> condition on remix here. (Maybe change the name and make it const, too.)

No, it really shouldn't. Done.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:364: bool resample
= in_frame.sample_rate_hz_ != enc->SampleRateHz();
On 2015/12/15 13:42:02, hlundin-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:368: bool down_mix
= in_frame.num_channels_ == 2 && enc->NumChannels() == 1;
On 2015/12/15 13:42:03, hlundin-webrtc wrote:
> const
> Add a TODO to generalize the condition; it should probably be
> "in_frame.num_channels_ > enc->NumChannels()"

Done.

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:449: int
AudioCodingModuleImpl::SetREDStatus(bool enable_red) {
On 2015/12/15 13:42:03, hlundin-webrtc wrote:
> Does this compile when WEBRTC_CODEC_RED is not defined?

It really should. It's not an error for a function to ignore one of its
arguments. Commenting out the name ensures that it's an error to try to use it,
but doing that conditionally as was done here is just a mess to read.

[hlundin-webrtc, 2015-12-16 11:58:35]

lgtm

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

https://codereview.webrtc.org/1520283006/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:202: return -1;
On 2015/12/16 10:57:48, kwiberg-webrtc wrote:
> On 2015/12/15 13:42:03, hlundin-webrtc wrote:
> > I'd like to see some logging, so that it is possible to understand what
> happened
> > and why.
> 
> codec_manager_.RegisterEncoder already logs in all cases where it returns
false,
> providing more detail than what we could produce at this point.
> 
> It'd be nice if logging was visible in the function signature... that'd be
> another advantage of passing a logger object to functions that want to log.

Acknowledged.

[kwiberg-webrtc, 2015-12-16 12:18:01]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2015-12-16 12:18:05]

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

[commit-bot: I haz the power, 2015-12-16 12:19:10]

Description was changed from

==========
Move Rent-A-Codec out of CodecManager

So that the two of them sit next to each other at the top level of
AudioCodingModuleImpl. CodecManager now manages the specifications for
Rent-A-Codec, rather than managing encoders directly.

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

to

==========
Move Rent-A-Codec out of CodecManager

So that the two of them sit next to each other at the top level of
AudioCodingModuleImpl. CodecManager now manages the specifications for
Rent-A-Codec, rather than managing encoders directly.

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

[commit-bot: I haz the power, 2015-12-16 12:19:11]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-12-16 12:19:22]

Description was changed from

==========
Move Rent-A-Codec out of CodecManager

So that the two of them sit next to each other at the top level of
AudioCodingModuleImpl. CodecManager now manages the specifications for
Rent-A-Codec, rather than managing encoders directly.

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

to

==========
Move Rent-A-Codec out of CodecManager

So that the two of them sit next to each other at the top level of
AudioCodingModuleImpl. CodecManager now manages the specifications for
Rent-A-Codec, rather than managing encoders directly.

BUG=webrtc:5028

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

[commit-bot: I haz the power, 2015-12-16 12:19:23]

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