Add BGRA output format support to qcms

Add BGRA output format support to qcms

Define QCMS_OUTPUT_BGRX and add qcms_transform_data_type() api to output
BGR or BGRA format for GRAY, GRAYA, RGB, and RGBA input data.

Update all color transform functions with an output format argument, use
that to select RGBX (the default) or BGRX output format.

Add google.patch: patch of the BGRA output changes against upstream qcms
and add README.chromium details about google.patch. TODO: send the patch
to qcms if accepted for review upstream.

Disable Visual Studio warnings when needed.

BUG=143
TEST=None

[Evan Stade, 2012-05-14 21:44:40]

rubber stamp lgtm but I think tpayne should take a look.

[tpayne, 2012-05-14 21:54:23]

On 2012/05/14 21:44:40, Evan Stade wrote:
> rubber stamp lgtm but I think tpayne should take a look.

As long as there are no special optimizations that happen when writing memory in
order that we lose by sometimes changing the order, this looks good.

It seems that the only input format supported is RGBx. Does that mean we will no
longer use the turbo swizzle for JPEGs?

[Noel Gordon, 2012-05-15 22:35:49]

> On 2012/05/14 21:54:23, tpayne wrote:
> As long as there are no special optimizations that happen when writing memory
in
> order that we lose by sometimes changing the order, this looks good.

I wonder what those optimizations might be.  Perhaps measure that before using
this patch so we can get a handle on it?  Note I don't intend to submit this
patch until you're ready for it, so let me know.

The alternative approach is to write bgra-specific versions of the transform
routines.  That'll double the code size.

[Noel Gordon, 2012-05-15 22:38:53]

On 2012/05/14 21:54:23, tpayne wrote:
> It seems that the only input format supported is RGBx. Does that mean we will
no
> longer use the turbo swizzle for JPEGs?

Yes GX and RGBx inputs only.  But no we can still use a swizzle but it has to be
a different type. Let's add hbono-san for confirmation.

[Noel Gordon, 2012-05-15 22:46:44]

@hbono in WebCore, we currently JPEG decode with a JCS_EXT_BGRA swizzle and it's
very fast.  If we switch to using a JCS_EXT_RGBA swizzle, that should be
similarly fast right?  I assume JCS_EXT_RGBA is implemented in SSE?

[Hironori Bono, 2012-05-16 07:39:32]

Greetings,

Sorry for my slow response. In general, JPEG swizzling can remove unnecessary
memory copies used just for swapping their pixel orders (e.g. from RGB to BGRA).
I assume this library also needs to process huge images and it should be a huge
win to remove unnecessary memory copies.

Regards,

Hironori Bono

[Hironori Bono, 2012-05-16 09:05:11]

Greetings,

Oops, I forgot noting about JCS_EXT_RGBA. It uses SSE(2) to convert 4 pixels (16
components) at once.

Regards,

Hironori Bono

[tpayne, 2012-05-18 01:52:35]

On 2012/05/15 22:35:49, noel chromium wrote:
> > On 2012/05/14 21:54:23, tpayne wrote:
> > As long as there are no special optimizations that happen when writing
memory
> in
> > order that we lose by sometimes changing the order, this looks good.
> 
> I wonder what those optimizations might be.  Perhaps measure that before using
> this patch so we can get a handle on it?  Note I don't intend to submit this
> patch until you're ready for it, so let me know.
> 
> The alternative approach is to write bgra-specific versions of the transform
> routines.  That'll double the code size.

I've updated my code to work with this patch. I'd like to see this land sooner
than later.

[Hironori Bono, 2012-05-18 02:13:42]

Greetings,

Sorry, I forgot sending an LGTM message. Tony, does this change look good for
you?

Regards,

Hironori Bono

[Noel Gordon, 2012-05-18 14:49:12]

On 2012/05/16 09:05:11, Hironori Bono wrote:
> Greetings,
> 
> Oops, I forgot noting about JCS_EXT_RGBA. It uses SSE(2) to convert 4 pixels
(16
> components) at once.

Thank you Bono-san, I appreciate your good thoughts as always.  I read from this
that JCS_EXT_RGBA and JCS_EXT_BGRA swizzles are both in SSE(2) and so are
equally as fast. Makes sense to me.

~noel

[Noel Gordon, 2012-05-18 15:27:32]

On 2012/05/16 07:39:32, Hironori Bono wrote:
> Greetings,
> 
> In general, JPEG swizzling can remove unnecessary
> memory copies used just for swapping their pixel orders (e.g. from RGB to
BGRA).

Indeed and, as you mentioned earlier, at 16 components per instruction!


> I assume this library also needs to process huge images and it should be a
huge
> win to remove unnecessary memory copies.

Exactly.  All the image pixel must be color corrected.  In essence, a look-up
table for each color component is used to map input R G B bytes to output R G B
bytes; the alpha channel passes straight through to the output.  Every pixel
must be mapped.

There are software implementations of the pixel read-write loops that map the
given input data through the color correction tables.  There are specialized
implementations in SSE1 and SSE2 that we will be using, namely,

   qcms/src/transform-sse1.c
   qcms/src/transform-sse2.c

If you have time to look at those some time, that would help.  I would be
curious to know if you see any potential for increasing the speed of that
SSE1/SSE2 code.

