Update aom to pull in recent changes. For this I'd like to use a branch with some non-default config.

aomenc/aomdec work with this commit, but I can't get Firefox to play the test file I made. https://people.xiph.org/~giles/2017/sintel_trailer_2k_480p24.av1.v0.1.0-5374-gebc9c9e.cpu3.webm

This is because aom has started producing mkv instead of webm.

I applied this patch series and https://demo.bitmovin.com/public/firefox/av1/simple.html doesn't play anymore. Am I doing something wrong?

simple.html is currently pulling segments encoded with libaom commit adbb0251996c8ebb8310567bea330ab7ae9abe4. This patch set bumps the decoder to expect content encoded with libaom commit ebc9c9ef76c23b57b5583081416b5ee839e1be0a. I wouldn't expect them to be compatible. I've been putting up test encodes at https://people.xiph.org/~giles/2017/ since people-mozilla.org was taken down. You could try those, but they didn't work for me either. I didn't figure out why before going on vacation. The full bitmovin demo should be updated next week sometime. I'll update the bug then.

I had trouble getting playback to work with this revision, and xiph-rc wasn't hitting bitrates in encoder testing. The new target is upstream commit f5bdeac22930ff4c6b219be49c843db35970b918 but there's something wrong with the assembly optimization mapping where I get lots of undefined symbols like > "_aom_highbd_10_masked_sub_pixel_variance4x16_c", referenced from: > _setup_rtcd_internal in aom_dsp_rtcd.o

I tracked down some of the issues which are blocking this update. The build failure is because we have extra functions defined in aom_dsp_rtcd.h compared to what upstream configure generates for the same options. Copying over the file the upstream configure generates resolves that issue. This is generated by aom_dsp/aom_dsp_rtcd_defs.pl, which we invoke directly. Presumedly the upstream build system is passing something slightly different, but I haven't figured out what yet, other than --enable-external-build. Then, as with the earlier update, we reject frames encoded with the new commit id. This is because the superframe index was moved to the beginning of the frame without updating decoder_peek_si_internal, so when we try to read the keyframe status we get a decode error. If we just pretend every frame is a keyframe, playback is successful. I filed https://bugs.chromium.org/p/aomedia/issues/detail?id=681 for this. aom_codec_get_frame() now returns 12-bit channels by default, at least with my test encode. This is great, because we just added 10 bit support, so I need to hook that up for this codec. It's also not great, because the input to the test encode was 8 bit, so we're probably wasting performance. Maybe the higher component resolution helps with error accumulation though.

Comment on attachment 8885429 [details] Bug 1380118 - aom: Filter out CONFIG_EXT_PARTITION_TYPES. https://reviewboard.mozilla.org/r/156272/#review171756 ::: commit-message-ae5a4:7 (Diff revision 4) > + > +Update our vendor script to support commit query and > +snapshot download from github as well as upstream's > +gitiles instance. > + > +This lets us work with experimental branches for testing. I ended up not using this for this particular update, but would like to land it anyway so it's available in the future.

Comment on attachment 8885431 [details] Bug 1380118 - aom: add x86-win32-gcc config. https://reviewboard.mozilla.org/r/156276/#review171816

Comment on attachment 8895182 [details] Bug 1380118: Make aom_config.asm match upstream. https://reviewboard.mozilla.org/r/166338/#review171818

Comment on attachment 8885431 [details] Bug 1380118 - aom: add x86-win32-gcc config. https://reviewboard.mozilla.org/r/156276/#review171822

Comment on attachment 8895183 [details] Bug 1380118 - aom: remove unused option. https://reviewboard.mozilla.org/r/166340/#review171824 Wouldn't it be better to leave it in so AVX gets disabled appropriately in the future? kinetik probably understands the issues here better than I do.

Comment on attachment 8895182 [details] Bug 1380118: Make aom_config.asm match upstream. https://reviewboard.mozilla.org/r/166338/#review171828

