For Video painting using the new Layers system we are using a 'hack' to use liboggplay's YUV to RGB color conversion routines. This should be replaced with other color conversion code integrated into the Layers system.

This patch uses the color conversion code from Chromium. This was picked based on testing from this blog post: http://www.bluishcoder.co.nz/2010/02/19/comparing-colour-space-conversion-libraries.html This is not feature complete with liboggplay's implementation at this point in time. There is no 4:2:2 or 4:4:4 routines and there is no runtime CPU detection. It assumes MMX on X86 32 and MMX2 on X86 64. Otherwise falls back to C code. Should we port runtime CPU detection from liboggplay or from other code that we might have?

We have SSE detection code you can use: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/SSE.h?force=1 Lets get rid of yuv_convert_unittest.cc, it won't even build. Lets change "namespace media" to "namespace mozilla" (or mozilla::gfx?) We already have Chromium build_config.h, basictypes.h, port.h in the tree, here: http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/build/build_config.h http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/basictypes.h http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/port.h Possibly we could share them, but actually I think we could just replace them here with stub files that define the stuff this code actually needs (which isn't much) in terms of our own types. Here's all I think we need: * uint8, int16, int32, uint32 (typedef to PR types) * the ARCH_ macros (base on our existing architecture macros) ... that's about it!

Addresses review comments and adds runtime CPU detection using Mozilla's SSE.h. Splits out the Linux generic C code so it's used on the other platforms when needed. Added an 'update.sh' script and a patch to be applied so Chromium code can be updated in the same manner as the other third party media code.

+ bool supports_mmx = mozilla::supports_mmx(); We don't need mozilla:: here since we're in the mozilla namespace, right? I think we can support 4:2:2 easily because that's just YV16 in the Chromium code. I'd like to see some kind of 4:4:4 support as well, even if it's just dumb C code. We should also be checking in BasicPlanarYCbCrImage::SetData that the relationship between UV size and Y size is actually one we can handle. We should document in ImageLayer.h that these are the three formats PLANAR_YUV can currently handle. We'll need a patch against comm-central to fix Seamonkey. The new library needs to be added here: http://mxr.mozilla.org/comm-central/source/suite/installer/package-manifest.in The patch file seems a bit weird. It seems to modify almost every line of yuv_row_c.cpp and yuv_linux.cpp? As a followup we should check the performance of SSE2 vs MMX to see if we should be using the SSE2 code on some 32-bit CPUs.

Added support for YUV 4:2:2 and commented header file as suggested. Raised bug 551378 to implement 4:4:4. Fixed patch file (was an error in update.sh). Will add comm-central patch shortly.

Possible comm-central patch? Is it only this one line change required? I don't really know what's expected here...

Comment on attachment 431554 [details] [diff] [review] Address review comments Looks good. As discussed on IRC, let's rename the library to 'ycbcr' and the public API to ConvertYCbCrToRGB32/ScaleYCbCrToRGB32.

API Changed as per IRC discussion. Changed library name to ycbcr. Carried forward r+.

Changed library name to ycbcr. Carried forward r+.

Rebased patch for video layers changes (some files were moved to gfx/layers/basic directory)

This rebases the patch to be applied on top of the new Ogg backend. Review carried forward. Note that this patch will result in a reftest failure on the test with an odd sized picture region. This is fixed in the patch to come.

This changes the YCbCr image in layers to take the entire frame of YCbCr data along with the picture region area. It then does the YCbCr to RGB conversion on the picture region storing that in the RGB buffer. This handles the Theora requirements for odd sized picture regions and the previously failing reftest now passes.

Should probably add a comment in ImageLayers.h explaining that the picture region is what actually gets rendered by the Image. In particular the size of the image is mPicSize, not mYSize or mCbCrSize. We should take out all the scaling code since we're not planning to use it. As mentioned F2F, this doesn't correctly handle the odd x-offset case. The easiest way to handle it might be to detect that case and specially handle the first pixel of each row in the y-loop of ConvertYCbCrToRGB32.

Comment on attachment 435096 [details] [diff] [review] Address review comments Nice. It would be great to have reftests for the odd offsets.

Rolled up previous two patches and rebased to trunk now that the new ogg backend has landed.

Pushed: http://hg.mozilla.org/mozilla-central/rev/f9a11b9b2b9f http://hg.mozilla.org/comm-central/rev/f6a698bde79c

