Closed Bug 1380118 Opened 3 years ago Closed 2 years ago

Update aom

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 2 obsolete files)

59 bytes, text/x-review-board-request
froydnj
: review+
kinetik
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
tjr
: review+
Details
59 bytes, text/x-review-board-request
kinetik
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
tjr
: review+
Details
59 bytes, text/x-review-board-request
kinetik
: review+
froydnj
: review+
Details
59 bytes, text/x-review-board-request
kinetik
: review+
Details
59 bytes, text/x-review-board-request
kinetik
: review+
Details
59 bytes, text/x-review-board-request
kinetik
: review+
Details
59 bytes, text/x-review-board-request
kinetik
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Update aom to pull in recent changes.

For this I'd like to use a branch with some non-default config.
Blocks: 1378529
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
Attachment #8885432 - Attachment is obsolete: true
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.
Attachment #8885429 - Flags: review?(nfroyd)
Attachment #8885431 - Flags: review?(tom)
Attachment #8885431 - Flags: review?(nfroyd)
Attachment #8895182 - Flags: review?(tom)
Attachment #8895182 - Flags: review?(nfroyd)
Attachment #8895183 - Flags: review?(nfroyd)
Attachment #8895183 - Flags: review?(kinetik)
Attachment #8885430 - Flags: review?(kinetik)
Attachment #8885433 - Flags: review?(kinetik)
Attachment #8895185 - Flags: review?(kinetik)
Attachment #8895186 - Flags: review?(kinetik)
Comment on attachment 8885431 [details]
Bug 1380118 - aom: add x86-win32-gcc config.

https://reviewboard.mozilla.org/r/156276/#review171822
Attachment #8885431 - Flags: review?(nfroyd) → review+
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.
Attachment #8895183 - Flags: review?(nfroyd)
Comment on attachment 8895182 [details]
Bug 1380118: Make aom_config.asm match upstream.

https://reviewboard.mozilla.org/r/166338/#review171828
Attachment #8895182 - Flags: review?(nfroyd) → review+
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?
Attachment #8885429 - Flags: review?(nfroyd) → review+
(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
Attachment #8895183 - Flags: review?(nfroyd) → review+
Comment on attachment 8885430 [details]
Bug 1380118 - aom: Support vendoring from github.

https://reviewboard.mozilla.org/r/156274/#review172026
Attachment #8885430 - Flags: review?(kinetik) → review+
Comment on attachment 8885433 [details]
Bug 1380118 - Export aom_config.h.

https://reviewboard.mozilla.org/r/156280/#review172028
Attachment #8885433 - Flags: review?(kinetik) → review+
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.
Attachment #8895183 - Flags: review?(kinetik) → review+
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?
Attachment #8895185 - Flags: review?(kinetik) → review+
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.
Attachment #8895186 - Flags: review?(kinetik) → review-
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
Attachment #8895186 - Flags: review?(kinetik) → review+
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
Attachment #8885429 - Flags: review?(kinetik) → review+
Comment on attachment 8895184 [details]
Bug 1380118 - Update aom library.

https://reviewboard.mozilla.org/r/166342/#review172938
Attachment #8895184 - Flags: review?(kinetik) → review+
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?
Attachment #8885430 - Flags: review?(nfroyd) → review+
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
Attachment #8897145 - Flags: review?(kinetik) → review+
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!
Attachment #8897213 - Flags: review?(nfroyd) → review+
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?
Attachment #8900324 - Flags: review?(kinetik) → review+
Yes, that's probably a better fix. It's obsolete upstream anyway, so the next update should work.
Attachment #8899932 - Attachment is obsolete: true
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
Blocks: 1394061
You need to log in before you can comment on or make changes to this bug.