Perform color correction on png decodes

Perform color correction on png decodes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

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

[msarett, 2016-07-26 15:05:55]

Description was changed from

==========
Add Premul to SkColorXform, handle Opaque/Unpremul in the same API

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

to

==========
Add Premul to SkColorXform, handle Opaque/Unpremul in the same API

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-07-26 15:16:25]

Patchset #1 (id:1) has been deleted

[msarett, 2016-07-26 15:54:35]

Patchset #1 (id:20001) has been deleted

[msarett, 2016-07-26 18:01:16]

Description was changed from

==========
Add Premul to SkColorXform, handle Opaque/Unpremul in the same API

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Add Premul to SkColorXform, handle Opaque/Unpremul in the same API

Performance on z620 (201295.jpg):

toSRGB:  356us -> 346us
Cool!  But why did this get better?

to2Dot2: 414us -> 426us

toF16:   302us -> 358us
The major pain here is that now we must convert the
original alpha to F16 (since in the unpremul case
it is not always just "1").  Maybe worth considering
a special case for F16 opaque?

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-07-27 18:32:55]

Patchset #1 (id:40001) has been deleted

[msarett, 2016-07-27 18:33:05]

Patchset #1 (id:60001) has been deleted

[msarett, 2016-07-28 18:10:10]

Patchset #1 (id:80001) has been deleted

[msarett, 2016-07-28 18:10:25]

Patchset #1 (id:100001) has been deleted

[msarett, 2016-07-28 19:06:23]

Patchset #2 (id:140001) has been deleted

[msarett, 2016-07-28 19:06:29]

Patchset #1 (id:120001) has been deleted

[msarett, 2016-07-28 19:14:51]

Patchset #1 (id:160001) has been deleted

[msarett, 2016-07-28 19:16:22]

Description was changed from

==========
Add Premul to SkColorXform, handle Opaque/Unpremul in the same API

Performance on z620 (201295.jpg):

toSRGB:  356us -> 346us
Cool!  But why did this get better?

to2Dot2: 414us -> 426us

toF16:   302us -> 358us
The major pain here is that now we must convert the
original alpha to F16 (since in the unpremul case
it is not always just "1").  Maybe worth considering
a special case for F16 opaque?

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Add Premul to SkColorXform, handle Opaque/Unpremul in the same API

Performance (Unpremul/Opaque) on z620 (201295.jpg):

toSRGB:  356us -> 363us
to2Dot2: 414us -> 423us
toF16:   302us -> 398us

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-07-28 19:19:41]

Description was changed from

==========
Add Premul to SkColorXform, handle Opaque/Unpremul in the same API

Performance (Unpremul/Opaque) on z620 (201295.jpg):

toSRGB:  356us -> 363us
to2Dot2: 414us -> 423us
toF16:   302us -> 398us

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Add Premul/Unpremul to SkColorXform, integrate with SkPngCodec

Performance (Unpremul/Opaque) on z620 (201295.jpg):

toSRGB:  356us -> 363us
to2Dot2: 414us -> 423us
toF16:   302us -> 398us

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[msarett, 2016-07-28 19:22:50]

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

[msarett, 2016-07-28 19:22:50]

Really happy with how this turned out.  I think it's a good refactor for
SkPngCodec.

[mtklein, 2016-07-28 19:35:12]

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41.cpp
File src/opts/SkOpts_sse41.cpp (right):

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41....
src/opts/SkOpts_sse41.cpp:24: color_xform_RGB1_to_2dot2               =
sse41::color_xform_RGB1_to_2dot2;
So, all 14 of these routines are really going to be hot?

[msarett, 2016-07-28 20:38:11]

Patchset #4 (id:240001) has been deleted

[msarett, 2016-07-28 20:39:05]

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41.cpp
File src/opts/SkOpts_sse41.cpp (right):

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41....
src/opts/SkOpts_sse41.cpp:24: color_xform_RGB1_to_2dot2               =
sse41::color_xform_RGB1_to_2dot2;
On 2016/07/28 19:35:12, mtklein wrote:
> So, all 14 of these routines are really going to be hot?

I think these 6 are critical.
RGBA->RGBA          (sRGB)
RGBA->BGRA          (sRGB)
RGBA->RGBA (Premul) (sRGB)
RGBA->BGRA (Premul) (sRGB)
RGBA->Half
RGBA->Half (Premul) 

