Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

Swizzle Bench Runtime
Nexus 6P     0.14x
Dell Venue 8 0.12x

CMYK Jpeg Decode Runtime
Nexus 6P     0.81x
Dell Venue 8 0.85x

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1676773003
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed: https://skia.googlesource.com/skia/+/c5c322d8ecfc05718f9f04360956c4f1f9dc33c1

[msarett, 2016-02-06 18:07:51]

Description was changed from

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

The conversion that we perform is relatively crude.  But we might
as well make it fast.

Swizzle Bench Runtime
Nexus 6P
Dell Venue 8

CMYK Jpeg Decode Runtime
Nexus 6P
Dell Venue 8

BUG=skia:
==========

to

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

The conversion that we perform is relatively crude.  But we might
as well make it fast.

Swizzle Bench Runtime
Nexus 6P
Dell Venue 8

CMYK Jpeg Decode Runtime
Nexus 6P
Dell Venue 8

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-02-06 18:11:46]

Description was changed from

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

The conversion that we perform is relatively crude.  But we might
as well make it fast.

Swizzle Bench Runtime
Nexus 6P
Dell Venue 8

CMYK Jpeg Decode Runtime
Nexus 6P
Dell Venue 8

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

The conversion that we perform is relatively crude.  But we might
as well make it fast.

Swizzle Bench Runtime
Nexus 6P     0.14x
Dell Venue 8 0.??x

CMYK Jpeg Decode Runtime
Nexus 6P     0.81x
Dell Venue 8 0.??x

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-02-08 14:42:01]

Description was changed from

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

The conversion that we perform is relatively crude.  But we might
as well make it fast.

Swizzle Bench Runtime
Nexus 6P     0.14x
Dell Venue 8 0.??x

CMYK Jpeg Decode Runtime
Nexus 6P     0.81x
Dell Venue 8 0.??x

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

The conversion that we perform is relatively crude.  But we might
as well make it fast.

Swizzle Bench Runtime
Nexus 6P     0.14x
Dell Venue 8 0.12x

CMYK Jpeg Decode Runtime
Nexus 6P     0.81x
Dell Venue 8 0.85x

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-02-08 14:42:39]

msarett@google.com changed reviewers:
+ mtklein@google.com, scroggo@google.com

[scroggo, 2016-02-08 14:59:43]

Deferring to Mike on the review, but I did have a question and a suggestion.

1gtm

https://codereview.chromium.org/1676773003/diff/1/src/core/SkOpts.h
File src/core/SkOpts.h (right):

https://codereview.chromium.org/1676773003/diff/1/src/core/SkOpts.h#newcode68
src/core/SkOpts.h:68: CMYK_to_RGB1,  // i.e. convert color space
I know this isn't the first appearance of it, but what does the "1" mean in this
context?

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h#...
src/opts/SkSwizzler_opts.h:135: uint8_t b = (y*k+127)/255,
This pattern appears a lot. Should it be a macro?

[mtklein, 2016-02-08 15:08:48]

https://codereview.chromium.org/1676773003/diff/1/src/core/SkOpts.h
File src/core/SkOpts.h (right):

https://codereview.chromium.org/1676773003/diff/1/src/core/SkOpts.h#newcode68
src/core/SkOpts.h:68: CMYK_to_RGB1,  // i.e. convert color space
On 2016/02/08 14:59:43, scroggo wrote:
> I know this isn't the first appearance of it, but what does the "1" mean in
this
> context?

opaque.  the alpha channel is 1.

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h#...
src/opts/SkSwizzler_opts.h:135: uint8_t b = (y*k+127)/255,
On 2016/02/08 14:59:43, scroggo wrote:
> This pattern appears a lot. Should it be a macro?

Don't think so.  When we put it in a macro, people are tempted to change the
macro to be faster and less correct.

[mtklein, 2016-02-08 15:14:26]

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h#...
src/opts/SkSwizzler_opts.h:137: r = (c*k+127)/255;
This is really the math?  Having never seen it before, this seems surprising. 
I'd think r goes inversely in proportion to c and k, g inversely to m and k, b
inversely to y and k, etc.

Is it not r == ((255-c)*(255-k)+127)/255, etc?

[msarett, 2016-02-08 15:23:08]

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h#...
src/opts/SkSwizzler_opts.h:137: r = (c*k+127)/255;
On 2016/02/08 15:14:26, mtklein wrote:
> This is really the math?  Having never seen it before, this seems surprising. 
> I'd think r goes inversely in proportion to c and k, g inversely to m and k, b
> inversely to y and k, etc.
> 
> Is it not r == ((255-c)*(255-k)+127)/255, etc?

The short answer is that libjpeg-turbo actually outputs inverted CMYK.  Maybe
these functions should be renamed?  Or maybe the below explanation (from
SkSwizzler.cpp) belongs here?

