Status

()

P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: rillian, Assigned: dminor)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(14 attachments, 6 obsolete attachments)

59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
292.00 KB, video/webm
Details
59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
cmanchester
: review+
Details
59 bytes, text/x-review-board-request
Details
1.47 KB, patch
cmanchester
: review+
Details | Diff | Splinter Review
Now that important bug fixes have landed, we should vendor a new version of the libaom reference implementation of the av1 codec.

Vendoring is automated by the `./mach vendor aom` command.

Unfortunately, upstream has removed the configure+make build system our import script relies on to generate code and moz.build files. So the larger portion of the work will be updating the script to use upstream's new cmake build system somehow.
dminor, you're used to wrangling with chrome code for the webrtc imports. Would you be willing to take this on? The current import script is based on chromium's gn generator for the third-party library.

I'd thought there would be an updated version we could get ideas from in the chromium source, but it looks like they haven't rolled a new dependency since last year when the 'configure' script was still available, so some cmake hacking might be necessary.

Other crazy ideas:
 - Implement cmake support in mozbuild.
 - Write a rust wrapper and use the cmake crate in a build.rs to do a native build.

Chris, any suggestions from the build side? Maintaining moz.build generators for active third-party libraries is no fun.
Assignee: nobody → dminor
Flags: needinfo?(cmanchester)
Blocks: 1437063
It looks like the default cmake backend for the project is "Unix Makefiles"... possibly we could parse those for the list of sources? Another thing that crossed my mind would be implementing a CMake generator that spits out the data we want in a format we can trivially translate to moz.build.

I'm really not crazy about the other crazy ideas from comment 1. Maybe we can get in touch with chromium folks to see what they have planned for this? Whatever they do would probably help us out here as we have some ability to handle GN definitions in moz.build.
Flags: needinfo?(cmanchester)
Blocks: 1445922
(Assignee)

Comment 3

a year ago
I spent a bit of time investigating this on Friday. CMake can now generate Ninja files directly, which would explain why the Chromium folks no longer need to worry about code generating gn files.

The Makefiles created by CMake are pretty terrible. It might be easier to parse the list of sources from the CMakeList files directly.

Writing a generator is an interesting idea. A CMake generator needs to be able to build executables on each platform so that CMake can do ABI identification, but we might be able to make a meta-generator that captures the information we want while delegating the actual work to the platform specific generators. The downside to this approach is that we're then stuck maintaining something against CMake internal APIs indefinitely. I'm guessing we would only need to override a handful of methods to make this work so it might not be too bad.
Status: NEW → ASSIGNED
Blocks: 1452683
(Assignee)

Comment 4

a year ago
WIP can be seen in this try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c04366b5684858871353b53be99f4a8de90f1fdd

I've written a parser which pulls the lists of sources out of the cmake lists directly. The above try run uses the cmake parser to generate a sources.mozbuild for the version of libaom in tree. It is building successfully for x86_64 and armv7 as well.

The next step is to modify the vendoring code to use cmake_parser.py rather than configure to generate sources.mozbuild and attempt an re-import of the version of libaom currently in tree. If that goes well, I'll start looking at importing from the tip of libaom.

Chris, I'm planning to ask you for review on this. Please let me know if you have any early feedback about the approach I'm taking here. Thanks!
Flags: needinfo?(cmanchester)
Writing our own parser for this is unfortunate, but this seems pretty straightforward. I'm ok with the approach.
Flags: needinfo?(cmanchester)
(Assignee)

Comment 6

11 months ago
This try run includes code generating the various aom_config.* files from python: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98ec401ea2191ab883b39d19895cd862d23241d3
(Assignee)

Comment 7

11 months ago
I would like to test this against: https://hacks.mozilla.org/2017/11/dash-playback-of-av1-video/ but unfortunately I'm getting browser crashes with or without my patches applied for an opt+debug build. I guess I'll get started on the actual update and then circle back to trying to verify that running the new scripts on the version in tree work as expected.
(Assignee)

Comment 8

11 months ago
Using an opt build works fine, both with my patches applied and without. The next logical step is to try to reproduce the build in tree using the mach vendor aom command, starting without my patches.

Unfortunately, even *without* my patches applied, I'm not able to reproduce the local build:

First I needed to do some fixups: the generate_sources_mozbuild.sh does not have the executable bit set, and the aom_version.h is not checked in.

chmod +x media/libaom/generate_sources_mozbuild.sh
cp media/libaom/config/aom_version.h media/libaom/aom_version.h
hg add media/libaom/aom_version.h
hg commit -m "fixups for aom vendor"
hg purge

After that, I attempted to vendor what I believe to be the current commit in tree (from media/libaom/README_MOZILLA, this should be e87fb2378f01103d5d6e477a4ef6892dc714e614)

./mach vendor aom -r e87fb2378f01103d5d6e477a4ef6892dc714e614
 0:01.86 Fetching commit id from https://aomedia.googlesource.com/aom/+/e87fb2378f01103d5d6e477a4ef6892dc714e614?format=JSON
 0:01.87 Starting new HTTPS connection (1): aomedia.googlesource.com
 0:02.64 Fetching https://aomedia.googlesource.com/aom/+archive/e87fb2378f01103d5d6e477a4ef6892dc714e614.tar.gz
 0:02.64 Starting new HTTPS connection (1): aomedia.googlesource.com
 0:05.66 rm -rf /home/dminor/src/firefox/third_party/aom
 0:05.73 Unpacking upstream files.
 0:05.99 Removing unnecessary files.
 0:05.99 Generating build files...
 0:05.99 rm -rf /home/dminor/src/firefox/media/libaom/config
 0:05.99 ./generate_sources_mozbuild.sh
 0:06.83 ERROR: DUPLICATE FILES FOUND
 0:06.83 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/x86/highbd_intrapred_sse2.asm
 0:06.83 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/x86/highbd_intrapred_sse2.c
 0:06.84 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct16x16_1_add_neon.c
 0:06.84 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct16x16_1_add_neon.asm
 0:06.84 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct16x16_add_neon.asm
 0:06.84 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct16x16_add_neon.c
 0:06.84 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct32x32_1_add_neon.c
 0:06.84 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct32x32_1_add_neon.asm
 0:06.84 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct32x32_add_neon.asm
 0:06.85 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct32x32_add_neon.c
 0:06.88 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct4x4_1_add_neon.c
 0:06.88 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct4x4_1_add_neon.asm
 0:06.89 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct4x4_add_neon.asm
 0:06.89 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct4x4_add_neon.c
 0:06.90 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct8x8_1_add_neon.c
 0:06.90 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct8x8_1_add_neon.asm
 0:06.90 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct8x8_add_neon.c
 0:06.90 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/idct8x8_add_neon.asm
 0:06.91 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/x86/intrapred_sse2.c
 0:06.91 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/x86/intrapred_sse2.asm
 0:06.92 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/x86/intrapred_ssse3.c
 0:06.92 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/x86/intrapred_ssse3.asm
 0:06.94 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/loopfilter_16_neon.asm
 0:06.94 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/loopfilter_16_neon.c
 0:06.94 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/loopfilter_4_neon.asm
 0:06.94 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/loopfilter_4_neon.c
 0:06.94 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/loopfilter_8_neon.asm
 0:06.94 /home/dminor/src/firefox/media/libaom/../../third_party/aom/aom_dsp/arm/loopfilter_8_neon.c
Error running mach:

    ['vendor', 'aom', '-r', 'e87fb2378f01103d5d6e477a4ef6892dc714e614']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

Exception: Process executed with non-0 exit code 1: [u'./generate_sources_mozbuild.sh']

  File "/home/dminor/src/firefox/python/mozbuild/mozbuild/mach_commands.py", line 2106, in vendor_aom
    vendor_command.vendor(**kwargs)
  File "/home/dminor/src/firefox/python/mozbuild/mozbuild/vendor_aom.py", line 238, in vendor
    self.generate_sources(glue_dir)
  File "/home/dminor/src/firefox/python/mozbuild/mozbuild/vendor_aom.py", line 195, in generate_sources
    cwd=target, log_name='generate_sources')
  File "/home/dminor/src/firefox/python/mach/mach/mixin/process.py", line 148, in run_process
    raise Exception('Process executed with non-0 exit code %d: %s' % (status, args))