~noel

[Noel Gordon, 2012-05-18 15:37:42]

Tom, you did the alternative approach right?  How did that go?  I'm happy to go
either way here with an eye on speed, but maybe the code size doubling wasn't so
bad?  

I have one eye on the mozilla qcms/turbo/swizzle bug and that's yet to be
resolved.

Need to updating my patch.  May break compilation on mac/linux.  Need to add a
few #if defined (_MSC_VER) in qcmsint.h@90-95.

[Noel Gordon, 2012-05-18 15:49:53]

> ... May break compilation on mac/linux.  Need to add a
> few #if defined (_MSC_VER) in qcmsint.h@90-95.

Correction: in qcms/src/qcmstypes.h

[tpayne_google.com, 2012-05-18 15:52:04]

I did do the other approach. It worked fine but I don't know any reason why
yours wouldn't work just as well. I didn't see any difference in render
time for my little benchmark.
On May 18, 2012 8:37 AM, <noel@chromium.org> wrote:

> Tom, you did the alternative approach right?  How did that go?  I'm happy
> to go
> either way here with an eye on speed, but maybe the code size doubling
> wasn't so
> bad?
>
> I have one eye on the mozilla qcms/turbo/swizzle bug and that's yet to be
> resolved.
>
> Need to updating my patch.  May break compilation on mac/linux.  Need to
> add a
> few #if defined (_MSC_VER) in qcmsint.h@90-95.
>
>
>
http://codereview.chromium.**org/10387099/<http://codereview.chromium.org/103...
>

[Noel Gordon, 2012-05-18 15:53:04]

http://codereview.chromium.org/10387099/diff/1/third_party/qcms/src/qcmstypes.h
File third_party/qcms/src/qcmstypes.h (right):

http://codereview.chromium.org/10387099/diff/1/third_party/qcms/src/qcmstypes...
third_party/qcms/src/qcmstypes.h:90: #pragma warning(push)
Note to self: wrap these pragmas in #if defined(_MSC_VER).

http://codereview.chromium.org/10387099/diff/1/third_party/qcms/src/qcmstypes...
third_party/qcms/src/qcmstypes.h:95: #pragma warning(pop)
Note to self: and the same here.

[Noel Gordon, 2012-05-18 16:03:13]

Ignore me. That qcmstypes.h code's in an #if defined(_MSC_VER) block &.&

[Noel Gordon, 2012-05-18 16:09:55]

On 2012/05/18 15:52:04, tpayne_google.com wrote:
> I did do the other approach. It worked fine but I don't know any reason why
> yours wouldn't work just as well. I didn't see any difference in render
> time for my little benchmark.

Ok, I guess my goal was a minimally intrusive change in qcms.  Let's go with
this for now. When you're ready, poke a reviewer.

[Noel Gordon, 2012-05-18 16:14:52]

Didn't need to do these at all on inspection.  All done.

http://codereview.chromium.org/10387099/diff/1/third_party/qcms/src/qcmstypes.h
File third_party/qcms/src/qcmstypes.h (right):

http://codereview.chromium.org/10387099/diff/1/third_party/qcms/src/qcmstypes...
third_party/qcms/src/qcmstypes.h:90: #pragma warning(push)
On 2012/05/18 15:53:07, noel chromium wrote:
> Note to self: wrap these pragmas in #if defined(_MSC_VER).

Done.

http://codereview.chromium.org/10387099/diff/1/third_party/qcms/src/qcmstypes...
third_party/qcms/src/qcmstypes.h:95: #pragma warning(pop)
On 2012/05/18 15:53:07, noel chromium wrote:
> Note to self: and the same here.

Done.

[commit-bot: I haz the power, 2012-05-21 18:11:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noel@chromium.org/10387099/1

[commit-bot: I haz the power, 2012-05-21 22:54:35]

Try job failure for 10387099-1 (retry) (retry) on win_rel for step
"sync_unit_tests".
It's a second try, previously, steps "base_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Noel Gordon, 2012-05-23 07:54:24]

hbono wrote:
> > I assume this library also needs to process huge images and it should be a
huge
> > win to remove unnecessary memory copies.


noel wrote:
> Exactly.  All the image pixel must be color corrected.  In essence, a look-up
> table for each color component is used to map input R G B bytes to output R G
B
> bytes; the alpha channel passes straight through to the output.  Every pixel
> must be mapped.
> 
> There are software implementations of the pixel read-write loops that map the
> given input data through the color correction tables.  There are specialized
> implementations in SSE1 and SSE2 that we will be using, namely,
> 
>    qcms/src/transform-sse1.c
>    qcms/src/transform-sse2.c
> 
> If you have time to look at those some time, that would help.  I would be
> curious to know if you see any potential for increasing the speed of that
> SSE1/SSE2 code.

Some work not yet completed, benefits older class machines (Pentiums, etc).
  https://bugzilla.mozilla.org/show_bug.cgi?id=523584

Other qcms bugs, some regarding the speed issue
  https://bugzilla.mozilla.org/buglist.cgi?quicksearch=GFX%3A+Color+Management

[Noel Gordon, 2012-05-23 07:57:05]

Patch landed, closing issue.
  http://src.chromium.org/viewvc/chrome?view=rev&revision=138414