1328744 - libvpx update scripts

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P1)

Description

[johannkoenig]

Attachment: generate_sources_mozbuild.sh

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.28 Safari/537.36

Steps to reproduce:

Replace mac-specific update scripts with generic ones



Expected results:

Empty out media/libvpx.

mkdir -p libvpx config/linux/x32 config/linux/x64
Copy in:
 .patch files (modified for new libvpx location)
 Makefile.in
 moz.build
 update.py
 generate_sources_mozbuild.sh
 lint_config.sh

./update.py --commit e67d45d4ce92468ba193288b59093fef0a502662
./generate_sources_mozbuild.sh

build for linux 64 bit (32 should work but untested)

Comment 1

[johannkoenig]

Attachment: update.py

Comment 2

[johannkoenig]

Attachment: 1237848-check-lookahead-ctx.patch

Comment 3

[johannkoenig]

Attachment: bug1137614.patch

Comment 4

[johannkoenig]

Attachment: patches.tarball

Comment 5

[johannkoenig]

Attachment: lint_config.sh

Comment 6

[johannkoenig]

Attachment: moz.build

Comment 7

[Ralph Giles (:rillian)]

Thanks, Johann!

Comment 8

[johannkoenig]

This is just proof of concept at the moment. Let me know if the approach looks reasonable. As I mentioned in the other bug, it'd be a lot easier to go straight to v1.6.0 but I wanted to get something up for comment regarding moving libvpx down a directory and the configs into config/ and modifying the -I include path.

