Closed Bug 507554 Opened 10 years ago Closed 10 years ago

Update in-tree libtheora to 1.1.0 (Thusnelda)


(Core :: Audio/Video, enhancement)

Not set



Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- wontfix


(Reporter: cajbir, Assigned: kinetik)




(2 files, 5 obsolete files)

The Theora Thusnelda version of the decoder has improvements to the decoding. It's faster and more efficient. We should look at moving to using this version of the decoder rather than the one we are using at the moment which is from the standard Theora svn repository.
Summary: For Theora decoder use the decoder from the Theora Thusnelda svn branch → For Theora decoding use the decoder from the Theora Thusnelda svn branch
Attached patch r16363 of theora-thusnelda svn (obsolete) — Splinter Review
This will need testing to see if it breaks any videos. I've only tried building on x86 32-bit Linux.
Tested build on tryserver and it highlighted a missing file. This patch adds it.
Attachment #391789 - Attachment is obsolete: true
I tested BillysBrowser480.ogg (30s video) with sync disabled on Mac.  In a debug build, this patch is about 32% slower.  In an opt build, it's about 10% slower.
What's weird is that dump_video shows current Thusnelda SVN being about 5-10% faster than Theora 1.0 using the same video.

That's using the dump_video.c from Thusnelda SVN with both library versions, since dump_video.c was replaced with a rewrite recently (and the old and new versions seem to perform differently).
Have I missed a configure flag? Has the setting to turn assembler optimisations on changed maybe?
Yep.  OC_X86ASM was renamed to OC_X86_ASM.
Updating that define in media/libtheora/lib/ gives us a 5% improvement over baseline in an opt build and 12% in debug (same test as comment #3).
Attached patch update to libtheora 1.1.0 (obsolete) — Splinter Review
Update to libtheora 1.1.0 release version.  Also rearrange the layout of the files to match the rearrangement that occurred upstream.  Fixes the OC_X86ASM typo.  Rebased and reapplied the WinCE build fixes.

The bug498770.patch fix is not applied, and won't apply easily because the file it applies to no longer exists.  The rewritten version of the functions it patches also use the EBX register without saving and restoring them, so I suspect we may run into the same crashes when built with FPO on Windows.
Attachment #391796 - Attachment is obsolete: true
Revisiting my numbers from comment 7, on my current system a debug build is 10% faster and an optimized build is 4.5% faster using BillysBrowser480.ogg with video tab hidden (so, no painting cost included).
I haven't tested an FPO build on Windows yet (supplied the wrong command line to build), but a regular optimized build is pretty broken.  Videos play, but the decoded image is corrupt.  I'm seeing lots of "magic pink" and when playing from the start of a video, it looks garbage is used as the keyframe.
A build with assembly optimizations disabled confirms that the asm is broken on Windows in the 1.1.0 release.  Still haven't tested the FPO/EBX issue, but suspect that will be a problem too.
Attached patch proposed fix (obsolete) — Splinter Review
Matthew, can you please try this patch, which replaces uses of ebx with edi or esi. This is in upstream svn as c16578.
A non-PGO/FPO build with that patch is still showing the corrupted decoded frames issue.  Investigating...
README_MOZILLA is out of date in my patch, must fix.
Thought I had narrowed it down to oc_state_frag_recon_mmx by binary searching the vtable init in mmxstate.c, but oc_state_frag_recon_mmx actually calls a bunch of the MMX routines directly without going via the vtable.

Running a debug build to see if disabling optimization changes the behaviour.
Attached image screenshot
Looks like the Windows problem is optimized-build only.  Reproduced in MSVC 2005 Pro SP1 and MSVC 2005 Express SP1, regular optimized build (no PGO).
Attached patch win32 asm fix (obsolete) — Splinter Review
That fix has been upstreamed, so I'll resync the sources with Theora SVN.

Fix explanation:  in the original code, |movd ECX,p| intends to move the value of p into ECX.  movd moves quadwords in and out of MMX registers, so the instruction ends up encoded as |movd MM1, p| (ECX and MM1 share the same encoding), leaving ECX containing whatever happened to be present earlier.   The fix is to use the correct zero-extending word mov instruction (movzx) to move p into ECX.

It's not a problem with optimization turned off because the compiler happens to compute |p| in ECX, so it has the correct value by luck without having to load it by hand.  In a release build, the compiler computes |p| using EAX and ECX contains the result of some other computation.
Updates to SVN r16584 (includes my fix, Cristian's EBX fix, and the WinCE define renames).  This removes all local patches against upstream.  Updated README_MOZILLA to reflect new reality.

Tryservers have been fed this tasty new snack.
Assignee: nobody → kinetik
Attachment #402753 - Attachment is obsolete: true
Attachment #402903 - Attachment is obsolete: true
Attachment #403019 - Attachment is obsolete: true
Summary: For Theora decoding use the decoder from the Theora Thusnelda svn branch → Update in-tree libtheora to 1.1.0 (Thusnelda)
Builds are turning up here, for anyone who wants to do early testing:
Tests passed on one unit test machine for each platform, and timed out for
another.  The timeouts look like two or three unrelated problems.

The tree I pushed to tryserver was about 200 revs behind current trunk, so I've
repushed patch with the latest trunk.
Attachment #403059 - Flags: review?(chris.double)
Comment on attachment 403059 [details] [diff] [review]
update to libtheora 1.1.0 + SVN

Only got one unit test box per platform this cycle.  Linux and Mac were green.  Windows timed out due to bug 518534 (unrelated).  So we've got 2x green cycles on Mac and Linux and 1x green cycle on Windows (plus 1x orange cycle on Linux and Mac and 2x orange cycles on Windows).
Attachment #403059 - Flags: review?(chris.double) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: wanted1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
We need this on the 1.9.2 branch to fix bug
Blocks: 515882
Comment on attachment 403059 [details] [diff] [review]
update to libtheora 1.1.0 + SVN

Let's take this on branch to fix bug 515882.
Attachment #403059 - Flags: approval1.9.2+
I believe the decision on this one for the 1.9.1 branch was "wontfix" since it's a feature upgrade rather than simply fixes.
Depends on: 752668
You need to log in before you can comment on or make changes to this bug.