Refactored SkColorSpace and added in a Lab PCS GM

Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

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

Committed: https://skia.googlesource.com/skia/+/9488833428e83c93a7e6002f4d056084fb57112f

[raftias, 2016-10-03 20:28:43]

Description was changed from

==========
Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

Common functionality for color space transformations was moved
from SkColorSpaceXform.cpp (which does not handle A2B0 info)
into SkColorSpaceXformPriv.h/cpp in preparation for a A2B0-aware
ColorSpaceXform that will use the shared functions, and so the
LabPCSDemo GM could utilize them.

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

to

==========
Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

Common functionality for color space transformations was moved
from SkColorSpaceXform.cpp (which does not handle A2B0 info)
into SkColorSpaceXformPriv.h/cpp in preparation for a A2B0-aware
ColorSpaceXform that will use the shared functions, and so the
LabPCSDemo GM could utilize them.

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

[raftias, 2016-10-03 20:38:05]

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

[commit-bot: I haz the power, 2016-10-03 20:38:13]

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-10-03 20:43:46]

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

[commit-bot: I haz the power, 2016-10-03 20:43:48]

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

[raftias, 2016-10-03 20:44:20]

Description was changed from

==========
Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

Common functionality for color space transformations was moved
from SkColorSpaceXform.cpp (which does not handle A2B0 info)
into SkColorSpaceXformPriv.h/cpp in preparation for a A2B0-aware
ColorSpaceXform that will use the shared functions, and so the
LabPCSDemo GM could utilize them.

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

to

==========
Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

Common functionality for color space transformations was moved
from SkColorSpaceXform.cpp (which does not handle A2B0 info)
into SkColorSpaceXformPriv.h/cpp in preparation for a A2B0-aware
ColorSpaceXform that will use the shared functions, and so the
LabPCSDemo GM could utilize them.

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

[raftias, 2016-10-03 20:44:20]

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

[raftias, 2016-10-03 21:20:00]

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

[commit-bot: I haz the power, 2016-10-03 21:20:06]

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-10-03 21:25:55]

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

[commit-bot: I haz the power, 2016-10-03 21:25:57]

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...)

[raftias, 2016-10-03 22:59:56]

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

[commit-bot: I haz the power, 2016-10-03 23:00:06]

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-10-03 23:06:54]

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

[commit-bot: I haz the power, 2016-10-03 23:06:55]

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...)

[raftias, 2016-10-04 15:15:33]

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

[commit-bot: I haz the power, 2016-10-04 15:15:36]

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-10-04 15:32:11]

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

[commit-bot: I haz the power, 2016-10-04 15:32:12]

Dry run: Try jobs failed on following builders:
  Test-Android-Clang-Nexus5-GPU-Adreno330-arm-Release-GN_Android-Trybot on
master.client.skia.android (JOB_FAILED,
http://build.chromium.org/p/client.skia.android/builders/Test-Android-Clang-N...)

[raftias, 2016-10-04 19:23:14]

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

[commit-bot: I haz the power, 2016-10-04 19:23:23]

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-10-04 19:41:00]

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

[commit-bot: I haz the power, 2016-10-04 19:41:01]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-10-05 19:05:51]

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

[msarett, 2016-10-05 19:05:53]

Looks like this is headed in the right direction.

