Closed Bug 485291 Opened 11 years ago Closed 11 years ago

Update liboggplay to latest upstream version


(Core :: Audio/Video, defect)

Not set





(Reporter: kinetik, Assigned: kinetik)



(Keywords: fixed1.9.1)


(5 files, 8 obsolete files)

133.69 KB, patch
Details | Diff | Splinter Review
19.35 KB, patch
: review+
Details | Diff | Splinter Review
16.23 KB, patch
Details | Diff | Splinter Review
2.24 KB, patch
: review+
Details | Diff | Splinter Review
12.37 KB, patch
Details | Diff | Splinter Review
The latest version of liboggplay has an improved yuv2rgb conversion routine, and contains a number of fixes we have been carrying as local patches.
Attached patch updated liboggplay (obsolete) — Splinter Review
Attached patch local changes required (obsolete) — Splinter Review
oggplay_yuv2bgr was renamed to oggplay_yuv2bgra in newer versions of liboggplay, so fix that.

Add a test for __align__ attribute support to our, needed to align some SSE2 constants in the new YUV conversion code.
We need to update the reftests before this can land, since the new routines result in slightly different coloured videos.

I also think that init_yuv_converters in oggplay_yuv2rgb.c should not try to use the SSE2 routines if ATTRIBUTE_ALIGNED_MAX is < 16.
Sent this patch upstream to address the last sentence in comment #3.  I'll see if upstream pick this up quickly and refresh if they do, otherwise I'll include it in a local patch.  We should aim to get this code landed soon, since the new YUV stuff is a big improvement to playback performance.
Comment on attachment 369432 [details] [diff] [review]
local changes required

I think Ted needs to look at the change, requesting review.

The oggplay update is covered by a standing rs=roc, I believe.
Attachment #369432 - Flags: review?(ted.mielczarek)
Attached patch fix reftests (obsolete) — Splinter Review
Changes the reftests to use <video> in the references, except basic, which becomes a != test.  This also removes some unnecessary reftest-wait stuff (bug 483556).

Requesting review from Robert to make sure the changes are sane.
Attachment #369945 - Flags: review?(roc)
+<div style="width:140px; height:100px; position:relative; left:100px; top:100px;">
+<video src="black140x100.ogv"></video>

Slightly saner to just style the <video> and lose the wrapper <div>, like you did in some of the other tests.
Attached patch fix reftests v2 (obsolete) — Splinter Review
Remove the wrapper div from aspect-ratio-{1,2}-ref.html and apply the style directly to the video.
Attachment #369945 - Attachment is obsolete: true
Attachment #369949 - Flags: review?(roc)
Attachment #369945 - Flags: review?(roc)
Blocks: 483556
Blocks: 474749
Attached patch updated liboggplay v2 (obsolete) — Splinter Review
Sync liboggplay to current HEAD, now includes fixes from comment #3 and fixes for bug 480014.
Attachment #369430 - Attachment is obsolete: true
Attachment #369438 - Attachment is obsolete: true
The latest oggplay patch fails to compile on MSVC because changes to oggplay_private.h use C99 and GCC-specific functionality.

Tests also fail on all platforms due to a scaling change in the presentation time calculation.
Comment on attachment 369432 [details] [diff] [review]
local changes required

+         for ac_cv_c_attr_align_try in 2 4 8 16 32 64; do
Are there platforms we support on which 2-byte alignment is the largest possible alignment? Seems like a lot of TRY_COMPILEs to find one value here.

r=me anyway, but would be nice to be able to do that without 6 TRY_COMPILEs most of the time.
Attachment #369432 - Flags: review?(ted.mielczarek) → review+
Attached patch local changes v2 (obsolete) — Splinter Review
Good point.  It's easy to avoid multiple compilation tests, and the common case is that __align__ is supported.

I've sent a similar patch to liboggplay upstream for their, too.
Attachment #369432 - Attachment is obsolete: true
Attachment #370106 - Flags: review?(ted.mielczarek)
Attached patch updated liboggplay v3 (obsolete) — Splinter Review
The latest breakage was backed out of liboggplay, so this updates to that version.  Build, mochitest, and reftests pass on OS X.  Reftest changes also pass on Linux.  I've pushed this set of patches to the tryserver--if it looks good there (and Ted's happy with the most recent change), this will be ready to land on mozilla-central.
Attachment #369962 - Attachment is obsolete: true
Reftest failures on Linux tryserver:

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/aspect-ratio-1a.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/aspect-ratio-1b.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/aspect-ratio-2a.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/aspect-ratio-2b.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/canvas-1a.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/canvas-1b.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/zoomed-1.html |

First test reports:
REFTEST number of differing pixels: 906

And others are failing with a similar low number of differing pixels.

I ran these on my local Linux machine last night and they worked, so I'm not sure what's going on.  I'll try to reproduce this tonight with the exact changesets I pushed to the try server.
Same failures on Win32, plus aspect-ratio-3{a,b}.html.

They all pass in my local Vista VM, and I just reran the tests on my Linux machine and they pass there too.  :-/

More investigation needed.
Comment on attachment 370106 [details] [diff] [review]
local changes v2

Nicer, thanks!
Attachment #370106 - Flags: review?(ted.mielczarek) → review+
Ran through the reftests on my local Linux and Windows systems again, forcing each of the three codepaths (SSE2, MMX, and vanilla), and the tests pass in each case, so it eliminates that problem, at least.