The 4 to2Dot2 routines are not so critical.  Looks like only 5% of Dsts are
2Dot2 (and we may end up treating these as sRGB anyway).

The 4 toTable routine are also not so critical.  They exist because we need a
"catch all" until I can convince the UMA that all Dsts are sRGB.

I don't feel too strongly about how we should implement these last 8 routines. 
The way they are written now is convenient because they share code with the
critical routines, but I'm opened to other ideas.

Actually, I really, really want to delete the toTable routines.  But I think
that depends on me figuring out what's going on with the 11% "nonstandard" Dst
profiles.

[msarett, 2016-07-29 18:12:18]

Patchset #5 (id:280001) has been deleted

[msarett, 2016-07-29 18:23:01]

Rebase on Sk4u.

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41.cpp
File src/opts/SkOpts_sse41.cpp (right):

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41....
src/opts/SkOpts_sse41.cpp:24: color_xform_RGB1_to_2dot2               =
sse41::color_xform_RGB1_to_2dot2;
On 2016/07/28 20:39:04, msarett wrote:
> On 2016/07/28 19:35:12, mtklein wrote:
> > So, all 14 of these routines are really going to be hot?
> 
> I think these 6 are critical.
> RGBA->RGBA          (sRGB)
> RGBA->BGRA          (sRGB)
> RGBA->RGBA (Premul) (sRGB)
> RGBA->BGRA (Premul) (sRGB)
> RGBA->Half
> RGBA->Half (Premul) 
> 
> The 4 to2Dot2 routines are not so critical.  Looks like only 5% of Dsts are
> 2Dot2 (and we may end up treating these as sRGB anyway).
> 
> The 4 toTable routine are also not so critical.  They exist because we need a
> "catch all" until I can convince the UMA that all Dsts are sRGB.
> 
> I don't feel too strongly about how we should implement these last 8 routines.

> The way they are written now is convenient because they share code with the
> critical routines, but I'm opened to other ideas.
> 
> Actually, I really, really want to delete the toTable routines.  But I think
> that depends on me figuring out what's going on with the 11% "nonstandard" Dst
> profiles.

I wrote a CL that removes the special case for 2Dot2.
https://codereview.chromium.org/2189193002/

Haven't published it yet because I'm not sure about it...  It removes a lot of
code but is more than 2x slower and is less correct.  It's definitely an option
though as it becomes necessary to keep this sane.

https://codereview.chromium.org/2184543003/diff/300001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.h:43: virtual void apply(void* dst, const RGBA32*
src, int len, SkColorType dstColorType,
I'm noticing that this is really the only public function we need now.  The
others can disappear and we can call directly into SkOpts.  Thoughts?

[mtklein, 2016-07-29 18:48:57]

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41.cpp
File src/opts/SkOpts_sse41.cpp (right):

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41....
src/opts/SkOpts_sse41.cpp:24: color_xform_RGB1_to_2dot2               =
sse41::color_xform_RGB1_to_2dot2;
On 2016/07/29 18:23:01, msarett wrote:
> On 2016/07/28 20:39:04, msarett wrote:
> > On 2016/07/28 19:35:12, mtklein wrote:
> > > So, all 14 of these routines are really going to be hot?
> > 
> > I think these 6 are critical.
> > RGBA->RGBA          (sRGB)
> > RGBA->BGRA          (sRGB)
> > RGBA->RGBA (Premul) (sRGB)
> > RGBA->BGRA (Premul) (sRGB)
> > RGBA->Half
> > RGBA->Half (Premul) 
> > 
> > The 4 to2Dot2 routines are not so critical.  Looks like only 5% of Dsts are
> > 2Dot2 (and we may end up treating these as sRGB anyway).
> > 
> > The 4 toTable routine are also not so critical.  They exist because we need
a
> > "catch all" until I can convince the UMA that all Dsts are sRGB.
> > 
> > I don't feel too strongly about how we should implement these last 8
routines.
> 
> > The way they are written now is convenient because they share code with the
> > critical routines, but I'm opened to other ideas.
> > 
> > Actually, I really, really want to delete the toTable routines.  But I think
> > that depends on me figuring out what's going on with the 11% "nonstandard"
Dst
> > profiles.
> 
> I wrote a CL that removes the special case for 2Dot2.
> https://codereview.chromium.org/2189193002/
> 
> Haven't published it yet because I'm not sure about it...  It removes a lot of
> code but is more than 2x slower and is less correct.  It's definitely an
option
> though as it becomes necessary to keep this sane.

