Declare that rent_a_codec depends on the audio codecs

Declare that rent_a_codec depends on the audio codecs

That these declarations were missing was a bug, which apparently
didn't actually cause build problems in either Chromium or WebRTC
standalone. (Presumably, because rent_a_codec was always linked
together with other build targets that did declare such dependencies.)

BUG=webrtc:5435
Committed: https://crrev.com/0edb05b3446e92aa90b39fb4dc5a8155eb29dd84
Cr-Commit-Position: refs/heads/master@{#11303}

[kwiberg-webrtc, 2016-01-18 13:21:48]

Description was changed from

==========
Declare that rent_a_codec depends on the audio codecs

That these declarations were missing was a bug, which apparently
didn't actually cause build problems in either Chromium or WebRTC
standalone. (Presumably, because rent_a_codec was always linked
together with other build targets that did declare such dependencies.)

BUG=webrtc:5435
==========

to

==========
Declare that rent_a_codec depends on the audio codecs

That these declarations were missing was a bug, which apparently
didn't actually cause build problems in either Chromium or WebRTC
standalone. (Presumably, because rent_a_codec was always linked
together with other build targets that did declare such dependencies.)

BUG=webrtc:5435
==========

[kwiberg-webrtc, 2016-01-18 13:21:48]

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

[kjellander_webrtc, 2016-01-18 14:12:47]

lgtm
you hit a flake on android_dbg, just retry it (or let the CQ do it for you).

[kwiberg-webrtc, 2016-01-18 23:38:09]

Patchset #2 (id:20001) has been deleted

[kwiberg-webrtc, 2016-01-18 23:51:49]

On 2016/01/18 14:12:47, kjellander (webrtc) wrote:
> lgtm
> you hit a flake on android_dbg, just retry it (or let the CQ do it for you).

Sorry, but could you take a look again? I had to do a nontrivial rebase because
of conflicts with my own iLBC CL that just landed, and I'd appreciate a second
sanity check.

[kjellander_webrtc, 2016-01-19 07:51:12]

I'd prefer if you could refactor the GN parts to eliminate the code duplication
in there.

https://codereview.webrtc.org/1607463002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1607463002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:30: defines += [ "WEBRTC_CODEC_ILBC" ]
There's quite some code duplication now. Do you think you could extract the
common defines and deps into one place similar to what you have for GYP?

It would surely reduce the risk for regressions in the future.

[kwiberg-webrtc, 2016-01-19 10:58:39]

https://codereview.webrtc.org/1607463002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1607463002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:30: defines += [ "WEBRTC_CODEC_ILBC" ]
On 2016/01/19 07:51:12, kjellander (webrtc) wrote:
> There's quite some code duplication now. Do you think you could extract the
> common defines and deps into one place similar to what you have for GYP?
> 
> It would surely reduce the risk for regressions in the future.

Better now?

[hlundin-webrtc, 2016-01-19 12:11:34]

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

[hlundin-webrtc, 2016-01-19 12:11:34]

LGTM with one question.

https://codereview.webrtc.org/1607463002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1607463002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:104: deps = audio_codec_deps + [
Do we still need ACM to depend directly on audio_codec_deps? Wouldn't it suffice
to depend on rent_a_codec?

[kwiberg-webrtc, 2016-01-19 12:21:50]

https://codereview.webrtc.org/1607463002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1607463002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:104: deps = audio_codec_deps + [
On 2016/01/19 12:11:34, hlundin-webrtc wrote:
> Do we still need ACM to depend directly on audio_codec_deps? Wouldn't it
suffice
> to depend on rent_a_codec?

I think it still needs it for the decoders, but I'm not sure---maybe that's all
located in neteq, which IIRC has its own deps on the codecs.

My evil master plan is to leave those possibly superfluous deps here for now,
and clean them all out at once when the decoders are also in Rent-A-Codec and
nobody else should need those dependencies.

[hlundin-webrtc, 2016-01-19 12:30:15]

https://codereview.webrtc.org/1607463002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1607463002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:104: deps = audio_codec_deps + [
On 2016/01/19 12:21:50, kwiberg-webrtc wrote:
> On 2016/01/19 12:11:34, hlundin-webrtc wrote:
> > Do we still need ACM to depend directly on audio_codec_deps? Wouldn't it
> suffice
> > to depend on rent_a_codec?
> 
> I think it still needs it for the decoders, but I'm not sure---maybe that's
all
> located in neteq, which IIRC has its own deps on the codecs.
> 
> My evil master plan is to leave those possibly superfluous deps here for now,
> and clean them all out at once when the decoders are also in Rent-A-Codec and
> nobody else should need those dependencies.

Fair enough.

[kjellander_webrtc, 2016-01-19 12:35:22]

lgtm

[kwiberg-webrtc, 2016-01-19 12:48:03]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-01-19 12:48:06]

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

[commit-bot: I haz the power, 2016-01-19 13:54:31]

Description was changed from

==========
Declare that rent_a_codec depends on the audio codecs

That these declarations were missing was a bug, which apparently
didn't actually cause build problems in either Chromium or WebRTC
standalone. (Presumably, because rent_a_codec was always linked
together with other build targets that did declare such dependencies.)

BUG=webrtc:5435
==========

to

==========
Declare that rent_a_codec depends on the audio codecs

That these declarations were missing was a bug, which apparently
didn't actually cause build problems in either Chromium or WebRTC
standalone. (Presumably, because rent_a_codec was always linked
together with other build targets that did declare such dependencies.)

BUG=webrtc:5435
==========

[commit-bot: I haz the power, 2016-01-19 13:54:32]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-01-19 13:54:34]

Description was changed from

==========
Declare that rent_a_codec depends on the audio codecs

That these declarations were missing was a bug, which apparently
didn't actually cause build problems in either Chromium or WebRTC
standalone. (Presumably, because rent_a_codec was always linked
together with other build targets that did declare such dependencies.)

BUG=webrtc:5435
==========

to

==========
Declare that rent_a_codec depends on the audio codecs

That these declarations were missing was a bug, which apparently
didn't actually cause build problems in either Chromium or WebRTC
standalone. (Presumably, because rent_a_codec was always linked
together with other build targets that did declare such dependencies.)

BUG=webrtc:5435

Committed: https://crrev.com/0edb05b3446e92aa90b39fb4dc5a8155eb29dd84
Cr-Commit-Position: refs/heads/master@{#11303}
==========

[commit-bot: I haz the power, 2016-01-19 13:54:35]

Patchset 3 (id:??) landed as
https://crrev.com/0edb05b3446e92aa90b39fb4dc5a8155eb29dd84
Cr-Commit-Position: refs/heads/master@{#11303}