1063356 - Update or cherry-pick decoding optimizations from libvpx upstream

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

Steven Robertson at YouTube pointed out that there have been libvpx decoding optimizations made in upstream libvpx that we'd benefit from.  Specifically, ceebbcc0cef1bbd2bb672576ccf1133168639346, which is currently in-use in the latest Chromium builds.

Comment 1

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #0)
> Specifically, ceebbcc0cef1bbd2bb672576ccf1133168639346

To clarify, this is a commit in Chromium's libvpx repo, and does not refer to a specific upstream libvpx change directly.  The last upstream libvpx import in this repo is 23c88870ec514b0dd7d22b9db99ae63f46c7d87f.  The changes in the Chromium libvpx repo beyond that seem to be build changes.

Comment 2

[Ralph Giles (:rillian)]

I guess we might as well pick a revision since there have been no point releases.

Comment 3

[Ralph Giles (:rillian)]

Attachment: Part 1: Fix update script for libvpx bump.

Changes to the update script necessary to import and build libvpx commit d95585fb0ec024f6abd96f7b02e0df58019d46af.

The vp8 and vp9 implementations depend on divergent definitions of identically named identifiers, so it's no longer possible to compile them as part of the same unified_sources module. For this bug I've just moved the vp9 files out of UNIFIED_SOURCES. I've filed bug 1072729 for coming up with a better fix.

Comment 4

[Ralph Giles (:rillian)]

Attachment: Part 2: Propagate asm flags in moz.build

We run libvpx configure at update time, which statically sets which asm flags a given platform supports, but we also need to propagate the same configure-time checks through moz.build to enable them at run-time. If there's a mismatch the compiler will complain.

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8494960 [details] [diff] [review]
Part 2: Propagate asm flags in moz.build

Adding Ted for build peer review. The other patches just update the source list.

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8494958 [details] [diff] [review]
Part 1: Fix update script for libvpx bump.

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

I assume the removed patches are in upstream now.

Comment 7

[Ralph Giles (:rillian)]

Attachment: Part 3: Update libvpx source

Result of running

  ./update.py --ndk ~/android/android-ndk-r9 --commit d95585fb0ec024f6abd96f7b02e0df58019d46af

That being the most recent upstream commit chromium had pulled.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8494962 [details] [diff] [review]
Part 3: Update libvpx source

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

rubberstamp+

Comment 9

[Ralph Giles (:rillian)]

