Stop using old AudioCodingModule::RegisterReceiveCodec overloads

Stop using old AudioCodingModule::RegisterReceiveCodec overloads

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

[kwiberg-webrtc, 2016-10-05 12:56:23]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-10-05 12:56:33]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2016-10-05 13:03:44]

kwiberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

[kwiberg-webrtc, 2016-10-05 13:03:45]

This obviously still needs some bugfixing, but it may be useful to start
reviewing it now in case you want me to change something large.

[ossu, 2016-10-05 13:39:59]

I've left some comments below. I bet you've thought of some of these already.

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_format.h (right):

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_format.h:60: SdpAudioFormat
CodecInstToSdp(const CodecInst& codec_inst);
You can have this, but not here. CodecInst is decidedly less public than
SdpAudioFormat, so maybe put it in common_types.h?
Do you need it for anything other than tests? If not, just shove it into
something related to the tests instead.

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/test/APITest.cc (right):

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/test/APITest.cc:144: EXPECT_EQ(true,
_acmB->RegisterReceiveCodec(dummyCodec.pltype,
Come to think of it, would it make sense to have the old RegisterReceiveCodec
just delegate to the new one (sort-of like this) and keep the tests using the
old call for now? I bet we still want to test that the old call works, since
that's what is currently used.

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/test/delay_test.cc (right):

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/test/delay_test.cc:113: << "Couldn't register
receive codec.\n";
Completely unrelated: It can't be the case that this call is the only one in the
world that requires a newline at the end of it. :)

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:476: if
(!audio_coding_->RegisterReceiveCodec(receiveCodec.pltype,
Don't make a CodecInst just to convert it to an SdpAudioFormat.
I don't believe pacsize is at all used further on, so the call to
audio_coding_->Codec should be removed as well.

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:993: const uint8_t nSupportedCodecs =
AudioCodingModule::NumberOfCodecs();
How should this be done now that there are two different sets of codecs? One
from a factory and one from AudioCodingModule? Do will still need to support
this default setup of registering everything? Oh, and I guess this is for both
receiving and sending?

[kwiberg-webrtc, 2016-10-06 12:14:53]

No new patch set posted yet.

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_format.h (right):

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_format.h:60: SdpAudioFormat
CodecInstToSdp(const CodecInst& codec_inst);
On 2016/10/05 13:39:58, ossu wrote:
> You can have this, but not here. CodecInst is decidedly less public than
> SdpAudioFormat, so maybe put it in common_types.h?
> Do you need it for anything other than tests? If not, just shove it into
> something related to the tests instead.

OK, makes sense to put the legacy->current converter with the legacy type and
not the current type. I'll try that.

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/test/APITest.cc (right):

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/test/APITest.cc:144: EXPECT_EQ(true,
_acmB->RegisterReceiveCodec(dummyCodec.pltype,
On 2016/10/05 13:39:58, ossu wrote:
> Come to think of it, would it make sense to have the old RegisterReceiveCodec
> just delegate to the new one (sort-of like this) and keep the tests using the
> old call for now? I bet we still want to test that the old call works, since
> that's what is currently used.

That would make sense if we were planning to keep the old methods. But I'm
hoping to remove them rather soon: https://codereview.webrtc.org/2392743004/

I'm hoping that the old methods can survive without being tested for the ~three
weeks it'll take to get the removal landed. But then I'm an optimist...

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/test/delay_test.cc (right):

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/test/delay_test.cc:113: << "Couldn't register
receive codec.\n";
On 2016/10/05 13:39:58, ossu wrote:
> Completely unrelated: It can't be the case that this call is the only one in
the
> world that requires a newline at the end of it. :)

Probably not (although I'd be surprised if no other ASSERT message had a
trailing newline...). Removed the message altogether.

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:476: if
(!audio_coding_->RegisterReceiveCodec(receiveCodec.pltype,
On 2016/10/05 13:39:59, ossu wrote:
> Don't make a CodecInst just to convert it to an SdpAudioFormat.
> I don't believe pacsize is at all used further on, so the call to
> audio_coding_->Codec should be removed as well.

The problem is that since the caller has given us the sample rate and the number
of channels, I need exactly the logic that's in CodecInstToSdp in order to fix
G722's clock rate and Opus's stereo parameter. The real solution is (probably)
to change the arguments so that the caller gives us an SdpAudioFormat instead,
but I wanted to do that in a separate CL. This one deals exclusively with
removing all callers of the old ACM::RegisterReceiveCodec methods.

https://codereview.webrtc.org/2388153004/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:993: const uint8_t nSupportedCodecs =
AudioCodingModule::NumberOfCodecs();
On 2016/10/05 13:39:58, ossu wrote:
> How should this be done now that there are two different sets of codecs? One
> from a factory and one from AudioCodingModule? Do will still need to support
> this default setup of registering everything? Oh, and I guess this is for both
> receiving and sending?

My thinking is that tests and stuff that want to iterate over "all" codecs
should ask the factory/-ies for the list of supported codecs (which is a thing
we intend to continue to support). The ACM methods that list codecs should be
removed when we no longer need them.

[kwiberg-webrtc, 2016-10-07 11:49:10]

New patch set(s) posted. I recommend that you look at the #2 -> #5 diff, so that
you don't have to see the rebase.

[kwiberg-webrtc, 2016-10-07 12:13:11]

https://codereview.webrtc.org/2388153004/diff/120001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/audio_coding.gypi (right):

https://codereview.webrtc.org/2388153004/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_coding.gypi:91:
'codecs/audio_format_conversion.h',
I'm cheating and just sticking the audio_format_conversion files here instead of
making separate build targets like for GN. I'm counting on the GYP files finally
being removed before this comes back to bite us.

[kwiberg-webrtc, 2016-10-21 09:22:58]

New rebase uploaded, since it's been a while, but it contains nothing
interesting. I still recommend that you look at the #2 -> #5 diff.

[ossu, 2016-10-21 12:24:05]

Ow, sorry! I think I thought there was more to be done here, or I plumb forgot
about it when going abroad. I'll give you an LGTM to bring home over the
week-end, alright? :)

https://codereview.chromium.org/2388153004/diff/40001/webrtc/voice_engine/cha...
File webrtc/voice_engine/channel.cc (right):

https://codereview.chromium.org/2388153004/diff/40001/webrtc/voice_engine/cha...
webrtc/voice_engine/channel.cc:993: const uint8_t nSupportedCodecs =
AudioCodingModule::NumberOfCodecs();
On 2016/10/06 12:14:52, kwiberg-webrtc wrote:
> On 2016/10/05 13:39:58, ossu wrote:
> > How should this be done now that there are two different sets of codecs? One
> > from a factory and one from AudioCodingModule? Do will still need to support
> > this default setup of registering everything? Oh, and I guess this is for
both
> > receiving and sending?
> 
> My thinking is that tests and stuff that want to iterate over "all" codecs
> should ask the factory/-ies for the list of supported codecs (which is a thing
> we intend to continue to support). The ACM methods that list codecs should be
> removed when we no longer need them.

Acknowledged.

[kwiberg-webrtc, 2016-10-21 12:54:17]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-21 12:54:24]

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-21 14:54:59]

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

[commit-bot: I haz the power, 2016-10-21 14:55:00]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/14110)
  presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-10-24 09:20:08]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-24 09:20:15]

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-24 11:20:43]

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

