Support color xforms for kIndex8 pngs

Support color xforms for kIndex8 pngs

This change started as: "Always use color xforms to
premultiply".  We need to be in a linear space to
premultiply correctly.

It became clear that we also need to support kIndex8
color xforms in order to make this change.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246143002

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

[msarett, 2016-08-15 17:40:18]

Description was changed from

==========
Always use color xform when we premul

In order to premultiply correctly, we must temporarily convert the
pixels to a linear color space

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

to

==========
Always use color xform when we premul

In order to premultiply correctly, we must temporarily convert the
pixels to a linear color space

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246143002
==========

[msarett, 2016-08-15 18:48:58]

Description was changed from

==========
Always use color xform when we premul

In order to premultiply correctly, we must temporarily convert the
pixels to a linear color space

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246143002
==========

to

==========
Support color xforms for kIndex8 pngs

This change started as: "Always use color xforms to
premultiply".  We need to be in a linear space to
premultiply correctly.

It became clear that we also need to support kIndex8
color xforms in order to make this change.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246143002
==========

[msarett, 2016-08-15 18:50:20]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 18:50:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 18:56:20]

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

[commit-bot: I haz the power, 2016-08-15 18:56:21]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[msarett, 2016-08-15 20:15:46]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 20:15:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarett, 2016-08-15 20:32:30]

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

[commit-bot: I haz the power, 2016-08-15 20:56:48]

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

[commit-bot: I haz the power, 2016-08-15 20:56:48]

Dry run: This issue passed the CQ dry run.

[mtklein, 2016-08-22 14:22:51]

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (left):

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:105: SkPMColor colorPtr[256];
Just noticed it's a little odd to have an array named ...Ptr.

Given how you always call it the color table, maybe, colorTable.

Instead of noting what these are not, you might want to explain what these can
be:

// Contents depend on tableColorType: {premul,unpremul} x {bgra,rgba} x ...
uint32_t colorTable[256];

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:115: // If we are performing a color xform, it will
handle the premultiply.
... otherwise we'll do it here.

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:117: bool doPremul =  needsPremul && !fColorXform;
Might be just as clear as

   bool premultiply = !fColorXform && needs_premul(dstInfo, this->getInfo());

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:161: fColorXform = nullptr;
Is there not some sort of early return we can make instead?  Code can get
confusing when variables are conditionally reassigned.

[msarett, 2016-08-22 15:01:35]

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (left):

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:105: SkPMColor colorPtr[256];
On 2016/08/22 14:22:51, mtklein wrote:
> Just noticed it's a little odd to have an array named ...Ptr.
> 
> Given how you always call it the color table, maybe, colorTable.
> 
> Instead of noting what these are not, you might want to explain what these can
> be:
> 
> // Contents depend on tableColorType: {premul,unpremul} x {bgra,rgba} x ...
> uint32_t colorTable[256];

Done.

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:115: // If we are performing a color xform, it will
handle the premultiply.
On 2016/08/22 14:22:51, mtklein wrote:
> ... otherwise we'll do it here.

Done.

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:117: bool doPremul =  needsPremul && !fColorXform;
On 2016/08/22 14:22:51, mtklein wrote:
> Might be just as clear as
> 
>    bool premultiply = !fColorXform && needs_premul(dstInfo, this->getInfo());

Done.

https://codereview.chromium.org/2246143002/diff/30001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:161: fColorXform = nullptr;
On 2016/08/22 14:22:51, mtklein wrote:
> Is there not some sort of early return we can make instead?  Code can get
> confusing when variables are conditionally reassigned.

Yeah this is confusing, I'll try this another way.

[mtklein, 2016-08-22 15:25:23]

The CQ bit was checked by mtklein@google.com

[mtklein, 2016-08-22 15:25:24]

lgtm

[commit-bot: I haz the power, 2016-08-22 15:25:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 15:48:42]

Description was changed from

==========
Support color xforms for kIndex8 pngs

This change started as: "Always use color xforms to
premultiply".  We need to be in a linear space to
premultiply correctly.

It became clear that we also need to support kIndex8
color xforms in order to make this change.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246143002
==========

to

==========
Support color xforms for kIndex8 pngs

This change started as: "Always use color xforms to
premultiply".  We need to be in a linear space to
premultiply correctly.

It became clear that we also need to support kIndex8
color xforms in order to make this change.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246143002

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

[commit-bot: I haz the power, 2016-08-22 15:48:43]

Committed patchset #5 (id:70001) as
https://skia.googlesource.com/skia/+/dcd5e65340f9ef239d769bf20bafd2fa819eab31