Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

This was submitted alone to reduce CL size. Tests that will use these changes will be introduced in the subsequent CLs that add on lut8/16Type A2B0 parsing. We already have lut16Type test images and these have been tested locally, but require additional code not submitted yet for lut16Type ICC profile parsing and A2B colorspace xforms.

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

Committed: https://skia.googlesource.com/skia/+/026f223d8641beeae19ed0bdbeca738be62256f5

[raftias, 2016-10-21 22:02:34]

Description was changed from

==========
Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

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

to

==========
Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

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

[raftias, 2016-10-21 22:03:45]

Description was changed from

==========
Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

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

to

==========
Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

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

[raftias, 2016-10-21 22:03:46]

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

[raftias, 2016-10-21 22:04:09]

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

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_A2B.h...
src/core/SkColorSpace_A2B.h:94: SkGammaNamed              fGammaNamed;
It would be possible to join these into a union which would save up to 76-88
bytes per instance, but would make the code pretty gross (manual ref/unref calls
to maintain ownership of the CLUT/gammas). I could write that but I don't really
think it's worth it, although I'm not really sure about how memory-constrained
Skia is or not so I don't think I can judge that properly.

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_A2B.h...
src/core/SkColorSpace_A2B.h:124: friend class SkColorSpace_A2B;
I wasn't sure about instead either making Element constructor, or changing this
friend line to friend the load_a2b0* functions and then change the 4
addElement() functions to just taking in an Element instance.

[msarett, 2016-10-24 13:34:38]

Do we have a test image that requires this refactor?  Can you write a test?

[raftias, 2016-10-24 13:57:03]

Description was changed from

==========
Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

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

to

==========
Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

This was submitted alone to reduce CL size. Tests that will use these changes
will be introduced in the subsequent CLs that add on lut8/16Type A2B0 parsing.
We already have lut16Type test images and these have been tested locally, but
require additional code not submitted yet for lut16Type ICC profile parsing and
A2B colorspace xforms.

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

[raftias, 2016-10-24 13:57:25]

On 2016/10/24 13:34:38, msarett wrote:
> Do we have a test image that requires this refactor?  Can you write a test?

Updated description.

[msarett1, 2016-10-24 14:12:41]

msarett@chromium.org changed reviewers:
+ msarett@chromium.org

[msarett1, 2016-10-24 14:12:42]

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

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_A2B.h...
src/core/SkColorSpace_A2B.h:94: SkGammaNamed              fGammaNamed;
On 2016/10/21 22:04:09, raftias wrote:
> It would be possible to join these into a union which would save up to 76-88
> bytes per instance, but would make the code pretty gross (manual ref/unref
calls
> to maintain ownership of the CLUT/gammas). I could write that but I don't
really
> think it's worth it, although I'm not really sure about how memory-constrained
> Skia is or not so I don't think I can judge that properly.

I think this should be a union.  I think it makes sense to leave that
optimization to be considered on its own in a later CL.

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_A2B.h...
src/core/SkColorSpace_A2B.h:124: friend class SkColorSpace_A2B;
On 2016/10/21 22:04:09, raftias wrote:
> I wasn't sure about instead either making Element constructor, or changing
this
> friend line to friend the load_a2b0* functions and then change the 4
> addElement() functions to just taking in an Element instance.

Let's pass in the elements to the constructor and make the ColorSpace behave as
if it's immutable.

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_A2B.h...
src/core/SkColorSpace_A2B.h:128: size_t elementCount() const { return
fElements.size(); }
nit: Call this count().

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_ICC.cpp
File src/core/SkColorSpace_ICC.cpp (right):

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_ICC.c...
src/core/SkColorSpace_ICC.cpp:727: *gammaNamed = kNonStandard_SkGammaNamed;
This doesn't seem wrong, but why is this necessary?

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_ICC.c...
src/core/SkColorSpace_ICC.cpp:983: // Recognize color profile specified by A2B0
tag.
Here's how I think this logic should go:
(Option 1) PCS is XYZ and XYZ tags exist -> use SkColorSpace_XYZ
(Option 2) A2B tag exists -> use SkColorSpace_A2B

Maybe this can be accomplished by checking "if (r && g && b && XYZ == PCS)"
before checking the A2B case?

[raftias, 2016-10-24 15:26:47]

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

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_A2B.h...
src/core/SkColorSpace_A2B.h:94: SkGammaNamed              fGammaNamed;
On 2016/10/24 14:12:42, msarett1 wrote:
> On 2016/10/21 22:04:09, raftias wrote:
> > It would be possible to join these into a union which would save up to 76-88
> > bytes per instance, but would make the code pretty gross (manual ref/unref
> calls
> > to maintain ownership of the CLUT/gammas). I could write that but I don't
> really
> > think it's worth it, although I'm not really sure about how
memory-constrained
> > Skia is or not so I don't think I can judge that properly.
> 
> I think this should be a union.  I think it makes sense to leave that
> optimization to be considered on its own in a later CL.

Acknowledged.

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_A2B.h...
src/core/SkColorSpace_A2B.h:124: friend class SkColorSpace_A2B;
On 2016/10/24 14:12:42, msarett1 wrote:
> On 2016/10/21 22:04:09, raftias wrote:
> > I wasn't sure about instead either making Element constructor, or changing
> this
> > friend line to friend the load_a2b0* functions and then change the 4
> > addElement() functions to just taking in an Element instance.
> 
> Let's pass in the elements to the constructor and make the ColorSpace behave
as
> if it's immutable.