Comment on attachment 8885429 [details] Bug 1380118 - aom: Filter out CONFIG_EXT_PARTITION_TYPES. https://reviewboard.mozilla.org/r/156272/#review171830 Largely assuming this is reasonable without running it myself. Some comments below. ::: python/mozbuild/mozbuild/vendor_aom.py:28 (Diff revision 4) > + elif 'github' in self.base_url: > + return mozpath.join(self.base_url, 'archive', revision + '.tar.gz') I'm going to assume that you might modify `VendorAOM.base_url` at some point in the future, or add an option to select which upstream to use to the script, because currently all this `'github' in self.base_url` code is dead code. Is that a correct assumption? You may want to add an explanatory comment before `base_url` saying that we have support for pulling from `https://github.com/mozilla/aom/` or such, too, and therefore have support for that below. May also want to verify servers for `base_url` somewhere, just to catch people trying to add a third option. ::: python/mozbuild/mozbuild/vendor_aom.py:55 (Diff revision 4) > # As of 2017 May, googlesource sends 4 garbage characters > # at the beginning of the json response. Work around this. > # https://bugs.chromium.org/p/chromium/issues/detail?id=718550 Hilarious. ::: python/mozbuild/mozbuild/vendor_aom.py:78 (Diff revision 4) > + > def fetch_and_unpack(self, revision, target): > '''Fetch and unpack upstream source''' > url = self.upstream_url(revision) > self.log(logging.INFO, 'fetch', {'url': url}, 'Fetching {url}') > - filename = 'libaom-' + revision + '.tar.gz' > + prefix = 'aom-' + revision Does everything use just `aom-` now? That seems like a surprising change to make for the googlesource version too?

