485291 - Update liboggplay to latest upstream version

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

The latest version of liboggplay has an improved yuv2rgb conversion routine, and contains a number of fixes we have been carrying as local patches.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: updated liboggplay

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: local changes required

oggplay_yuv2bgr was renamed to oggplay_yuv2bgra in newer versions of liboggplay, so fix that.

Add a test for __align__ attribute support to our configure.in, needed to align some SSE2 constants in the new YUV conversion code.

Comment 3

[Matthew Gregan [:kinetik]]

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.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: oggplay change to address comment #3

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 5

[Matthew Gregan [:kinetik]]

Comment on attachment 369432 [details] [diff] [review]
local changes required

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

The oggplay update is covered by a standing rs=roc, I believe.

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: fix reftests

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+<div style="width:140px; height:100px; position:relative; left:100px; top:100px;">
+<video src="black140x100.ogv"></video>
+</div>

Slightly saner to just style the <video> and lose the wrapper <div>, like you did in some of the other tests.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: fix reftests v2

Remove the wrapper div from aspect-ratio-{1,2}-ref.html and apply the style directly to the video.

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: updated liboggplay v2

Sync liboggplay to current HEAD, now includes fixes from comment #3 and fixes for bug 480014.

Comment 10

[Matthew Gregan [:kinetik]]

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 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Matthew Gregan [:kinetik]]

Attachment: local changes v2

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 configure.ac, too.

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: updated liboggplay v3

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 configure.in change), this will be ready to land on mozilla-central.

Comment 14

[Matthew Gregan [:kinetik]]

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.

Comment 15

[Matthew Gregan [:kinetik]]

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 16

[(not currently active) Ted Mielczarek]

Comment on attachment 370106 [details] [diff] [review]
local changes v2

Nicer, thanks!

Comment 17

[Matthew Gregan [:kinetik]]

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.

Comment 18

[Matthew Gregan [:kinetik]]

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

Comment 19

[Matthew Gregan [:kinetik]]

Attachment: updated liboggplay v4

Comment 20

[Matthew Gregan [:kinetik]]

Attachment: local changes v3

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)

Comment 21

[Matthew Gregan [:kinetik]]

Attachment: fix reftests v3

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.

Comment 22

[Matthew Gregan [:kinetik]]

GetPrincipal bug is bug 486646.

Comment 23

[Matthew Gregan [:kinetik]]

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.

Comment 24

[Matthew Gregan [:kinetik]]

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

Comment 25

[Matthew Gregan [:kinetik]]

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.

Comment 26

[Matthew Gregan [:kinetik]]

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.

Comment 27

[Matthew Gregan [:kinetik]]

Yay!  Pushed http://hg.mozilla.org/mozilla-central/rev/46c478fbd908.

Comment 28

[J. Gatellier]

Endian problem ?
Using the tryserver build, a few videos are scrambled (again) on PPC Mac: 

http://tinyvid.tv/show/18zdo6uksb6vf
http://tinyvid.tv/show/z50hwrcnt7wc

Some other videos still play fine:
http://tinyvid.tv/show/1k5w235t7kx0u

Comment 29

[Matthew Gregan [:kinetik]]

Which tryserver build did you use?  The final patch is in the build here: https://build.mozilla.org/tryserver-builds/2009-04-03_05:[e-mail]-try-84b9322bc76/

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.

Comment 30

[J. Gatellier]

(In reply to comment #29)
> Which tryserver build did you use?  The final patch is in the build here:
> https://build.mozilla.org/tryserver-builds/2009-04-03_05:[e-mail]-try-84b9322bc76/

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:
http://www.gamekyo.com/images_1/29dc9c9e7b05e5a47ddfea7f9720f71920090404014125.png

Comment 31

[cajbir (:cajbir)]

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?

Comment 32

[Matthew Gregan [:kinetik]]

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.

Comment 33

[Matthew Gregan [:kinetik]]

Attachment: endian fix v0

Untested, but pretty sure this'll do the trick.  It's the same thing we do in liboggz.

Comment 34

[Matthew Gregan [:kinetik]]

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!

Comment 35

[Matthew Gregan [:kinetik]]

Marking as needs 1.9.1 landing because dependent bugs are blocking 1.9.1.

Comment 36

[J. Gatellier]

All tested videos are now looking good with the tryserver build. 

Thank you!

Comment 37

[Matthew Gregan [:kinetik]]

Great, thanks.  I've pushed that patch: http://hg.mozilla.org/mozilla-central/rev/d8d2c0480c77

Note for whomever merges to 1.9.1, the following revs are needed:
http://hg.mozilla.org/mozilla-central/rev/46c478fbd908
http://hg.mozilla.org/mozilla-central/rev/d8d2c0480c77

Comment 38

[Matthew Gregan [:kinetik]]

You'll also need this bustage fix:
http://hg.mozilla.org/mozilla-central/rev/1b300467cdb3

Not sure how this made it through the tryserver.

Comment 39

[Viktor Gal]

Attachment: fix for SIMD yuv conversion having any custom sized video frames.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d2a99f6f0d0c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/afb4cc34206d

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/58012513d817