Pushed to fix build bustage on PPC and ARM: http://hg.mozilla.org/mozilla-central/rev/4238959a7b0c

Additional changes to fix build bustage: 1) Missing argument to the C conversion path on ARM and PPC builds 2) Fix export on public API function for comm-central builds

Comment on attachment 437225 [details] [diff] [review] Build bustage fixes There's extraneous goop in breakage.patch --- Makefile.in and README at least aren't upstream files. Other than that it looks fine.

Thanks, breakage.patch fixed and pushed: http://hg.mozilla.org/mozilla-central/rev/87acb65c6318

From IRC: <smontagu> doublec: bug 551277 seems to have broken 64-bit OSX <smontagu> yuv_row_mac.cpp <smontagu> {standard input}:2:`pusha' is not supported in 64-bit mode <smontagu> {standard input}:42:`popa' is not supported in 64-bit mode <smontagu> gmake[5]: *** [yuv_row_mac.o] Error 1 The fix for comm-central also didn't work (same error) so I'm backing out.

Backout: http://hg.mozilla.org/mozilla-central/rev/ac18decab3b3 http://hg.mozilla.org/mozilla-central/rev/83e18ba3893d http://hg.mozilla.org/mozilla-central/rev/1d3fc492dfb2 http://hg.mozilla.org/mozilla-central/rev/56850227b294 http://hg.mozilla.org/mozilla-central/rev/f0ca1ffaa5b8 http://hg.mozilla.org/mozilla-central/rev/e49a2bda77c2 http://hg.mozilla.org/mozilla-central/rev/88baf8233995

Also, I hope that you consider Windows x64 build that doesn't support MMX and inline assembler.

For the Win 64 and Mac OS X 64 cases I'll fix the patch so that for the platforms that don't have assembler it falls back to the C code. That way it'll at least build and run on all platforms. I'll then file bugs to get optimized routines working for those platforms where it's needed. Although in the case of Mac OS X 64 at least I suspect all 64 bit Mac OS X machines have support for the hardware accelerated routines that are in development so maybe it's not needed for that case.

It's quite likely that Bas will want to use these routines in some cases even when the GL layers backend is available. For example if we're doing canvas.drawImage(video) it's probably faster to YUV-convert on the CPU than upload to the GPU, YUV-convert, and read back.