(In reply to Matthew Gregan [:kinetik] from comment #6)

> I assume the removed patches are in upstream now.

That's correct.

Thanks for the quick review!

Comment 10

[Ralph Giles (:rillian)]

Pushed to try to verify platform builds. https://tbpl.mozilla.org/?tree=Try&rev=fb93f7734259

Comment 11

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/851513ae05d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/4047d8588c85
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4d8b824407

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/851513ae05d0
https://hg.mozilla.org/mozilla-central/rev/4047d8588c85
https://hg.mozilla.org/mozilla-central/rev/2e4d8b824407

Comment 13

[Ralph Giles (:rillian)]

Attachment: Part 4: Update the rest of libvpx

I didn't import all the changes correctly in the part 3 patch. Here's the rest of it. Mostly documentation fixes, new header guard style, removing obsolete files, but there are a few source changes as well.

Comment 14

[Ralph Giles (:rillian)]

Thanks. Unfortunately it only builds on Mac now. Testing various mitigation patches.

Comment 15

[Ralph Giles (:rillian)]

Attachment: Part 5: Fix optimization flags

WIP; enables AVX2 on Windows, propagates asm flags to compiler invocations.

Comment 16

[Ralph Giles (:rillian)]

I'm somewhat stuck here. I can just disable the windows avx2 support, but I still have failures with the linux b2g desktop and valgrind builds. These use the linux vpx_config, but we build with a much older compiler which doesn't support avx2 instructions.

Any suggestions how to switch based on compiler version, or pass the HAVE_X86_AVX2 flag from configure to the vpx_config_*.h and vpx_config_*.asm files?

There's also a problem with referencing 'rand' from asm code in the android x86 build.

Comment 17

[Ralph Giles (:rillian)]

This is taking a long time, so I've backed out the partial commits.

https://hg.mozilla.org/integration/mozilla-inbound/rev/61588fcd074f

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/61588fcd074f

Comment 19

[John Drinkwater (:beta)]

(In reply to Ralph Giles (:rillian) from comment #4)
> Created attachment 8494960 [details] [diff] [review]
> Part 2: Propagate asm flags in moz.build
> 
> We run libvpx configure at update time, which statically sets which asm
> flags a given platform supports, but we also need to propagate the same
> configure-time checks through moz.build to enable them at run-time. If
> there's a mismatch the compiler will complain.

This appears to have triggered Bug 1074860, Nightly from Mozilla now crashes on the machine I have.

Comment 20

[Ralph Giles (:rillian)]

Attachment: Part 5: Disable AVX2 on linux

libvpx now requires avx2 support in both the compiler and yasm, and some of our linux try build configs are too old. Temporarily disable avx2 acceleration so we can land.

Re-enable is bug 1073319.

Comment 21

[Ralph Giles (:rillian)]

Attachment: Part 6: Compile libvpx with -mfpu=neon on arm

Enable neon when compiling libvpx with arm assembly support we can use the new arm_neon.h intrinsics codes.

Comment 22

[Ralph Giles (:rillian)]

Comment on attachment 8505854 [details] [diff] [review]
Part 6: Compile libvpx with -mfpu=neon on arm

Adding mshal for build peer review. (Ted's away)

Comment 23

[Ralph Giles (:rillian)]

Thanks for the prompt reviews. One issue remaining with __ANDROID__ not being defined for yasm in the x86 fennec build.

https://tbpl.mozilla.org/?tree=Try&rev=13b4f7246a95

Comment 24

[Ralph Giles (:rillian)]

Attachment: Part 7: Define __ANDROID__ for yasm

This is needed to fix the undefined symbol warning on the x86 Android build.

Comment 25

[Ralph Giles (:rillian)]

Comment on attachment 8506502 [details] [diff] [review]
Part 7: Define __ANDROID__ for yasm

Adding :mshal for build peer review.

Comment 26

[Michael Shal [:mshal]]

Comment on attachment 8506502 [details] [diff] [review]
Part 7: Define __ANDROID__ for yasm

Do you need to append to VPX_ASFLAGS? What if you just append to ASFLAGS after the initial assignment? IMO since VPX_ASFLAGS is defined in configure, we shouldn't be modifying it in a Makefile.

Comment 27

[Ralph Giles (:rillian)]

Attachment: Part 7: Define __ANDROID__ for yasm v2

Yes, we can just append to ASFLAGS instead. Is this better?

updating patch for review comments, carrying forward r=kinetik.

Comment 28

[Michael Shal [:mshal]]

Comment on attachment 8506999 [details] [diff] [review]
Part 7: Define __ANDROID__ for yasm v2

Looks good - thanks!

Comment 29

[Ralph Giles (:rillian)]

Thanks for the quick reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bca0cbbecf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3093663c5c22
https://hg.mozilla.org/integration/mozilla-inbound/rev/3363b89ef1b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c07229d5dbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f249d3d1dcfd
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ff6f005483

Comment 30

[Ralph Giles (:rillian)]

This seems to require a clobber. The build system is apparently confused by vp9_thread.c->vp9_dthread.c with a new common/vp9_thread.c.

Comment 31

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3957daffe6

Comment 32

[Mike Hommey [:glandium]]

(In reply to Ralph Giles (:rillian) from comment #30)
> This seems to require a clobber. The build system is apparently confused by
> vp9_thread.c->vp9_dthread.c with a new common/vp9_thread.c.

Link to the error?

Comment 33

[Ralph Giles (:rillian)]

(In reply to Mike Hommey [:glandium] from comment #32)

> Link to the error?

I don't know how to link to error summaries on treeherder. Here are some example logs:

https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux-debug/1413561831/mozilla-inbound-linux-debug-bm72-build1-build1411.txt.gz
https://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-emulator-kk-debug/1413567002/b2g_mozilla-inbound_emulator-kk-debug_nonunified-bm91-build1-build206.txt.gz
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-st-an-debug/1413567349/mozilla-inbound-linux64-st-an-debug-bm71-build1-build685.txt.gz

A brief log example: https://tbpl.mozilla.org/php/getParsedLog.php?id=50461425&tree=Mozilla-Inbound

Comment 34

[Mike Hommey [:glandium]]

Thanks. That's a known issue with the dependency system, whereby dependencies for the .o contain the old location for the source, where the file doesn't exist anymore. IIRC there's an existing bug about this, I can't find it, though.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7bca0cbbecf1
https://hg.mozilla.org/mozilla-central/rev/3093663c5c22
https://hg.mozilla.org/mozilla-central/rev/3363b89ef1b7
https://hg.mozilla.org/mozilla-central/rev/5c07229d5dbd
https://hg.mozilla.org/mozilla-central/rev/f249d3d1dcfd
https://hg.mozilla.org/mozilla-central/rev/82ff6f005483
https://hg.mozilla.org/mozilla-central/rev/1c3957daffe6

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

By any chance, do these patches change any assumptions we make about NEON-enabled-ness?  Still wrapping my head around bug 1085599, but I see a lot of changes to NEON stuff.

Comment 37

[Ralph Giles (:rillian)]

Yes, we now assume the arm compiler supports neon intrinsics. If you have a non-neon device, it would be nice to verify the runtime cpu detection still properly gates actual execution of neon code.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm building on gcc 4.4.3.  Don't know about NEON intrinsics, but that's the ICS compiler.

Comment 39

[Ralph Giles (:rillian)]

Let's follow up in bug 1085599.

Comment 40

[Frederic Bezies]

Sorry to spam this bug, but could it be related to this one ? https://bugzilla.mozilla.org/show_bug.cgi?id=1089525

Comment 41

[Gian-Carlo Pascutto [:gcp]]

(In reply to Ralph Giles (:rillian) from comment #37)
> Yes, we now assume the arm compiler supports neon intrinsics. If you have a
> non-neon device, it would be nice to verify the runtime cpu detection still
> properly gates actual execution of neon code.

Bug 1087359 is reported on Tegra 2 devices, so the answer is no, it doesn't.