Update BUILD.gn for [P]NaCl.

Update BUILD.gn for [P]NaCl.

Now it's possible to build libvpx for [P]NaCl. It will be used to build
remoting PNaCl plugin.

BUG=512899
R=johannkoenig@google.com

Committed: https://chromium.googlesource.com/chromium/deps/libvpx/+/955b838bb02b2d2db4317b15417b18992bb72652

[Sergey Ulanov, 2015-09-02 23:48:48]

sergeyu@chromium.org changed reviewers:
+ tomfinegan@chromium.org

[Johann, 2015-09-03 02:14:41]

I'm kinda stuck trying to land an upstream roll:
https://codereview.chromium.org/1312353005/

If you're in a hurry to get this all the way through let me know.

Otherwise, couple comments about the code.

[Johann, 2015-09-03 02:14:50]

johannkoenig@google.com changed reviewers:
+ johannkoenig@google.com

[Johann, 2015-09-03 02:14:51]

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn#newcode13
BUILD.gn:13: } if (is_nacl) {
this doesn't seem necessary, but if you want to keep it should it be else if?

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn#newcode40
BUILD.gn:40: if (os_category == "nacl") {
this could be if(is_nacl), yeah?

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn#newcode42
BUILD.gn:42: "//third_party/libvpx/source/config/$os_category"
this could be hard coded source/config/nacl

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn#newcode229
BUILD.gn:229: } if (current_cpu == "x86") {
else if?

[Johann, 2015-09-03 02:16:20]

also performance is going to be horrendous. is the intention to actually run
this code?

[Sergey Ulanov, 2015-09-03 04:25:11]

Patchset #2 (id:20001) has been deleted

[Sergey Ulanov, 2015-09-03 04:25:20]

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn#newcode13
BUILD.gn:13: } if (is_nacl) {
On 2015/09/03 02:14:51, Johann wrote:
> this doesn't seem necessary, but if you want to keep it should it be else if?

Actually with your other suggestion below os_category is not used in is_nacl
case, so I moved os_category inside !is_nacl if block below.

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn#newcode40
BUILD.gn:40: if (os_category == "nacl") {
On 2015/09/03 02:14:51, Johann wrote:
> this could be if(is_nacl), yeah?

Done.

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn#newcode42
BUILD.gn:42: "//third_party/libvpx/source/config/$os_category"
On 2015/09/03 02:14:51, Johann wrote:
> this could be hard coded source/config/nacl

Done.

https://codereview.chromium.org/1326043003/diff/1/BUILD.gn#newcode229
BUILD.gn:229: } if (current_cpu == "x86") {
On 2015/09/03 02:14:51, Johann wrote:
> else if?

Yes, of course! Thanks for catching it!

[Sergey Ulanov, 2015-09-03 04:29:37]

On 2015/09/03 02:16:20, Johann wrote:
> also performance is going to be horrendous. is the intention to actually run
> this code?

We normally use PPB_VideoDecode API for video decoding. libvpx is used directly
in NaCl only as a fallback when PPB_Graphics3D is not available (which is a
corner case - most clients should have 3D-enabled hardware). Performance is
actually acceptable.

[Johann, 2015-09-03 05:43:03]

Does the "accelerated" version go to Chrome's optimized version? Just trying to
figure out why you need a fallback.

https://codereview.chromium.org/1326043003/diff/40001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1326043003/diff/40001/BUILD.gn#newcode41
BUILD.gn:41: "//third_party/libvpx/source/config/$os_category/$cpu_arch_full"
ah, ok this moved so you have $cpu_arch_full defined.

https://codereview.chromium.org/1326043003/diff/40001/BUILD.gn#newcode47
BUILD.gn:47: platform_include_dir,
$platform_include_dir?

[Johann, 2015-09-03 05:44:23]

On 2015/09/03 04:29:37, Sergey Ulanov wrote:
> On 2015/09/03 02:16:20, Johann wrote:
> > also performance is going to be horrendous. is the intention to actually run
> > this code?
> 
> We normally use PPB_VideoDecode API for video decoding. libvpx is used
directly
> in NaCl only as a fallback when PPB_Graphics3D is not available (which is a
> corner case - most clients should have 3D-enabled hardware). Performance is
> actually acceptable.

What provides PPB_VideoDecode? Very few devices have vp8/vp9 HW, especially
encoder.

[Sergey Ulanov, 2015-09-03 16:33:37]

On 2015/09/03 05:43:03, Johann wrote:
> Does the "accelerated" version go to Chrome's optimized version? Just trying
to
> figure out why you need a fallback.

PPB_VideoDecoder API depends on PPB_Graphics3D interface which is not always
supported, depending on hardware (this mostly affects Linux when OpenGL may not
be available).
PPB_VideoDecoder uses hardware decoding when available and falls back to
software decoding otherwise (with CPU-specific optimizations).

https://codereview.chromium.org/1326043003/diff/40001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1326043003/diff/40001/BUILD.gn#newcode41
BUILD.gn:41: "//third_party/libvpx/source/config/$os_category/$cpu_arch_full"
On 2015/09/03 05:43:03, Johann wrote:
> ah, ok this moved so you have $cpu_arch_full defined.

GN doesn't allow to define variable and never use it. So it was complaining
about os_category being unused when compiling with is_nacl, that's why I had to
move here.

https://codereview.chromium.org/1326043003/diff/40001/BUILD.gn#newcode47
BUILD.gn:47: platform_include_dir,
On 2015/09/03 05:43:03, Johann wrote:
> $platform_include_dir?

this can be either platform_include_dir or "$platform_include_dir" (with
quotes). I don't see any reason to prefer the second variant.

[Sergey Ulanov, 2015-09-03 16:36:13]

On 2015/09/03 05:44:23, Johann wrote:
> On 2015/09/03 04:29:37, Sergey Ulanov wrote:
> > On 2015/09/03 02:16:20, Johann wrote:
> > > also performance is going to be horrendous. is the intention to actually
run
> > > this code?
> > 
> > We normally use PPB_VideoDecode API for video decoding. libvpx is used
> directly
> > in NaCl only as a fallback when PPB_Graphics3D is not available (which is a
> > corner case - most clients should have 3D-enabled hardware). Performance is
> > actually acceptable.
> 
> What provides PPB_VideoDecode? Very few devices have vp8/vp9 HW, especially
> encoder.

see my other response. PPB_VideoDecoder supports fallback to software decoding.
In other case it's not related to this CL - we already ship libvpx built for
PNaCl (with GYP). I'm just trying to migrate it to GN.

[Johann, 2015-09-03 16:41:45]

LGTM

Still seems weird to have the C code as a fallback. If it makes it that far,
something else should have broken first. But yeah, having parity between gyp and
gn is good. I wish we only had to support one.

https://codereview.chromium.org/1326043003/diff/40001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1326043003/diff/40001/BUILD.gn#newcode47
BUILD.gn:47: platform_include_dir,
On 2015/09/03 16:33:37, Sergey Ulanov wrote:
> On 2015/09/03 05:43:03, Johann wrote:
> > $platform_include_dir?
> 
> this can be either platform_include_dir or "$platform_include_dir" (with
> quotes). I don't see any reason to prefer the second variant.

Weird. OK!

[Sergey Ulanov, 2015-09-03 16:47:30]

Committed patchset #2 (id:40001) manually as
955b838bb02b2d2db4317b15417b18992bb72652 (presubmit successful).

[Johann, 2015-09-03 21:09:56]

On 2015/09/03 16:47:30, Sergey Ulanov wrote:
> Committed patchset #2 (id:40001) manually as
> 955b838bb02b2d2db4317b15417b18992bb72652 (presubmit successful).

FYI - potentially rolling as part of this change:
https://codereview.chromium.org/1312353005/