Move CNG and RED management into the Rent-A-Codec

Move CNG and RED management into the Rent-A-Codec

This leaves CodecOwner without a job, so we eliminate it.

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

[kwiberg-webrtc, 2015-11-14 16:01:27]

Description was changed from

==========
Move CNG and RED management into the Rent-A-Codec

This leaves CodecOwner without a job, so we eliminate it.

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

to

==========
Move CNG and RED management into the Rent-A-Codec

This leaves CodecOwner without a job, so we eliminate it.

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

[kwiberg-webrtc, 2015-11-14 16:01:28]

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

[kwiberg-webrtc, 2015-11-14 16:04:33]

Please throw an eye at this CL.

[hlundin-webrtc, 2015-11-16 08:59:17]

LG, but please consider my proposal for reducing code duplication. If you refuse
it, I will have to accept that. 

"These are my principles, and if you don't like them... Well, I have others."

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:267: enc, dtx_enabled_
The two conditionals in this expression are slightly horrible. And they return a
few lines further down. I think you should break them out into two private
member functions:

rtc::Optional<RentACodec::CngConfig> OptionalCngConfig(int
rtp_timestamp_rate_hz) const;

rtc::Optional<int> OptionalRedPayloadType(int rtp_timestamp_rate_hz) const;

Or, break out the whole expression into a private member function:
void RentEncoderStack(int rtp_timestamp_rate_hz) const {
  rent_a_codec_.RentEncoderStack(...);
  RTC_DCHECK(CurrentEncoder());
}

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc (right):

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc:223: red_encoder_ =
CreateRedEncoder(encoder_stack_, *red_payload_type);
Just to verify, assignment will properly dispose of any previous object in
red_encoder_, right?

[kwiberg-webrtc, 2015-11-16 12:01:38]

New patch set posted.

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:267: enc, dtx_enabled_
On 2015/11/16 08:59:17, hlundin-webrtc wrote:
> The two conditionals in this expression are slightly horrible. And they return
a
> few lines further down. I think you should break them out into two private
> member functions:
> 
> rtc::Optional<RentACodec::CngConfig> OptionalCngConfig(int
> rtp_timestamp_rate_hz) const;
> 
> rtc::Optional<int> OptionalRedPayloadType(int rtp_timestamp_rate_hz) const;
> 
> Or, break out the whole expression into a private member function:
> void RentEncoderStack(int rtp_timestamp_rate_hz) const {
>   rent_a_codec_.RentEncoderStack(...);
>   RTC_DCHECK(CurrentEncoder());
> }

Done.

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc (right):

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc:223: red_encoder_ =
CreateRedEncoder(encoder_stack_, *red_payload_type);
On 2015/11/16 08:59:17, hlundin-webrtc wrote:
> Just to verify, assignment will properly dispose of any previous object in
> red_encoder_, right?

Yes. Once you hand a pointer to scoped_ptr, you can stop worrying about it. In
the particular case of operator=, the lhs will delete its pointer before
overwriting it.

[hlundin-webrtc, 2015-11-16 12:28:06]

lgtm

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc (right):

https://codereview.webrtc.org/1443653004/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc:223: red_encoder_ =
CreateRedEncoder(encoder_stack_, *red_payload_type);
On 2015/11/16 12:01:37, kwiberg-webrtc wrote:
> On 2015/11/16 08:59:17, hlundin-webrtc wrote:
> > Just to verify, assignment will properly dispose of any previous object in
> > red_encoder_, right?
> 
> Yes. Once you hand a pointer to scoped_ptr, you can stop worrying about it. In
> the particular case of operator=, the lhs will delete its pointer before
> overwriting it.

Acknowledged.

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

https://codereview.webrtc.org/1443653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager.cc:452: int sample_rate_hz)
{
I'd argue that the name should be rtp_timestamp_rate_hz, but since (a) the
corresponding parameter in RedPayloadType is called sample_rate_hz and (b) that
function is limited to only 8 kHz I consider it TBD later.

[kwiberg-webrtc, 2015-11-16 12:32:58]

The CQ bit was checked by kwiberg@webrtc.org

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

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

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

Committed patchset #2 (id:20001)

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

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