I did discover a new problem: my change to avoid using the SSE2 routines when ATTRIBUTE_ALIGNED_MAX is undefined means we never use that code on Win32, since it's never defined.  It needs to be added to liboggplay's config_win32.h, or the ifdef test needs to be changed to deal with Win32 explicitly.
Thanks to roc's help, I found the problem with the failing reftests.  It's fairly obvious in retrospect, but the changed reftests made the problem less obvious than it should have been.  And the new tests usually only failed in optimized builds.

The problem is that the test video we're using is 140x100, but the SSE2 YUV conversion routine only processes 16 pixel multiples, and since 140/16 = 8.75, we process 128 pixels and leave a strip of 12x100 unprocessed along the right edge.  In debug builds they ended to be left as all-zeros, resulting in a transparent strip with the background below shining through.  Same problem with the MMX routine, except 140/8 = 17.5 so we process 136 and miss 4x100.  The vanilla routine processes 2 at a time, so it could also be a problem there, since the Theora spec implies that a video's picture region may be an arbitrary size (I'll need to check if this is true in practice).

So, the plan of attack is:
- update oggplay to pick up the fixes for bug 482461
- condition oggplay's yuv2rgb code on the width of the video, falling back to MMX and vanilla if necessary
- change the reftests back to using a <div> reference, and use SVG filters to threshold the video so that small colour changes caused by encoding/decoding doesn't cause test failures
- add more videos to the reftests so we test all three YUV codepaths and for more than just simple black video frames
- test on big endian, since we might need to remove the ifdef from nsOggDecoder as the YUV code tries to deal with this internally now
Attachment #370123 - Attachment is obsolete: true
Attached patch local changes v3Splinter Review
Updated local changes:
- remove compile-time test for machine endianness from nsOggDecoder as the new YUV code switches behaviour internally
- dispatch to YUV routines based on video dimensions and routine's alignment requirements (fixes bug described in comment 18)
Attachment #370106 - Attachment is obsolete: true
Attachment #370818 - Flags: review?(chris.double)
Attached patch fix reftests v3Splinter Review
Use SVG filter to threshold video colours.

Note that this marks the two canvas tests as random due to an existing bug exposed by the test changes.  Now that the tests are loaded over HTTP, we run into a case where nsMediaStream's GetPrincipal returns null because its channel has been nulled out too early.

The zooming test also doesn't use the threshold filter because I ran into a problem where the zoomed image would have blended grey lines on the righthand edge.
Attachment #369949 - Attachment is obsolete: true
Blocks: 482461
GetPrincipal bug is bug 486646.
Attachment #370818 - Flags: review?(chris.double) → review+
I've emailed the liboggplay YUV changes upstream.  The changes all look good locally (tested OS X debug and opt builds, Linux debug build), and I've pushed it to the tryserver.  If all goes well I'll push this into trunk once the tryservers cycle green.
I forgot to add a new mochitest for the oggz-chop bug that this import fixes.  Oh well, I'll do that shortly.  It doesn't look like this is going to be landing tonight:

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/aspect-ratio-1a.xhtml |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/aspect-ratio-1b.xhtml |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/aspect-ratio-3a.xhtml |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/aspect-ratio-3b.xhtml |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-linux-unittest/mozilla/layout/reftests/ogg-video/basic-1.xhtml |
*** 27850 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_delay_load.html | onload was not delayed until after metadataloaded - got 4, expected 5
The reftest failures look weird.  For each of the failing tests there was at least one test page served as a 404, and in some cases both the test and the reference were served as 404s.

The test_delay_load failure is almost definitely bug 486556.
Reftests failed on Mac too, and it's obvious why now: the file renames from html->xhtml were silently lost when I applied the hg export-format patches using patch to the tree I push changes out of.  D'oh.

Fixed that up, added the oggz-chop test, and repushed to tryserver.
Yay!  Pushed
Closed: 11 years ago
Resolution: --- → FIXED
Endian problem ?
Using the tryserver build, a few videos are scrambled (again) on PPC Mac:

Some other videos still play fine:
Which tryserver build did you use?  The final patch is in the build here:

I'm not sure why only some videos would have problems, we call the same codepath for every type of video on PPC.  What are you seeing exactly?

Can you please file a follow up bug with more detail and CC me? I'll look into it.
(In reply to comment #29)
> Which tryserver build did you use?  The final patch is in the build here:

yes i used that one.

> I'm not sure why only some videos would have problems, we call the same
> codepath for every type of video on PPC.  What are you seeing exactly?

Here's a screen shot:
Is the flag indicating a big endian system being set correctly? Previously we didn't need this for liboggplay - we checked the endian in our own code and picked the liboggplay routines from that. Might need to change liboggplay's config.h?
Er... yeah, that'll be it.  That's why some videos work, since we hit the right case if PPC and video width % 16 == 0 and video height % 2 == 0.  I'll post a fix in a bit.
Attached patch endian fix v0Splinter Review
Untested, but pretty sure this'll do the trick.  It's the same thing we do in liboggz.
Attachment #371019 - Flags: review?(chris.double)
Attachment #371019 - Flags: review?(chris.double) → review+
J., I've pushed that patch to the tryserver, so please give it a spin when it's done and let me know if it works.  Thanks!
Marking as needs 1.9.1 landing because dependent bugs are blocking 1.9.1.
Whiteboard: [needs 191 landing]
All tested videos are now looking good with the tryserver build. 

Thank you!
Great, thanks.  I've pushed that patch:

Note for whomever merges to 1.9.1, the following revs are needed:
You'll also need this bustage fix:

Not sure how this made it through the tryserver.
Version: unspecified → Trunk
Depends on: 494336
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.