Opus implementation of the AudioEncoderFactoryTemplate API

Opus implementation of the AudioEncoderFactoryTemplate API

Now the templated AudioEncoderFactory can create Opus encoders!

BUG=webrtc:7831
Review-Url: https://codereview.webrtc.org/2930243003
Cr-Commit-Position: refs/heads/master@{#18645}
Committed: https://chromium.googlesource.com/external/webrtc/+/fe1aa82c63380c2f79c60cbe1f50ed13144a093f

[kwiberg-webrtc, 2017-06-12 08:49:52]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 08:49:57]

Dry run: 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-12 09:16:37]

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

[commit-bot: I haz the power, 2017-06-12 09:16:37]

Dry run: Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/24855)

[kwiberg-webrtc, 2017-06-12 11:51:43]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 11:51:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-06-12 11:53:20]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-06-12 12:20:22]

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

[commit-bot: I haz the power, 2017-06-12 12:20:23]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-06-12 12:24:04]

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

[kwiberg-webrtc, 2017-06-13 11:42:41]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 11:42:47]

Dry run: 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-13 11:44:36]

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

[commit-bot: I haz the power, 2017-06-13 11:44:36]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26412)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18095)

[kwiberg-webrtc, 2017-06-13 12:10:49]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 12:10:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-06-13 12:16:40]

New patch set. As solenberg@ suggsted in the G722 CL, I moved the Opus files in
api/ to a subdir, renamed AudioEncoderOpus to AudioEncoderOpusImpl, and renamed
AudioEncoderOpusApi to AudioEncoderOpus.

[commit-bot: I haz the power, 2017-06-13 12:38:24]

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

[commit-bot: I haz the power, 2017-06-13 12:38:24]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-06-16 09:29:32]

Description was changed from

==========
Opus implementation of the AudioEncoderFactoryTemplate API

Now the templated AudioEncoderFactory can create Opus encoders!

BUG=webrtc:5806
==========

to

==========
Opus implementation of the AudioEncoderFactoryTemplate API

Now the templated AudioEncoderFactory can create Opus encoders!

BUG=webrtc:7831
==========

[ossu, 2017-06-16 09:53:53]

I'm mostly happy with this. I have a couple of questions, though. I'm willing to
trade a couple of answers for an ell-gee-tee-em. :)

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
File webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:18: // If we are on
Android, iOS and/or ARM, use a lower complexity setting by
Is this comment copied from somewhere? Otherwise, using a lower complexity to
save complexity doesn't really say much. :)
"use a lower complexity setting by default, to save CPU time.", maybe?

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:35: :
frame_size_ms(kDefaultFrameSizeMs),
Would it be preferable to have these directly in the header instead? That way,
if you want to customize the Opus codec by implementing the static interface
yourself, you'd easily see what the defaults were.

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:55: if (frame_size_ms
<= 0 || frame_size_ms % 10 != 0)
Is this strictly true, though? Eh, nvm: is this how it's implemented right now?
:)

According to RTC 7587: "Possible values are 3, 5, 10, 20, 40, 60, or an
arbitrary
      multiple of an Opus frame size rounded up to the next full integer
      value, up to a maximum value of 120, as defined in Section 4."