Seems like we want to keep all 14 for now.

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkCodecPriv....
src/codec/SkCodecPriv.h:313: inline bool needs_color_xform(const SkImageInfo&
dstInfo, const SkImageInfo& srcInfo) {
All these header methods should probably be non-exported, either static or in an
anonymous namespace.  (I like static, as you have to search less to find that
the function is static.)

It's a good rule of thumb that every function in a .h should start "static
inline"

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkPngCodec.h
File src/codec/SkPngCodec.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkPngCodec.h...
src/codec/SkPngCodec.h:52: protected:
You're already protected here.

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkPngCodec.h...
src/codec/SkPngCodec.h:67: int                                fBitDepth;
I usually find that when data is protected as opposed to public or private,
there's usually a better (flatter) way to design the hierarchy.  

Could you, e.g., make a standalone PngCommon class in the .cpp and a bunch of
SkPngCodecFoo that implement SkCodec directly, each having an instance of
PngCommon?

https://codereview.chromium.org/2184543003/diff/300001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.h:43: virtual void apply(void* dst, const RGBA32*
src, int len, SkColorType dstColorType,
On 2016/07/29 18:23:01, msarett wrote:
> I'm noticing that this is really the only public function we need now.  The
> others can disappear and we can call directly into SkOpts.  Thoughts?

This sounds like a good move to me.

I think that moots the typedefs too.

https://codereview.chromium.org/2184543003/diff/300001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.h:50: class SkColorSpaceXform_Base : public
SkColorSpaceXform {
Isn't there only one SkColorSpaceXform now?  Why don't we collapse everything
into SkColorSpaceXform?  (Maybe first before this CL.)

https://codereview.chromium.org/2184543003/diff/300001/src/opts/SkColorXform_...
File src/opts/SkColorXform_opts.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:44: template <DstGamma kDstGamma, bool kPremul,
bool kSwapRB>
to think about, bool kPremul -> {Premul, Unpremul, Opaque}

https://codereview.chromium.org/2184543003/diff/300001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:80: iAlphas = Sk4i::Load(&uAlphas);
// We'll need iAlphas again later.

https://codereview.chromium.org/2184543003/diff/300001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:87: Sk4i dstIAlphas;
Do we really need dstIAlphas and iAlphas?  Can we get away with just one?

[msarett, 2016-07-29 20:06:31]

Patchset #6 (id:320001) has been deleted

[msarett, 2016-07-29 20:07:51]

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41.cpp
File src/opts/SkOpts_sse41.cpp (right):

https://codereview.chromium.org/2184543003/diff/180001/src/opts/SkOpts_sse41....
src/opts/SkOpts_sse41.cpp:24: color_xform_RGB1_to_2dot2               =
sse41::color_xform_RGB1_to_2dot2;
On 2016/07/29 18:48:56, mtklein wrote:
> On 2016/07/29 18:23:01, msarett wrote:
> > On 2016/07/28 20:39:04, msarett wrote:
> > > On 2016/07/28 19:35:12, mtklein wrote:
> > > > So, all 14 of these routines are really going to be hot?
> > > 
> > > I think these 6 are critical.
> > > RGBA->RGBA          (sRGB)
> > > RGBA->BGRA          (sRGB)
> > > RGBA->RGBA (Premul) (sRGB)
> > > RGBA->BGRA (Premul) (sRGB)
> > > RGBA->Half
> > > RGBA->Half (Premul) 
> > > 
> > > The 4 to2Dot2 routines are not so critical.  Looks like only 5% of Dsts
are
> > > 2Dot2 (and we may end up treating these as sRGB anyway).
> > > 
> > > The 4 toTable routine are also not so critical.  They exist because we
need
> a
> > > "catch all" until I can convince the UMA that all Dsts are sRGB.
> > > 
> > > I don't feel too strongly about how we should implement these last 8
> routines.
> > 
> > > The way they are written now is convenient because they share code with
the
> > > critical routines, but I'm opened to other ideas.
> > > 
> > > Actually, I really, really want to delete the toTable routines.  But I
think
> > > that depends on me figuring out what's going on with the 11% "nonstandard"
> Dst
> > > profiles.
> > 
> > I wrote a CL that removes the special case for 2Dot2.
> > https://codereview.chromium.org/2189193002/
> > 
> > Haven't published it yet because I'm not sure about it...  It removes a lot
of
> > code but is more than 2x slower and is less correct.  It's definitely an
> option
> > though as it becomes necessary to keep this sane.
> 
> Seems like we want to keep all 14 for now.

Acknowledged.

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkCodecPriv....
src/codec/SkCodecPriv.h:313: inline bool needs_color_xform(const SkImageInfo&
dstInfo, const SkImageInfo& srcInfo) {
On 2016/07/29 18:48:56, mtklein wrote:
> All these header methods should probably be non-exported, either static or in
an
> anonymous namespace.  (I like static, as you have to search less to find that
> the function is static.)
> 
> It's a good rule of thumb that every function in a .h should start "static
> inline"

Done.

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkPngCodec.h
File src/codec/SkPngCodec.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkPngCodec.h...
src/codec/SkPngCodec.h:52: protected:
On 2016/07/29 18:48:56, mtklein wrote:
> You're already protected here.

Of course, got it.

https://codereview.chromium.org/2184543003/diff/300001/src/codec/SkPngCodec.h...
src/codec/SkPngCodec.h:67: int                                fBitDepth;
On 2016/07/29 18:48:56, mtklein wrote:
> I usually find that when data is protected as opposed to public or private,
> there's usually a better (flatter) way to design the hierarchy.  
> 
> Could you, e.g., make a standalone PngCommon class in the .cpp and a bunch of
> SkPngCodecFoo that implement SkCodec directly, each having an instance of
> PngCommon?

I think you make a good point...

I did my best to refactor with Leon's CL in mind
(https://codereview.chromium.org/1997703003/) to minimize the headache for him
when he gets back.  I think I've managed a similar design.  I'll probably try to
avoid shaking things up any more than I already have.

https://codereview.chromium.org/2184543003/diff/300001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.h:43: virtual void apply(void* dst, const RGBA32*
src, int len, SkColorType dstColorType,
On 2016/07/29 18:48:56, mtklein wrote:
> On 2016/07/29 18:23:01, msarett wrote:
> > I'm noticing that this is really the only public function we need now.  The
> > others can disappear and we can call directly into SkOpts.  Thoughts?
> 
> This sounds like a good move to me.
> 
> I think that moots the typedefs too.

Done.

https://codereview.chromium.org/2184543003/diff/300001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.h:50: class SkColorSpaceXform_Base : public
SkColorSpaceXform {
On 2016/07/29 18:48:57, mtklein wrote:
> Isn't there only one SkColorSpaceXform now?  Why don't we collapse everything
> into SkColorSpaceXform?  (Maybe first before this CL.)

I think maybe we can get there eventually.  We just chatted in person: think the
current design makes sense while SkColorSpaceXform_Base is templated.

https://codereview.chromium.org/2184543003/diff/300001/src/opts/SkColorXform_...
File src/opts/SkColorXform_opts.h (right):

https://codereview.chromium.org/2184543003/diff/300001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:44: template <DstGamma kDstGamma, bool kPremul,
bool kSwapRB>
On 2016/07/29 18:48:57, mtklein wrote:
> to think about, bool kPremul -> {Premul, Unpremul, Opaque}

Agreed, that would be better, particularly for F16.

https://codereview.chromium.org/2184543003/diff/300001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:80: iAlphas = Sk4i::Load(&uAlphas);
On 2016/07/29 18:48:57, mtklein wrote:
> // We'll need iAlphas again later.

Done.

https://codereview.chromium.org/2184543003/diff/300001/src/opts/SkColorXform_...
src/opts/SkColorXform_opts.h:87: Sk4i dstIAlphas;
On 2016/07/29 18:48:57, mtklein wrote:
> Do we really need dstIAlphas and iAlphas?  Can we get away with just one?

Yeah we need it because we interleave loads and stores.  We will load the next
iAlphas before storing the first iAlphas.  This actually caused me a bug in my
first implementation.

[msarett, 2016-07-29 20:21:50]

Patchset #8 (id:380001) has been deleted

[msarett, 2016-08-02 18:17:35]

Description was changed from

==========
Add Premul/Unpremul to SkColorXform, integrate with SkPngCodec

Performance (Unpremul/Opaque) on z620 (201295.jpg):

toSRGB:  356us -> 363us
to2Dot2: 414us -> 423us
toF16:   302us -> 398us

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Perform color correction on png decodes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

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

I've rebased this on the changes to SkColorXformOpts.

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXformOpts.h (right):

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformOpts.h:133: Sk4i da =
SkNx_cast<int32_t>(Sk4u::Load(src) & 0xFF000000);
No new changes to this file.  Uggh I think I lose these in the rebase...

[mtklein, 2016-08-03 14:25:50]

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:656: return color_xform_RGBA<kDst, true, false>
May want to make these true / false into little two-value enums, so it's clearer
here what each meanss and to prevent ordering mixups.  I can't really remember
what these two bits are or which order they come in (must be premul first right,
then swapRB?) just by reading this.

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.h:34: bool premul) const  = 0;
Seems like what I was getting at before with a tri-state is actually just to
pass an `SkAlphaType dstAlphaType` here.  That brings back the opaque path as
kOpaque_SkAlphaType.

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXformOpts.h (right):

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformOpts.h:133: Sk4i da =
SkNx_cast<int32_t>(Sk4u::Load(src) & 0xFF000000);
On 2016/08/02 18:18:08, msarett wrote:
> No new changes to this file.  Uggh I think I lose these in the rebase...

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformOpts.h:133: Sk4i da =
SkNx_cast<int32_t>(Sk4u::Load(src) & 0xFF000000);
This can just be Sk4i da = Sk4i::Load(src) & 0xFF000000 I think.  Should be no
need to mention Sk4u.

[msarett, 2016-08-03 16:48:50]

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:656: return color_xform_RGBA<kDst, true, false>
On 2016/08/03 14:25:50, mtklein wrote:
> May want to make these true / false into little two-value enums, so it's
clearer
> here what each meanss and to prevent ordering mixups.  I can't really remember
> what these two bits are or which order they come in (must be premul first
right,
> then swapRB?) just by reading this.

Done.

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.h:34: bool premul) const  = 0;
On 2016/08/03 14:25:50, mtklein wrote:
> Seems like what I was getting at before with a tri-state is actually just to
> pass an `SkAlphaType dstAlphaType` here.  That brings back the opaque path as
> kOpaque_SkAlphaType.

Changing to a tri-state strategy.

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXformOpts.h (right):

https://codereview.chromium.org/2184543003/diff/460001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformOpts.h:133: Sk4i da =
SkNx_cast<int32_t>(Sk4u::Load(src) & 0xFF000000);
On 2016/08/03 14:25:50, mtklein wrote:
> This can just be Sk4i da = Sk4i::Load(src) & 0xFF000000 I think.  Should be no
> need to mention Sk4u.

Of course, thanks.

https://codereview.chromium.org/2184543003/diff/480001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXformOpts.h (right):

https://codereview.chromium.org/2184543003/diff/480001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformOpts.h:231: static inline void store_f16_opaque(void*
dst, const uint32_t* src,
Not sure if it's worth it to optimize the opaque case on the other store
function...will investigate.

But it's definitely worth it here.

[mtklein, 2016-08-03 17:54:47]

lgtm

[msarett, 2016-08-03 17:56:22]

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

[commit-bot: I haz the power, 2016-08-03 17:56: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-03 18:02:31]

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

[commit-bot: I haz the power, 2016-08-03 18:02:32]

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

[msarett, 2016-08-03 18:42:21]

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

[commit-bot: I haz the power, 2016-08-03 18:42:24]

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-03 19:07:15]

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

[commit-bot: I haz the power, 2016-08-03 19:07:16]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-08-03 19:58:09]

The CQ bit was checked by msarett@google.com

[msarett, 2016-08-03 19:58:09]

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/2184543003/#ps500001
(title: "Fixes")

[commit-bot: I haz the power, 2016-08-03 19:58:15]

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-03 19:59:30]

Description was changed from

==========
Perform color correction on png decodes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Perform color correction on png decodes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2184543003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

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

[commit-bot: I haz the power, 2016-08-03 19:59:31]

Committed patchset #13 (id:500001) as
https://skia.googlesource.com/skia/+/d1ec89b1aca8f37a460a425259a23275f4d9a81d