Closed
Bug 1063356
Opened 10 years ago
Closed 10 years ago
Update or cherry-pick decoding optimizations from libvpx upstream
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: kinetik, Assigned: rillian)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(7 files, 2 obsolete files)
11.61 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
kinetik
:
review+
ted
:
review+
|
Details | Diff | Splinter Review |
1.54 MB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
6.14 MB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
82.83 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
725 bytes,
patch
|
kinetik
:
review+
mshal
:
review+
|
Details | Diff | Splinter Review |
1.49 KB,
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
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•10 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•10 years ago
|
||
I guess we might as well pick a revision since there have been no point releases.
Assignee | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
Attachment #8494958 -
Attachment description: Fix update script for libvpx bump. → Part 1: Fix update script for libvpx bump.
Assignee | ||
Comment 4•10 years ago
|
||
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•10 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•10 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•10 years ago
|
Attachment #8494960 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 7•10 years ago
|
||
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)
Reporter | ||
Comment 8•10 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•10 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!
Assignee | ||
Comment 10•10 years ago
|
||
Pushed to try to verify platform builds. https://tbpl.mozilla.org/?tree=Try&rev=fb93f7734259
Updated•10 years ago
|
Attachment #8494960 -
Flags: review?(ted) → review+
Assignee | ||
Comment 11•10 years ago
|
||
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•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee | ||
Comment 13•10 years ago
|
||
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•10 years ago
|
Attachment #8495572 -
Attachment description: Part 3: Update the rest of libvpx → Part 4: Update the rest of libvpx
Reporter | ||
Updated•10 years ago
|
Attachment #8495572 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Thanks. Unfortunately it only builds on Mac now. Testing various mitigation patches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•10 years ago
|
||
WIP; enables AVX2 on Windows, propagates asm flags to compiler invocations.
Assignee | ||
Comment 16•10 years ago
|
||
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.
Assignee | ||
Comment 17•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/61588fcd074f
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 19•10 years ago
|
||
(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•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
Enable neon when compiling libvpx with arm assembly support we can use the new arm_neon.h intrinsics codes.
Attachment #8505854 -
Flags: review?(kinetik)
Assignee | ||
Comment 22•10 years ago
|
||
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•10 years ago
|
Attachment #8505849 -
Flags: review?(kinetik) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8505854 -
Flags: review?(kinetik) → review+
Updated•10 years ago
|
Attachment #8505854 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 23•10 years ago
|
||
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
Assignee | ||
Comment 24•10 years ago
|
||
This is needed to fix the undefined symbol warning on the x86 Android build.
Attachment #8506502 -
Flags: review?(kinetik)
Assignee | ||
Comment 25•10 years ago
|
||
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•10 years ago
|
Attachment #8506502 -
Flags: review?(kinetik) → review+
Comment 26•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
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 28•10 years ago
|
||
Comment on attachment 8506999 [details] [diff] [review] Part 7: Define __ANDROID__ for yasm v2 Looks good - thanks!
Attachment #8506999 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 29•10 years ago
|
||
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
Assignee | ||
Comment 30•10 years ago
|
||
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.
Assignee | ||
Comment 31•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3957daffe6
Comment 32•10 years ago
|
||
(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?
Assignee | ||
Comment 33•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
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.
Assignee | ||
Comment 37•10 years ago
|
||
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.
Assignee | ||
Comment 39•10 years ago
|
||
Let's follow up in bug 1085599.
Comment 40•10 years ago
|
||
Sorry to spam this bug, but could it be related to this one ? https://bugzilla.mozilla.org/show_bug.cgi?id=1089525
Comment 41•10 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•