Not sure we could reasonably deal with < 10ms, though, since that's the block
size used internally.

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
File webrtc/api/audio_codecs/opus/audio_encoder_opus_config.h (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.h:21: struct
AudioEncoderOpusConfig {
Why is AudioEncoderOpusConfig separate from AudioEncoderOpus? They're even in
separate build targets.
It would be neat to have AudioEncoderOpus::Config, instead.

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:500:
std::min(AudioEncoderOpusConfig::kMaxBitrateBps,
A candidate for SafeClamp here. Perhaps not in this CL, tho.

[the sun, 2017-06-16 12:44:11]

lgtm

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
File webrtc/api/audio_codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus.cc:29: "opus", 48000, 2,
{{"minptime", "10"}, {"useinbandfec", "1"}}};
stereo?

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
File webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:35: :
frame_size_ms(kDefaultFrameSizeMs),
On 2017/06/16 09:53:53, ossu wrote:
> Would it be preferable to have these directly in the header instead? That way,
> if you want to customize the Opus codec by implementing the static interface
> yourself, you'd easily see what the defaults were.

+1

[kwiberg-webrtc, 2017-06-16 12:44:14]

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
File webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:18: // If we are on
Android, iOS and/or ARM, use a lower complexity setting by
On 2017/06/16 09:53:53, ossu wrote:
> Is this comment copied from somewhere? Otherwise, using a lower complexity to
> save complexity doesn't really say much. :)
> "use a lower complexity setting by default, to save CPU time.", maybe?

Yes, it's a copy. :-) And yes, it's a tautology---"complexity" just means the
amount of computing to be done.

Fixed.

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:35: :
frame_size_ms(kDefaultFrameSizeMs),
On 2017/06/16 09:53:53, ossu wrote:
> Would it be preferable to have these directly in the header instead? That way,
> if you want to customize the Opus codec by implementing the static interface
> yourself, you'd easily see what the defaults were.

The defaults used to be in the header, but I moved them here to reduce clutter
(this is a problem especially for the #ifdeffed stuff). I figured that anyone
who wants to see the defaults can just look here.

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:55: if (frame_size_ms
<= 0 || frame_size_ms % 10 != 0)
On 2017/06/16 09:53:53, ossu wrote:
> Is this strictly true, though? Eh, nvm: is this how it's implemented right
now?
> :)
> 
> According to RTC 7587: "Possible values are 3, 5, 10, 20, 40, 60, or an
> arbitrary
>       multiple of an Opus frame size rounded up to the next full integer
>       value, up to a maximum value of 120, as defined in Section 4."
> Not sure we could reasonably deal with < 10ms, though, since that's the block
> size used internally.

Supporting < 10 ms frames would be difficult, yes.

I have a feeling that you're right, and this condition is a bit too permissive.
However, I've just copied it from the old Opus AudioEncoder config struct;
changing it should probably be a separate CL.

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
File webrtc/api/audio_codecs/opus/audio_encoder_opus_config.h (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.h:21: struct
AudioEncoderOpusConfig {
On 2017/06/16 09:53:53, ossu wrote:
> Why is AudioEncoderOpusConfig separate from AudioEncoderOpus? They're even in
> separate build targets.
> It would be neat to have AudioEncoderOpus::Config, instead.

They need to be in separate build targets, because AudioEncoderOpus depends on
AudioEncoderOpusImpl, and both of them depend on AudioEncoderOpusConfig---so if
I put AudioEncoderOpus and AudioEncoderOpusConfig in the same build target, we
have a dependency cycle.

I could break that by not letting AudioEncoderOpusImpl use this config struct,
but then it needs another config struct that'll basically be a copy of this one.
I didn't want to do that.

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:500:
std::min(AudioEncoderOpusConfig::kMaxBitrateBps,
On 2017/06/16 09:53:53, ossu wrote:
> A candidate for SafeClamp here. Perhaps not in this CL, tho.

Yes, this part of the CL is just search+replace followed by clang-format. Best
not to leave human fingerprints here.

[ossu, 2017-06-16 13:18:27]

Great! LGTM!

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
File webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:35: :
frame_size_ms(kDefaultFrameSizeMs),
On 2017/06/16 12:44:14, kwiberg-webrtc wrote:
> On 2017/06/16 09:53:53, ossu wrote:
> > Would it be preferable to have these directly in the header instead? That
way,
> > if you want to customize the Opus codec by implementing the static interface
> > yourself, you'd easily see what the defaults were.
> 
> The defaults used to be in the header, but I moved them here to reduce clutter
> (this is a problem especially for the #ifdeffed stuff). I figured that anyone
> who wants to see the defaults can just look here.

Alright. I guess if you're going to modify the parameters to an encoder, looking
in the .cc file shouldn't be too much of a hindrance. :)

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc:55: if (frame_size_ms
<= 0 || frame_size_ms % 10 != 0)
On 2017/06/16 12:44:14, kwiberg-webrtc wrote:
> On 2017/06/16 09:53:53, ossu wrote:
> > Is this strictly true, though? Eh, nvm: is this how it's implemented right
> now?
> > :)
> > 
> > According to RTC 7587: "Possible values are 3, 5, 10, 20, 40, 60, or an
> > arbitrary
> >       multiple of an Opus frame size rounded up to the next full integer
> >       value, up to a maximum value of 120, as defined in Section 4."
> > Not sure we could reasonably deal with < 10ms, though, since that's the
block
> > size used internally.
> 
> Supporting < 10 ms frames would be difficult, yes.
> 
> I have a feeling that you're right, and this condition is a bit too
permissive.
> However, I've just copied it from the old Opus AudioEncoder config struct;
> changing it should probably be a separate CL.

Agreed.

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
File webrtc/api/audio_codecs/opus/audio_encoder_opus_config.h (right):

https://codereview.webrtc.org/2930243003/diff/80001/webrtc/api/audio_codecs/o...
webrtc/api/audio_codecs/opus/audio_encoder_opus_config.h:21: struct
AudioEncoderOpusConfig {
On 2017/06/16 12:44:14, kwiberg-webrtc wrote:
> On 2017/06/16 09:53:53, ossu wrote:
> > Why is AudioEncoderOpusConfig separate from AudioEncoderOpus? They're even
in
> > separate build targets.
> > It would be neat to have AudioEncoderOpus::Config, instead.
> 
> They need to be in separate build targets, because AudioEncoderOpus depends on
> AudioEncoderOpusImpl, and both of them depend on AudioEncoderOpusConfig---so
if
> I put AudioEncoderOpus and AudioEncoderOpusConfig in the same build target, we
> have a dependency cycle.
> 
> I could break that by not letting AudioEncoderOpusImpl use this config struct,
> but then it needs another config struct that'll basically be a copy of this
one.
> I didn't want to do that.

Ah, right!

[the sun, 2017-06-16 17:53:08]

The CQ bit was checked by solenberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 17:53:12]

Dry run: 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-16 18:30:41]

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

[commit-bot: I haz the power, 2017-06-16 18:30:42]

Dry run: This issue passed the CQ dry run.

[the sun, 2017-06-16 19:16:32]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2017-06-16 19:16:41]

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

[commit-bot: I haz the power, 2017-06-16 19:16:42]

This CL has an open dependency (Issue 2934833002 Patch 140001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[kwiberg-webrtc, 2017-06-18 00:36:44]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-18 00:36:46]

Dry run: 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-18 00:56:30]

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

[commit-bot: I haz the power, 2017-06-18 00:56:31]

Dry run: Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/22253)

[kwiberg-webrtc, 2017-06-18 01:00:07]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2017-06-18 01:00:10]

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-18 01:23:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497747607735870,
"parent_rev": "b8727aebc161e94b818dd1323895fcd674990ad7", "commit_rev":
"fe1aa82c63380c2f79c60cbe1f50ed13144a093f"}

[commit-bot: I haz the power, 2017-06-18 01:23:08]

Description was changed from

==========
Opus implementation of the AudioEncoderFactoryTemplate API

Now the templated AudioEncoderFactory can create Opus encoders!

BUG=webrtc:7831
==========

to

==========
Opus implementation of the AudioEncoderFactoryTemplate API

Now the templated AudioEncoderFactory can create Opus encoders!

BUG=webrtc:7831

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

[commit-bot: I haz the power, 2017-06-18 01:23:09]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/fe1aa82c63380c2f79c60cbe1...

[charujain, 2017-06-18 09:38:50]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.webrtc.org/2947563002/ by charujain@webrtc.org.

The reason for reverting is: Breaking google3 projects.