CMYK is stored as four bytes per pixel.
We will implement a crude conversion from CMYK -> RGB using formulas
from easyrgb.com.
CMYK -> CMY
C = C * (1 - K) + K
M = M * (1 - K) + K
Y = Y * (1 - K) + K
libjpeg actually gives us inverted CMYK, so we must subtract the
original terms from 1.
CMYK -> CMY
C = (1 - C) * (1 - (1 - K)) + (1 - K)
M = (1 - M) * (1 - (1 - K)) + (1 - K)
Y = (1 - Y) * (1 - (1 - K)) + (1 - K)
Simplifying the above expression.
CMYK -> CMY
C = 1 - CK
M = 1 - MK
Y = 1 - YK
CMY -> RGB
R = (1 - C) * 255
G = (1 - M) * 255
B = (1 - Y) * 255
Therefore the full conversion is below.  This can be verified at
www.rapidtables.com (assuming inverted CMYK).
CMYK -> RGB
R = C * K * 255
G = M * K * 255
B = Y * K * 255
As a final note, we have treated the CMYK values as if they were on
a scale from 0-1, when in fact they are 8-bit ints scaling from 0-255.
We must divide each CMYK component by 255 to obtain the true conversion
we should perform.
CMYK -> RGB
R = C * K / 255
G = M * K / 255
B = Y * K / 255

[mtklein, 2016-02-08 16:47:15]

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h#...
src/opts/SkSwizzler_opts.h:137: r = (c*k+127)/255;
On 2016/02/08 15:23:08, msarett wrote:
> On 2016/02/08 15:14:26, mtklein wrote:
> > This is really the math?  Having never seen it before, this seems
surprising. 
> > I'd think r goes inversely in proportion to c and k, g inversely to m and k,
b
> > inversely to y and k, etc.
> > 
> > Is it not r == ((255-c)*(255-k)+127)/255, etc?
> 
> The short answer is that libjpeg-turbo actually outputs inverted CMYK.  Maybe
> these functions should be renamed?  Or maybe the below explanation (from
> SkSwizzler.cpp) belongs here?
> 
> CMYK is stored as four bytes per pixel.
> We will implement a crude conversion from CMYK -> RGB using formulas
> from http://easyrgb.com.
> CMYK -> CMY
> C = C * (1 - K) + K
> M = M * (1 - K) + K
> Y = Y * (1 - K) + K
> libjpeg actually gives us inverted CMYK, so we must subtract the
> original terms from 1.
> CMYK -> CMY
> C = (1 - C) * (1 - (1 - K)) + (1 - K)
> M = (1 - M) * (1 - (1 - K)) + (1 - K)
> Y = (1 - Y) * (1 - (1 - K)) + (1 - K)
> Simplifying the above expression.
> CMYK -> CMY
> C = 1 - CK
> M = 1 - MK
> Y = 1 - YK
> CMY -> RGB
> R = (1 - C) * 255
> G = (1 - M) * 255
> B = (1 - Y) * 255
> Therefore the full conversion is below.  This can be verified at
> http://www.rapidtables.com (assuming inverted CMYK).
> CMYK -> RGB
> R = C * K * 255
> G = M * K * 255
> B = Y * K * 255
> As a final note, we have treated the CMYK values as if they were on
> a scale from 0-1, when in fact they are 8-bit ints scaling from 0-255.
> We must divide each CMYK component by 255 to obtain the true conversion
> we should perform.
> CMYK -> RGB
> R = C * K / 255
> G = M * K / 255
> B = Y * K / 255

Let's go with a name change (inverted_CMYK_to_...) and a pointer to that
explanation.

[msarett, 2016-02-08 17:22:42]

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1676773003/diff/1/src/opts/SkSwizzler_opts.h#...
src/opts/SkSwizzler_opts.h:137: r = (c*k+127)/255;
On 2016/02/08 16:47:15, mtklein wrote:
> On 2016/02/08 15:23:08, msarett wrote:
> > On 2016/02/08 15:14:26, mtklein wrote:
> > > This is really the math?  Having never seen it before, this seems
> surprising. 
> > > I'd think r goes inversely in proportion to c and k, g inversely to m and
k,
> b
> > > inversely to y and k, etc.
> > > 
> > > Is it not r == ((255-c)*(255-k)+127)/255, etc?
> > 
> > The short answer is that libjpeg-turbo actually outputs inverted CMYK. 
Maybe
> > these functions should be renamed?  Or maybe the below explanation (from
> > SkSwizzler.cpp) belongs here?
> > 
> > CMYK is stored as four bytes per pixel.
> > We will implement a crude conversion from CMYK -> RGB using formulas
> > from http://easyrgb.com.
> > CMYK -> CMY
> > C = C * (1 - K) + K
> > M = M * (1 - K) + K
> > Y = Y * (1 - K) + K
> > libjpeg actually gives us inverted CMYK, so we must subtract the
> > original terms from 1.
> > CMYK -> CMY
> > C = (1 - C) * (1 - (1 - K)) + (1 - K)
> > M = (1 - M) * (1 - (1 - K)) + (1 - K)
> > Y = (1 - Y) * (1 - (1 - K)) + (1 - K)
> > Simplifying the above expression.
> > CMYK -> CMY
> > C = 1 - CK
> > M = 1 - MK
> > Y = 1 - YK
> > CMY -> RGB
> > R = (1 - C) * 255
> > G = (1 - M) * 255
> > B = (1 - Y) * 255
> > Therefore the full conversion is below.  This can be verified at
> > http://www.rapidtables.com (assuming inverted CMYK).
> > CMYK -> RGB
> > R = C * K * 255
> > G = M * K * 255
> > B = Y * K * 255
> > As a final note, we have treated the CMYK values as if they were on
> > a scale from 0-1, when in fact they are 8-bit ints scaling from 0-255.
> > We must divide each CMYK component by 255 to obtain the true conversion
> > we should perform.
> > CMYK -> RGB
> > R = C * K / 255
> > G = M * K / 255
> > B = Y * K / 255
> 
> Let's go with a name change (inverted_CMYK_to_...) and a pointer to that
> explanation.

