RentACodec: New class that takes over part of ACMCodecDB's job

RentACodec: New class that takes over part of ACMCodecDB's job

Following CLs will finish the takeover completely. After that,
RentACodec will also start creating and owning codecs, at which point
its name will start making sense.

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

[kwiberg-webrtc, 2015-10-26 22:24:41]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2015-10-26 22:24:51]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2015-10-27 08:49:34]

Description was changed from

==========
RentACodec: New class that takes over part of ACMCodecDB's job

Following CLs will finish the takeover completely. After that,
RentACodec will also start creating and owning codecs, at which point
its name will start making sense.

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

to

==========
RentACodec: New class that takes over part of ACMCodecDB's job

Following CLs will finish the takeover completely. After that,
RentACodec will also start creating and owning codecs, at which point
its name will start making sense.

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

[kwiberg-webrtc, 2015-10-27 08:49:34]

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

[kwiberg-webrtc, 2015-10-27 08:51:17]

英

[the sun, 2015-10-27 11:56:42]

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

[the sun, 2015-10-27 11:56:43]

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_codec_database.cc:356: int
ACMCodecDB::CodecFreq(int codec_id) {
const?

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_codec_database.cc:358: auto db =
RentACodec::Database();
const auto?

[kwiberg-webrtc, 2015-10-27 12:16:46]

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_codec_database.cc:356: int
ACMCodecDB::CodecFreq(int codec_id) {
On 2015/10/27 11:56:43, the sun wrote:
> const?

Where?

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_codec_database.cc:358: auto db =
RentACodec::Database();
On 2015/10/27 11:56:43, the sun wrote:
> const auto?

Sure, it might be worth it even though it's easy to see that we don't assign to
db below. (Assignment to db is the only thing const protects against; mutation
of the pointed-to array is regulated by the ArrayView's element type.)

[the sun, 2015-10-27 12:27:40]

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_codec_database.cc:356: int
ACMCodecDB::CodecFreq(int codec_id) {
On 2015/10/27 12:16:46, kwiberg-webrtc wrote:
> On 2015/10/27 11:56:43, the sun wrote:
> > const?
> 
> Where?

On the method

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_codec_database.cc:358: auto db =
RentACodec::Database();
On 2015/10/27 12:16:46, kwiberg-webrtc wrote:
> On 2015/10/27 11:56:43, the sun wrote:
> > const auto?
> 
> Sure, it might be worth it even though it's easy to see that we don't assign
to
> db below. (Assignment to db is the only thing const protects against; mutation
> of the pointed-to array is regulated by the ArrayView's element type.)

Good. Thing is though, when I read this I don't see that the type is ArrayView.
std::vector would be a more common assumption when reading C++.

[kwiberg-webrtc, 2015-10-27 12:33:18]

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_codec_database.cc:356: int
ACMCodecDB::CodecFreq(int codec_id) {
On 2015/10/27 12:27:40, the sun wrote:
> On 2015/10/27 12:16:46, kwiberg-webrtc wrote:
> > On 2015/10/27 11:56:43, the sun wrote:
> > > const?
> > 
> > Where?
> 
> On the method

It's static.

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_codec_database.cc:358: auto db =
RentACodec::Database();
On 2015/10/27 12:27:40, the sun wrote:
> On 2015/10/27 12:16:46, kwiberg-webrtc wrote:
> > On 2015/10/27 11:56:43, the sun wrote:
> > > const auto?
> > 
> > Sure, it might be worth it even though it's easy to see that we don't assign
> to
> > db below. (Assignment to db is the only thing const protects against;
mutation
> > of the pointed-to array is regulated by the ArrayView's element type.)
> 
> Good. Thing is though, when I read this I don't see that the type is
ArrayView.
> std::vector would be a more common assumption when reading C++.

Ah, good point.

[hlundin-webrtc, 2015-10-27 13:52:02]

Mostly lg, but see comments.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:118: void
AddSetOfCodecs(const RentACodec::CodecId(&ids)[N]) {
Space between RentACodec::CodecId and (&ids)?

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:193:
CodecInst ref_codec2 = codec1.inst;
Drop ref_ on this one too?

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module.cc:57: CodecInst*
codec,
Order of parameters is wrong for this method. |codec| should be last. But you
decide whether to fix or just let it die later on.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:511: uint8_t
pl_type = static_cast<uint8_t>(db[i].pltype);
const, or maybe move to the line where it is used and skip the intermediate
variable?

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:512: int fs =
db[i].plfreq;
Same again.

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:564:
RTC_CHECK(codec_index);
Add << *codec_id or something similar, so that the failing CHECK message also
contains a reference to the id.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc:19: namespace {}  //
namespace
WAT?

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/rent_a_codec.h (right):

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.h:103: static inline
rtc::Maybe<int> CodecIndexFromId(CodecId codec_id) {
I wouldn't mind an empty line between each method here.

[the sun, 2015-10-27 14:07:07]

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/rent_a_codec.h (right):

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.h:116: static
rtc::Maybe<CodecInst> CodecInstByParams(const char* payload_name,
Sorry for the naïve question, but... Isn't this the only function needed (pretty
much)? When do we actually want to use the above list of CodecId:s?

[kwiberg-webrtc, 2015-10-27 14:43:23]

New patch set posted.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:118: void
AddSetOfCodecs(const RentACodec::CodecId(&ids)[N]) {
On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> Space between RentACodec::CodecId and (&ids)?

clang-format removes it again when I run it, so unless you insist, I'm inclined
to leave it out.

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:193:
CodecInst ref_codec2 = codec1.inst;
On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> Drop ref_ on this one too?

Done.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module.cc:57: CodecInst*
codec,
On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> Order of parameters is wrong for this method. |codec| should be last. But you
> decide whether to fix or just let it die later on.

This method is going away, so I decided to not change its interface right now.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:511: uint8_t
pl_type = static_cast<uint8_t>(db[i].pltype);
On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> const, or maybe move to the line where it is used and skip the intermediate
> variable?

Done.

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:512: int fs =
db[i].plfreq;
On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> Same again.

Done.

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:564:
RTC_CHECK(codec_index);
On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> Add << *codec_id or something similar, so that the failing CHECK message also
> contains a reference to the id.

Done.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc:19: namespace {}  //
namespace
On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> WAT?

What, have you never seen an empty anonymous namespace before?

It's just like a box of chocolates long after you've eaten the chocolates and
thrown away the empty box, leaving only the distant memory of containment.

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/rent_a_codec.h (right):

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.h:103: static inline
rtc::Maybe<int> CodecIndexFromId(CodecId codec_id) {
On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> I wouldn't mind an empty line between each method here.

Done.

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.h:116: static
rtc::Maybe<CodecInst> CodecInstByParams(const char* payload_name,
On 2015/10/27 14:07:07, the sun wrote:
> Sorry for the naïve question, but... Isn't this the only function needed
(pretty
> much)? When do we actually want to use the above list of CodecId:s?

They're all needed at the moment, because old callers that I haven't changed.
The plan is that the Rent-A-Codec will swallow the codec creation code that
calls all of these, at which point they'll disappear inside its event horizon,
so that no one outside the Rent-A-Codec will see them. Further
Rent-A-Codec-internal refactoring can then take place.

[the sun, 2015-10-27 15:00:12]

solenberg@webrtc.org changed reviewers:
- solenberg@webrtc.org

[hlundin-webrtc, 2015-10-27 15:31:27]

lgtm

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:118: void
AddSetOfCodecs(const RentACodec::CodecId(&ids)[N]) {
On 2015/10/27 14:43:23, kwiberg-webrtc wrote:
> On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> > Space between RentACodec::CodecId and (&ids)?
> 
> clang-format removes it again when I run it, so unless you insist, I'm
inclined
> to leave it out.

Acknowledged.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/audio_coding_module.cc:57: CodecInst*
codec,
On 2015/10/27 14:43:23, kwiberg-webrtc wrote:
> On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> > Order of parameters is wrong for this method. |codec| should be last. But
you
> > decide whether to fix or just let it die later on.
> 
> This method is going away, so I decided to not change its interface right now.

Acknowledged.

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

https://codereview.webrtc.org/1412683006/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/rent_a_codec.cc:19: namespace {}  //
namespace
On 2015/10/27 14:43:23, kwiberg-webrtc wrote:
> On 2015/10/27 13:52:02, hlundin-webrtc wrote:
> > WAT?
> 
> What, have you never seen an empty anonymous namespace before?
> 
> It's just like a box of chocolates long after you've eaten the chocolates and
> thrown away the empty box, leaving only the distant memory of containment.

Yes, but very time you enter this file, you will leave a new distant memory of
containment, leading to a memory leak. We cannot have that.

[kwiberg-webrtc, 2015-10-27 18:37:35]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2015-10-27 18:37:40]

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

[commit-bot: I haz the power, 2015-10-27 18:40:27]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-10-27 18:40:38]

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