Done.

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_A2B.h...
src/core/SkColorSpace_A2B.h:128: size_t elementCount() const { return
fElements.size(); }
On 2016/10/24 14:12:42, msarett1 wrote:
> nit: Call this count().

Done.

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_ICC.cpp
File src/core/SkColorSpace_ICC.cpp (right):

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_ICC.c...
src/core/SkColorSpace_ICC.cpp:727: *gammaNamed = kNonStandard_SkGammaNamed;
On 2016/10/24 14:12:42, msarett1 wrote:
> This doesn't seem wrong, but why is this necessary?

To have some default value. It gets set on line 741 if it's a named gamma, but
if it is indeed non-standard it needs to get set to that somewhere. The
alternative to putting it here would be to assign it to non-standard in the
different places that the SkGammas is getting set (line 756, 799).

It was an error in my previous code to not assign it (I think it started as
kNonStandard elsewhere?) which would cause the SkGammas info to be skipped since
I had skLinear as the default (initialized outside), but I think initializing it
inside parse_and_load_gammas() makes more sense than requiring the caller to do
it.

https://codereview.chromium.org/2444553002/diff/1/src/core/SkColorSpace_ICC.c...
src/core/SkColorSpace_ICC.cpp:983: // Recognize color profile specified by A2B0
tag.
On 2016/10/24 14:12:42, msarett1 wrote:
> Here's how I think this logic should go:
> (Option 1) PCS is XYZ and XYZ tags exist -> use SkColorSpace_XYZ
> (Option 2) A2B tag exists -> use SkColorSpace_A2B
> 
> Maybe this can be accomplished by checking "if (r && g && b && XYZ == PCS)"
> before checking the A2B case?

Done.

[msarett1, 2016-10-24 15:35:41]

lgtm

Good CL size.  Thanks for breaking this up.

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
File src/core/SkColorSpace_A2B.cpp (right):

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
src/core/SkColorSpace_A2B.cpp:10: SkColorSpace_A2B::SkColorSpace_A2B(PCS pcs,
sk_sp<SkData> profileData, std::vector<Element> elements)
nit: 100 chars

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
File src/core/SkColorSpace_A2B.h (right):

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
src/core/SkColorSpace_A2B.h:123: friend bool
load_a2b0_a_to_b_type(std::vector<Element>*,const uint8_t*, size_t);
nit: Can we just make the constructors public?  It's not super dangerous to
allow people to construct Elements since they can't really tell us to do
anything with them anyway.

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
src/core/SkColorSpace_A2B.h:141: PCS                                     fPCS;
nit: Maybe don't need this much white space between field type and field name.

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_I...
File src/core/SkColorSpace_ICC.cpp (right):

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_I...
src/core/SkColorSpace_ICC.cpp:134: switch (fPCS) {
This sort of thing should probably be a separate change, but it's fine here.

[raftias, 2016-10-24 15:52:39]

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
File src/core/SkColorSpace_A2B.cpp (right):

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
src/core/SkColorSpace_A2B.cpp:10: SkColorSpace_A2B::SkColorSpace_A2B(PCS pcs,
sk_sp<SkData> profileData, std::vector<Element> elements)
On 2016/10/24 15:35:41, msarett1 wrote:
> nit: 100 chars

Acknowledged.

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
File src/core/SkColorSpace_A2B.h (right):

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
src/core/SkColorSpace_A2B.h:123: friend bool
load_a2b0_a_to_b_type(std::vector<Element>*,const uint8_t*, size_t);
On 2016/10/24 15:35:41, msarett1 wrote:
> nit: Can we just make the constructors public?  It's not super dangerous to
> allow people to construct Elements since they can't really tell us to do
> anything with them anyway.

Done.

https://codereview.chromium.org/2444553002/diff/20001/src/core/SkColorSpace_A...
src/core/SkColorSpace_A2B.h:141: PCS                                     fPCS;
On 2016/10/24 15:35:41, msarett1 wrote:
> nit: Maybe don't need this much white space between field type and field name.

Done. This was just left over from some other field that I was going to have
then decided to remove.

[msarett1, 2016-10-24 15:55:18]

LGTM

[raftias, 2016-10-24 16:21:32]

The CQ bit was checked by raftias@google.com

[commit-bot: I haz the power, 2016-10-24 16:21:38]

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-24 16:52:28]

Description was changed from

==========
Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

This was submitted alone to reduce CL size. Tests that will use these changes
will be introduced in the subsequent CLs that add on lut8/16Type A2B0 parsing.
We already have lut16Type test images and these have been tested locally, but
require additional code not submitted yet for lut16Type ICC profile parsing and
A2B colorspace xforms.

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

to

==========
Refactored SkColorSpace_A2B to allow arbitrary ordering of elements

This is essential for representing non-lutAtoBType A2B tags such as
lut16Type, lut8Type, mpet. Parsing of A2B0 tags was also moved ahead
of the TRC/XYZ-matrix parsing, as profiles examined with both tags
either had the TRC/XYZ tags as a fall-back or were incorrectly displayed
if only the TRC/XYZ tags were used.

This was submitted alone to reduce CL size. Tests that will use these changes
will be introduced in the subsequent CLs that add on lut8/16Type A2B0 parsing.
We already have lut16Type test images and these have been tested locally, but
require additional code not submitted yet for lut16Type ICC profile parsing and
A2B colorspace xforms.

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

Committed:
https://skia.googlesource.com/skia/+/026f223d8641beeae19ed0bdbeca738be62256f5
==========

[commit-bot: I haz the power, 2016-10-24 16:52:29]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/026f223d8641beeae19ed0bdbeca738be62256f5