[commit-bot: I haz the power, 2016-10-24 11:20:44]

Try jobs failed on following builders:
  android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-10-24 13:08:15]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-24 13:08:20]

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-24 15:08:49]

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

[commit-bot: I haz the power, 2016-10-24 15:08:53]

Try jobs failed on following builders:
  linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-10-24 20:43:57]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-10-24 20:44:07]

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-24 20:47:12]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-10-24 20:47:19]

Description was changed from

==========
Stop using old AudioCodingModule::RegisterReceiveCodec overloads

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

to

==========
Stop using old AudioCodingModule::RegisterReceiveCodec overloads

BUG=webrtc:5801

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

[commit-bot: I haz the power, 2016-10-24 20:47:20]

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

[kjellander_webrtc, 2016-11-02 06:37:52]

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

[kjellander_webrtc, 2016-11-02 06:37:53]

https://codereview.webrtc.org/2388153004/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/audio_coding.gypi (right):

https://codereview.webrtc.org/2388153004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_coding.gypi:90:
'codecs/audio_format_conversion.cc',
Can we make the same changes here as in the GN build please? It's causing
problems switching from GYP to GN for internal projects.

[kwiberg-webrtc, 2016-11-02 10:25:27]

https://codereview.webrtc.org/2388153004/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/audio_coding.gypi (right):

https://codereview.webrtc.org/2388153004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_coding.gypi:90:
'codecs/audio_format_conversion.cc',
On 2016/11/02 06:37:53, kjellander_webrtc wrote:
> Can we make the same changes here as in the GN build please? It's causing
> problems switching from GYP to GN for internal projects.

This won't be needed if https://codereview.webrtc.org/2472643003/ lands, right?

[kjellander_webrtc, 2016-11-02 10:28:11]

On 2016/11/02 10:25:27, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2388153004/diff/140001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/audio_coding.gypi (right):
> 
>
https://codereview.webrtc.org/2388153004/diff/140001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/audio_coding.gypi:90:
> 'codecs/audio_format_conversion.cc',
> On 2016/11/02 06:37:53, kjellander_webrtc wrote:
> > Can we make the same changes here as in the GN build please? It's causing
> > problems switching from GYP to GN for internal projects.
> 
> This won't be needed if https://codereview.webrtc.org/2472643003/ lands,
right?

Right, I think that'll solve the build problem (but it remains to be tested to
be sure).