Replace liboggplay YUV to RGB color conversion code

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Audio/Video
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
mozilla1.9.3a5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 12 obsolete attachments)

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
(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 431461 [details] [diff] [review]
Use color conversion code from Chromium

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)
(Assignee)

Updated

8 years ago
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!
(Assignee)

Comment 3

8 years ago
Created attachment 431534 [details] [diff] [review]
Address review comments

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.
(Assignee)

Updated

8 years ago
Blocks: 551378
(Assignee)

Comment 5

8 years ago
Created attachment 431554 [details] [diff] [review]
Address review comments

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)
(Assignee)

Comment 6

8 years ago
Created attachment 431556 [details] [diff] [review]
comm-central patch?

Possible comm-central patch? Is it only this one line change required? I don't really know what's expected here...
Attachment #431556 - Flags: review+
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+
(Assignee)

Comment 8

8 years ago
Created attachment 431769 [details] [diff] [review]
Change public API as discussed on IRC

API Changed as per IRC discussion. Changed library name to ycbcr. Carried forward r+.
Attachment #431554 - Attachment is obsolete: true
Attachment #431769 - Flags: review+
(Assignee)

Comment 9

8 years ago
Created attachment 431773 [details] [diff] [review]
comm-central patch

Changed library name to ycbcr. Carried forward r+.
Attachment #431556 - Attachment is obsolete: true
Attachment #431773 - Flags: review+
(Assignee)

Comment 10

8 years ago
Created attachment 433206 [details] [diff] [review]
Rebased patch

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+
(Assignee)

Comment 11

8 years ago
Created attachment 435078 [details] [diff] [review]
Rebased patch to apply on new ogg backend

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+
(Assignee)

Updated

8 years ago
Depends on: 531340
No longer depends on: 538323
(Assignee)

Comment 12

8 years ago
Created attachment 435079 [details] [diff] [review]
handle odd sized picture regions

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.
(Assignee)

Comment 14

8 years ago
Created attachment 435096 [details] [diff] [review]
Address review comments
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
Blocks: 556455
(Assignee)

Comment 16

8 years ago
Created attachment 436824 [details] [diff] [review]
Rolled up, rebased patch

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+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 17

8 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/f9a11b9b2b9f
http://hg.mozilla.org/comm-central/rev/f6a698bde79c
(Assignee)

Comment 18

8 years ago
Pushed to fix build bustage on PPC and ARM:
http://hg.mozilla.org/mozilla-central/rev/4238959a7b0c
(Assignee)

Comment 19

8 years ago
Created attachment 437225 [details] [diff] [review]
Build bustage fixes

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+
(Assignee)

Comment 21

8 years ago
Thanks, breakage.patch fixed and pushed:
http://hg.mozilla.org/mozilla-central/rev/87acb65c6318
(Assignee)

Comment 22

8 years ago
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.
(Assignee)

Comment 23

8 years ago
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
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Also, I hope that you consider Windows x64 build that doesn't support MMX and inline assembler.
Version: unspecified → Trunk
(Assignee)

Comment 25

8 years ago
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.

Comment 27

8 years ago
(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.
(Assignee)

Comment 28

8 years ago
Created attachment 437502 [details] [diff] [review]
Rolled up patch with bustage fixes

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
(Assignee)

Updated

8 years ago
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.
(Assignee)

Comment 30

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

Comment 31

8 years ago
(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!
(Assignee)

Comment 32

8 years ago
Created attachment 437746 [details] [diff] [review]
OpenGL changes for picture region

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)
Attachment #437502 - Flags: review?(roc) → review+
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.
Created attachment 438853 [details] [diff] [review]
OpenGL changes for picture region Suggestion

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.
(Assignee)

Comment 37

8 years ago
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+
(Assignee)

Updated

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 39

8 years ago
Created attachment 440956 [details] [diff] [review]
patch to fix comm-central breakage on windows

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)
Attachment #440956 - Flags: review?(roc) → review+
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Comment 40

8 years ago
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| :-|
(Assignee)

Comment 42

8 years ago
(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?

Comment 43

8 years ago
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

Updated

8 years ago
Depends on: 561344
(Assignee)

Comment 44

8 years ago
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 → ---
(Assignee)

Comment 45

8 years ago
Pushed fix identified in comment 44:
http://hg.mozilla.org/mozilla-central/rev/f8825507a492
(Assignee)

Comment 46

8 years ago
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.
(Assignee)

Comment 47

8 years ago
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.
(Assignee)

Comment 48

8 years ago
Created attachment 441047 [details] [diff] [review]
fix for shared builds bustage

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.
(Assignee)

Updated

8 years ago
Attachment #441047 - Flags: review?(roc)
(Assignee)

Comment 49

8 years ago
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)

Comment 53

8 years ago
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.
(Assignee)

Comment 54

8 years ago
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.