Update or cherry-pick decoding optimizations from libvpx upstream

RESOLVED FIXED in mozilla35

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: kinetik, Assigned: rillian)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
(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.
(Assignee)

Comment 2

3 years ago
I guess we might as well pick a revision since there have been no point releases.
(Assignee)

Updated

3 years ago
Depends on: 1067377
(Assignee)

Comment 3

3 years ago
Created attachment 8494958 [details] [diff] [review]
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.
Assignee: nobody → giles
Attachment #8494958 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8494958 - Attachment description: Fix update script for libvpx bump. → Part 1: Fix update script for libvpx bump.
(Assignee)

Comment 4

3 years ago
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.
Attachment #8494960 - Flags: review?(kinetik)
(Assignee)

Comment 5

3 years ago
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.
Attachment #8494960 - Flags: review?(ted)
(Reporter)

Comment 6

3 years ago
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.
Attachment #8494958 - Flags: review?(kinetik) → review+
(Reporter)

Updated

3 years ago
Attachment #8494960 - Flags: review?(kinetik) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8494962 [details] [diff] [review]
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.
Attachment #8494962 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Blocks: 1072729
(Reporter)

Comment 8

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

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

rubberstamp+
Attachment #8494962 - Flags: review?(kinetik) → review+
(Assignee)

Comment 9

3 years ago
(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!
Pushed to try to verify platform builds. https://tbpl.mozilla.org/?tree=Try&rev=fb93f7734259
Attachment #8494960 - Flags: review?(ted) → review+
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
https://hg.mozilla.org/mozilla-central/rev/851513ae05d0
https://hg.mozilla.org/mozilla-central/rev/4047d8588c85
https://hg.mozilla.org/mozilla-central/rev/2e4d8b824407
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Created attachment 8495572 [details] [diff] [review]
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.
Attachment #8495572 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8495572 - Attachment description: Part 3: Update the rest of libvpx → Part 4: Update the rest of libvpx
(Assignee)

Updated

3 years ago
Blocks: 1073319
(Reporter)

Updated

3 years ago
Attachment #8495572 - Flags: review?(kinetik) → review+
Thanks. Unfortunately it only builds on Mac now. Testing various mitigation patches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8497116 [details] [diff] [review]
Part 5: Fix optimization flags

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

Updated

3 years ago
Depends on: 1075183
(Assignee)

Updated

3 years ago
Depends on: 965151
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.
This is taking a long time, so I've backed out the partial commits.

https://hg.mozilla.org/integration/mozilla-inbound/rev/61588fcd074f
https://hg.mozilla.org/mozilla-central/rev/61588fcd074f
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(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.
(Reporter)

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8505849 [details] [diff] [review]
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.
Attachment #8497116 - Attachment is obsolete: true
Attachment #8505849 - Flags: review?(kinetik)
Created attachment 8505854 [details] [diff] [review]
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.
Attachment #8505854 - Flags: review?(kinetik)
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)
Attachment #8505854 - Attachment description: Part 5: Compile libvpx with -mfpu=neon on arm → Part 6: Compile libvpx with -mfpu=neon on arm
Attachment #8505854 - Flags: review?(mshal)
(Reporter)

Updated

3 years ago
Attachment #8505849 - Flags: review?(kinetik) → review+
(Reporter)

Updated

3 years ago
Attachment #8505854 - Flags: review?(kinetik) → review+

Updated

3 years ago
Attachment #8505854 - Flags: review?(mshal) → review+
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
Created attachment 8506502 [details] [diff] [review]
Part 7: Define __ANDROID__ for yasm

This is needed to fix the undefined symbol warning on the x86 Android build.
Attachment #8506502 - Flags: review?(kinetik)
Comment on attachment 8506502 [details] [diff] [review]
Part 7: Define __ANDROID__ for yasm

Adding :mshal for build peer review.
Attachment #8506502 - Flags: review?(mshal)
(Reporter)

Updated

3 years ago
Attachment #8506502 - Flags: review?(kinetik) → review+
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.
Attachment #8506502 - Flags: review?(mshal) → feedback+
Created attachment 8506999 [details] [diff] [review]
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.
Attachment #8506502 - Attachment is obsolete: true
Attachment #8506999 - Flags: review?(mshal)
Comment on attachment 8506999 [details] [diff] [review]
Part 7: Define __ANDROID__ for yasm v2

Looks good - thanks!
Attachment #8506999 - Flags: review?(mshal) → review+
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
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3957daffe6
(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?
(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
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.
Depends on: 1085171
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
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1085599
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.
Depends on: 1085607
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.
I'm building on gcc 4.4.3.  Don't know about NEON intrinsics, but that's the ICS compiler.
Let's follow up in bug 1085599.

Updated

3 years ago
Depends on: 1087359

Comment 40

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

Updated

3 years ago
Depends on: 1074860
Depends on: 1089525
Depends on: 1090672
(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.

Updated

3 years ago
Depends on: 1092136
No longer depends on: 1089525
Depends on: 1122745
(Assignee)

Updated

2 years ago
Blocks: 1225217
Depends on: 918550
You need to log in before you can comment on or make changes to this bug.