AudioCodingModule: Specify decoders using SdpAudioFormat

AudioCodingModule: Specify decoders using SdpAudioFormat

NetEq already uses SdpAudioFormat internally; this CL adds an
AudioCodingModule::RegisterReceiveCodec overload that accepts
SdpAudioFormat, and propagates it through AcmReceiver into NetEq.

The intention is to get rid of the other ways to specify decoders and
always use SdpAudioFormat. (And eventually to do the same for encoders
too.)

NOTRY=true
BUG=5801
Committed: https://crrev.com/5adaf735dc8caba2cd082b61bc0760ce9f0450f9
Cr-Commit-Position: refs/heads/master@{#14506}

[kwiberg-webrtc, 2016-09-28 12:24:23]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-09-28 12:29:10]

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

[ossu, 2016-09-28 14:12:28]

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/acm_receive_test_oldapi.cc (left):

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/acm_receive_test_oldapi.cc:108:
acm_(webrtc::AudioCodingModule::Create(0, &clock_)),
Hooray! Can we now start deprecating the old AudioCodingModule::Create
overloads?

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

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:1077: << " for external
decoder.";
Why distinguish "external decoder" here?

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

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/include/audio_coding_module.h:514: // Registers a
decoder for the given payload type. Returns true iff
Place this one directly under the other RegisterReceiveCodec.

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

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database.cc:148: if (rtp_payload_type
> 0x7f) {
Since rtp_payload_type is an int in this overload, you should probably check for
< 0 as well.

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:288: case DecoderDatabase::kOK:
Ugh! I really dislike this construct (though it's not like you introduced it).
Why do we have separate sets of error codes where one set (the one from
DecoderDatabase in this case) is a subset of the other (the one from NetEq)? Why
doesn't DecoderDatabase just return one of the NetEq error codes? To top it all
off: why do these methods return int rather than an appropriate enum?

[kwiberg-webrtc, 2016-09-29 12:38:35]

New patch set posted.

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/acm_receive_test_oldapi.cc (left):

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/acm_receive_test_oldapi.cc:108:
acm_(webrtc::AudioCodingModule::Create(0, &clock_)),
On 2016/09/28 14:12:28, ossu wrote:
> Hooray! Can we now start deprecating the old AudioCodingModule::Create
> overloads?

Yes, I think so.

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

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:1077: << " for external
decoder.";
On 2016/09/28 14:12:28, ossu wrote:
> Why distinguish "external decoder" here?

Because I cribbed this code from RegisterExternalReceiveCodec?

Fixed.

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

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/include/audio_coding_module.h:514: // Registers a
decoder for the given payload type. Returns true iff
On 2016/09/28 14:12:28, ossu wrote:
> Place this one directly under the other RegisterReceiveCodec.

Done.

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

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database.cc:148: if (rtp_payload_type
> 0x7f) {
On 2016/09/28 14:12:28, ossu wrote:
> Since rtp_payload_type is an int in this overload, you should probably check
for
> < 0 as well.

Done.

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:288: case DecoderDatabase::kOK:
On 2016/09/28 14:12:28, ossu wrote:
> Ugh! I really dislike this construct (though it's not like you introduced it).
> Why do we have separate sets of error codes where one set (the one from
> DecoderDatabase in this case) is a subset of the other (the one from NetEq)?
Why
> doesn't DecoderDatabase just return one of the NetEq error codes? To top it
all
> off: why do these methods return int rather than an appropriate enum?

Acknowledged.

[ossu, 2016-10-03 09:16:00]

lgtm, go, go, go!

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2365653004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:288: case DecoderDatabase::kOK:
On 2016/09/29 12:38:35, kwiberg-webrtc wrote:
> On 2016/09/28 14:12:28, ossu wrote:
> > Ugh! I really dislike this construct (though it's not like you introduced
it).
> > Why do we have separate sets of error codes where one set (the one from
> > DecoderDatabase in this case) is a subset of the other (the one from NetEq)?
> Why
> > doesn't DecoderDatabase just return one of the NetEq error codes? To top it
> all
> > off: why do these methods return int rather than an appropriate enum?
> 
> Acknowledged.

Good! :)

[kwiberg-webrtc, 2016-10-03 09:33:02]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-03 09:33:12]

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-10-03 09:34:21]

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

[commit-bot: I haz the power, 2016-10-03 09:34:22]

Try jobs failed on following builders:
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/1087)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/11178)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/13480)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/18732)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/17404)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[kwiberg-webrtc, 2016-10-04 16:31:33]

Description was changed from

==========
AudioCodingModule: Specify decoders using SdpAudioFormat

NetEq already uses SdpAudioFormat internally; this CL adds an
AudioCodingModule::RegisterReceiveCodec overload that accepts
SdpAudioFormat, and propagates it through AcmReceiver into NetEq.

The intention is to get rid of the other ways to specify decoders and
always use SdpAudioFormat. (And eventually to do the same for encoders
too.)