The additional work is figuring out what other platforms it needs to build for (do you need windows with msvc and mingw or just one of those?) and figuring out what variables to use in moz.build (right now it's using CONFIG['VPX_X86_ASM'] and '64' in CONFIG['OS_TEST'] to target the x86 build - I assume those need to be refined to linux + x86 + 64 bit, but I'm not sure what in the build system to use.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Thanks Johann!  We've been wanting to push this forward for some time for webrtc use.  

Don't know if it helps, but we *can* use gyp in our builds; see media/webrtc/trunk/webrtc, media/libyuv, media/webrtc/signaling etc.  Our master gyp config stuff is in build/gyp.mozbuild

Comment 10

[johannkoenig]

Unfortunately I just removed all the gyp tooling from Chromium in favor of gn ... It wouldn't be super difficult to get them back, but writing sources.mozbuild is pretty easy. Unlike the Chromium solution, it looks like all the files can be dumped in one monolithic variable and the x86 (sss3, avx2, etc) and arm (neon) flags are added in moz.build. You also do not appear to need the header files included (expect in EXPORTS) which trims things down a fair bit.

The other consideration is the libvpx directory is now complete. Previously the script removed a bunch of files to keep things tidy. That could be added back but I find it's much easier to keep the directory unmodified; especially while working on the script.

Comment 11

[johannkoenig]

Also for WebRTC I think you should remove --enable-error-concealment (https://bugzilla.mozilla.org/show_bug.cgi?id=1328330) and add --enable-realtime-only (slight benefit to binary size, WebRTC only ever uses the realtime mode)

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: updated code to import and build libvpx

I've rolled your code up into a patch, feedback from rillian and gps wanted.  Builds; haven't run it.  I presume the changeset in question is the current rev we're using

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: update of existing .patch files for libvpx rs=jesup

the patches tarball as a patch

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to johannkoenig from comment #10)
> Unfortunately I just removed all the gyp tooling from Chromium in favor of
> gn ... It wouldn't be super difficult to get them back, but writing
> sources.mozbuild is pretty easy. Unlike the Chromium solution, it looks like

FWIW, jrmuizel was asking about supporting gn via moz.build recently because Skia is converting from gyp to gn. I looked into it and I think it shouldn't be a huge amount of work, since gn supports an "output JSON" mode that we could use to get its dependency graph into moz.build land.

Comment 15

[johannkoenig]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> FWIW, jrmuizel was asking about supporting gn via moz.build recently because
> Skia is converting from gyp to gn. I looked into it and I think it shouldn't
> be a huge amount of work, since gn supports an "output JSON" mode that we
> could use to get its dependency graph into moz.build land.

That would be convenient. Then the script could potentially be copied directly from chromium. Sounds like it would minimize the ongoing maintenence cost borne by FF. That update process is described here:
https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx/README.chromium

Still need something to do the job of roll_dep.py but a stripped down version of the existing update.py should suffice.

Comment 16

[johannkoenig]

(In reply to Randell Jesup [:jesup] from comment #12)
> I presume the changeset in question is the current rev we're using

Yes I got it from here:
https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/README_MOZILLA
"The git commit ID used was e67d45d4ce92468ba193288b59093fef0a502662"

This should be a drop-in replacement for the existing code, just moving things around and changing out the build process.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a0219114a956aa1dbc618c365646cdea16ac5b

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8824482 [details] [diff] [review]
update of existing .patch files for libvpx rs=jesup

rs=me

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

A todo for the updates script (not critical; this can be done as part of checkin) is to add new files and delete old ones.

Windows builds failed (and linux x32).  Windows failed because vpx_config.h defines way too much... like HAVE_PTHREAD_H

Comment 20

[johannkoenig]

(In reply to Randell Jesup [:jesup] from comment #19)
> Windows builds failed (and linux x32).  Windows failed because vpx_config.h
> defines way too much... like HAVE_PTHREAD_H

x32 failed because I used the wrong asm conversion script:
/builds/slave/try-lx-00000000000000000000000/build/src/media/libvpx/config/linux/x32/vpx_config.asm:1: error: instruction expected after label

It checks for ia32, not x32, in generate_sources_mozbuild.sh:
  if [[ "$1" == *x64* ]] || [[ "$1" == *ia32* ]]; then
My fault, I copied it from the Chromium setup. I couldn't figure out how to force my local build to x32 and so didn't check it carefully enough.

As mentioned way above, additional targets need to be added and the correct defines for moz.build sorted out. I only made directories for linux x64 (and incorrectly, x32). The file lists in newer version of libvpx *should* coincide for x64 on win/linux/mac, but the easy thing to do is add win and mac targets to the generation script, expand the directories in config/, add new file lists to sources.mozbuild, and use build variables to pick the correct ones in moz.build.

Then, for example, you could have a windows specific config (which defaults to --disable-pthread[0]).

Chromium has config directories[1] for (ia32, x64)(win, linux, mac), (arm, arm64)(linux, ios), as well as some others like nacl(C only), mips, arm with cpu detection for neon (a relic I would like to remove).

I can help add the other config directories and file lists, but I don't know the build system well enough to figure out how to modify moz.build to pick the correct ones.

[0] https://chromium.googlesource.com/webm/libvpx/+/master/configure#536
[1] https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx/generate_gni.sh#321

Comment 21

[Ralph Giles (:rillian)]

(In reply to johannkoenig from comment #11)
> Also for WebRTC I think you should [...] add
> --enable-realtime-only (slight benefit to binary size, WebRTC only ever uses
> the realtime mode)

We also use the libvpx encoder behind the MediaRecorder api, which uses VPX_RC_ONE_PASS. I think we want to keep the non-realtime modes for that application.

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8824481 [details] [diff] [review]
updated code to import and build libvpx

Review of attachment 8824481 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable.

Feel free to send future requests to ted, as he has taken an "interest" in the integration of 3rd party build systems and has more context paged in than I do at the moment.

Comment 23

[johannkoenig]

(In reply to Gregory Szorc [:gps] from comment #22)
> This seems reasonable.

Can I jump straight to 1.6.0 or do you want it to work for the current version?

Comment 24

[Ralph Giles (:rillian)]

It's easier to review if you work with the current version, but of course we want to just to 1.6 immediately after that.

Comment 25

[johannkoenig]

Can anyone give me a list of supported targets and moz.build variables that correspond to them?

I figure there is at least:
linux: x86, x86_64, arm, arm64
mac: x86, x86_64, arm, arm64
win: x86, x86_64, arm, arm64
generic catchall

There do not appear to be existing PPC or MIPS builds.

linux and mac I assume use gcc and clang and there shouldn't be any appreciable difference.

what build environments do you have for windows and do they need to be considered separately?

Comment 26

[(not currently active) Ted Mielczarek]

The list of things we care about in what order is kept here:
https://developer.mozilla.org/en-US/docs/Supported_build_configurations

You're expected to make the build pass on all the Tier-1 platforms, and if you can make it work on any of the Tier-3 platforms that we currently have build system support for without a lot of extra effort, that's great. If not, it's up to the people maintaining those ports to fix it.

The target system CPU can be found in CONFIG['CPU_ARCH'] in moz.build, and the set of possible values is here:
https://dxr.mozilla.org/mozilla-central/rev/7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4/python/mozbuild/mozbuild/configure/constants.py#41

The target OS is in CONFIG['OS_TARGET'], and the list of values is here:
https://dxr.mozilla.org/mozilla-central/rev/7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4/python/mozbuild/mozbuild/configure/constants.py#18

...although it's a little fuzzy, in that for OS X and Linux the values will be 'Darwin' and 'Linux' from the Kernel list below that.

Comment 27

[johannkoenig]

Would someone provide a voucher for try access please?
https://bugzilla.mozilla.org/show_bug.cgi?id=1330042

Comment 28

[Ralph Giles (:rillian)]

Done. Let me know if you have something you'd like me to push while you're waiting.

Comment 29

[johannkoenig]

Thanks. The patch is 9mb so it's a little tricky to pass it around. I think it's going to be a bit unwieldy.

Comment 30

[johannkoenig]

Attachment: One giant pile

Well, turns out I need to figure out how to get gcc 4.9 now. While I'm working on that here's a patch attempt.

Comment 31

[johannkoenig]

I've built locally for x86_64 and android. The previous patch *appears* to have passed[0] linux/win/mac x86 and x86_64 (the build failures don't look like they are my fault).

I think this is pretty complete for Tier 1 targets and has a generic compile fallback for everyone else. There is another patch up [1] and I've triggered some jobs but I really don't know how to use the system very well.

I guess this [2] is the canonical patchset for this but not totally sure how to attach it to this bug.

[0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c25d736b66ba9e2ba9604d09c2443592ebca49d
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd120edd2630beb8880e6eea043efaa7931f0672
[2] https://hg.mozilla.org/try/rev/fd120edd2630beb8880e6eea043efaa7931f0672

Comment 32

[Ralph Giles (:rillian)]

The build failure is definitely an issue with the patch:

${topsrcdir}/media/libvpx/config/linux/x32/./vp9_rtcd.h:591: undefined reference to `vp9_block_error_fp_sse2'
${topsrcdir}/gcc/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../x86_64-unknown-linux-gnu/bin/ld: ../../media/libvpx/vp9_rtcd.o: relocation R_386_GOTOFF against undefined hidden symbol `vp9_block_error_fp_sse2' can not be used when making a shared object
${topsrcdir}/gcc/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../x86_64-unknown-linux-gnu/bin/ld: final link failed: Bad value

Comment 33

[johannkoenig]

Woops I was referring to the [0] try. [1] has some weird stuff going on. Must have mixed something up.

Comment 34

[johannkoenig]

Attachment: Most recent version

This patch is the same as this[0] tryjob. It was previously run as[1] which passed for everything except Android. With a one-line change (path to ads2gas.pl) Android passes[2] as well.

So ... review comments welcome.

[0] https://hg.mozilla.org/try/rev/fdb34c36fb84112a5c3a38fbdbe66f7052d7a0fa
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ad62986095ddc51548099df51c9eb978f21c9f9
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb34c36fb84112a5c3a38fbdbe66f7052d7a0fa

Comment 35

[Ralph Giles (:rillian)]

Mac cpp unit tests failed:

> dyld: Library not loaded: /usr/lib/libc++.1.dylib
> Referenced from: /builds/slave/test/build/tests/cppunittest/ShowSSEConfig
> Reason: image not found

Is this a new dependency for the libvpx build? Unfortunately this job needs to pass for your patch to land.

Comment 36

[johannkoenig]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #35)
> Mac cpp unit tests failed:
> 
> > dyld: Library not loaded: /usr/lib/libc++.1.dylib
> > Referenced from: /builds/slave/test/build/tests/cppunittest/ShowSSEConfig
> > Reason: image not found
> 
> Is this a new dependency for the libvpx build? Unfortunately this job needs
> to pass for your patch to land.

I didn't change any of the dependencies. Is this from the OS X 10.6 run? I have no idea why 10.7 and 10.8 passed but 10.6 did not. Is there any additional context from the breakage that would help figure out what's up?

Comment 37

[Ralph Giles (:rillian)]

Comment on attachment 8825929 [details] [diff] [review]
Most recent version

Review of attachment 8825929 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this! I'm fine with the overall approach, but I had some questions. Please address the comments below and request review again.

NB to Ted: this patch adds a fourth copy of gtest (and a second of libyuv) to the repo. If you don't want those in the tree, I think we can probably remove them without breaking our build, but if not I'd rather do that in a follow-up commit; it's important to get our libvpx updated.

- Please describe the motivation for the changes in your commit message, and be clear that the code itself should be the same version. The message should explain "Why" as well as "What".

- It would also be nice if you could split this into the 'update script' changes and the source move so it's easier to see what's an independent change. If that's too hard I can cope though.

::: media/libvpx/config/linux/x32/vpx_config.asm
@@ +16,5 @@
> +%define HAVE_SSE3 1
> +%define HAVE_SSSE3 1
> +%define HAVE_SSE4_1 1
> +%define HAVE_AVX 1
> +%define HAVE_AVX2 1

Is `x32` a gn thing? We tend to use `x86` or just `linux32`. It doesn't matter except that "x32" is a different abi from the i686-unknown-linux-gnu, i686-apple-darwin and i686-pc-windows-msvc platforms we target. https://en.wikipedia.org/wiki/X32_ABI

::: media/libvpx/generate_sources_mozbuild.sh
@@ +5,5 @@
> +# found in the LICENSE file.
> +
> +# Modified from chromium/src/third_party/libvpx/generate_gni.sh
> +
> +# This script is used to generate .gni files and files in the

s/.gni/mozbuild/

@@ +10,5 @@
> +# config/platform directories needed to build libvpx.
> +# Every time libvpx source code is updated just run this script.
> +#
> +# Usage:
> +# $ ./generate_gni.sh

./generate_sources_mozbuild.sh

::: media/libvpx/libvpx/build/.gitattributes
@@ +1,2 @@
> +*-vs8/*.rules -crlf
> +*-msvs/*.rules -crlf

I don't think you need to include this file. It's nice to have everything, but our build system won't be generating the rules files it refers to, and firefox devs using mercurial won't have it enforced.

::: media/libvpx/libvpx/build/.gitignore
@@ +1,1 @@
> +x86*-win32-vs*

I don't think you need this either, but if you do you should duplicate it into an `.hgignore`.

You've also added these files here, but not the ones in the parent libvpx directory.

::: media/libvpx/moz.build
@@ -60,5 @@
> -# using RTCD, so we have to make sure we only add one of the two.
> -if 'vp8/encoder/arm/armv5te/boolhuff_armv5te.asm' not in arm_asm_files:
> -    SOURCES += [
> -        'vp8/encoder/boolhuff.c',
> -    ]

I'm willing to find out if removing this this breaks anyone.

@@ -101,5 @@
> -if CONFIG['CLANG_CXX'] or CONFIG['CLANG_CL']:
> -    CFLAGS += [
> -        '-Wno-unreachable-code',
> -        '-Wno-unneeded-internal-declaration',
> -    ]

Seems like removing these will make the build noisier. Better to leave this until you update the libvpx source, if upstream has addressed these.

::: media/libvpx/update.py
@@ +21,4 @@
>  def prepare_upstream(prefix, commit=None):
>      upstream_url = 'https://chromium.googlesource.com/webm/libvpx'
> +    shutil.rmtree(os.path.join(base, 'libvpx/'))
> +    subprocess.call(['git', 'clone', upstream_url, prefix])

You must have better bandwidth to googlesource.com than I do. :)

@@ -606,5 @@
>      os.system("patch -p3 < 1237848-check-lookahead-ctx.patch")
>      # Bug 1263384 - Check input frame resolution
>      os.system("patch -p3 < input_frame_validation.patch")
> -    # Bug 1315288 - Check input frame resolution for vp9
> -    os.system("patch -p3 < input_frame_validation_vp9.patch")

Please explain in the commit message why it's ok to drop this patch?

@@ +60,5 @@
>      os.system("patch -p3 < input_frame_validation.patch")
> +    # Fix the make files so they don't refer to the duplicate-named files.
> +    os.system("patch -p1 < rename_duplicate_files.patch")
> +    # Correctly identify vp9_block_error_fp as using x86inc
> +    os.system("patch -p1 < block_error_fp.patch")

You need to add `block_error_fp.patch` to the commit.

Comment 38

[Ralph Giles (:rillian)]

(In reply to johannkoenig from comment #36)

> I didn't change any of the dependencies. Is this from the OS X 10.6 run? I
> have no idea why 10.7 and 10.8 passed but 10.6 did not. Is there any
> additional context from the breakage that would help figure out what's up?

Yes. I believe this runs C++ unittests (not googletests) built against Firefox from the Mac 10.7 build job on MacOS 10.6. Looks like it's somehow linking something which isn't present in the 10.6 environment. Wrong C++ abi maybe?

Comment 39

[johannkoenig]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #37)
> NB to Ted: this patch adds a fourth copy of gtest (and a second of libyuv)
> to the repo. If you don't want those in the tree, I think we can probably
> remove them without breaking our build, but if not I'd rather do that in a
> follow-up commit; it's important to get our libvpx updated.

They can probably be removed in update.py without too much fuss. They are not used at all. I was just trying to keep modifications to the tree to a minimum.
 
> - Please describe the motivation for the changes in your commit message, and
> be clear that the code itself should be the same version. The message should
> explain "Why" as well as "What".

Will do.

> - It would also be nice if you could split this into the 'update script'
> changes and the source move so it's easier to see what's an independent
> change. If that's too hard I can cope though.

I can't see a reasonable way to do that. Sorry. You can use 'meld' or similar to diff the trees. Outside the new files, everything should match up.

> ::: media/libvpx/config/linux/x32/vpx_config.asm
> 
> Is `x32` a gn thing? We tend to use `x86` or just `linux32`. It doesn't
> matter except that "x32" is a different abi from the i686-unknown-linux-gnu,
> i686-apple-darwin and i686-pc-windows-msvc platforms we target.
> https://en.wikipedia.org/wiki/X32_ABI

No, I don't think gn has any opinion on it. Chromium uses 'ia32'. I thought I saw something about X32 in the existing code but I must be making that up. I'll switch it back to ia32.
 
> ::: media/libvpx/generate_sources_mozbuild.sh
> s/.gni/mozbuild/

and other branding. Will do.
 
> ::: media/libvpx/libvpx/build/.gitattributes
> I don't think you need to include this file.

Will remove with the rest of the .git* files in update.py

> You've also added these files here, but not the ones in the parent libvpx
> directory.

I think 'git add' did subdirectories but not the top level? It was not intentional.

> ::: media/libvpx/moz.build
> @@ -60,5 @@
> > -# using RTCD, so we have to make sure we only add one of the two.
> > -if 'vp8/encoder/arm/armv5te/boolhuff_armv5te.asm' not in arm_asm_files:
> > -    SOURCES += [
> > -        'vp8/encoder/boolhuff.c',
> > -    ]
> 
> I'm willing to find out if removing this this breaks anyone.

Previously the source list was shared between arches. Now arm has it's own source list and boolhuff.c is not in that. It has since been deleted upstream. The compiler caught up.

> @@ -101,5 @@
> > -if CONFIG['CLANG_CXX'] or CONFIG['CLANG_CL']:
> > -    CFLAGS += [
> > -        '-Wno-unreachable-code',
> > -        '-Wno-unneeded-internal-declaration',
> > -    ]
> 
> Seems like removing these will make the build noisier. Better to leave this
> until you update the libvpx source, if upstream has addressed these.

Woops didn't mean to remove that.
 
> ::: media/libvpx/update.py
> @@ +21,4 @@
> >  def prepare_upstream(prefix, commit=None):
> >      upstream_url = 'https://chromium.googlesource.com/webm/libvpx'
> > +    shutil.rmtree(os.path.join(base, 'libvpx/'))
> > +    subprocess.call(['git', 'clone', upstream_url, prefix])
> 
> You must have better bandwidth to googlesource.com than I do. :)

Quite possible. Shouldn't need to be run often though ...

> @@ -606,5 @@
> >      os.system("patch -p3 < 1237848-check-lookahead-ctx.patch")
> >      # Bug 1263384 - Check input frame resolution
> >      os.system("patch -p3 < input_frame_validation.patch")
> > -    # Bug 1315288 - Check input frame resolution for vp9
> > -    os.system("patch -p3 < input_frame_validation_vp9.patch")
> 
> Please explain in the commit message why it's ok to drop this patch?

Woops, didn't mean to.

> @@ +60,5 @@
> >      os.system("patch -p3 < input_frame_validation.patch")
> > +    # Fix the make files so they don't refer to the duplicate-named files.
> > +    os.system("patch -p1 < rename_duplicate_files.patch")
> > +    # Correctly identify vp9_block_error_fp as using x86inc
> > +    os.system("patch -p1 < block_error_fp.patch")
> 
> You need to add `block_error_fp.patch` to the commit.

Again, woops!

This one was previously built on 32bit archs, even though it used x86inc. This change "fixes" it so it respects --disable-use-x86inc

x86inc has been fixed upstream so it works with 32bit archs, so this goes away when it updates.

Comment 40

[johannkoenig]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #38)
> (In reply to johannkoenig from comment #36)
> 
> > I didn't change any of the dependencies. Is this from the OS X 10.6 run? I
> > have no idea why 10.7 and 10.8 passed but 10.6 did not. Is there any
> > additional context from the breakage that would help figure out what's up?
> 
> Yes. I believe this runs C++ unittests (not googletests) built against
> Firefox from the Mac 10.7 build job on MacOS 10.6. Looks like it's somehow
> linking something which isn't present in the 10.6 environment. Wrong C++ abi
> maybe?

I'm not sure what I could have changed which would have added this dependency. It still only builds libvpx, not any of the tests, not libyuv, not libwebm. Strictly C89 code.

Comment 41

[johannkoenig]

Attachment: Addresses Ralph's comments

Waiting on try jobs but nothing red yet:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51424921fa9904b4c557719e189099f0cd6f7f09

Comment 42

[johannkoenig]

> > ::: media/libvpx/config/linux/x32/vpx_config.asm
> > 
> > Is `x32` a gn thing? We tend to use `x86` or just `linux32`. It doesn't
> > matter except that "x32" is a different abi from the i686-unknown-linux-gnu,
> > i686-apple-darwin and i686-pc-windows-msvc platforms we target.
> > https://en.wikipedia.org/wiki/X32_ABI
> 
> No, I don't think gn has any opinion on it. Chromium uses 'ia32'. I thought
> I saw something about X32 in the existing code but I must be making that up.
> I'll switch it back to ia32.

Woops, updated x32 but not X32. Updated locally, but won't bother updating the patchset until there are other comments.

Comment 43

[johannkoenig]

> > ::: media/libvpx/moz.build
> > @@ -60,5 @@
> > > -# using RTCD, so we have to make sure we only add one of the two.
> > > -if 'vp8/encoder/arm/armv5te/boolhuff_armv5te.asm' not in arm_asm_files:
> > > -    SOURCES += [
> > > -        'vp8/encoder/boolhuff.c',
> > > -    ]
> > 
> > I'm willing to find out if removing this this breaks anyone.
> 
> Previously the source list was shared between arches. Now arm has it's own
> source list and boolhuff.c is not in that. It has since been deleted
> upstream. The compiler caught up.

Woops boolhuff_armv5te.asm was removed before the current ff version. But anyway, yeah, gone, not a problem.

Comment 44

[Ralph Giles (:rillian)]

Comment on attachment 8826005 [details] [diff] [review]
Addresses Ralph's comments

Review of attachment 8826005 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with the following changes:

> vp8_cx_iface.c.orig
> vpx_integer.h.orig
> vp8_cx_iface.c.rej

Diff cruft crept into this version of the patch. Please remove those.

The .rej probably came from input_validation_vp9.patch not actually applying. That's probably my error; I'll attach a fixup you can roll into your patch.

If you want to make update.py fail more obviously when a patch doesn't apply that might be safer for the future. Use `subprocess.check_call()` instead of `os.system()` maybe?

> Previously the source list was shared between arches. Now arm has it's own
> source list and boolhuff.c is not in that. It has since been deleted
> upstream. The compiler caught up.

I see. Thanks for explaining!

Comment 45

[Ralph Giles (:rillian)]

Attachment: fixup input_frame_validation_vp9.patch

Fix the diff context so input_frame_validation_vp9.patch applies cleanly.

Comment 46

[johannkoenig]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #44)
> LGTM with the following changes:
> 
> Diff cruft crept into this version of the patch. Please remove those.

Done.
 
> The .rej probably came from input_validation_vp9.patch not actually
> applying. That's probably my error; I'll attach a fixup you can roll into
> your patch.

Also done.

> If you want to make update.py fail more obviously when a patch doesn't apply
> that might be safer for the future. Use `subprocess.check_call()` instead of
> `os.system()` maybe?

It's pretty noisy. There isn't much output to bury the error. I was ignoring it because I looked and the rejected part was the comment, so I didn't worry about it. This was also the existing design and I'm not great with python ...

Comment 47

[johannkoenig]

Attachment: Review comments part deux

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5fee27897e
Replace libvpx update scripts. r=rillian

Comment 49

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/404f8c9adbfd
Backed out changeset de5fee27897e for bustage

Comment 50

[(not currently active) Ted Mielczarek]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #38)
> (In reply to johannkoenig from comment #36)
> 
> > I didn't change any of the dependencies. Is this from the OS X 10.6 run? I
> > have no idea why 10.7 and 10.8 passed but 10.6 did not. Is there any
> > additional context from the breakage that would help figure out what's up?
> 
> Yes. I believe this runs C++ unittests (not googletests) built against
> Firefox from the Mac 10.7 build job on MacOS 10.6. Looks like it's somehow
> linking something which isn't present in the 10.6 environment. Wrong C++ abi
> maybe?

It's libc++, which is the "newer" standard C++ library. We dropped support for 10.6, so that's not important (I'm not sure why those jobs are still running on try.)

Comment 51

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=68397233&repo=mozilla-inbound

Comment 52

[johannkoenig]

(In reply to Carsten Book [:Tomcat] from comment #51)
> backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=68397233&repo=mozilla-
> inbound

Huh, weird that 10.7 opt passed and 10.7 debug is failing ... Are there any other builds that failed that I can look at? I thought maybe another part of the codebase was referring to alloccommon.c directly but I can't seem to find anything. Still poking around though.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #50)
> It's libc++, which is the "newer" standard C++ library. We dropped support
> for 10.6, so that's not important (I'm not sure why those jobs are still
> running on try.)

I picked it manually from the list. Is there a general "Tier 1" batch try job I can use? I've just been picking randomish bots.

Comment 53

[johannkoenig]

(In reply to Carsten Book [:Tomcat] from comment #51)
> backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=68397233&repo=mozilla-
> inbound

To be clear, that says:
No rule to make target `/builds/slave/m-in-m64-d-0000000000000000000/build/src/media/libvpx/vp8/common/alloccommon.c', needed by `alloccommon.o'. 

I searched for references to 'media/libvpx/vp' and 'allocccommon.c' in case some moz.build file somewhere else was referring to it. The only references I found were in media/libvpx/libvpx/vp8/vp8_common.mk (not used by mozilla build) and media/libvpx/sources.mozbuild, which has the correct relative paths (otherwise none of the targets would work)

I kicked off trys for the patchset I posted:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92074ba31a22564b8129b576f03a1ffe8abc93c6
for which 10.7 opt ran yesterday and passed.

Comment 54

[Carsten Book [:Tomcat]]

jesup mentioned on irc that this just maybe need a clobber

Comment 55

[johannkoenig]

Attachment: Update CLOBBER

Comment 56

[johannkoenig]

I think I updated the CLOBBER file correctly. That was easier than I expected.

Comment 57

[Randell Jesup [:jesup] (needinfo me)]

Attachment: touch CLOBBER due to libvpx build system changes and moves rs=me

Comment 58

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8826199 [details] [diff] [review]
touch CLOBBER due to libvpx build system changes and moves rs=me

Oops, he beat me to it

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddb6c3191d11
Replace libvpx update scripts. r=rillian, r=jesup

Comment 60

[Ralph Giles (:rillian)]

(In reply to johannkoenig from comment #55)
> Created attachment 8826196 [details] [diff] [review]
> Update CLOBBER

Sorry, johann. I noticed you'd probably need to touch clobber, but forgot to include it in the review.

Comment 61

[Ralph Giles (:rillian)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #50)

> It's libc++, which is the "newer" standard C++ library. We dropped support
> for 10.6, so that's not important (I'm not sure why those jobs are still
> running on try.)

Ah, thanks Ted. I'm glad to hear the 10.6 try runs are spurious.

Comment 62

[Ralph Giles (:rillian)]

Johann, looks like even with the clobber, android-x86 isn't working.

https://treeherder.mozilla.org/logviewer.html#?job_id=68501988&repo=autoland&lineNumber=12977

Comment 63

[johannkoenig]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #62)
> Johann, looks like even with the clobber, android-x86 isn't working.
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=68501988&repo=autoland&lineNumber=12977

I'm guessing it's here:
elif CONFIG['CPU_ARCH'] == 'x86':
    EXPORTS.vpx += files['IA32_EXPORTS']
    SOURCES += files['IA32_SOURCES']
    if CONFIG['OS_TARGET'] == 'Linux':
        ASFLAGS += [ '-I%s/media/libvpx/config/linux/ia32/' % TOPSRCDIR ]
        CFLAGS += [ '-I%s/media/libvpx/config/linux/ia32/' % TOPSRCDIR ]

It got the file list, but not the include directory for vpx_config.h
I'll switch that line to search OS_TARGET for Android as well as Linux. Do you want a small patch or a reroll?

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5ef68aa940e
Fix Android x86 bustage. r=jesup on a CLOSED TREE

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ddb6c3191d11
https://hg.mozilla.org/mozilla-central/rev/c5ef68aa940e