Done.

[mtklein, 2016-02-08 17:31:26]

Just curious: what aspect of what we're doing here is crude?  Seems solid to me.
 Loss of precision?

Only nits about names.

https://codereview.chromium.org/1676773003/diff/20001/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1676773003/diff/20001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:439: template <bool kSwapRB>
Let's do some renaming to get "swap" out of these names?  It's sort of weird to
say swap when there's no starting red and blue values.  E.g.

enum Format { kRGB1, kBGR1 };

template <Format format>
static void inverted_cmyk_to(...) {


}
(Called as inverted_cmyk_to_<kRGB1>, inverted_cmyk_to<kBGR1>)

[msarett, 2016-02-08 17:42:32]

Description was changed from

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

The conversion that we perform is relatively crude.  But we might
as well make it fast.

Swizzle Bench Runtime
Nexus 6P     0.14x
Dell Venue 8 0.12x

CMYK Jpeg Decode Runtime
Nexus 6P     0.81x
Dell Venue 8 0.85x

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

Swizzle Bench Runtime
Nexus 6P     0.14x
Dell Venue 8 0.12x

CMYK Jpeg Decode Runtime
Nexus 6P     0.81x
Dell Venue 8 0.85x

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-02-08 18:09:47]

I've removed the comment about the "crude" conversion from the commit message. 
I'm not really sure if it is crude or not without doing further work on color
space / color profiles.

libjpeg-turbo rejected a patch to convert CMYK->RGB on the grounds that the
conversion is not well-defined without interpreting a color profile.
http://sourceforge.net/p/libjpeg-turbo/patches/15/

I can confirm a few of their concerns:
(1) It is hard to find a CMYK->RGB formula on the web.
(2) Many of the proposed "formulas" are actually different.  I don't know who is
"correct".

https://codereview.chromium.org/1676773003/diff/20001/src/opts/SkSwizzler_opts.h
File src/opts/SkSwizzler_opts.h (right):

https://codereview.chromium.org/1676773003/diff/20001/src/opts/SkSwizzler_opt...
src/opts/SkSwizzler_opts.h:439: template <bool kSwapRB>
On 2016/02/08 17:31:26, mtklein wrote:
> Let's do some renaming to get "swap" out of these names?  It's sort of weird
to
> say swap when there's no starting red and blue values.  E.g.
> 
> enum Format { kRGB1, kBGR1 };
> 
> template <Format format>
> static void inverted_cmyk_to(...) {
> 
> 
> }
> (Called as inverted_cmyk_to_<kRGB1>, inverted_cmyk_to<kBGR1>)

Done.

[mtklein, 2016-02-08 20:45:41]

lgtm

[msarett, 2016-02-08 20:49:23]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-02-08 20:49:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1676773003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1676773003/40001

[commit-bot: I haz the power, 2016-02-08 20:52:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 20:52:10]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[msarett, 2016-02-08 20:59:01]

The CQ bit was checked by msarett@google.com

[msarett, 2016-02-08 20:59:02]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1676773003/#ps60001
(title: "Windows")

[commit-bot: I haz the power, 2016-02-08 20:59:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1676773003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1676773003/60001

[commit-bot: I haz the power, 2016-02-08 21:26:27]

Description was changed from

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

Swizzle Bench Runtime
Nexus 6P     0.14x
Dell Venue 8 0.12x

CMYK Jpeg Decode Runtime
Nexus 6P     0.81x
Dell Venue 8 0.85x

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Optimize CMYK->RGBA (BGRA) transform for jpeg decodes

Swizzle Bench Runtime
Nexus 6P     0.14x
Dell Venue 8 0.12x

CMYK Jpeg Decode Runtime
Nexus 6P     0.81x
Dell Venue 8 0.85x

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed:
https://skia.googlesource.com/skia/+/c5c322d8ecfc05718f9f04360956c4f1f9dc33c1
==========

[commit-bot: I haz the power, 2016-02-08 21:26:27]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/c5c322d8ecfc05718f9f04360956c4f1f9dc33c1