Closed Bug 551277 Opened 10 years ago Closed 10 years ago

Replace liboggplay YUV to RGB color conversion code

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: cajbir, Assigned: cajbir)

References

Details

Attachments

(5 files, 12 obsolete files)

417 bytes, patch
cajbir
: review+
Details | Diff | Splinter Review
159.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.83 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
1.51 KB, patch
roc
: review+
Details | Diff | Splinter Review
556 bytes, patch
roc
: review+
Details | Diff | Splinter Review
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?
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attachment #431461 - Flags: review?(roc)
Depends on: 538323
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!
Attached patch Address review comments (obsolete) — Splinter Review
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.
Attachment #431461 - Attachment is obsolete: true
Attachment #431534 - Flags: review?(roc)
Attachment #431461 - Flags: review?(roc)
+  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.
Blocks: 551378
Attached patch Address review comments (obsolete) — Splinter Review
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.
Attachment #431534 - Attachment is obsolete: true
Attachment #431554 - Flags: review?(roc)
Attachment #431534 - Flags: review?(roc)
Attached patch comm-central patch? (obsolete) — Splinter Review
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.
Attachment #431554 - Flags: review?(roc) → review+
API Changed as per IRC discussion. Changed library name to ycbcr. Carried forward r+.
Attachment #431554 - Attachment is obsolete: true
Attachment #431769 - Flags: review+
Changed library name to ycbcr. Carried forward r+.
Attachment #431556 - Attachment is obsolete: true
Attachment #431773 - Flags: review+
Attached patch Rebased patch (obsolete) — Splinter Review
Rebased patch for video layers changes (some files were moved to gfx/layers/basic directory)
Attachment #431769 - Attachment is obsolete: true
Attachment #433206 - Flags: review+
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.
Attachment #433206 - Attachment is obsolete: true
Attachment #435078 - Flags: review+
Depends on: 531340
No longer depends on: 538323
Attached patch handle odd sized picture regions (obsolete) — Splinter Review
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.
Attachment #435079 - Flags: review?(roc)
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.
Attached patch Address review comments (obsolete) — Splinter Review
Attachment #435079 - Attachment is obsolete: true
Attachment #435096 - Flags: review?(roc)
Attachment #435079 - Flags: review?(roc)
Comment on attachment 435096 [details] [diff] [review]
Address review comments

Nice.

It would be great to have reftests for the odd offsets.
Attachment #435096 - Flags: review?(roc) → review+
Depends on: 555609
Blocks: 555609
No longer depends on: 555609
Attached patch Rolled up, rebased patch (obsolete) — Splinter Review
Rolled up previous two patches and rebased to trunk now that the new ogg backend has landed.
Attachment #435078 - Attachment is obsolete: true
Attachment #435096 - Attachment is obsolete: true
Attachment #436824 - Flags: review+
Keywords: checkin-needed
Pushed to fix build bustage on PPC and ARM:
http://hg.mozilla.org/mozilla-central/rev/4238959a7b0c
Attached patch Build bustage fixes (obsolete) — Splinter Review
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
Attachment #437225 - Flags: review?(roc)
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.
Attachment #437225 - Flags: review?(roc) → review+
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.
Keywords: checkin-needed
Also, I hope that you consider Windows x64 build that doesn't support MMX and inline assembler.
Version: unspecified → Trunk
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.
Attachment #436824 - Attachment is obsolete: true
Attachment #437225 - Attachment is obsolete: true
Attachment #437502 - Flags: review?(roc)
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
Attachment #437746 - Flags: review?(bas.schouten)
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.
Attachment #438853 - Flags: review+
Attachment #437746 - Attachment is obsolete: true
Attachment #437746 - Flags: review?(bas.schouten)
http://hg.mozilla.org/mozilla-central/rev/ae968e2a4792

Hmm, I guess comment #36 never got addressed. What's the story there?
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Attachment #440956 - Flags: review?(roc)
Target Milestone: --- → mozilla1.9.3a5
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
Depends on: 561344
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.
Target Milestone: mozilla1.9.3a5 → ---
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.
Attachment #441047 - Flags: review?(roc)
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.
Depends on: 561412
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.
Attachment #441047 - Flags: review?(roc)
Attachment #441047 - Flags: review+
Attachment #441047 - Flags: feedback?(ted.mielczarek)
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.
Target Milestone: --- → mozilla1.9.3a5
Comment on attachment 441047 [details] [diff] [review]
fix for shared builds bustage

Gross. We should just produce fewer static libs for the final link.
Attachment #441047 - Flags: feedback?(ted.mielczarek) → feedback+
Depends on: 577645
You need to log in before you can comment on or make changes to this bug.