Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

"Edge" cases include:
(1) Matrices with translation
(2) colorLUTs

Performance on HP z620:

201295.jpg
to2Dot2: 386us -> 414us
toSRGB:  346us -> 371us
toHalf:  282us -> 302us

strange0-translate.jpg
toSRGB:  1060us -> 244us

strange1-colorLUT.jpg
toSRGB:  2.74ms -> 2.00ms

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2177173003
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/+/3418c0e797e2ee841d1c031ca9d7a5ba73205f51

[msarett, 2016-07-25 17:46:13]

Description was changed from

==========
Try to figure it out

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

to

==========
Try to figure it out

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

[msarett, 2016-07-25 18:29:59]

Patchset #1 (id:1) has been deleted

[msarett, 2016-07-25 18:30:05]

Patchset #1 (id:20001) has been deleted

[msarett, 2016-07-25 18:43:30]

Description was changed from

==========
Try to figure it out

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

to

==========
Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

"Edge" cases include:
(1) Matrices with translation
(2) colorLUTs

Performance on HP z620:

201295.jpg
to2Dot2: 386us -> 414us
toSRGB:  346us -> 371us
toHalf:  282us -> 300us

strange0-translate.jpg
toSRGB:  

strange1-colorLUT.jpg
toSRGB:

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

[msarett, 2016-07-25 18:47:09]

Description was changed from

==========
Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

"Edge" cases include:
(1) Matrices with translation
(2) colorLUTs

Performance on HP z620:

201295.jpg
to2Dot2: 386us -> 414us
toSRGB:  346us -> 371us
toHalf:  282us -> 300us

strange0-translate.jpg
toSRGB:  

strange1-colorLUT.jpg
toSRGB:

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

to

==========
Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

"Edge" cases include:
(1) Matrices with translation
(2) colorLUTs

Performance on HP z620:

201295.jpg
to2Dot2: 386us -> 414us
toSRGB:  346us -> 371us
toHalf:  282us -> 300us

strange0-translate.jpg
toSRGB:  1060us -> 244us

strange1-colorLUT.jpg
toSRGB:  2.74ms -> 2.00ms

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

[msarett, 2016-07-25 18:51:30]

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

[msarett, 2016-07-25 18:51:31]

I think this is a really positive change in terms of code
design/complexity.

I'm grumpy about the performance regressions, but I just
can't justify adding another template to handle
"translations".  At least I have seen that
"sk_linear_to_srgb_needs_trunc" will recover things a
bit in the srgb case.

https://codereview.chromium.org/2177173003/diff/40001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2177173003/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:624: : fColorLUT(sk_ref_sp((SkColorLookUpTable*)
as_CSB(srcSpace)->colorLUT()))
This line is new, otherwise this function is just moved.

https://codereview.chromium.org/2177173003/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:636: if (fColorLUT) {
The "if (fColorLUT) {}" blocks are new, but otherwise these functions are just
moved.

[msarett, 2016-07-25 19:12:34]

Description was changed from

==========
Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

"Edge" cases include:
(1) Matrices with translation
(2) colorLUTs

Performance on HP z620:

201295.jpg
to2Dot2: 386us -> 414us
toSRGB:  346us -> 371us
toHalf:  282us -> 300us

strange0-translate.jpg
toSRGB:  1060us -> 244us

strange1-colorLUT.jpg
toSRGB:  2.74ms -> 2.00ms

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

to

==========
Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

"Edge" cases include:
(1) Matrices with translation
(2) colorLUTs

Performance on HP z620:

201295.jpg
to2Dot2: 386us -> 414us
toSRGB:  346us -> 371us
toHalf:  282us -> 302us

strange0-translate.jpg
toSRGB:  1060us -> 244us

strange1-colorLUT.jpg
toSRGB:  2.74ms -> 2.00ms

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

[mtklein, 2016-07-25 21:35:54]

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:268:
SkColorSpaceXform_Base<SkColorSpace::kNonStandard_GammaNamed>::kDstGammaTableSize;
I think you might just be able to

using
SkColorSpaceXform_Base<SkColorSpace::kNonStandard_GammaNamed>::kDstGammaTableSize;

?

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:672: fStorage.reset(storageBytes);
Oh wow hey super not threadsafe.

You need some sort of synchronization here (SkOnce, SkMutex), or to make this
method not const.

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.h:66: mutable SkAutoMalloc      fStorage;
-_-