More concerningly, hg status shows some source code changes from what is in tree, which makes it look like what we have in tree does not match the commit mentioned in the readme:

M third_party/aom/aom_dsp/x86/aom_highbd_convolve_hip_ssse3.c
M third_party/aom/aom_dsp/x86/highbd_intrapred_avx2.c
M third_party/aom/aom_ports/msvc.h
M third_party/aom/av1/common/onyxc_int.h
M third_party/aom/av1/common/quant_common.c
M third_party/aom/av1/common/quant_common.h
M third_party/aom/av1/common/reconinter.c
M third_party/aom/av1/common/reconinter.h
M third_party/aom/av1/common/thread_common.c
M third_party/aom/av1/common/thread_common.h
M third_party/aom/av1/common/x86/selfguided_sse4.c
M third_party/aom/av1/encoder/x86/hybrid_fwd_txfm_avx2.c
(Assignee)

Comment 9

11 months ago
I've tested my patches locally and on try by running generate_mozbuild_sources.sh directly, rather than through the vendor command.
(In reply to Dan Minor [:dminor] from comment #8)
> M third_party/aom/aom_dsp/x86/aom_highbd_convolve_hip_ssse3.c
> M third_party/aom/aom_dsp/x86/highbd_intrapred_avx2.c
> M third_party/aom/aom_ports/msvc.h
> M third_party/aom/av1/common/onyxc_int.h
> M third_party/aom/av1/common/quant_common.c
> M third_party/aom/av1/common/quant_common.h
> M third_party/aom/av1/common/reconinter.c
> M third_party/aom/av1/common/reconinter.h
> M third_party/aom/av1/common/thread_common.c
> M third_party/aom/av1/common/thread_common.h
> M third_party/aom/av1/common/x86/selfguided_sse4.c
> M third_party/aom/av1/encoder/x86/hybrid_fwd_txfm_avx2.c

From spot checking a few of these it seems like a couple of fixes landed after the initial import (bug 1314147):
bug 1414507, bug 1423814, bug 1413734
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

11 months ago
I've pushed my WIP update to MozReview. There are two immediate problems with compiling:

 0:02.88 /home/dminor/src/firefox/config/rules.mk:775: warning: overriding recipe for target 'highbd_intrapred_sse2.o'
 0:02.88 /home/dminor/src/firefox/config/rules.mk:753: warning: ignoring old recipe for target 'highbd_intrapred_sse2.o'
 0:02.88 /home/dminor/src/firefox/config/rules.mk:775: warning: overriding recipe for target 'intrapred_sse2.o'
 0:02.88 /home/dminor/src/firefox/config/rules.mk:753: warning: ignoring old recipe for target 'intrapred_sse2.o'
 0:02.94 yasm: FATAL: `sse2' is not a valid machine for architecture `x86'
 0:02.94 /home/dminor/src/firefox/config/rules.mk:775: recipe for target 'highbd_intrapred_sse2.o' failed

In the old version, highbd_intrapred_sse2.asm was renamed to highbd_intrapred_sse2_asm.asm, I believe to avoid conflicting with highbd_intrapred_sse2.c. I assume we'll have to do something similar here and update the CMake files accordingly.

There are also at least a few API changes:

0:16.94 In file included from /home/dminor/src/firefox/obj-x86_64-pc-linux-gnu/dom/media/platforms/Unified_cpp_dom_media_platforms0.cpp:11:0:
 0:16.94 /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp: In function ‘aom_codec_err_t mozilla::highbd_img_downshift(aom_image_t*, aom_image_t*, int)’:
 0:16.94 /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp:157:10: error: ‘AOM_IMG_FMT_I440’ was not declared in this scope
 0:16.94      case AOM_IMG_FMT_I440:
 0:16.94           ^~~~~~~~~~~~~~~~
 0:16.94 /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp:157:10: note: suggested alternative: ‘AOM_IMG_FMT_I444’
 0:16.94      case AOM_IMG_FMT_I440:
 0:16.94           ^~~~~~~~~~~~~~~~
 0:16.94           AOM_IMG_FMT_I444
 0:16.94 /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp:166:10: error: ‘AOM_IMG_FMT_I44016’ was not declared in this scope
 0:16.94      case AOM_IMG_FMT_I44016:
 0:16.94           ^~~~~~~~~~~~~~~~~~
 0:16.94 /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp:166:10: note: suggested alternative: ‘AOM_IMG_FMT_I44416’
 0:16.94      case AOM_IMG_FMT_I44016:
 0:16.94           ^~~~~~~~~~~~~~~~~~
 0:16.95           AOM_IMG_FMT_I44416
 0:16.95 /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp: In member function ‘RefPtr<mozilla::MozPromise<nsTArray<RefPtr<mozilla::MediaData> >, mozilla::MediaResult, true> > mozilla::AOMDecoder::ProcessDecode(mozilla::MediaRawData*)’:
 0:16.95 /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp:206:97: error: too many arguments to function ‘aom_codec_err_t aom_codec_decode(aom_codec_ctx_t*, const uint8_t*, unsigned int, void*)’
 0:16.95    if (aom_codec_err_t r = aom_codec_decode(&mCodec, aSample->Data(), aSample->Size(), nullptr, 0)) {
 0:16.95                                                                                                  ^
 0:16.95 In file included from /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.h:13:0,
 0:16.95                  from /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp:7,
 0:16.95                  from /home/dminor/src/firefox/obj-x86_64-pc-linux-gnu/dom/media/platforms/Unified_cpp_dom_media_platforms0.cpp:11:
 0:16.95 /home/dminor/src/firefox/obj-x86_64-pc-linux-gnu/dist/include/aom/aom_decoder.h:213:17: note: declared here
 0:16.95  aom_codec_err_t aom_codec_decode(aom_codec_ctx_t *ctx, const uint8_t *data,
 0:16.95                  ^~~~~~~~~~~~~~~~
 0:18.13 /home/dminor/src/firefox/config/rules.mk:1024: recipe for target 'Unified_cpp_dom_media_platforms0.o' failed

There may also be additional CMake work required that won't show up until link time.
Thomas, this is the libaom update where we would highly appreciate your help.
Flags: needinfo?(tdaede)

Comment 19

11 months ago
mozreview-review
Comment on attachment 8968889 [details]
Bug 1445683 - Add generate_sources_mozbuild.py;

https://reviewboard.mozilla.org/r/237602/#review244434

It is unclear to me how this is handling the special flags needed for intrinsics files.

::: media/libaom/generate_sources_mozbuild.py:91
(Diff revision 1)
> +
> +    shared_variables = {
> +        'CMAKE_CURRENT_SOURCE_DIR': AOM_DIR,
> +        'CONFIG_AV1_DECODER': 1,
> +        'CONFIG_AV1_ENCODER': 1,
> +        'CONFIG_BGSPRITE': 0,

Remove:

CONFIG_BGSPRITE
CONFIG_CDEF_SINGLEPASS
CONFIG_CFL
CONFIG_HASH_ME
CONFIG_HIGH_BITDEPTH
CONFIG_LV_MAP
CONFIG_MOTION_VAR
CONFIG_NCOBMC_ADAPT_WEIGHT
CONFIG_PVQ
CONFIG_XIPHRC

Add

'CONFIG_LOWBITDEPTH': 1
You will need to drop all the I440 conditions.

I am not sure how the build system is choosing what options to pass to yasm. I'll have to apply them locally to see what's going on.
Flags: needinfo?(tdaede)
(Assignee)

Comment 21

11 months ago
Thanks!

As far as I can tell, the assembler flags are coming from this file: media/libaom/moz.build. We were ignoring any flags defined in the Makefiles previously so I've been doing that for cmake as well.
(Assignee)

Comment 22

11 months ago
More on the yasm thing, it looks like we're trying to use it to compile a .c file:

586  0:01.23 /usr/bin/yasm -o highbd_intrapred_sse2.o -f elf64 -rnasm -pnasm -g dwarf2     -I/home/dminor/src/firefox/media/libaom/config/linux/x64/ -DPIC -I. -I/home/dminor/src/firefox/third_party/aom -msse2  /home/dminor/src/firefox/third_party/aom/aom_dsp/x86/highbd_intrapred_sse2.c

In the update, we have both a .asm and a .c with the same prefix.

../../third_party/aom/aom_dsp/x86/highbd_intrapred_sse2.asm
../../third_party/aom/aom_dsp/x86/highbd_intrapred_sse2.c

In tree, that file is called highbd_intrapred_sse2_asm.asm, so I'm guessing I'll have to rename it in the update as well to keep the build system happy. I'm not certain if renaming was a manual process, or part of the old generate sources script that I missed.
Facebook just released a demo video encoded with the libaom cc92258a08d98f469dff1be288acbc322632377b, which is the version used in Chromium.

VLC appears to have chosen 6f49b5a214fa48c226be3bc28f5c597edb81ed8c.

I'm not saying we should update to any of these. Just easier to have these information available here in the ticket.
(Assignee)

Comment 24

11 months ago
I had successful local x64 Linux build this afternoon, so I believe the build system work is quite likely almost there. The next step will be to choose a commit and test things out.

cc92258a08d98f469dff1be288acbc322632377b is quite old, from November 22. 6f49b5a214fa48c226be3bc28f5c597edb81ed8c is more recent, from April 3rd. :dmajor was looking for a fix from a bug that was marked as resolved on April 6, which unfortunately did not mention the commit which fixed the bug.

I'd say either match VLC or just take whatever happens to be tip the next time I spend some time on this, probably next week.
(Assignee)

Comment 25

10 months ago
What's the proper way to create a test file?

I tried ./aomenc -o carphone.av1 ~/Downloads/carphone_qcif.y4m (with https://xiph-media.net/video/derf/y4m/carphone_qcif.y4m) and waited a longish amount of time. I'm getting this error if I try it in firefox:

dminor@treebeard:~/src/firefox$ ./mach run carphone.av1 
 0:00.21 /home/dminor/src/firefox/obj-x86_64-pc-linux-gnu/dist/bin/firefox carphone.av1 -no-remote -profile /home/dminor/src/firefox/obj-x86_64-pc-linux-gnu/tmp/profile-default
JavaScript error: chrome://global/content/bindings/videocontrols.xml, line 280: NS_ERROR_NOT_AVAILABLE: 
[Child 10836, MediaPlayback #1] WARNING: Decoder=7feed9db1800 Decode error: NS_ERROR_DOM_MEDIA_DECODE_ERR (0x806e0004) - RefPtr<mozilla::MozPromise<nsTArray<RefPtr<mozilla::MediaData> >, mozilla::MediaResult, true> > mozilla::AOMDecoder::ProcessDecode(mozilla::MediaRawData*): AOM error decoding AV1 sample: Corrupt frame detected: file /home/dminor/src/firefox/dom/media/MediaDecoderStateMachine.cpp, line 3411

Unless something went strangely with the update script, both aom and the firefox should be at commit 6f49b5a214fa48c226be3bc28f5c597edb81ed8c.

Before I spent any time investigating the decode error in firefox I wanted to make sure I had run the encoder properly. Thanks!
Flags: needinfo?(tdaede)
That's a correct way to produce the sample. You can use --cpu-used=3 if you'd like it to complete faster. I assume you want to use a WebM container? First thing I'd try is making the output "carphone.webm" instead to make sure we're writing the correct container format (though the error seems to imply that we are).

Secondly, it's very likely the list of variables got out of date again. You should look at the aom_config.h of your local build of libaom and make sure that it matches the shared_variables list.
Flags: needinfo?(tdaede)
(Assignee)

Comment 27

10 months ago
Thank you!

I did find some problems with the list of variables. After I've corrected the ones I think are significant, what I'm left with is:

diff linux/x64/aom_config.h ~/src/aom/out/aom_config.h
13d12
< 
40c39
< #define CONFIG_LIBYUV 0
---
> #define CONFIG_LIBYUV 1
53,54c52,53
< #define CONFIG_UNIT_TESTS 0
< #define CONFIG_WEBM_IO 0
---
> #define CONFIG_UNIT_TESTS 1
> #define CONFIG_WEBM_IO 1
71a71
> #define HAVE_UNISTD_H 1
74d73
< #define INCLUDE_INSTALL_DIR INSTALLDIR/include
76d74
< #define LIB_INSTALL_DIR INSTALLDIR/lib
78c76
< #endif /* AOM_CONFIG_H_ */
---
> #endif  /* AOM_CONFIG_H_ */

diff linux/x64/aom_config.asm ~/src/aom/out/aom_config.asm
11d10
< 
38c37
< CONFIG_LIBYUV equ 0
---
> CONFIG_LIBYUV equ 1
51,52c50,51
< CONFIG_UNIT_TESTS equ 0
< CONFIG_WEBM_IO equ 0
---
> CONFIG_UNIT_TESTS equ 1
> CONFIG_WEBM_IO equ 1
69a69
> HAVE_UNISTD_H equ 1

CONFIG_LIBYUV and CONFIG_WEBM_IO only seem to affect the aomenc application itself and not the library, so I thought it safe to leave them undefined. Please let me know if this is incorrect.

The specific error I'm seeing is "No sequence header" at [1]. This is occurring on the 6th call to read_uncompressed_header, so it looks like things are working ok at first.

My next thought was to check compiler flags between the Firefox build and the standalone CMake build. There are quite a lot of differences, but nothing stands out as obviously causing problems.

Please let me know if you have any suggestions. Thanks!

[1] https://aomedia.googlesource.com/aom/+/6f49b5a214fa48c226be3bc28f5c597edb81ed8c/av1/decoder/decodeframe.c#2533
Flags: needinfo?(tdaede)
Correct, LIBYUV and WEBM_IO only affect the binary. It sounds like something with the demuxer is wrong. Can you confirm that you're testing a WebM file?
Flags: needinfo?(tdaede)
(Assignee)

Comment 29

10 months ago
I'm fairly certain, I changed the output filename to end in .webm, and the head of the file looks like this:
^ZEߣ<9f>B<86><81>^AB÷<81>^ABò<81>^DBó<81>^HB<82><84>webmB<87><81>^DB<85><81>^B^XS<80>g^Aÿÿÿÿÿÿÿìì^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^     @^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^UI©f¿*×±<83>^OB@D<89><84>?<     80>^@^@M<80><8f>libwebm-0.2.1.0WA<9c>aomenc 0.1.0-9043-g6f49b5a21^VT®k­®«×<81>^AsÅ<87>Å^SÿÁä@<9a><83><81>^A<86><85>V_AV1à<92>°<82>^A`º<82>^A T°<82>^A<81>Tº<82>^A ^_C¶u^Aÿÿ     ÿÿÿÿÿç<81>^@£ D<88><81>^@^@<80>^R^@
OK, looking at a WebM file generated here it looks like the sequence OBUs are placed in the frame SimpleBlocks, so no luck with that being the problem. One thought would be to issue all the ctls that aomdec.c:733 issues.

The sequence header should occur in the very first frame. If you get that error later, it's because you are somehow destroying and recreating a decoder context. Is some error handling path being invoked?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8968893 - Attachment is obsolete: true
(Assignee)

Comment 42

10 months ago
Posted video city_cif.webm
The attached file fails to play in Firefox when aom support is built using the attached patches, but works fine in a local build of vlc using the same version of libaom.

I built vlc like this:

AOM_CFLAGS=-I/home/dminor/src/aom AOM_LIBS="-L/home/dminor/src/aom/out -laom -lm" ./configure --enable-aom --disable-lua --disable-a52

:jya, Nils mentioned that you might be willing to have a look and see where I'm going wrong with this. Thanks!
Flags: needinfo?(jyavenard)
Can I just pull from this mozreview branch and test it?
(Assignee)

Comment 44

10 months ago
Yes, everything is on the mozreview branch, so there's no need to run the update script or anything like that.
The bug is there:
https://reviewboard-hg.mozilla.org/gecko/file/34e09022e288ff9aefa3a10137e96b1bd3662027/third_party/aom/av1/av1_dx_iface.c#l183

so every frames is marked as a keyframe.. nothing bad in itself, however decoder_peek_si is also used to determine the frame size, and if the frame size change, we tear-down the decoder and feed another one).

The problem is that the next decoder created, the new frame it will get is not a keyframe but marked as such: this leads to a decoding error.

Additionally, the code to determine the frame dimensions in AV1 is also broken.

There's been changed in more recent of AV1 decoder, but at a glance I don't think it's good enough yet.

I'll post a quick patch to work around for now...
Flags: needinfo?(jyavenard)
diff --git a/dom/media/platforms/agnostic/AOMDecoder.cpp b/dom/media/platforms/agnostic/AOMDecoder.cpp
index e2f43a52aad6..eaf2a29ae6da 100644
--- a/dom/media/platforms/agnostic/AOMDecoder.cpp
+++ b/dom/media/platforms/agnostic/AOMDecoder.cpp
@@ -77,6 +77,7 @@ InitContext(AOMDecoder& aAOMDecoder,
   PodZero(&config);
   config.threads = decode_threads;
   config.w = config.h = 0; // set after decode
+  config.allow_lowbitdepth = true;
 
   aom_codec_flags_t flags = 0;
 
diff --git a/dom/media/webm/WebMDemuxer.cpp b/dom/media/webm/WebMDemuxer.cpp
index 7b940ece5971..95b65ea3e66c 100644
--- a/dom/media/webm/WebMDemuxer.cpp
+++ b/dom/media/webm/WebMDemuxer.cpp
@@ -676,7 +676,8 @@ WebMDemuxer::GetNextPacket(TrackInfo::TrackType aType,
     if (aType == TrackInfo::kAudioTrack) {
       isKeyframe = true;
     } else if (aType == TrackInfo::kVideoTrack) {
-      if (packetEncryption == NESTEGG_PACKET_HAS_SIGNAL_BYTE_ENCRYPTED) {
+      if (packetEncryption == NESTEGG_PACKET_HAS_SIGNAL_BYTE_ENCRYPTED ||
+          mVideoCodec == NESTEGG_CODEC_AV1) {
         // Packet is encrypted, can't peek, use packet info
         isKeyframe = nestegg_packet_has_keyframe(holder->Packet())
                      == NESTEGG_PACKET_HAS_KEYFRAME_TRUE;
@@ -711,7 +712,7 @@ WebMDemuxer::GetNextPacket(TrackInfo::TrackType aType,
           NS_WARNING("Cannot detect keyframes in unknown WebM video codec");
           return NS_ERROR_FAILURE;
         }
-        if (isKeyframe) {
+        if (isKeyframe && mVideoCodec != NESTEGG_CODEC_AV1) {
           // For both VP8 and VP9, we only look for resolution changes
           // on keyframes. Other resolution changes are invalid.
           auto dimensions = gfx::IntSize(0, 0);

upgrading to the latest aom tree will fix the change of resolution issue, when so you can remove the change in if (isKeyframe && mVideoCodec != NESTEGG_CODEC_AV1)  

The change to AOMDecoder.cpp gives proper support for 8 bits videos, greatly increasing efficiency as you no longer need to convert 16->8 bits
bug https://bugs.chromium.org/p/aomedia/issues/detail?id=1934 was opened in regards to keyframe incorrectly set.

Upgrading aom to trunk's HEAD will fix the dimension calculations.
(Assignee)

Comment 48

10 months ago
:drno, given what Jean-Yves mentioned above, I think it makes sense to do the update to head rather than to an earlier revision. For vlc, anyway, a local build with configuration options is required for av1 support anyway. I assume the situation is the same for ffmpeg.
Flags: needinfo?(drno)
(Assignee)

Comment 49

10 months ago
discussed on irc, we'll go to a recent revision.
Flags: needinfo?(drno)
(Assignee)

Comment 50

10 months ago
I've updated to rev 1e954337be798ddb841de69b3ff0d435fa620fd, head as of yesterday morning.

When testing with a .webm encoded using a standalone checkout of libaom at the same revision, I get the following crash:

Thread 74 "MediaPD~oder #1" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffd21fe700 (LWP 12574)]
0x00007fffe76e58c1 in av1_make_masked_inter_predictor (
    pre=0x7fffb804c3d5 "yupjgebWUZ\\noamvuZTTVYW_hYTa\\ORW[dhjfa^ab``bkpprvqns}\201\177\177\177\200\201\200\177\204\213\205z|}|\177\205\206\206\207\210\207\211\216\216\217\207yqkfg}\224\226}^`]\\g]_pvtcXZXWYYZfsr`_s]^\217iIWV^mo[dd`b_f`_lwropfjyutxwrvvqvwpuwsu|xw|vp|}sw\177zy{\177\214x_[\\mvqprst{xw}xw|yv|ytz{wy|x"..., pre_stride=928, 
    dst=0x7fffb7e4c3d0 "", dst_stride=928, subpel_params=0x7fffd21c4860, 
    sf=0x7fffb9a38b68, w=16, h=16, conv_params=0x7fffd21c4810, interp_filters=131074, 
    plane=0, warp_types=0x7fffd21c47a0, p_col=144, p_row=48, ref=1, 
    xd=0x7fffb9a3f620, can_use_previous=1)
    at /home/dminor/src/firefox/third_party/aom/av1/common/reconinter.c:615
615	  const INTERINTER_COMPOUND_DATA comp_data = { mi->wedge_index, mi->wedge_sign,
(gdb) bt
#0  0x00007fffe76e58c1 in av1_make_masked_inter_predictor (pre=0x7fffb804c3d5 "yupjgebWUZ\\noamvuZTTVYW_hYTa\\ORW[dhjfa^ab``bkpprvqns}\201\177\177\177\200\201\200\177\204\213\205z|}|\177\205\206\206\207\210\207\211\216\216\217\207yqkfg}\224\226}^`]\\g]_pvtcXZXWYYZfsr`_s]^\217iIWV^mo[dd`b_f`_lwropfjyutxwrvvqvwpuwsu|xw|vp|}sw\177zy{\177\214x_[\\mvqprst{xw}xw|yv|ytz{wy|x"..., pre_stride=928, dst=0x7fffb7e4c3d0 "", dst_stride=928, subpel_params=0x7fffd21c4860, sf=0x7fffb9a38b68, w=16, h=16, conv_params=0x7fffd21c4810, interp_filters=131074, plane=0, warp_types=0x7fffd21c47a0, p_col=144, p_row=48, ref=1, xd=0x7fffb9a3f620, can_use_previous=1)
    at /home/dminor/src/firefox/third_party/aom/av1/common/reconinter.c:615
#1  0x00007fffe77691cb in dec_build_inter_predictors (cm=<optimized out>, xd=<optimized out>, plane=<optimized out>, mi=<optimized out>, build_for_obmc=<optimized out>, bw=<optimized out>, bh=16, mi_x=144, mi_y=48)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:543
#2  0x00007fffe7765f7e in dec_build_inter_predictors_for_planes (cm=0x7fffb9a37d40, xd=<optimized out>, mi_row=12, mi_col=<optimized out>, plane_from=0, plane_to=0, bsize=<optimized out>)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:575
#3  0x00007fffe7765f7e in dec_build_inter_predictors_sby (ctx=<optimized out>, cm=<optimized out>, xd=<optimized out>, mi_row=<optimized out>, mi_col=<optimized out>, bsize=<optimized out>)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:583
#4  0x00007fffe7765f7e in dec_build_inter_predictors_sb (cm=<optimized out>, xd=<optimized out>, mi_row=<optimized out>, mi_col=<optimized out>, ctx=<optimized out>, bsize=<optimized out>)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:617
#5  0x00007fffe7765f7e in decode_token_and_recon_block (pbi=0x7fffb9a2c020, xd=<optimized out>, mi_row=12, mi_col=<optimized out>, r=<optimized out>, bsize=BLOCK_16X16)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:863
#6  0x00007fffe7765f7e in decode_block (pbi=0x7fffb9a2c020, xd=<optimized out>, mi_row=12, mi_col=<optimized out>, r=<optimized out>, partition=PARTITION_NONE, bsize=BLOCK_16X16) at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:1098
#7  0x00007fffe7764bd8 in decode_partition (pbi=<optimized out>, xd=<optimized out>, mi_row=<optimized out>, mi_col=<optimized out>, r=<optimized out>, bsize=<optimized out>) at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:1186
#8  0x00007fffe7764d59 in decode_partition (pbi=<optimized out>, xd=<optimized out>, mi_row=<optimized out>, mi_col=<optimized out>, r=<optimized out>, bsize=<optimized out>) at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:1199
#9  0x00007fffe7764d40 in decode_partition (pbi=<optimized out>, xd=<optimized out>, mi_row=<optimized out>, mi_col=<optimized out>, r=<optimized out>, bsize=<optimized out>) at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:1198
#10 0x00007fffe776413c in decode_tile_sb_row (td=0x7fffb9a3f600, tile_info=..., pbi=<optimized out>, mi_row=<optimized out>)
---Type <return> to continue, or q <return> to quit---
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:2248
#11 0x00007fffe776413c in decode_tile (pbi=<optimized out>, td=0x7fffb9a3f600, tile_row=-1180428936, tile_col=<optimized out>)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:2294
#12 0x00007fffe7762599 in decode_tiles (pbi=0x7fffb9a2c020, data=<optimized out>, data_end=0x7fffc4b91280 "2\233\004(", <incomplete sequence \342>, startTile=0, endTile=0)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:2413
#13 0x00007fffe7762599 in av1_decode_tg_tiles_and_wrapup (pbi=0x7fffb9a2c020, data=<optimized out>, data_end=0x7fffc4b91280 "2\233\004(", <incomplete sequence \342>, p_data_end=0x7fffd21fd3f0, startTile=0, endTile=0, initialize_flag=1)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decodeframe.c:3985
#14 0x00007fffe7778044 in read_one_tile_group_obu (data=0x7fffc4b90f38 "\340\206\310\372f\322}o\263\245\207\363F\336I\212Ϲ\246\344\315_\216,\237{\351\020+\322\315\356\315\372`\026\060jЙ\374\245\222\251\022\003\242\331\360\251#\330\065.\235'9LP\277\020/8K&l\006\205&\271A\036\237r\177\234\307Z\v\300\240\222\364\025\274\350Cm\222\342\236\372P!c\033\247\300\340\324\305\003`i\252q\200G\321\065ӽ-_\240\255\377\301\355\274c\234+\366\342.\317\313k\031\362\246|\237\n\212/\366\311\336\t\262\272q\026\334b\341\342\254ń\203\226", data_end=0x7fffb7e4c3d0 "", p_data_end=0x7fffd21fd3f0, pbi=<optimized out>, rb=<optimized out>, is_first_tg=<optimized out>, is_last_tg=<optimized out>, tile_start_implicit=<optimized out>) at /home/dminor/src/firefox/third_party/aom/av1/decoder/obu.c:355
#15 0x00007fffe7778044 in aom_decode_frame_from_obus (pbi=<optimized out>, data=0x7fffc4b90f25 "(", <incomplete sequence \344>, data_end=<optimized out>, p_data_end=0x7fffd21fd3f0) at /home/dminor/src/firefox/third_party/aom/av1/decoder/obu.c:682
#16 0x00007fffe7773bba in av1_receive_compressed_data (pbi=0x7fffb9a2c020, size=<optimized out>, psource=0x7fffd21fd3f0)
    at /home/dminor/src/firefox/third_party/aom/av1/decoder/decoder.c:410
#17 0x00007fffe769df61 in frame_worker_hook (arg1=0x7fffccbadbc0, arg2=<optimized out>) at /home/dminor/src/firefox/third_party/aom/av1/av1_dx_iface.c:330
#18 0x00007fffe76969bc in execute (worker=0x7fffc4bf5a20)
    at /home/dminor/src/firefox/third_party/aom/aom_util/aom_thread.c:135
#19 0x00007fffe769c828 in decode_one (ctx=0x7fffb9a17010, user_priv=0x0, data=<optimized out>, data_sz=<optimized out>)
    at /home/dminor/src/firefox/third_party/aom/av1/av1_dx_iface.c:483
#20 0x00007fffe769c828 in decoder_decode (ctx=0x7fffb9a17010, data=<optimized out>, data_sz=140736493852143, user_priv=0x0)
    at /home/dminor/src/firefox/third_party/aom/av1/av1_dx_iface.c:548
#21 0x00007fffe74119c1 in aom_codec_decode (ctx=0x7fffc4bf0360, data=0x3a0 <error: Cannot access memory at address 0x3a0>, data_sz=140736278610896, user_priv=0x3a0)
    at /home/dminor/src/firefox/third_party/aom/aom/src/aom_decoder.c:111
#22 0x00007fffe68d58f7 in mozilla::AOMDecoder::ProcessDecode(mozilla::MediaRawData*) (this=0x7fffc4bf0340, aSample=0x7fffc4b7f300)
    at /home/dminor/src/firefox/dom/media/platforms/agnostic/AOMDecoder.cpp:205

There are also problems on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a61caffb3bccf5392bbe970962f6f7c15f926d1

It looks like 32 bit linux may be running out of memory during linking. It looks like 32 bit Windows builds are hitting an internal compiler error, maybe also memory related.

Despite this, I'd like to start getting reviews on the build system changes. I've done diffs against the generated files from my local cmake build and am not seeing any significant differences between the two. This, combined with the work jya did in comment 46 leads me to believe that the problems are most likely with libaom or how we are integrating it, not with the build system changes.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8981595 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8981592 - Attachment is obsolete: true
(Assignee)

Comment 93

9 months ago
It looks like it is necessary to set PIC for linux32 builds [1] due to how the x86 specific code in aom_subpixel_8t_ssse3.asm is written.

That leaves the win32 build errors [2]. It looks like the compiler is encountering an internal error on av1_inv_txfm_avx2.c. It also seems to be running out of heap space in that try run, but that did not occur in my previous try runs.

The crash in comment 50 remains even after updating to head of libaom as of yesterday morning. My test video works with ffmpeg compiled against the same version of libaom.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=50295accb28343e394d7fbb307d9630e6277529d
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d60072aafa6fd36205f4509b4c1578986801ed9
Thomas do you have any idea what the crash in comment #50 could be?
Flags: needinfo?(tdaede)
(Assignee)

Comment 95

9 months ago
I'm experimenting on try to see if commenting out the body of the function causing the avx2 compiler crash on win32 helps. Another possibility would be to just disable avx2 for win32 for now.
(Assignee)

Comment 96

9 months ago
Fwiw, the crash in comment 50 does not reproduce with a debug ASAN build.
Attachment #8968888 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8968889 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8968890 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8968891 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8968892 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8981590 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8981591 - Flags: review?(core-build-config-reviews) → review?(cmanchester)

Comment 97

9 months ago
mozreview-review
Comment on attachment 8968890 [details]
Bug 1445683 - Update generate_sources_mozbuild.sh;

https://reviewboard.mozilla.org/r/237604/#review258048

::: media/libaom/generate_sources_mozbuild.py:74
(Diff revision 4)
> -        'CONFIG_LOWBITDEPTH': 1,
> +        'CONFIG_LOWBITDEPTH': 0,
>          'CONFIG_LV_MAP': 0,
>          'CONFIG_MOTION_VAR': 0,
>          'CONFIG_MULTITHREAD': 1,
>          'CONFIG_NCOBMC_ADAPT_WEIGHT': 0,
> -        'CONFIG_PIC': 1,
> +        'CONFIG_PIC': 0,
>          'CONFIG_PVQ': 0,

This looks like it belongs in the prior patch.
Attachment #8968890 - Flags: review?(cmanchester) → review+

Comment 98

9 months ago
mozreview-review
Comment on attachment 8968891 [details]
Bug 1445683 - Set executable bit on generate_sources_mozbuild.sh;

https://reviewboard.mozilla.org/r/237606/#review258050
Attachment #8968891 - Flags: review?(cmanchester) → review+

Comment 99

9 months ago
mozreview-review
Comment on attachment 8968892 [details]
Bug 1445683 - Add aom_version.h;

https://reviewboard.mozilla.org/r/237608/#review258052

This is just a rubber stamp from me.
Attachment #8968892 - Flags: review?(cmanchester) → review+
Comment on attachment 8981590 [details]
Bug 1445683 - Add support for SSE 4.2 to libaom moz.build;

https://reviewboard.mozilla.org/r/247702/#review258054
Attachment #8981590 - Flags: review?(cmanchester) → review+
Comment on attachment 8981591 [details]
Bug 1445683 - Handle JSONDecodeError in aom vendor command;

https://reviewboard.mozilla.org/r/247704/#review258056
Attachment #8981591 - Flags: review?(cmanchester) → review+
Comment on attachment 8968889 [details]
Bug 1445683 - Add generate_sources_mozbuild.py;

https://reviewboard.mozilla.org/r/237602/#review258044

Looks good. A few small questions.

::: media/libaom/generate_sources_mozbuild.py:22
(Diff revision 4)
> +    cache_variables = filter(lambda x: not x.startswith((' ', 'CMAKE', 'AOM')),
> +                             cache_variables)

I'm told list comprehensions are preferred in python over map/filter/etc. Anyway it's probably better not to mix them, so this should probably be `[x for x in cache_variables if not x.starstwith...`

::: media/libaom/generate_sources_mozbuild.py:56
(Diff revision 4)
> +                f.write('.equ %s, %s\n' % (var, variables[var]))
> +            else:
> +                f.write('%s equ %s\n' % (var, variables[var]))
> +
> +        if arch == 'arm':
> +	        f.write('.section	.note.GNU-stack,"",%progbits')

This block is indented too far.

::: media/libaom/generate_sources_mozbuild.py:62
(Diff revision 4)
> +
> +
> +if __name__ == '__main__':
> +    import sys
> +
> +    shared_variables = {

We might want review from someone familiar with the meaning of these options.

::: media/libaom/generate_sources_mozbuild.py:145
(Diff revision 4)
> +            # Remove header files
> +            sources = sorted(filter(lambda x: not x.endswith('.h'), sources))

This subsumes the loop just above where we remove each export, right?

::: media/libaom/generate_sources_mozbuild.py:145
(Diff revision 4)
> +            # Remove header files
> +            sources = sorted(filter(lambda x: not x.endswith('.h'), sources))

This subsumes the work of the loop just above where we remove each export, right?

::: media/libaom/generate_sources_mozbuild.py:153
(Diff revision 4)
> +            # The build system is unhappy if two files have the same prefix
> +            # In libaom, sometimes .asm and .c files share the same prefix
> +            for i in xrange(len(sources) - 1):
> +                if sources[i].endswith('.asm'):
> +                    if os.path.splitext(sources[i])[0] == os.path.splitext(sources[i + 1])[0]:
> +                        sources[i] = sources[i].replace('.asm', '_asm.asm')

I must be missing something here, don't we need to copy the file over to its newly prefixed location as well?
Attachment #8968889 - Flags: review?(cmanchester) → review+
Comment on attachment 8968888 [details]
Bug 1445683 - Add Python cmake parser;

https://reviewboard.mozilla.org/r/237600/#review258036

This is basically good to go but I have a few questions. I understand this is a best effort sort of thing to get the cmake output for libaom to parse, so most of my comments can probably be dropped.

::: media/libaom/cmakeparser.py:15
(Diff revision 4)
> +arguments << (argument | (Literal('(') + ZeroOrMore(arguments) + Literal(')')))
> +identifier = Word(alphas, alphanums+'_')
> +command = Group(identifier + arguments)

Something looks a little off here... are we enforcing that the "top-level" arguments are surrounded by parens? It sort of looks like `cmd"arg"(arg2)` would be recognized.

::: media/libaom/cmakeparser.py:139
(Diff revision 4)
> +                        variables[variable] = ''
> +                    variables[variable] += ' ' + ' '.join(values)

We're ok with spaces at the beginning of lists?

::: media/libaom/cmakeparser.py:166
(Diff revision 4)
> +            except IndexError:
> +                pass
> +            except KeyError:
> +                pass

Lines like these can be consolidated to `except (IndexError, KeyError):`

::: media/libaom/cmakeparser.py:259
(Diff revision 4)
> +        elif op == 'STREQUAL':
> +            rhs = lookup_variable(arguments[2])
> +            if not rhs:
> +                rhs = arguments[2]
> +            return lhs == rhs
> +    except IndexError:

This looks like it's catching the cases we have  `len(arguments) == 0` or dangling operators. If so this would be better as an if/else with some kind of error surfaced in the case of dangling operators rather than try/except.

::: media/libaom/test_cmakeparser.py:1
(Diff revision 4)
> +import unittest
> +
> +import cmakeparser as cp

License header? It also would be good to hook these in to be run in automation, I doubt they take very long to run.

::: media/libaom/test_cmakeparser.py:8
(Diff revision 4)
> +        self.assertFalse(cp.evaluate_boolean({}, ['NOT', 'TRUE']))
> +        self.assertFalse(cp.evaluate_boolean({}, ['TRUE', 'AND', 'FALSE']))
> +        self.assertTrue(cp.evaluate_boolean({}, ['ABC', 'MATCHES', '^AB']))
> +        self.assertTrue(cp.evaluate_boolean({}, ['TRUE', 'OR', 'FALSE']))

I don't know if it's relevant to the kinds of inputs we're likely to get, but some tests of this weird nested parens syntax I'm seeing in some examples (like `if(FALSE AND (FALSE OR TRUE)) # evaluates to FALSE` ) might be good.

::: media/libaom/test_cmakeparser.py:130
(Diff revision 4)
> +if __name__ == '__main__':
> +    unittest.main()

A few tests demonstrating the parser doesn't accept malformed inputs might be nice.
Attachment #8968888 - Flags: review?(cmanchester)
(Assignee)

Comment 104

9 months ago
As jya pointed out on irc, the crash I'm seeing on Linux is due to running out of stack space, changing [1] to use the default stack size fixes things.

[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/xpcom/threads/nsIThreadManager.idl#53
Flags: needinfo?(tdaede)
(Assignee)

Comment 105

9 months ago
mozreview-review-reply
Comment on attachment 8968889 [details]
Bug 1445683 - Add generate_sources_mozbuild.py;

https://reviewboard.mozilla.org/r/237602/#review258044

> We might want review from someone familiar with the meaning of these options.

If these change in the future, definitely yes, but in this case, I've already done a diff against a local run of cmake and asked TD-Linux about the discrepancies (in bug comment 27) so I don't think we need an additional review this time.

> This subsumes the loop just above where we remove each export, right?

Yes, but we have to write out the exports later on. This chunk of code was derived directly from generate_sources_mozbuild.sh. I played around a little bit, and it looks like the exports are necessary for the build to work, so unless I'm missing something, I don't think this can be simplified.
(Assignee)

Comment 106

9 months ago
:dmajor, I'm having some trouble with a win32 only internal compiler error [1]. The problem seems to be two functions in av1_inv_txfm_avx2.c. I've tried a few things, like breaking the function up into smaller pieces, but I haven't gotten anywhere. One workaround would be to disable avx2 support on win32, but before I do that, I thought I would check with you and see if you have any suggestions for me. Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d60072aafa6fd36205f4509b4c1578986801ed9
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce5c24a7b4d08a521cf5625e723efa7dad78929c
Flags: needinfo?(dmajor)
c1: fatal error C1060: compiler is out of heap space

That's... troubling.

If I'm reading your patch stack correctly, it looks like your update has undone attachment 8966669 [details] [diff] [review] by reintroducing `#define INLINE __forceinline`. Does changing that help?
Flags: needinfo?(dmajor)
(Assignee)

Comment 108

9 months ago
(In reply to David Major [:dmajor] from comment #107)
> c1: fatal error C1060: compiler is out of heap space
> 
> That's... troubling.
> 
> If I'm reading your patch stack correctly, it looks like your update has
> undone attachment 8966669 [details] [diff] [review] by reintroducing
> `#define INLINE __forceinline`. Does changing that help?

Yep, that was the problem. That patch landed after I started this work, and I have only been comparing flags on Linux, so I didn't catch the change. Thanks!
(Assignee)

Comment 109

9 months ago
Looks like I spoke too soon, we still run out of memory on win32 pgo builds.
(Assignee)

Comment 110

9 months ago
Recent try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5a8ad27d2ec7329e630fd0d39fec42f5c230464

The win32 pgo builds still run out of memory, perhaps it is an intermittent problem, as I had a green try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d095093ee25b6b7c0e39487201eb7d2365ccb03 where I forced no inlining in libaom.

I'm going to push the patches up for (re-)review on the assumption that we'll find a fix or workaround for win32 pgo.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8985693 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8981594 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8984119 - Attachment is obsolete: true

Comment 123

9 months ago
mozreview-review
Comment on attachment 8988280 [details]
Bug 1445683 - Increase stack size in nsIThreadManager to 512*1024;

https://reviewboard.mozilla.org/r/253536/#review260130

I'm uneasy about changing a global setting to solve an av1 problem. Can we do something more targeted?

For example, I presume av1 is generating huge stack frames due to excessive inlining. Can we find the most problematic stack frames and slice them apart with MOZ_NEVER_INLINE? I can help with looking at frame sizes from a crashed minidump if you have one.
Attachment #8988280 - Flags: review?(dmajor)

Comment 124

9 months ago
mozreview-review-reply
Comment on attachment 8988280 [details]
Bug 1445683 - Increase stack size in nsIThreadManager to 512*1024;

https://reviewboard.mozilla.org/r/253536/#review260130

The main difference is that local coefficient buffers grew from 32x32 in libvpx to 128x128 in libaom. Looking at the original crashing stack trace, there isn't that much inlining, and more importantly I don't think inlining would increase stack usage in this case.
(Assignee)

Comment 125

9 months ago
(In reply to David Major [:dmajor] from comment #123)
> Comment on attachment 8988280 [details]
> Bug 1445683 - Increase stack size in nsIThreadManager to 512*1024;
> 
> https://reviewboard.mozilla.org/r/253536/#review260130
> 
> I'm uneasy about changing a global setting to solve an av1 problem. Can we
> do something more targeted?
> 
> For example, I presume av1 is generating huge stack frames due to excessive
> inlining. Can we find the most problematic stack frames and slice them apart
> with MOZ_NEVER_INLINE? I can help with looking at frame sizes from a crashed
> minidump if you have one.

Hmm, well after I rebased on central, I no longer get a crash, which doesn't inspire a lot of confidence. I'm about to leave on PTO, I'll rebase when I get back on July 9th and if the crash reappears, I'll generate a minidump.

If we're on the edge of running out of stack space, I'm not sure if we should land things as is, even with av1 playback behind a pref. The stack trace for the crash I was seeing is in comment 50.

Comment 126

9 months ago
mozreview-review
Comment on attachment 8981593 [details]
Bug 1445683 - Updates to AOMDecoder;

https://reviewboard.mozilla.org/r/247708/#review260150

::: dom/media/platforms/agnostic/AOMDecoder.cpp
(Diff revision 5)
>        return AOM_CODEC_INVALID_PARAM;
>    switch (dst->fmt) {
>      case AOM_IMG_FMT_I420:
>      case AOM_IMG_FMT_I422:
>      case AOM_IMG_FMT_I444:
> -    case AOM_IMG_FMT_I440:

Those don't exist any more?
Attachment #8981593 - Flags: review?(jyavenard) → review+

Comment 127

9 months ago
mozreview-review-reply
Comment on attachment 8981593 [details]
Bug 1445683 - Updates to AOMDecoder;

https://reviewboard.mozilla.org/r/247708/#review260150

> Those don't exist any more?

Correct, AV1 dropped support for 4:4:0 and I subsequently removed it from the API.

Comment 128

9 months ago
mozreview-review
Comment on attachment 8981593 [details]
Bug 1445683 - Updates to AOMDecoder;

https://reviewboard.mozilla.org/r/247708/#review260156

::: commit-message-ba355:2
(Diff revision 5)
> +Bug 1445683 - Updates to AOMDecoder; r=jya
> +

Please provide more details on the  for these changes

Comment 129

9 months ago
mozreview-review
Comment on attachment 8988278 [details]
Bug 1445683 - Update aom to rev 70bffc7d824b299cfdb16e3c96f277ab6475836f;

https://reviewboard.mozilla.org/r/253532/#review260160

::: dom/media/platforms/agnostic/AOMDecoder.cpp:352
(Diff revision 1)
>  {
>    // While AV1 is under development, we describe support
>    // for a specific aom commit hash so sites can check
>    // compatibility.
>    auto version = NS_ConvertASCIItoUTF16("av1.experimental.");
> -  version.AppendLiteral("e87fb2378f01103d5d6e477a4ef6892dc714e614");
> +  version.AppendLiteral("70bffc7d824b299cfdb16e3c96f277ab6475836f");

Seeing that AV1 is  now behind aa pref and following head. Shouldn't this special mimetype be now dropped?

::: media/libaom/config/generic/config/aom_config.h:22
(Diff revision 1)
> +#define ARCH_X86 0
> +#define ARCH_X86_64 0
> +#define CONFIG_ACCOUNTING 0
> +#define CONFIG_ANALYZER 0
> +#define CONFIG_AV1_DECODER 1
> +#define CONFIG_AV1_ENCODER 1

Do we need the encoder today? It isn't used and I'm guessing takes aa significant amount of
Attachment #8988278 - Flags: review?(jyavenard) → review+

Comment 130

9 months ago
mozreview-review
Comment on attachment 8988278 [details]
Bug 1445683 - Update aom to rev 70bffc7d824b299cfdb16e3c96f277ab6475836f;

https://reviewboard.mozilla.org/r/253532/#review260162

II find it terrifying that it could generate that many config files...
(In reply to David Major [:dmajor] from comment #123)
> Comment on attachment 8988280 [details]
> Bug 1445683 - Increase stack size in nsIThreadManager to 512*1024;
> 
> https://reviewboard.mozilla.org/r/253536/#review260130
> 
> I'm uneasy about changing a global setting to solve an av1 problem. Can we
> do something more targeted?
> 
> For example, I presume av1 is generating huge stack frames due to excessive
> inlining. Can we find the most problematic stack frames and slice them apart
> with MOZ_NEVER_INLINE? I can help with looking at frame sizes from a crashed
> minidump if you have one.

It's not the first time we had to bump those limits. First on Mac and the H264 software decoder, then other platforms.
I feel that 256kB was a number plucked out of thin air with no analysis on what was actually needed. And whenever this turned out to not be enough we waste days troubleshooting things as identifying this as the cause isn't straightforward
Comment on attachment 8988279 [details]
Bug 1445683 - Do not build aomstats unless examples are enabled;

https://reviewboard.mozilla.org/r/253534/#review260230
Attachment #8988279 - Flags: review?(cmanchester) → review+
Comment on attachment 8984118 [details]
Bug 1445683 - Generated files now live under 'config';

https://reviewboard.mozilla.org/r/249632/#review260232
Attachment #8984118 - Flags: review?(cmanchester) → review+
Comment on attachment 8968888 [details]
Bug 1445683 - Add Python cmake parser;

https://reviewboard.mozilla.org/r/237600/#review260278

Thanks for the updates here.
Attachment #8968888 - Flags: review?(cmanchester) → review+
Hi folks.

I'm just a Firefox user and have been following the AV1 project closely.

I noticed the upstream aom repo has a "v1.0.0" tag just 23 commits past the commit it looks like you're planning to update to.

(It seems like you are updating to 70bffc7d824b299cfdb16e3c96f277ab6475836f, and the upstream "v1.0.0" tag now points to d14c5bb4f336ef1842046089849dee4a301fbbf0).

I would like to suggest that now is a great time to update directly to the "v1.0.0" milestone. (If everyone starts doing that, we could all have pretty good interoperability. I know VLC already has updated to the "v1.0.0" milestone for their decoder, for example.)

Thanks for your consideration. (And thanks Mozilla, for helping to make Daala and AV1 happen! And for making Firefox!)
It looks like this is just waiting on the stack size increase? What can we do here?
(Assignee)

Comment 137

9 months ago
(In reply to Dan D (Not an employee, just a tester) from comment #135)
> Hi folks.
> 
> I'm just a Firefox user and have been following the AV1 project closely.
> 
> I noticed the upstream aom repo has a "v1.0.0" tag just 23 commits past the
> commit it looks like you're planning to update to.
> 
> (It seems like you are updating to 70bffc7d824b299cfdb16e3c96f277ab6475836f,
> and the upstream "v1.0.0" tag now points to
> d14c5bb4f336ef1842046089849dee4a301fbbf0).
> 
> I would like to suggest that now is a great time to update directly to the
> "v1.0.0" milestone. (If everyone starts doing that, we could all have pretty
> good interoperability. I know VLC already has updated to the "v1.0.0"
> milestone for their decoder, for example.)
> 
> Thanks for your consideration. (And thanks Mozilla, for helping to make
> Daala and AV1 happen! And for making Firefox!)

Thank you for pointing that out, I think that makes sense. The plan was to do an update and then update again to "v1.0.0" once it was available, but this update has taken much longer than expected!
(Assignee)

Comment 138

9 months ago
(In reply to Thomas Daede [:TD-Linux] from comment #136)
> It looks like this is just waiting on the stack size increase? What can we
> do here?

I rebased on central and updated to v1.0.0 today and I'm no longer seeing the stack size related crash. Since this is currently behind a pref I guess we could land with the current stack size and see if we run into trouble.

The other problem is that we run out of memory on win32 pgo builds causing intermittent build failures such as [1]. We definitely need to fix that before we can land.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2507bb93d4466c00d02d727f5ce0aba3a4546b89
Duplicate of this bug: 1453997
(Assignee)

Comment 140

9 months ago
:dmajor, any suggestions on how to proceed with the intermittent OOM on win32 pgo builds [1]?

I tried setting:
#define INLINE __declspec(noinline)

which I think would disable inlining completely, but I'm still seeing failures, although maybe they are less frequent. Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2507bb93d4466c00d02d727f5ce0aba3a4546b89
Flags: needinfo?(dmajor)
Well if __declspec(noinline) didn't work then this isn't looking very hopeful. None of the options are very good.

* Talk releng into putting this on a bigger machine instance?
* Move AOM to its own DLL?
* Disable <something> on Win32 builds, where I don't know how big that something is. AVX in AOM? PGO in AOM? All optimization in AOM? All of AOM?

The one good news is that we're getting close to switching official builds to clang-cl in bug 1443590. Maybe we can live with an unoptimized AOM until then.

Ryan, do you happen to have a VS preview up on tooltool just on the off chance that they've fixed this recently?
Flags: needinfo?(dmajor) → needinfo?(ryanvm)
I haven't created one, no. Don't really have cycles to at the moment either.
Flags: needinfo?(ryanvm)
(Assignee)

Comment 143

9 months ago
I think we already have PGO disabled for aom for win32 builds: https://searchfox.org/mozilla-central/source/media/libaom/moz.build#50.
(Assignee)

Comment 144

9 months ago
I discussed the stack size related crash with :drno and he's fine with landing things as they are now, since we're behind a pref anyway. I filed Bug 1474684 as a follow on for stack problems.

That means the only blocker here is the win32 pgo problem.
According to our Telemetry data https://hardware.metrics.mozilla.com/ only ~30% of the Firefox users still use 32bit binaries. And only ~18% are still running on 32bit OS's.

Assuming these are old machine not powerful enough any way to decode AV1 (at least as of right now) I think another option could be to simply not build AV1 for 32bit Windows. If we are going to build an AV1 performance test (similar to the VP9 test we have already) it would probably disable AV1 playback on these machines any way.
30% is a big hit to take, many of these machines probably can't decode high resolution AV1 but can happily decode lower resolution. Unlike when the VP9 test landed, we now have the Media Capabilities API to expose this to the web page.

How horrible is putting AOM in a separate dll?

I suppose we could also land without win32 for now and try to add it later.
:dmajor an alternative would be for only the media decoders to use a 512kB stack for now, everything else could use 256kB.

In fact the default stack size was increased from 128kB because some 3rd party frameworks (Apple CoreVideo, libvpx) needed over 200kB. So if we use different stack size depending on the type of threads use, we could overall gain massively as all threads could be set to use 128kB again.
Flags: needinfo?(dmajor)
What info is needed in the needinfo?

Now that bug 1474684 is filed let's move stack size discussion there.
Flags: needinfo?(dmajor)
(Assignee)

Comment 149

9 months ago
It looks like the clang-cl switch fixed the win32 pgo build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be303f0e21a7d1295c0fd9fb2a05ca3890ec4f7

I've done some retriggers there just in case, and I'll do another full set of builds on try, but barring anything unexpected, I think this is now ok to land.
Hi again, folks. Another quick observation about the upstream aom repo:

There are a few more commits past "v1.0.0" at the "av1-normative" branch. https://aomedia.googlesource.com/aom/+log/av1-normative (They're bug fixes.)

I assume the "v1" milestone is actively maintained on the "av1-normative" branch, but that's just an "educated guess" to be honest. Reading between the lines, these three bug fix commits are by are folks from two companies (Argon Design, Allegro DVT) who are each making fuzzing systems for AV1 decoders. I think they must have found these bugs in the process of making their fuzzing systems.
Seeing the rate at which the AV1 decoder is being worked on, if we were to always wait for the last version, we would never push anything!

Let get this landed, we can perform further update later (which would be much minor than this current task)
Yeah, the changes on av1-normative past v1.0.0 are "errata". We'll actually want to follow the master branch instead, because it'll also contain speed improvements and the like. But we don't need to do that in this bug.

The existing patches LGTM, without the stack size increase (assuming it's working now without).

Comment 153

8 months ago
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f760232724f
Add Python cmake parser; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b9f6b34ea89
Add generate_sources_mozbuild.py; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e6b648152b5
Update generate_sources_mozbuild.sh; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/6baf4a04beb6
Set executable bit on generate_sources_mozbuild.sh; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f4f0860c7e
Add aom_version.h; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1bd9413b789
Add support for SSE 4.2 to libaom moz.build; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1c87de8b85
Handle JSONDecodeError in aom vendor command; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/26fb32120d5c
Don't update mimetype when updating aom; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ade4353bf78
Update aom to v1.0.0; r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/83f43f8c169f
Do not build aomstats unless examples are enabled; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a9e3c9ca90
Generated files now live under 'config'; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68935c355ab
Updates to AOMDecoder; r=jya
(Assignee)

Comment 155

8 months ago
This would have been backed out when Bug 1474756 lands. It sounds as though the intention is to run msvc builds for a long enough time that we'll have to try another workaround for the win32 pgo problem.
Flags: needinfo?(dminor)
(In reply to Thomas Daede [:TD-Linux] from comment #146)
> I suppose we could also land without win32 for now and try to add it later.

Yes I think that is what we should do here. We can look at what it takes to get Win32 working later in a follow up bug.
Blocks: 1475318
(Assignee)

Comment 158

8 months ago
I've filed Bug 1475564 to track reenabling av1 support for win32.

A try run with libaom disabled for win32: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6378797aec7a6be42a23b7bf4f928827b8b56f64
(Assignee)

Comment 159

8 months ago
Disables av1 by default on win32.

Using splinter instead of MozReview as this is from a different repo than the one from which I originally pushed to MozReview, and I had a bad experience once where pushing like that reset all of the review requests to r?.
Attachment #8991931 - Flags: review?(cmanchester)
Attachment #8991931 - Flags: review?(cmanchester) → review+

Comment 160

8 months ago
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97322a202c75
Add Python cmake parser; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b4bbd9e956
Add generate_sources_mozbuild.py; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/a156e4250811
Update generate_sources_mozbuild.sh; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd74abb4b1a
Set executable bit on generate_sources_mozbuild.sh; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc746b7b438
Add aom_version.h; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/b13d106a0a86
Add support for SSE 4.2 to libaom moz.build; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/a77a850eead4
Handle JSONDecodeError in aom vendor command; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/383fb5a261ff
Don't update mimetype when updating aom; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f16daade35d
Update aom to v1.0.0; r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/be48a07172d3
Do not build aomstats unless examples are enabled; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/e855ea274395
Generated files now live under 'config'; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a7c4597152c
Updates to AOMDecoder; r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ac908007b9
Disable av1 on win32; r=chmanchester
Depends on: 1476248
(Assignee)

Updated

8 months ago
No longer depends on: 1476248
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ac908007b9
> Disable av1 on win32; r=chmanchester

Interestingly, this made win32 installers 800k/1.5% smaller. That's a lot of code!
You need to log in before you can comment on or make changes to this bug.