(In reply to comment #25) > For the Win 64 and Mac OS X 64 cases I'll fix the patch so that for the > platforms that don't have assembler it falls back to the C code. That way it'll > at least build and run on all platforms. I think such a fallback is always good to have, esp. for some non-tier-1 platforms for which we might not have assembler code around.

This is a rolled up patch with all bustage fixes. The changes are: - Get comm-central builds working by adding the ycbcr library to the layers makefile, removing FORCE_STATIC_BUILD from ycbcr makefile. - Make Win and Mac OS X 64 bit builds fall back to C code - Make unknown platforms fall back to C code I've uploaded to tryserver and have tested seamonkey builds locally.

Looks good. There is only one issue: the GL layers backend needs to be updated to account for the picture rect.

Is there a bug for that to make this dependant on? Who is working on it?

(In reply to comment #28) > I've uploaded to tryserver and have tested seamonkey builds locally. Chris, thanks a lot for testing our (SeaMonkey) configuration as well!

This is my attempt at fixing the opengl layers code to take into account of the picture region for the YCbCr conversion. Unfortunately I don't run windows and don't have a machine that can do OpenGL anyway so this is untested. Bas, can you please review and try this code? A sample video is: http://v2v.cc/~j/theora_testsuite/offset_test.ogv This should look like: http://v2v.cc/~j/theora_testsuite/offset_test.pass.png And not: http://v2v.cc/~j/theora_testsuite/offset_test.fail.png

The GL code looks OK to me. It doesn't do the correct Theora sampling, obviously, but that's OK for now.

This code doesn't work, it looks all garbled up, I'll look into it.

So the issue here is that OpenGL does not 'really' support stride on all platforms we want to support. It just supports alignment (i.e. 1/2/4/8 bytes). I've changed this code to make the intermediate storage limited to the size of the picture rect, and then copy the correct data in line by line, the testcase works with this patch. In the progress I've corrected the indentation and brace style to match the rest of the file. Here's a patch of what I ended up with.

+ mData.mCbCrStride = mData.mCbCrSize.width = aData.mPicSize.width >> width_shift; Shouldn't an odd width round up instead of down? Otherwise looks good.

Comment on attachment 438853 [details] [diff] [review] OpenGL changes for picture region Suggestion As discussed with roc, r+-ing this patch.

http://hg.mozilla.org/mozilla-central/rev/ae968e2a4792 Hmm, I guess comment #36 never got addressed. What's the story there?

This is to hopefully fix the comm-central breakage on windows that occurred when the patches in this bug landed: mozilla/gfx/ycbcr/yuv_convert.cpp(41) : error C2373: 'mozilla::gfx::ConvertYCbCrToRGB32' : redefinition; different type modifiers e:\builds\slave\comm-central-trunk-win32\build\mozilla\gfx\ycbcr\yuv_convert.h(25) : see declaration of 'mozilla::gfx::ConvertYCbCrToRGB32' NEXT ERROR make[6]: *** [yuv_convert.obj] Error 2

A fix equivalent to Attachment 440956 [details] [diff] landed by Mark Banner to fix comm-central bustage: http://hg.mozilla.org/mozilla-central/rev/fe937d72a9ce

(In reply to comment #17) > http://hg.mozilla.org/comm-central/rev/f6a698bde79c Ftr, I don't understand why this entry was added in the middle rather than sorted: anyway... I'll skip removing it in c-1.9.1 as it's in a |#ifndef MOZ_STATIC_BUILD| :-|

(In reply to comment #41) > I'll skip removing it in c-1.9.1 as it's in a |#ifndef MOZ_STATIC_BUILD| :-| I don't understand what you mean by this. Why do you want to remove it?

Hmm, looks like static SeaMonkey builds (i.e. nightlies) on Linux (32 and 64bit) are broken at least, see http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1272011238.1272016136.6325.gz and http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1272010136.1272014731.1703.gz

I found the problem identified in comment 43. The link like for static builds of thunderbird and seamonkey had libycbcr.a listed before liblayers. It needs to be listed after to successfully link. Finding out how this link order is created was a bit of a mission but it turns out to be the order of the directories listed in the DIRS line in gfx/Makefile.in. Placing ycrbcr at the end of this fixes things.

Pushed fix identified in comment 44: http://hg.mozilla.org/mozilla-central/rev/f8825507a492

Gah, the fix in comment 45 broke Windows thunderbird builds: NEXT ERROR make[6]: *** No rule to make target `../../dist/lib/ycbcr.lib', needed by `layers.dll'. Stop.

But did fix the linux static build. I'm beginning to think it's not possible to get this ycbcr stuff building across all platforms, and linking options.

This is my proposed fix for the issue in my last comment. I swap the order of ycbcr and thebes depending on whether we're doing static or shared builds. Testing builds now.

Comment on attachment 441047 [details] [diff] [review] fix for shared builds bustage What are your thoughts on this fix Robert? Is there a better way of organising the library dependancy?

The new gfx/ycbcr/Makefile.in assumes GTK2=Linux, which breaks Darwin/X11. Bug 561412 has patch that changes the MOZ_WIDGET_TOOLKIT tests to OS_ARCH, which then lets the build complete and run.

Landed cdouble's fix for shared builds bustage (from comment 48-49), with review still pending, to fix the burning on Thunderbird tinderboxen & on unsuspecting developers' machines. (my build & dbaron's build were both broken by this bug's checkin, & this followup fixed it for me.) http://hg.mozilla.org/mozilla-central/rev/e825fb134e7a Feel free to backout and/or improve with a better fix if one is agreed upon after this gets reviewed -- I'm just trying to stop the burning for now.

Comment on attachment 441047 [details] [diff] [review] fix for shared builds bustage weird.

Thanks. My Linux build of SM2.1 broke on mozilla/gfx/layers/opengl/ImageLayerOGL.cpp with make[6]: *** No rule to make target `-lycbcr', needed by `liblayers.so'. Stop. Now it has completed.

Regarding comment 51, I'd like to note that I didn't like to keep the Thunderbird boxes burning and would have pushed the fix to stop it without review but I had pushed a previous fix and would have had to wait another hour or two for the metered check-in window. I discussed on #maildev and they were ok for waiting for half a day or so for me to get advice on the fix.

Comment on attachment 441047 [details] [diff] [review] fix for shared builds bustage Gross. We should just produce fewer static libs for the final link.