BUG=5801
==========

to

==========
AudioCodingModule: Specify decoders using SdpAudioFormat

NetEq already uses SdpAudioFormat internally; this CL adds an
AudioCodingModule::RegisterReceiveCodec overload that accepts
SdpAudioFormat, and propagates it through AcmReceiver into NetEq.

The intention is to get rid of the other ways to specify decoders and
always use SdpAudioFormat. (And eventually to do the same for encoders
too.)

NOTRY=true
BUG=5801
==========

[kwiberg-webrtc, 2016-10-04 16:31:45]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-10-04 16:31:46]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2365653004/#ps100001 (title:
"rebase")

[commit-bot: I haz the power, 2016-10-04 16:31:52]

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-10-04 16:33:30]

Description was changed from

==========
AudioCodingModule: Specify decoders using SdpAudioFormat

NetEq already uses SdpAudioFormat internally; this CL adds an
AudioCodingModule::RegisterReceiveCodec overload that accepts
SdpAudioFormat, and propagates it through AcmReceiver into NetEq.

The intention is to get rid of the other ways to specify decoders and
always use SdpAudioFormat. (And eventually to do the same for encoders
too.)

NOTRY=true
BUG=5801
==========

to

==========
AudioCodingModule: Specify decoders using SdpAudioFormat

NetEq already uses SdpAudioFormat internally; this CL adds an
AudioCodingModule::RegisterReceiveCodec overload that accepts
SdpAudioFormat, and propagates it through AcmReceiver into NetEq.

The intention is to get rid of the other ways to specify decoders and
always use SdpAudioFormat. (And eventually to do the same for encoders
too.)

NOTRY=true
BUG=5801
==========

[commit-bot: I haz the power, 2016-10-04 16:33:31]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-10-04 16:33:40]

Description was changed from

==========
AudioCodingModule: Specify decoders using SdpAudioFormat

NetEq already uses SdpAudioFormat internally; this CL adds an
AudioCodingModule::RegisterReceiveCodec overload that accepts
SdpAudioFormat, and propagates it through AcmReceiver into NetEq.

The intention is to get rid of the other ways to specify decoders and
always use SdpAudioFormat. (And eventually to do the same for encoders
too.)

NOTRY=true
BUG=5801
==========

to

==========
AudioCodingModule: Specify decoders using SdpAudioFormat

NetEq already uses SdpAudioFormat internally; this CL adds an
AudioCodingModule::RegisterReceiveCodec overload that accepts
SdpAudioFormat, and propagates it through AcmReceiver into NetEq.

The intention is to get rid of the other ways to specify decoders and
always use SdpAudioFormat. (And eventually to do the same for encoders
too.)

NOTRY=true
BUG=5801

Committed: https://crrev.com/5adaf735dc8caba2cd082b61bc0760ce9f0450f9
Cr-Commit-Position: refs/heads/master@{#14506}
==========

[commit-bot: I haz the power, 2016-10-04 16:33:41]

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

[hlundin-webrtc, 2016-10-05 13:39:15]

LGTM, but with a question and a nit.

https://codereview.webrtc.org/2365653004/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2365653004/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:993: bool
AudioCodingModuleImpl::RegisterReceiveCodec(
Why do you not have to handle the "iSAC case" like we do in the other
RegisterReceiveCodec methods?

https://codereview.webrtc.org/2365653004/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/2365653004/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:226: constexpr int
rtp_payload_type = 0;
Compile-time constant => kRtpPayloadType.

[kwiberg-webrtc, 2016-10-06 09:20:47]

https://codereview.webrtc.org/2365653004/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2365653004/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:993: bool
AudioCodingModuleImpl::RegisterReceiveCodec(
On 2016/10/05 13:39:14, hlundin-webrtc wrote:
> Why do you not have to handle the "iSAC case" like we do in the other
> RegisterReceiveCodec methods?

In principle: Because the user has previously injected (or had the option of
injecting) the factory that'll be used to instantiate the decoders. That factory
can take care of things like coupling the state of iSAC decoders with iSAC
encoders.

In practice: Because we never used the same ACM for sending and receiving, the
encoder and decoder were never coupled anyway.

https://codereview.webrtc.org/2365653004/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/2365653004/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:226: constexpr int
rtp_payload_type = 0;
On 2016/10/05 13:39:14, hlundin-webrtc wrote:
> Compile-time constant => kRtpPayloadType.

That naming rule is mandatory only for variables with static storage duration.
For variables with automatic storage duration, it's optional (and since I don't
particularly like how names look in that style, I chose not to use it).

Speculation: The reason to enforce a special style for static but not automatic
constants is probably that the latter are typically used far from where they're
declared, whereas the latter are used much more locally. In this case, the
variable goes out of scope four lines after it's declared, so it isn't hard to
see what's going on even without the kNamingStyle.