Support desktop media pipeline on Android for GN build.

Support desktop media pipeline on Android for GN build.

This CL ports GYP changes to GN from
https://codereview.chromium.org/1230793009/

BUG=542425
Committed: https://crrev.com/6f2d2f6ffa532fe810d5475556883bcbfb5c4e92
Cr-Commit-Position: refs/heads/master@{#354348}

[Tima Vaisburd, 2015-10-14 18:33:46]

timav@chromium.org changed reviewers:
+ xhwang@chromium.org

[Tima Vaisburd, 2015-10-14 21:46:05]

timav@chromium.org changed reviewers:
+ watk@chromium.org

[Tima Vaisburd, 2015-10-14 21:46:05]

Chris, could you, please, take a look?

[watk, 2015-10-14 21:51:43]

sweet

https://codereview.chromium.org/1405783002/diff/1/media/blink/BUILD.gn
File media/blink/BUILD.gn (right):

https://codereview.chromium.org/1405783002/diff/1/media/blink/BUILD.gn#newcode6
media/blink/BUILD.gn:6: import("//media/media_options.gni")
Order alphabetically?

https://codereview.chromium.org/1405783002/diff/1/media/blink/BUILD.gn#newcode79
media/blink/BUILD.gn:79: if (is_android && !media_use_ffmpeg) {
I think this would be more idiomatic gn (and clearer imo) to remove these
sources from the above source list, and conditionally add them with:

if (media_use_ffmpeg) {
  sources += [
     ...
  ]
}

[Tima Vaisburd, 2015-10-14 22:08:31]

https://codereview.chromium.org/1405783002/diff/1/media/blink/BUILD.gn
File media/blink/BUILD.gn (right):

https://codereview.chromium.org/1405783002/diff/1/media/blink/BUILD.gn#newcode6
media/blink/BUILD.gn:6: import("//media/media_options.gni")
On 2015/10/14 21:51:42, watk wrote:
> Order alphabetically?

Done.

https://codereview.chromium.org/1405783002/diff/1/media/blink/BUILD.gn#newcode79
media/blink/BUILD.gn:79: if (is_android && !media_use_ffmpeg) {
On 2015/10/14 21:51:42, watk wrote:
> I think this would be more idiomatic gn (and clearer imo) to remove these
> sources from the above source list, and conditionally add them with:
> 
> if (media_use_ffmpeg) {
>   sources += [
>      ...
>   ]
> }

Done.

[watk, 2015-10-14 22:17:12]

lgtm

[Tima Vaisburd, 2015-10-14 22:21:06]

timav@chromium.org changed reviewers:
+ henrika@chromium.org

[Tima Vaisburd, 2015-10-14 22:21:06]

Henrik,

Could you, please, review the opus related part?

[xhwang, 2015-10-14 22:33:37]

lgtm. Thanks for putting these together so quickly!

[henrika (OOO until Aug 14), 2015-10-15 07:47:24]

henrika@chromium.org changed reviewers:
+ tlegrand@chromium.org

[henrika (OOO until Aug 14), 2015-10-15 07:49:11]

LGTM but I don't know the Opus details well enough; hence adding Tina as well.
Please wait for OK from her as well. Thanks.

[minyue, 2015-10-15 08:20:24]

minyue@chromium.org changed reviewers:
+ minyue@chromium.org

[minyue, 2015-10-15 08:20:24]

I am not expert in GN but I'd like to know if this GN change is equivalent to
the corresponding GYP change in the given link.

https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD.gn
File third_party/opus/BUILD.gn (right):

https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD....
third_party/opus/BUILD.gn:24: defines += [ "OPUS_FIXED_POINT" ]
The corresponding switch in gyp is configured this way:

https://codereview.chromium.org/1230793009/diff/240001/third_party/opus/opus.gyp

I think in GYP, OPUS_FIXED_POINT is to tell targets that depend on this Opus
build that this Opus is FIXED_PONT.

Is the proposed GN change equivalent? why having "defines = []"

[Tima Vaisburd, 2015-10-15 18:11:50]

https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD.gn
File third_party/opus/BUILD.gn (right):

https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD....
third_party/opus/BUILD.gn:24: defines += [ "OPUS_FIXED_POINT" ]
On 2015/10/15 08:20:24, minyue wrote:
> The corresponding switch in gyp is configured this way:
> 
>
https://codereview.chromium.org/1230793009/diff/240001/third_party/opus/opus.gyp
> 
> I think in GYP, OPUS_FIXED_POINT is to tell targets that depend on this Opus
> build that this Opus is FIXED_PONT.
> 
> Is the proposed GN change equivalent? why having "defines = []"

Unfortunately I'm not an expert in GN either. Here is what Cookbook
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...
says about direct_dependent_settings:
"In GN, put the settings in a config (declared above your target), and then
reference that as a public config in your target" (there is an example there.

I tried to follow that document and make the settings that are equivalent to
GYP.
I believe in this case the config("opus_config") is the config that can use and
it is already made public on l.79

> why having "defines = []

I believe this is the part of GN syntax. Without prior (empty) list the
following operator '+=' does not work.

[minyue, 2015-10-15 18:32:15]

https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD.gn
File third_party/opus/BUILD.gn (right):

https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD....
third_party/opus/BUILD.gn:24: defines += [ "OPUS_FIXED_POINT" ]
On 2015/10/15 18:11:49, Tima Vaisburd wrote:
> On 2015/10/15 08:20:24, minyue wrote:
> > The corresponding switch in gyp is configured this way:
> > 
> >
>
https://codereview.chromium.org/1230793009/diff/240001/third_party/opus/opus.gyp
> > 
> > I think in GYP, OPUS_FIXED_POINT is to tell targets that depend on this Opus
> > build that this Opus is FIXED_PONT.
> > 
> > Is the proposed GN change equivalent? why having "defines = []"
> 
> Unfortunately I'm not an expert in GN either. Here is what Cookbook
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...
> says about direct_dependent_settings:
> "In GN, put the settings in a config (declared above your target), and then
> reference that as a public config in your target" (there is an example there.
> 
> I tried to follow that document and make the settings that are equivalent to
> GYP.
> I believe in this case the config("opus_config") is the config that can use
and
> it is already made public on l.79
> 
> > why having "defines = []
> 
> I believe this is the part of GN syntax. Without prior (empty) list the
> following operator '+=' does not work. 

Thank you for pointing out the doc. That is quite clear. I think the define = []
is still unnecessary.

May you try
   if (use_opus_fixed_point) {
      defines = [ "OPUS_FIXED_POINT" ]

[Tima Vaisburd, 2015-10-15 18:51:50]

https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD.gn
File third_party/opus/BUILD.gn (right):

https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD....
third_party/opus/BUILD.gn:24: defines += [ "OPUS_FIXED_POINT" ]
On 2015/10/15 18:32:15, minyue wrote:
> On 2015/10/15 18:11:49, Tima Vaisburd wrote:
> > On 2015/10/15 08:20:24, minyue wrote:
> > > The corresponding switch in gyp is configured this way:
> > > 
> > >
> >
>
https://codereview.chromium.org/1230793009/diff/240001/third_party/opus/opus.gyp
> > > 
> > > I think in GYP, OPUS_FIXED_POINT is to tell targets that depend on this
Opus
> > > build that this Opus is FIXED_PONT.
> > > 
> > > Is the proposed GN change equivalent? why having "defines = []"
> > 
> > Unfortunately I'm not an expert in GN either. Here is what Cookbook
> >
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...
> > says about direct_dependent_settings:
> > "In GN, put the settings in a config (declared above your target), and then
> > reference that as a public config in your target" (there is an example
there.
> > 
> > I tried to follow that document and make the settings that are equivalent to
> > GYP.
> > I believe in this case the config("opus_config") is the config that can use
> and
> > it is already made public on l.79
> > 
> > > why having "defines = []
> > 
> > I believe this is the part of GN syntax. Without prior (empty) list the
> > following operator '+=' does not work. 
> 
> Thank you for pointing out the doc. That is quite clear. I think the define =
[]
> is still unnecessary.
> 
> May you try
>    if (use_opus_fixed_point) {
>       defines = [ "OPUS_FIXED_POINT" ]

Yes, that seems to work too. Modified the way you proposed.

[minyue, 2015-10-15 19:31:05]

On 2015/10/15 18:51:50, Tima Vaisburd wrote:
>
https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD.gn
> File third_party/opus/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1405783002/diff/40001/third_party/opus/BUILD....
> third_party/opus/BUILD.gn:24: defines += [ "OPUS_FIXED_POINT" ]
> On 2015/10/15 18:32:15, minyue wrote:
> > On 2015/10/15 18:11:49, Tima Vaisburd wrote:
> > > On 2015/10/15 08:20:24, minyue wrote:
> > > > The corresponding switch in gyp is configured this way:
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1230793009/diff/240001/third_party/opus/opus.gyp
> > > > 
> > > > I think in GYP, OPUS_FIXED_POINT is to tell targets that depend on this
> Opus
> > > > build that this Opus is FIXED_PONT.
> > > > 
> > > > Is the proposed GN change equivalent? why having "defines = []"
> > > 
> > > Unfortunately I'm not an expert in GN either. Here is what Cookbook
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...
> > > says about direct_dependent_settings:
> > > "In GN, put the settings in a config (declared above your target), and
then
> > > reference that as a public config in your target" (there is an example
> there.
> > > 
> > > I tried to follow that document and make the settings that are equivalent
to
> > > GYP.
> > > I believe in this case the config("opus_config") is the config that can
use
> > and
> > > it is already made public on l.79
> > > 
> > > > why having "defines = []
> > > 
> > > I believe this is the part of GN syntax. Without prior (empty) list the
> > > following operator '+=' does not work. 
> > 
> > Thank you for pointing out the doc. That is quite clear. I think the define
=
> []
> > is still unnecessary.
> > 
> > May you try
> >    if (use_opus_fixed_point) {
> >       defines = [ "OPUS_FIXED_POINT" ]
> 
> Yes, that seems to work too. Modified the way you proposed.

lgtm

[Tima Vaisburd, 2015-10-15 19:39:19]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-10-15 19:39:20]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
xhwang@chromium.org, henrika@chromium.org
Link to the patchset: https://codereview.chromium.org/1405783002/#ps60001
(title: "Simplified Opus")

[commit-bot: I haz the power, 2015-10-15 19:40:01]

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

[commit-bot: I haz the power, 2015-10-15 20:42:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-15 20:44:08]

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