(In reply to Nathan Froyd [:froydnj] from comment #34) > https://reviewboard.mozilla.org/r/166340/#review171824 > > Wouldn't it be better to leave it in so AVX gets disabled appropriately in > the future? I thought it was dead code because all our supported toolchains can generate AVX instructions, and was simply cargo culted from the libvpx scripts. I couldn't find anything actually setting them. If it's helpful for tier-3 builds, I'm happy to leave it in.

Comment on attachment 8885429 [details] Bug 1380118 - aom: Filter out CONFIG_EXT_PARTITION_TYPES. https://reviewboard.mozilla.org/r/156272/#review171830 > I'm going to assume that you might modify `VendorAOM.base_url` at some point in the future, or add an option to select which upstream to use to the script, because currently all this `'github' in self.base_url` code is dead code. Is that a correct assumption? You may want to add an explanatory comment before `base_url` saying that we have support for pulling from `https://github.com/mozilla/aom/` or such, too, and therefore have support for that below. > > May also want to verify servers for `base_url` somewhere, just to catch people trying to add a third option. Yes, that was the idea. I did test pulling from https://github.com/mozilla/aom, but ended up dropping the patch that did so for this update since there were other issues with the fork I wanted to try. I'll add a comment and see if I can come up with some validation. > Does everything use just `aom-` now? That seems like a surprising change to make for the googlesource version too? This is just the filename we save things to, independent of what the website sends. Both googlesource and github set content-disposition headers with a filename of `aom-$version.tar.gz` so while we don't honour that header directly, using it for both sites is more consistent than using `libaom-$version.tar.gz` would have been. The googlesource tarball unpacks into the current directory, while the github tarball more properly unpacks into an `aom-$version` subdirectory, matching the indicated filename, so changing to the expected default makes more sense there too.

Comment on attachment 8885429 [details] Bug 1380118 - aom: Filter out CONFIG_EXT_PARTITION_TYPES. https://reviewboard.mozilla.org/r/156272/#review171830 > Yes, that was the idea. I did test pulling from https://github.com/mozilla/aom, but ended up dropping the patch that did so for this update since there were other issues with the fork I wanted to try. I'll add a comment and see if I can come up with some validation. I added a `--repo` switch and a validation routine, rearranged things a bit, and made commit lookup work against a specified github fork, not just the mozilla one. Hopefully this is less confusing.

Two remaining issues: generate_sources.sh is producing different aom_config.* from the native build, and in particular aom_dsp_rtcd.h results in undefined symbol errors. I've hacked this for now by copying the files generated by native builds for linux and mac. The current patch set builds and plays https://people.xiph.org/~giles/2017/sintel_trailer_2k_480p24.v0.1.0-5529-gf5bdeac22.cpu1.webm The page at https://demo.bitmovin.com/public/firefox/av1/ has also been updated and plays, but there is significant frame drop with my local builds. I'd like to understand that better before updating. It looks like it's skipping to the next keyframe on the higher-bitrate segments, but MOZ_LOG=MediaFormatReader:5 doesn't say that's happening. It could be a performance issue, but the behaviour is similar on my 13" macbook pro to my fast desktop. Jean-Yves said problems with the timestamps leading to incorrect frame durations can present similarly.

Comment on attachment 8895183 [details] Bug 1380118 - aom: remove unused option. https://reviewboard.mozilla.org/r/166340/#review171954

Comment on attachment 8885430 [details] Bug 1380118 - aom: Support vendoring from github. https://reviewboard.mozilla.org/r/156274/#review172026

Comment on attachment 8885433 [details] Bug 1380118 - Export aom_config.h. https://reviewboard.mozilla.org/r/156280/#review172028

Comment on attachment 8895183 [details] Bug 1380118 - aom: remove unused option. https://reviewboard.mozilla.org/r/166340/#review172036 Looks like it's unused (i.e. never set in our build) for the libvpx version, too. Seems to have originated in Chromium's script, but they actually set it when necessary.

Comment on attachment 8895185 [details] Bug 1380118 - Fix stream info peeking. https://reviewboard.mozilla.org/r/166344/#review172040 Just confirming: this will be rolled in to the next update, so we don't need to carry a .patch for this, right?

Comment on attachment 8895186 [details] Bug 1380118 - aom: Resample high bit depth frames. https://reviewboard.mozilla.org/r/166346/#review172046 r- due to allocator issue, but go ahead and land without re-review once that is addressed ::: dom/media/platforms/agnostic/AOMDecoder.cpp:183 (Diff revision 2) > __func__); > } > > aom_codec_iter_t iter = nullptr; > aom_image_t *img; > + UniquePtr<aom_image_t> img8; Mismatched allocator - this needs to use aom_img_free.

Comment on attachment 8895185 [details] Bug 1380118 - Fix stream info peeking. https://reviewboard.mozilla.org/r/166344/#review172040 That's correct.

Comment on attachment 8895186 [details] Bug 1380118 - aom: Resample high bit depth frames. https://reviewboard.mozilla.org/r/166346/#review172512 ::: dom/media/platforms/agnostic/AOMDecoder.cpp:183 (Diff revision 2) > __func__); > } > > aom_codec_iter_t iter = nullptr; > aom_image_t *img; > + UniquePtr<aom_image_t> img8; Good point. Thanks!

Comment on attachment 8885429 [details] Bug 1380118 - aom: Filter out CONFIG_EXT_PARTITION_TYPES. https://reviewboard.mozilla.org/r/156272/#review172516 ::: python/mozbuild/mozbuild/vendor_aom.py:208 (Diff revisions 4 - 6) > > Please commit or stash these changes before vendoring, or re-run with `--ignore-modified`. > '''.format(files='\n'.join(sorted(modified)))) > sys.exit(1) > > - def vendor(self, revision, ignore_modified=False): > + def vendor(self, revision, repo, ignore_modified=False): I forgot to commit the corresponding changes in `mach_commands.py`.

Comment on attachment 8895186 [details] Bug 1380118 - aom: Resample high bit depth frames. https://reviewboard.mozilla.org/r/166346/#review172524

Comment on attachment 8885429 [details] Bug 1380118 - aom: Filter out CONFIG_EXT_PARTITION_TYPES. https://reviewboard.mozilla.org/r/156272/#review172526 ::: python/mozbuild/mozbuild/vendor_aom.py:79 (Diff revision 6) > return (info['commit'], info['committer']['time']) > > + def upstream_github_commit(self, revision): > + '''Query the github api for a git commit id and timestamp.''' > + repo = urlparse(self.repo_url).path > + url = mozpath.join('https://api.github.com/repos/', repo, 'commits', revision) I broke this method in cleanup. `urlparse` returns the path as an absolute path, and it turns out that absolute paths in `mozpath.join` clobber whatever was previously in the list, so this never constructed a valid url. What I get for using different split and join implementations, I guess. I'll fix it by appending [1:] to the `repo` initialization.

Comment on attachment 8885429 [details] Bug 1380118 - aom: Filter out CONFIG_EXT_PARTITION_TYPES. https://reviewboard.mozilla.org/r/156272/#review172936

Comment on attachment 8895184 [details] Bug 1380118 - Update aom library. https://reviewboard.mozilla.org/r/166342/#review172938

Comment on attachment 8885430 [details] Bug 1380118 - aom: Support vendoring from github. https://reviewboard.mozilla.org/r/156274/#review173226 I guess we have the repo used and/or the revision noted someplace else in the tree?

Comment on attachment 8885430 [details] Bug 1380118 - aom: Support vendoring from github. https://reviewboard.mozilla.org/r/156274/#review173226 The revision is noted in the tree (and in the code, but that's temporary for unstable codec versioning. https://dxr.mozilla.org/mozilla-central/source/media/libaom/README_MOZILLA The vendor script doesn't update the repo link there; I'll see if I can add that easily.

Comment on attachment 8897145 [details] Bug 1380118 - aom: Don't resample 8-bit images. https://reviewboard.mozilla.org/r/168442/#review173696

Comment on attachment 8897213 [details] Bug 1380118 - aom: Record the upstream repo we vendored from. https://reviewboard.mozilla.org/r/168490/#review173740 Nice, thank you!

Just to summarize recent activity: I spent quite some time trying to track down a performance regression with the bitmovin demo, but wasn't successful. So we decided to land this so it was easier to other to investigate. That blocked on Visual Studio 2015 failing to compile for win32 debug targets because of undefined behaviour with #if conditionals inside the arguments of an assert() macro. This is resolved upstream already, so I just patched out the issue. When that's passing review, I'll land the patch and follow up the performance issues in a separate bug.

Comment on attachment 8900324 [details] Bug 1380118 - aom: Fix win32 debug build. https://reviewboard.mozilla.org/r/171698/#review177072 LGTM but won't this fire if we build with CONFIG_EXT_PARTITION or CONFIG_TX64X64? If we'll never use those features, no problem. But otherwise it seems like it'd be slightly better to just move the ifdef outside of the assert and suffer the tiny bit of duplication?

Yes, that's probably a better fix. It's obsolete upstream anyway, so the next update should work.

Pushed by rgiles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f6f6c4eb77cc aom: add x86-win32-gcc config. r=froydnj,tjr https://hg.mozilla.org/integration/autoland/rev/49f0c2506176 Make aom_config.asm match upstream. r=froydnj,tjr https://hg.mozilla.org/integration/autoland/rev/ba8c00fbbfd9 aom: Filter out CONFIG_EXT_PARTITION_TYPES. r=froydnj,kinetik https://hg.mozilla.org/integration/autoland/rev/c24a4aedd2cf aom: remove unused option. r=froydnj,kinetik https://hg.mozilla.org/integration/autoland/rev/d780ed24d711 aom: Support vendoring from github. r=froydnj,kinetik https://hg.mozilla.org/integration/autoland/rev/351182a4c9dc Update aom library. r=kinetik https://hg.mozilla.org/integration/autoland/rev/0f27e7ef7316 Export aom_config.h. r=kinetik https://hg.mozilla.org/integration/autoland/rev/2e562962d359 Fix stream info peeking. r=kinetik https://hg.mozilla.org/integration/autoland/rev/f64262997765 aom: Resample high bit depth frames. r=kinetik https://hg.mozilla.org/integration/autoland/rev/75cced3f5498 aom: Don't resample 8-bit images. r=kinetik https://hg.mozilla.org/integration/autoland/rev/0bbdff75aa2d aom: Record the upstream repo we vendored from. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1f4dfff256e8 aom: Fix win32 debug build. r=kinetik

https://hg.mozilla.org/mozilla-central/rev/f6f6c4eb77cc https://hg.mozilla.org/mozilla-central/rev/49f0c2506176 https://hg.mozilla.org/mozilla-central/rev/ba8c00fbbfd9 https://hg.mozilla.org/mozilla-central/rev/c24a4aedd2cf https://hg.mozilla.org/mozilla-central/rev/d780ed24d711 https://hg.mozilla.org/mozilla-central/rev/351182a4c9dc https://hg.mozilla.org/mozilla-central/rev/0f27e7ef7316 https://hg.mozilla.org/mozilla-central/rev/2e562962d359 https://hg.mozilla.org/mozilla-central/rev/f64262997765 https://hg.mozilla.org/mozilla-central/rev/75cced3f5498 https://hg.mozilla.org/mozilla-central/rev/0bbdff75aa2d https://hg.mozilla.org/mozilla-central/rev/1f4dfff256e8