There shouldn't ever be mutable fields without some synchronization mechanism
guarding them...

[msarett, 2016-07-25 21:58:12]

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:268:
SkColorSpaceXform_Base<SkColorSpace::kNonStandard_GammaNamed>::kDstGammaTableSize;
On 2016/07/25 21:35:53, mtklein wrote:
> I think you might just be able to
> 
> using
>
SkColorSpaceXform_Base<SkColorSpace::kNonStandard_GammaNamed>::kDstGammaTableSize;
> 
> ?

This doesn't seem to work.

Looks like "using" wants class names, not class members.

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:672: fStorage.reset(storageBytes);
On 2016/07/25 21:35:53, mtklein wrote:
> Oh wow hey super not threadsafe.
> 
> You need some sort of synchronization here (SkOnce, SkMutex), or to make this
> method not const.

Yes thanks.

I want the function to be const because that's logically how the client should
see it.  Using the stack instead.

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2177173003/diff/60001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.h:66: mutable SkAutoMalloc      fStorage;
On 2016/07/25 21:35:54, mtklein wrote:
> -_-
> 
> There shouldn't ever be mutable fields without some synchronization mechanism
> guarding them...

Of course, will use the stack instead.

[mtklein, 2016-07-25 22:01:43]

lgtm

https://codereview.chromium.org/2177173003/diff/80001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2177173003/diff/80001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:672: SkAutoSMalloc<1024 * sizeof(RGBA32)>
storage(storageBytes);
Just FYI, you may find you need to reduce this to roll into google3.  The stack
sizes there are smaller than you'd think, and this may push close.  If you git
grep for GOOGLE3, you'll see a bunch of cases guarded for this sort of problem.

[msarett, 2016-07-25 22:17:27]

On 2016/07/25 22:01:43, mtklein wrote:
> lgtm
> 
>
https://codereview.chromium.org/2177173003/diff/80001/src/core/SkColorSpaceXf...
> File src/core/SkColorSpaceXform.cpp (right):
> 
>
https://codereview.chromium.org/2177173003/diff/80001/src/core/SkColorSpaceXf...
> src/core/SkColorSpaceXform.cpp:672: SkAutoSMalloc<1024 * sizeof(RGBA32)>
> storage(storageBytes);
> Just FYI, you may find you need to reduce this to roll into google3.  The
stack
> sizes there are smaller than you'd think, and this may push close.  If you git
> grep for GOOGLE3, you'll see a bunch of cases guarded for this sort of
problem.

Gotcha thanks, using less space on GOOGLE3.

[msarett, 2016-07-25 22:17:33]

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

[commit-bot: I haz the power, 2016-07-25 22:17:43]

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-07-25 22:41:12]

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

[commit-bot: I haz the power, 2016-07-25 22:41:13]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-07-26 01:22:19]

The CQ bit was checked by msarett@google.com

[msarett, 2016-07-26 01:22:20]

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/2177173003/#ps100001
(title: "Use less space on Google3")

[commit-bot: I haz the power, 2016-07-26 01:22:23]

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-07-26 01:23:21]

Description was changed from

==========
Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

"Edge" cases include:
(1) Matrices with translation
(2) colorLUTs

Performance on HP z620:

201295.jpg
to2Dot2: 386us -> 414us
toSRGB:  346us -> 371us
toHalf:  282us -> 302us

strange0-translate.jpg
toSRGB:  1060us -> 244us

strange1-colorLUT.jpg
toSRGB:  2.74ms -> 2.00ms

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

to

==========
Delete SkDefaultXform, handle edge cases in SkColorSpaceXform_Base

"Edge" cases include:
(1) Matrices with translation
(2) colorLUTs

Performance on HP z620:

201295.jpg
to2Dot2: 386us -> 414us
toSRGB:  346us -> 371us
toHalf:  282us -> 302us

strange0-translate.jpg
toSRGB:  1060us -> 244us

strange1-colorLUT.jpg
toSRGB:  2.74ms -> 2.00ms

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2177173003
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/+/3418c0e797e2ee841d1c031ca9d7a5ba73205f51
==========

[commit-bot: I haz the power, 2016-07-26 01:23:23]

Committed patchset #4 (id:100001) as
https://skia.googlesource.com/skia/+/3418c0e797e2ee841d1c031ca9d7a5ba73205f51