Expose ILBC codec in webrtc/api/audio_codecs/

Expose ILBC codec in webrtc/api/audio_codecs/

BUG=webrtc:7834, webrtc:7840
Review-Url: https://codereview.webrtc.org/2951873002
Cr-Commit-Position: refs/heads/master@{#18803}
Committed: https://chromium.googlesource.com/external/webrtc/+/db3c9b0f72e16a53f9a17af6e8df03b575932b92

[the sun, 2017-06-22 07:59:41]

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

[the sun, 2017-06-22 07:59:42]

FYI, started on this - not ready for review yet.

[the sun, 2017-06-26 21:46:39]

Description was changed from

==========
Expose ILBC codec in webrtc/api/audio_codecs/

BUG=webrtc:7834, webrtc:7840
==========

to

==========
Expose ILBC codec in webrtc/api/audio_codecs/

BUG=webrtc:7834, webrtc:7840
==========

[the sun, 2017-06-26 21:46:39]

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

[the sun, 2017-06-26 21:47:02]

Should be ok to review now.

[kwiberg-webrtc, 2017-06-27 11:09:23]

lgtm

https://codereview.webrtc.org/2951873002/diff/20001/webrtc/api/audio_codecs/i...
File webrtc/api/audio_codecs/ilbc/audio_encoder_ilbc.cc (right):

https://codereview.webrtc.org/2951873002/diff/20001/webrtc/api/audio_codecs/i...
webrtc/api/audio_codecs/ilbc/audio_encoder_ilbc.cc:17: #include
"webrtc/base/safe_conversions.h"
Are you using this one?

[ossu, 2017-06-27 11:28:38]

This looks pretty good but why are the robots so angry?

[the sun, 2017-06-27 12:05:47]

On 2017/06/27 11:28:38, ossu wrote:
> This looks pretty good but why are the robots so angry?

I've seen the same tests fail on the linux32_rel bot for another CL I tried to
land. I think it is unrelated to these changes.

[the sun, 2017-06-27 12:07:19]

https://codereview.webrtc.org/2951873002/diff/20001/webrtc/api/audio_codecs/t...
File webrtc/api/audio_codecs/test/audio_decoder_factory_template_unittest.cc
(right):

https://codereview.webrtc.org/2951873002/diff/20001/webrtc/api/audio_codecs/t...
webrtc/api/audio_codecs/test/audio_decoder_factory_template_unittest.cc:139:
AudioCodecSpec{{"ILBC", 8000, 1}, {8000, 1, 13300}}));
What do you think about bitrate being 13300 here and 13333 in some other places?
IIUC the behavior is in the current code and just copied over here, though
there's the possibility that I've messed it up...

[kwiberg-webrtc, 2017-06-27 12:50:44]

https://codereview.webrtc.org/2951873002/diff/20001/webrtc/api/audio_codecs/t...
File webrtc/api/audio_codecs/test/audio_decoder_factory_template_unittest.cc
(right):

https://codereview.webrtc.org/2951873002/diff/20001/webrtc/api/audio_codecs/t...
webrtc/api/audio_codecs/test/audio_decoder_factory_template_unittest.cc:139:
AudioCodecSpec{{"ILBC", 8000, 1}, {8000, 1, 13300}}));
On 2017/06/27 12:07:19, the sun (OOO until July 24th) wrote:
> What do you think about bitrate being 13300 here and 13333 in some other
places?
> IIUC the behavior is in the current code and just copied over here, though
> there's the possibility that I've messed it up...

I hadn't caught that, actually.

For the decoder, which doesn't know the frame length, this value seems made up.
I suspect it may just be an artifact of us using the same info struct type for
both encoders and decoders.

Anyway, keeping the existing value should be fine for this CL.

[ossu, 2017-06-27 12:53:14]

On 2017/06/27 12:05:47, the sun (OOO until July 24th) wrote:
> On 2017/06/27 11:28:38, ossu wrote:
> > This looks pretty good but why are the robots so angry?
> 
> I've seen the same tests fail on the linux32_rel bot for another CL I tried to
> land. I think it is unrelated to these changes.

Ah, alright. LGTM then!

https://codereview.webrtc.org/2951873002/diff/20001/webrtc/api/audio_codecs/t...
File webrtc/api/audio_codecs/test/audio_decoder_factory_template_unittest.cc
(right):

https://codereview.webrtc.org/2951873002/diff/20001/webrtc/api/audio_codecs/t...
webrtc/api/audio_codecs/test/audio_decoder_factory_template_unittest.cc:139:
AudioCodecSpec{{"ILBC", 8000, 1}, {8000, 1, 13300}}));
On 2017/06/27 12:07:19, the sun (OOO until July 24th) wrote:
> What do you think about bitrate being 13300 here and 13333 in some other
places?
> IIUC the behavior is in the current code and just copied over here, though
> there's the possibility that I've messed it up...

Hmm... acm_codec_database.cc claims that it's 13300 and that's the value used in
most places. I doubt it matters massively, but perhaps that value should be used
everywhere? Though 13333 is the more correct one.

I'd be fine with you leaving this as-is here and changing the values in a
separate CL. That one should be easy to revert in case something really depends
on these exact values.

[the sun, 2017-06-28 08:20:01]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2017-06-28 08:20:08]

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, 2017-06-28 08:35:20]

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

[commit-bot: I haz the power, 2017-06-28 08:35:21]

Try jobs failed on following builders:
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[the sun, 2017-06-28 08:42:23]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-06-28 08:42:23]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org,
kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2951873002/#ps40001 (title:
"fuzzer test updated")

[commit-bot: I haz the power, 2017-06-28 08:42:28]

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, 2017-06-28 09:05:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498639343212530,
"parent_rev": "cd9dd458d0550acd646a2eb5c80130ff061d53cc", "commit_rev":
"db3c9b0f72e16a53f9a17af6e8df03b575932b92"}

[commit-bot: I haz the power, 2017-06-28 09:05:14]

Description was changed from

==========
Expose ILBC codec in webrtc/api/audio_codecs/

BUG=webrtc:7834, webrtc:7840
==========

to

==========
Expose ILBC codec in webrtc/api/audio_codecs/

BUG=webrtc:7834, webrtc:7840

Review-Url: https://codereview.webrtc.org/2951873002
Cr-Commit-Position: refs/heads/master@{#18803}
Committed:
https://chromium.googlesource.com/external/webrtc/+/db3c9b0f72e16a53f9a17af6e...
==========

[commit-bot: I haz the power, 2017-06-28 09:05:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/db3c9b0f72e16a53f9a17af6e...