Adding Brian and Mike for general comments on the design.

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

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:51: }
After we do this check, let's just cast to ColorSpace_XYZ once and be done
pretending that they might be anything else.

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

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformPriv.cpp:11: void interp_3d_clut(float dst[3], float
src[3], const SkColorLookUpTable* colorLUT) {
Please delete this file and then put the implementations themselves in the .h
file.  Sorry I was unclear, that's what I meant by sharing these but keeping
them inline.

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXformPriv.h (right):

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformPriv.h:18: void build_table_linear_from_gamma(float*
outTable, float exponent);
It seems like there were some functions move here that don't need to do be
here...

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_A...
File src/core/SkColorSpace_A2B0.h (right):

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_A...
src/core/SkColorSpace_A2B0.h:54: sk_sp<SkData>             fProfileData;
nit: This field is already on the base.

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_B...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_B...
src/core/SkColorSpace_Base.h:185: enum class Type : uint8_t {
Can we make this protected?

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_B...
src/core/SkColorSpace_Base.h:190: Type type() const { return fType; }
Can we make this a protected virtual?  Then only friend classes (ex:
SkColorSpaceXform) can call it.

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_X...
File src/core/SkColorSpace_XYZTRC.h (right):

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_X...
src/core/SkColorSpace_XYZTRC.h:33: sk_sp<SkData>             fProfileData;
nit: This field is already on the base.

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_X...
src/core/SkColorSpace_XYZTRC.h:45: inline SkColorSpace_XYZTRC*
as_CSXYZ(SkColorSpace* colorSpace) {
These casts are convenient in SkColorSpace_Base because we know that every
SkColorSpace is actually a SkColorSpace_Base.

In this case, where we might not know, they seem dangerous to me.

https://codereview.chromium.org/2389983002/diff/80001/src/gpu/GrColorSpaceXfo...
File src/gpu/GrColorSpaceXform.cpp (right):

https://codereview.chromium.org/2389983002/diff/80001/src/gpu/GrColorSpaceXfo...
src/gpu/GrColorSpaceXform.cpp:54:
srcToDst.setConcat(as_CSXYZ(dst)->fromXYZD50(), as_CSXYZ(src)->toXYZD50());
Can we expose these functions on SkColorSpace_Base?

virtual SkMatrix44* toXYZD50() const;
or
virtual bool toXYZD50(SkMatrix44* toXYZD50);

And then we assert that they return success here?

[raftias, 2016-10-06 14:39:46]

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

[commit-bot: I haz the power, 2016-10-06 14:39:56]

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

[raftias, 2016-10-06 14:43:04]

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

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:51: }
On 2016/10/05 19:05:52, msarett wrote:
> After we do this check, let's just cast to ColorSpace_XYZ once and be done
> pretending that they might be anything else.

Done.

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

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformPriv.cpp:11: void interp_3d_clut(float dst[3], float
src[3], const SkColorLookUpTable* colorLUT) {
On 2016/10/05 19:05:52, msarett wrote:
> Please delete this file and then put the implementations themselves in the .h
> file.  Sorry I was unclear, that's what I meant by sharing these but keeping
> them inline.

Done.

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXformPriv.h (right):

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXformPriv.h:18: void build_table_linear_from_gamma(float*
outTable, float exponent);
On 2016/10/05 19:05:52, msarett wrote:
> It seems like there were some functions move here that don't need to do be
> here...

Done.

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_A...
File src/core/SkColorSpace_A2B0.h (right):

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_A...
src/core/SkColorSpace_A2B0.h:54: sk_sp<SkData>             fProfileData;
On 2016/10/05 19:05:52, msarett wrote:
> nit: This field is already on the base.

This was a mistake. I meant for it to be stored in Base.

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_B...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_B...
src/core/SkColorSpace_Base.h:185: enum class Type : uint8_t {
On 2016/10/05 19:05:53, msarett wrote:
> Can we make this protected?

Done.

https://codereview.chromium.org/2389983002/diff/80001/src/core/SkColorSpace_B...
src/core/SkColorSpace_Base.h:190: Type type() const { return fType; }
On 2016/10/05 19:05:52, msarett wrote:
> Can we make this a protected virtual?  Then only friend classes (ex:
> SkColorSpaceXform) can call it.

Done.

[commit-bot: I haz the power, 2016-10-06 14:57:25]

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

[commit-bot: I haz the power, 2016-10-06 14:57:27]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-10-06 17:04:46]

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

[msarett, 2016-10-06 17:04:47]

+mtklein

I intend to take a closer look at this, though it might be useful to finish up
our discussion of the higher level design.

[msarett, 2016-10-07 01:48:36]

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:80: return sk_sp<SkColorSpace>(new
SkColorSpace_XYZTRC(kNonStandard_SkGammaNamed,
This line makes me feel good about this change: just seems more logical now.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:174: sk_sp<SkColorSpace>
SkColorSpace::makeLinearGamma() {
Let's make this virutal on SkColorSpace_Base.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:178: // A2B0 Color Spaces do not have a single Gamma,
so this method should not be
nit: Convert tabs to spaces

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:180: SkASSERT(as_CSB(this)->type() ==
SkColorSpace_Base::Type::kXYZTRC);
This is really tricky...  I don't know what we should do here uggh.

Any thoughts reed@, brianosman@?

I'm leaning towards returning nullptr...
Or we could just throw them linear sRGB? (that's likely what they'll want when
they realize this doesn't work)

We could also try to retract this from the public API, but I think it's useful
and I know that Chrome is using it.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:188: switch (as_CSB(this)->type())
Can we make all of these virtual on SkColorSpace_Base?

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:195: case SkColorSpace_Base::Type::kA2B0:
Returning false in this case is just fine.  No need to assert or anything.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXform.cpp (left):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:1356: #if defined(GOOGLE3)
Make me feel good to see this deleted...

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

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:355: if (as_CSB(dstSpace)->type() ==
SkColorSpace_Base::Type::kA2B0) {
nit: Skia style

Constants come first in == comparisons. Ex:
if (SkColorSpace_Base::Type::kA2B == as_CSB(dstSpace)->type()) {

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:360: // Check whether we need to to an A2B0
transformation
Don't think there's a need for this comment, the code is pretty clear.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:361: if (as_CSB(srcSpace)->type() ==
SkColorSpace_Base::Type::kA2B0) {
Same nit as above.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:363: // as SkColorSpaceXform_Base only supports
XYZ+TRC based SkColorSpaces
nit: I think this comment could be worded a little more clearly.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXformPriv.h (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformPriv.h:14: inline void interp_3d_clut(float dst[3],
float src[3], const SkColorLookUpTable* colorLUT) {
Make this static inline.

Edit: Actually, sorry to be annoying, but if your gm is the only code currently
calling this, you can just move this there and delete this file.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
File src/core/SkColorSpace_A2B0.h (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:15: const SkMatrix44* requestToXYZD50() const
override { return nullptr; }
Brief comment on why these return null?

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:19: enum class PCS : uint8_t {
Same enum class question as in SkColorSpace_Base

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:24: PCS pcs() const { return fPCS; }
Maybe lead with a summary comment about all the cool stuff we can find in A2B
tags?

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:26: SkGammaNamed aCurveNamed() const { return
fACurveNamed; }
Maybe bother trying to "name" the A, B, or M curves?  At least at first?  Will
this interfere with sharing the parsing code?

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:40: const SkMatrix44 toPCS() const { return fToPCS;
}
Maybe just call this "matrix"?  I don't have an intuitive understanding of what
this is actually meant to do.  But it seems like the CLUT typically does most of
the "to PCS" work.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:53: sk_sp<SkColorLookUpTable> fColorLUT;
My "convention" is to list these in the order that they would be used.  I
*think* that's:
A
CLUT
M
Matrix
B
PCS

Same for the parameter order above and getters above.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:187: * Get the toXYZD50 storage -> PCS matrix
How about?

Describes color space gamut as a transformation to XYZ D50.  Returns nullptr if
color gamut cannot be described in terms of XYZ D50.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:189: * @return A pointer to the matrix, or nullptr
if an error occured
nit: Two spaces after stars (just to be consistent with rest of the file).

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:191: virtual const SkMatrix44* requestToXYZD50()
const = 0;
I think just "toXYZD50" is fine.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:194: * Get the fromXYZD50 PCS -> screen matrix
Same comments on toXYZD50 apply here.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:203: enum class Type : uint8_t {
Skia style would typically be:

enum Type : uint8_t {
    kXYZ_Type,
    kA2B_Type,
}

But I've just read about "enum class" in C++11...  seems cool.
@mtklein does Skia use "enum class"?  In that case, should we drop _Type?

Edit:

I actually think we might not need this - just need more virtual functions.

Maybe we need to keep it around for SkColorSpaceXform?  Or maybe we can just
check for the presence of a matrix (temporary hack)?

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:204: kXYZTRC,
I don't love these names, though I'm not sure I can suggest anything better. 
Have any ideas?  Maybe just XYZ and A2B?

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:227: friend class SkColorSpaceXform;
Note to self: why does this need to be a friend class?

[raftias, 2016-10-10 20:11:56]

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

[commit-bot: I haz the power, 2016-10-10 20:12:04]

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-10-10 20:36:09]

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

[commit-bot: I haz the power, 2016-10-10 20:36:10]

Dry run: This issue passed the CQ dry run.

[raftias, 2016-10-10 20:37:33]

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:174: sk_sp<SkColorSpace>
SkColorSpace::makeLinearGamma() {
On 2016/10/07 01:48:35, msarett wrote:
> Let's make this virutal on SkColorSpace_Base.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:178: // A2B0 Color Spaces do not have a single Gamma,
so this method should not be
On 2016/10/07 01:48:35, msarett wrote:
> nit: Convert tabs to spaces

Done. I had forgot to change my difftool to spaces.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:180: SkASSERT(as_CSB(this)->type() ==
SkColorSpace_Base::Type::kXYZTRC);
On 2016/10/07 01:48:35, msarett wrote:
> This is really tricky...  I don't know what we should do here uggh.
> 
> Any thoughts reed@, brianosman@?
> 
> I'm leaning towards returning nullptr...
> Or we could just throw them linear sRGB? (that's likely what they'll want when
> they realize this doesn't work)
> 
> We could also try to retract this from the public API, but I think it's useful
> and I know that Chrome is using it.

If linear sRGB is what they want, they can get that other ways though, can't
they? If I was a user I wouldn't like it if it gave me linear sRGB when it's not
what I wanted, without any warning or anything.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:188: switch (as_CSB(this)->type())
On 2016/10/07 01:48:35, msarett wrote:
> Can we make all of these virtual on SkColorSpace_Base?

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:195: case SkColorSpace_Base::Type::kA2B0:
On 2016/10/07 01:48:35, msarett wrote:
> Returning false in this case is just fine.  No need to assert or anything.

Acknowledged.

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

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:355: if (as_CSB(dstSpace)->type() ==
SkColorSpace_Base::Type::kA2B0) {
On 2016/10/07 01:48:35, msarett wrote:
> nit: Skia style
> 
> Constants come first in == comparisons. Ex:
> if (SkColorSpace_Base::Type::kA2B == as_CSB(dstSpace)->type()) {

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:360: // Check whether we need to to an A2B0
transformation
On 2016/10/07 01:48:35, msarett wrote:
> Don't think there's a need for this comment, the code is pretty clear.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:361: if (as_CSB(srcSpace)->type() ==
SkColorSpace_Base::Type::kA2B0) {
On 2016/10/07 01:48:35, msarett wrote:
> Same nit as above.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:363: // as SkColorSpaceXform_Base only supports
XYZ+TRC based SkColorSpaces
On 2016/10/07 01:48:35, msarett wrote:
> nit: I think this comment could be worded a little more clearly.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXformPriv.h (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformPriv.h:14: inline void interp_3d_clut(float dst[3],
float src[3], const SkColorLookUpTable* colorLUT) {
On 2016/10/07 01:48:35, msarett wrote:
> Make this static inline.
> 
> Edit: Actually, sorry to be annoying, but if your gm is the only code
currently
> calling this, you can just move this there and delete this file.

Removed, but the file will show up (with more stuff) in the CL with the A2B0
xform.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
File src/core/SkColorSpace_A2B0.h (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:15: const SkMatrix44* requestToXYZD50() const
override { return nullptr; }
On 2016/10/07 01:48:36, msarett wrote:
> Brief comment on why these return null?

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:19: enum class PCS : uint8_t {
On 2016/10/07 01:48:36, msarett wrote:
> Same enum class question as in SkColorSpace_Base

Acknowledged.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:24: PCS pcs() const { return fPCS; }
On 2016/10/07 01:48:35, msarett wrote:
> Maybe lead with a summary comment about all the cool stuff we can find in A2B
> tags?

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:26: SkGammaNamed aCurveNamed() const { return
fACurveNamed; }
On 2016/10/07 01:48:35, msarett wrote:
> Maybe bother trying to "name" the A, B, or M curves?  At least at first?  Will
> this interfere with sharing the parsing code?

Like CLUTGamma, FinalGamma, MatrixGamma, or did you mean something else?

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:40: const SkMatrix44 toPCS() const { return fToPCS;
}
On 2016/10/07 01:48:35, msarett wrote:
> Maybe just call this "matrix"?  I don't have an intuitive understanding of
what
> this is actually meant to do.  But it seems like the CLUT typically does most
of
> the "to PCS" work.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:53: sk_sp<SkColorLookUpTable> fColorLUT;
On 2016/10/07 01:48:36, msarett wrote:
> My "convention" is to list these in the order that they would be used.  I
> *think* that's:
> A
> CLUT
> M
> Matrix
> B
> PCS
> 
> Same for the parameter order above and getters above.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:187: * Get the toXYZD50 storage -> PCS matrix
On 2016/10/07 01:48:36, msarett wrote:
> How about?
> 
> Describes color space gamut as a transformation to XYZ D50.  Returns nullptr
if
> color gamut cannot be described in terms of XYZ D50.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:189: * @return A pointer to the matrix, or nullptr
if an error occured
On 2016/10/07 01:48:36, msarett wrote:
> nit: Two spaces after stars (just to be consistent with rest of the file).

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:191: virtual const SkMatrix44* requestToXYZD50()
const = 0;
On 2016/10/07 01:48:36, msarett wrote:
> I think just "toXYZD50" is fine.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:194: * Get the fromXYZD50 PCS -> screen matrix
On 2016/10/07 01:48:36, msarett wrote:
> Same comments on toXYZD50 apply here.

Done.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:203: enum class Type : uint8_t {
On 2016/10/07 01:48:36, msarett wrote:
> Skia style would typically be:
> 
> enum Type : uint8_t {
>     kXYZ_Type,
>     kA2B_Type,
> }
> 
> But I've just read about "enum class" in C++11...  seems cool.
> @mtklein does Skia use "enum class"?  In that case, should we drop _Type?
> 
> Edit:
> 
> I actually think we might not need this - just need more virtual functions.
> 
> Maybe we need to keep it around for SkColorSpaceXform?  Or maybe we can just
> check for the presence of a matrix (temporary hack)?

enum classes seem to be used in other parts of skia (from a brief codesearch)
and follow the chromium guide where _Type is dropped since they are scoped with
Type::. I did if (cs->toXYZD50()) in a lot of the tests, etc, but I kept the
type() call in SkColorSpaceXform, since it seemed a little hacky to just have
that - although it does also increase the amount of friend classes if we keep
type() there as protected.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:204: kXYZTRC,
On 2016/10/07 01:48:36, msarett wrote:
> I don't love these names, though I'm not sure I can suggest anything better. 
> Have any ideas?  Maybe just XYZ and A2B?

Acknowledged.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:227: friend class SkColorSpaceXform;
On 2016/10/07 01:48:36, msarett wrote:
> Note to self: why does this need to be a friend class?

Acknowledged.

[msarett, 2016-10-11 13:40:31]

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
File src/core/SkColorSpace_A2B0.h (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:26: SkGammaNamed aCurveNamed() const { return
fACurveNamed; }
On 2016/10/10 20:37:32, raftias wrote:
> On 2016/10/07 01:48:35, msarett wrote:
> > Maybe bother trying to "name" the A, B, or M curves?  At least at first? 
Will
> > this interfere with sharing the parsing code?
> 
> Like CLUTGamma, FinalGamma, MatrixGamma, or did you mean something else?

Sorry I mean, let's not have GammaNamed getters (or GammaNamed fields at all) if
it doesn't complicate the implementation.

This might not necessarily be a useful optimization in this case.

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/100001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:203: enum class Type : uint8_t {
On 2016/10/10 20:37:33, raftias wrote:
> On 2016/10/07 01:48:36, msarett wrote:
> > Skia style would typically be:
> > 
> > enum Type : uint8_t {
> >     kXYZ_Type,
> >     kA2B_Type,
> > }
> > 
> > But I've just read about "enum class" in C++11...  seems cool.
> > @mtklein does Skia use "enum class"?  In that case, should we drop _Type?
> > 
> > Edit:
> > 
> > I actually think we might not need this - just need more virtual functions.
> > 
> > Maybe we need to keep it around for SkColorSpaceXform?  Or maybe we can just
> > check for the presence of a matrix (temporary hack)?
> 
> enum classes seem to be used in other parts of skia (from a brief codesearch)
> and follow the chromium guide where _Type is dropped since they are scoped
with
> Type::. I did if (cs->toXYZD50()) in a lot of the tests, etc, but I kept the
> type() call in SkColorSpaceXform, since it seemed a little hacky to just have
> that - although it does also increase the amount of friend classes if we keep
> type() there as protected.

Style sgtm.

I understand what you're saying.  Not feeling great about either option.  SGTM,
as is.

https://codereview.chromium.org/2389983002/diff/120001/gm/labpcsdemo.cpp
File gm/labpcsdemo.cpp (right):

https://codereview.chromium.org/2389983002/diff/120001/gm/labpcsdemo.cpp#newc...
gm/labpcsdemo.cpp:196: // Lab has ranges [0,100] for L and [-128,128] for a and
b
I'm sure what you're dong here is appropriate.

Wanted to mention that I just noticed some resources for Lab->XYZ conversions in
Appendix A3.

https://codereview.chromium.org/2389983002/diff/120001/include/core/SkColorSp...
File include/core/SkColorSpace.h (right):

https://codereview.chromium.org/2389983002/diff/120001/include/core/SkColorSp...
include/core/SkColorSpace.h:69: *  If this is not possible due to the color
space specification, nullptr is returned.
I'm still struggling with this...  Though I don't have the answer.

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:246: // If we have a named profile, only write the
enum.
Please move this comment down to just above the "if" statement.

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:428: // gammas, but also include other wacky features
that cause us to save the data.
I believe you don't need any of this?  Won't the fProfileData case catch this?

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:472: sk_sp<SkData> serializedSrcData =
src->serialize();
Thanks, I should not have shadowed my other variable names...

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

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:260: int gammaTableSize, const
SkColorSpace_XYZTRC* space,
nit: fits on the previous line

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:356: SkDebugf("A2B0 destinations not
supported\n");
Please SkColorSpacePrintf.  Some people feel strongly about not seeing print
statements that can't be turned off.

In addition, we may want to test this (and not see anything printed in quiet
modes).

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:370: }
nit: tabs

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXformPriv.h (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformPriv.h:2: * Copyright 2016 Google Inc.
Please remove

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
File src/core/SkColorSpace_A2B0.h (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:15: // color spaces (CIELAB instead of just
CIEXYZ), use color-lookup-tables to do
profile connection spaces?

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:72: // the intermediate processing color space that
this color space
profile connection space

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:198: virtual sk_sp<SkColorSpace>
makeLinearGammaImpl() = 0;
nit: Skia style onMakeLinearGamma()

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:200: virtual bool gammaCloseToSRGBImpl() const = 0;
nit: onGammaCloseToSRGB

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:202: virtual bool gammaIsLinearImpl() const = 0;
nit: onGammaIsLinear

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
File src/core/SkColorSpace_ICC.cpp (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_ICC.cpp:10: #include "SkColorSpace_A2B0.h"
nit: Alphabetical order

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_ICC.cpp:695: static bool load_matrix(SkMatrix44* toXYZ,
const uint8_t* src, size_t len, float scale = 1.f) {
I think you've got the scale logic correct (in that Lab doesn't need it).

But, let's require this parameter and make sure to pass in the correct value?

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_ICC.cpp:748: static bool
parse_and_load_gamma(SkGammaNamed* gammaNamed, sk_sp<SkGammas>* gammas,
Thanks for sharing this code :)

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_ICC.cpp:837: static bool
load_a2b0(sk_sp<SkColorLookUpTable>* colorLUT,
I'm pleasantly surprised by your changes to this function.  If anything I find
it easier to understand :).

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_ICC.cpp:895: if (!load_matrix(matrix, src +
offsetToMatrix, len - offsetToMatrix)) {
Pass in encodingFactor here?

Reading the spec, it looks to me like it should be 32768/65535 for XYZ and 1.0
for Lab.

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_ICC.cpp:1103: // if we're a LAB profile, but specified as
a 3-component matrix-based profile
Can we hold off on this until we're sure that we need it?

Maybe just fail for now (closer to the top of this fn).  Then we can investigate
the right thing to do for XYZ/A2B/Lab profiles in another CL...

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_ICC.cpp:1148: if (header.fPCS == kXYZ_PCSSpace) {
Can we delete this too?  Do we need it anywhere to pass tests?  There are two
images that will look wrong for a bit, but that's fine.  Will remind us to
finish implementing.

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_ICC.cpp:1154: SkColorSpacePrintf("Treating A2B0 profile as
one specified as XYZ matrices/TRCs\n");
nit: 100 chars

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
File src/core/SkColorSpace_XYZTRC.cpp (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_XYZTRC.cpp:53: SkDEBUGFAIL("Non-invertible XYZ matrix,
defaulting to sRGB");
Looking at this code, I kind of think we might want to adopt something similar
for makeLinearGamma()...  Returning nullptr when the client expects non-null
isn't very safe.

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
File src/core/SkColorSpace_XYZTRC.h (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace_...
src/core/SkColorSpace_XYZTRC.h:27: 
nit: extra white space

[raftias, 2016-10-14 17:10:15]

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:428: // gammas, but also include other wacky features
that cause us to save the data.
On 2016/10/11 13:40:31, msarett wrote:
> I believe you don't need any of this?  Won't the fProfileData case catch this?

Yeah, I guess there wouldn't be any A2B0 profiles without profile data, unlike
the XYZTRC ones.

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpaceX...
File src/core/SkColorSpaceXformPriv.h (right):

https://codereview.chromium.org/2389983002/diff/120001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXformPriv.h:2: * Copyright 2016 Google Inc.
On 2016/10/11 13:40:31, msarett wrote:
> Please remove

Oops, I forgot to git rm it.

[msarett, 2016-10-14 19:32:54]

Looks close.

https://codereview.chromium.org/2389983002/diff/140001/gm/labpcsdemo.cpp
File gm/labpcsdemo.cpp (right):

https://codereview.chromium.org/2389983002/diff/140001/gm/labpcsdemo.cpp#newc...
gm/labpcsdemo.cpp:181: for (int x = 0; x < imageWidth; ++x) {
Why does this seem to skip all of the transfer functions?  Are they linear or
not there ( if so this is fine)?

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:352: // Profile data is mandatory for A2B0 color
spaces.
Sorry I think I was unclear here previously.  I think the comment: // Profile
data... goes directly above your assert.

I think the comment: // If we have a named... goes directly above if (this ==
gSRGB).

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:353: if (!as_CSB(this)->fProfileData) {
nit: whitespace

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:564: if (as_CSB(src)->type() != as_CSB(dst)->type()) {
nit: This is unnecessary.  Let's not add it in.

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:581: // It's important to check fProfileData before
named gammas.  Some profiles may have named
Can you please delete my comment?

// It's important...

It no longer makes any sense.

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:596: }
I think all changes to this function starting here are unnecessary...  Please
remove.  Fine to keep the renamed variabales "serializedSrcData" etc. though.

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

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:14: //#include "SkColorSpaceXform_A2B0.h"
nit: Should be removed.

[raftias, 2016-10-14 20:36:11]

Description was changed from

==========
Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

Common functionality for color space transformations was moved
from SkColorSpaceXform.cpp (which does not handle A2B0 info)
into SkColorSpaceXformPriv.h/cpp in preparation for a A2B0-aware
ColorSpaceXform that will use the shared functions, and so the
LabPCSDemo GM could utilize them.

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

to

==========
Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

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

[raftias, 2016-10-14 20:41:03]

https://codereview.chromium.org/2389983002/diff/140001/gm/labpcsdemo.cpp
File gm/labpcsdemo.cpp (right):

https://codereview.chromium.org/2389983002/diff/140001/gm/labpcsdemo.cpp#newc...
gm/labpcsdemo.cpp:181: for (int x = 0; x < imageWidth; ++x) {
On 2016/10/14 19:32:53, msarett wrote:
> Why does this seem to skip all of the transfer functions?  Are they linear or
> not there ( if so this is fine)?

They're all linear in the ICC file this GM is using. The CLUT seems to handle
all the work of adjusting gamma in this profile.

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:352: // Profile data is mandatory for A2B0 color
spaces.
On 2016/10/14 19:32:53, msarett wrote:
> Sorry I think I was unclear here previously.  I think the comment: // Profile
> data... goes directly above your assert.
> 
> I think the comment: // If we have a named... goes directly above if (this ==
> gSRGB).

Done. That makes a lot more sense.

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:353: if (!as_CSB(this)->fProfileData) {
On 2016/10/14 19:32:53, msarett wrote:
> nit: whitespace

Removed.

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:564: if (as_CSB(src)->type() != as_CSB(dst)->type()) {
On 2016/10/14 19:32:53, msarett wrote:
> nit: This is unnecessary.  Let's not add it in.

I'll remove it. I guess the length check before the memcmp means that the memcmp
wouldn't get hit much in situations where this if would return false.

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:581: // It's important to check fProfileData before
named gammas.  Some profiles may have named
On 2016/10/14 19:32:53, msarett wrote:
> Can you please delete my comment?
> 
> // It's important...
> 
> It no longer makes any sense.

Done.

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:596: }
On 2016/10/14 19:32:53, msarett wrote:
> I think all changes to this function starting here are unnecessary...  Please
> remove.  Fine to keep the renamed variabales "serializedSrcData" etc. though.

Done.

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

https://codereview.chromium.org/2389983002/diff/140001/src/core/SkColorSpaceX...
src/core/SkColorSpaceXform.cpp:14: //#include "SkColorSpaceXform_A2B0.h"
On 2016/10/14 19:32:53, msarett wrote:
> nit: Should be removed.

Done.

[msarett, 2016-10-14 21:31:40]

I *think* these are my final nits.

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:564: 
nit: remove line

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:599: 
nit: remove line

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace_...
File src/core/SkColorSpace_A2B0.h (right):

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace_...
src/core/SkColorSpace_A2B0.h:26: class SkColorSpace_A2B0 : public
SkColorSpace_Base {
I would prefer that your subclasses be called:
SkColorSpace_A2B
SkColorSpace_XYZ

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace_...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:192: enum class Type : uint8_t {
I think I've been proven wrong about this...

I would rather this be public, and us assert(kXYZ == Type) before calling
makeLinearGamma().  It's just weird to use the presence of a matrix as the
indicator.

FWIW, using the presence of the matrix seems fine when we actually want the
matrix :).

https://codereview.chromium.org/2389983002/diff/160001/src/gpu/GrColorSpaceXf...
File src/gpu/GrColorSpaceXform.cpp (right):

https://codereview.chromium.org/2389983002/diff/160001/src/gpu/GrColorSpaceXf...
src/gpu/GrColorSpaceXform.cpp:53: const SkMatrix44* toXYZD50   =
as_CSB(src)->toXYZD50();
Asserts in test code are good.  Asserts in *unreachable* code are also good. 
Unfortuntately, I realize now that this code is reachable with A2B profiles. 
Clients can make whatever they want with NewICC() and then send them here.  For
that reason, we need to fail gracefully.

const SkMatrix44* toXYZD50   = as_CSB(src)->toXYZD50();
const SkMatrix44* fromXYZD50 = as_CSB(dst)->fromXYZD50();
if (!toXYZD50 || !fromXYZD50) {
    return nullptr;
}

Hopefully we haven't missed any other places where we need to fail nicely?

[raftias, 2016-10-17 15:28:03]

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:564: 
On 2016/10/14 21:31:39, msarett wrote:
> nit: remove line

Done.

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace....
src/core/SkColorSpace.cpp:599: 
On 2016/10/14 21:31:39, msarett wrote:
> nit: remove line

Done.

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace_...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/160001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:192: enum class Type : uint8_t {
On 2016/10/14 21:31:40, msarett wrote:
> I think I've been proven wrong about this...
> 
> I would rather this be public, and us assert(kXYZ == Type) before calling
> makeLinearGamma().  It's just weird to use the presence of a matrix as the
> indicator.
> 
> FWIW, using the presence of the matrix seems fine when we actually want the
> matrix :).

Done. It was weird when we wanted to access the gamma and were checking for a
matrix to see if we could, too.

https://codereview.chromium.org/2389983002/diff/160001/src/gpu/GrColorSpaceXf...
File src/gpu/GrColorSpaceXform.cpp (right):

https://codereview.chromium.org/2389983002/diff/160001/src/gpu/GrColorSpaceXf...
src/gpu/GrColorSpaceXform.cpp:53: const SkMatrix44* toXYZD50   =
as_CSB(src)->toXYZD50();
On 2016/10/14 21:31:40, msarett wrote:
> Asserts in test code are good.  Asserts in *unreachable* code are also good. 
> Unfortuntately, I realize now that this code is reachable with A2B profiles. 
> Clients can make whatever they want with NewICC() and then send them here. 
For
> that reason, we need to fail gracefully.
> 
> const SkMatrix44* toXYZD50   = as_CSB(src)->toXYZD50();
> const SkMatrix44* fromXYZD50 = as_CSB(dst)->fromXYZD50();
> if (!toXYZD50 || !fromXYZD50) {
>     return nullptr;
> }
> 
> Hopefully we haven't missed any other places where we need to fail nicely?

Done. Everywhere else seems to either be test code, tools, or SkColorSpace*
related stuff that is aware of the types. Besides the SampleApp.cpp that checks
the monitor space, I think most are definitely going to be XYZ color spaces.
There was one that loaded an external ICC profile, but it was a XYZ ICC profile.
Besides that I think there were 2 spots in test code where it wasn't known
(CodecTest and TestConfigParsing?), but I think that TestConfigParsing calls
into a place where it just returns SRGB or Linear SRGB, since I hit an error
there when I forgot a line (resulting in a null colorspace) in somewhere I was
setting the colorspace to Linear SRGB.

[raftias, 2016-10-17 15:28:25]

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

[commit-bot: I haz the power, 2016-10-17 15:28:29]

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-10-17 15:29:19]

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

[commit-bot: I haz the power, 2016-10-17 15:29:20]

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...)
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
  Test-Android-Clang-Nexus5-GPU-Adreno330-arm-Release-GN_Android-Trybot on
master.client.skia.android (JOB_FAILED,
http://build.chromium.org/p/client.skia.android/builders/Test-Android-Clang-N...)

[msarett, 2016-10-17 15:32:30]

lgtm

https://codereview.chromium.org/2389983002/diff/180001/src/core/SkColorSpace_...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/2389983002/diff/180001/src/core/SkColorSpace_...
src/core/SkColorSpace_Base.h:217: //friend class SkColorSpaceXform;
nit: Remove line

[raftias, 2016-10-17 17:34:53]

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

[commit-bot: I haz the power, 2016-10-17 17:35:03]

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-10-17 17:42:32]

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

[commit-bot: I haz the power, 2016-10-17 17:42:33]

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...)

[raftias, 2016-10-17 18:30:36]

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

[commit-bot: I haz the power, 2016-10-17 18:30:43]

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

[borenet, 2016-10-17 19:06:17]

The CQ bit was unchecked by borenet@google.com

[raftias, 2016-10-18 15:06:02]

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

[commit-bot: I haz the power, 2016-10-18 15:06:13]

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-10-18 15:08:01]

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

[commit-bot: I haz the power, 2016-10-18 15:08:02]

Dry run: Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[raftias, 2016-10-18 15:35:54]

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

[commit-bot: I haz the power, 2016-10-18 15:36:03]

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-10-18 16:10:08]

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

[commit-bot: I haz the power, 2016-10-18 16:10:09]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-10-18 17:00:58]

On 2016/10/17 15:32:30, msarett wrote:
> lgtm
> 
>
https://codereview.chromium.org/2389983002/diff/180001/src/core/SkColorSpace_...
> File src/core/SkColorSpace_Base.h (right):
> 
>
https://codereview.chromium.org/2389983002/diff/180001/src/core/SkColorSpace_...
> src/core/SkColorSpace_Base.h:217: //friend class SkColorSpaceXform;
> nit: Remove line

LGTM

[raftias, 2016-10-18 17:01:26]

The CQ bit was checked by raftias@google.com

[commit-bot: I haz the power, 2016-10-18 17:01: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-10-18 17:02:55]

Description was changed from

==========
Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

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

to

==========
Refactored SkColorSpace and added in a Lab PCS GM

The refactoring breaks off A2B0 tag support into a separate
subclass of SkColorSpace_Base, while keeping the current
(besides CLUT) functionality in a XYZTRC subclass.

ICC profile loading is now aware of this and creates the A2B0
subclass when SkColorSpace::NewICC() is called on a profile
in need of the A2B0 functionality.

The LabPCSDemo GM loads a .icc profile containing a LAB PCS and
then runs a Lab->XYZ conversion on an image using it so we can
display it and test out the A2B0 SkColorSpace functionality,
sans a/b/m-curves, as well as the Lab->XYZ conversion code.

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

Committed:
https://skia.googlesource.com/skia/+/9488833428e83c93a7e6002f4d056084fb57112f
==========

[commit-bot: I haz the power, 2016-10-18 17:02:57]

Committed patchset #14 (id:260001) as
https://skia.googlesource.com/skia/+/9488833428e83c93a7e6002f4d056084fb57112f