Write ICC profiles from SkColorSpace object

Write ICC profiles from SkColorSpace object

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

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

[msarett, 2016-05-23 21:44:04]

Description was changed from

==========
Write ICC profiles from SkColorSpace object

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

to

==========
Write ICC profiles from SkColorSpace object

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

[msarett, 2016-05-24 14:15:25]

Patchset #2 (id:20001) has been deleted

[msarett, 2016-05-24 14:15:33]

Patchset #1 (id:1) has been deleted

[msarett, 2016-05-24 15:36:58]

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

[msarett, 2016-05-24 15:36:59]

https://codereview.chromium.org/2001203003/diff/80001/include/core/SkColorSpa...
File include/core/SkColorSpace.h (right):

https://codereview.chromium.org/2001203003/diff/80001/include/core/SkColorSpa...
include/core/SkColorSpace.h:69: sk_sp<SkData> writeToICC();
We may not need this to be a public API if/when we integrate this with
serialization/deserialization.

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

https://codereview.chromium.org/2001203003/diff/80001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1052: // We choose to hold onto the profile here, in
case we need to serialize again.
I'm not sure if this is the right decision...

[scroggo, 2016-05-24 15:57:51]

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

https://codereview.chromium.org/2001203003/diff/80001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:214: static const uint32_t kDisplay_Profile =
SkSetFourByteTag('m', 'n', 't', 'r');
Now that we don't have to worry about VS 2013, should we make these constexpr?

https://codereview.chromium.org/2001203003/diff/80001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1004: void* profile =
sk_malloc_throw(kICCProfileSize);
Should this be in an auto deleter of some kind?

https://codereview.chromium.org/2001203003/diff/80001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1052: // We choose to hold onto the profile here, in
case we need to serialize again.
On 2016/05/24 15:36:58, msarett wrote:
> I'm not sure if this is the right decision...

Add a comment? (Or leave it out for now?)

https://codereview.chromium.org/2001203003/diff/80001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/2001203003/diff/80001/tests/ColorSpaceTest.cp...
tests/ColorSpaceTest.cpp:129: sk_sp<SkColorSpace> monitorSpace =
SkColorSpace::NewICC(monitorData->data(),
Make sure this doesn't crash if monitorData was null?

[reed1, 2016-05-24 16:04:47]

not clear it has to be on the base class (the impl). Could defer to subclasses
as to how they will compute/return this.

https://codereview.chromium.org/2001203003/diff/80001/include/core/SkColorSpa...
File include/core/SkColorSpace.h (right):

https://codereview.chromium.org/2001203003/diff/80001/include/core/SkColorSpa...
include/core/SkColorSpace.h:69: sk_sp<SkData> writeToICC();
On 2016/05/24 15:36:58, msarett wrote:
> We may not need this to be a public API if/when we integrate this with
> serialization/deserialization.

1. should be const
2. no need to mention "serialization" in the comment imho
3. could make it private for now, but somehow this seems sensible and so I'm
fine if its public

[msarett, 2016-05-24 17:37:19]

Patchset #4 (id:100001) has been deleted

[msarett, 2016-05-24 18:38:06]

> not clear it has to be on the base class (the impl). Could defer to subclasses
> as to how they will compute/return this.

I've moved this function (declaration and impl) to
SkColorSpace_Base.  Now that I reread your comment, I'm not
exactly sure that's what you were asking for here (though
I think it's a fine change).

We could potentially subclass SkColorSpace_Base, but I'm
not sure I see a clear split (yet).  Ex, we could do:
CreatedFromICCColorSpace vs. CreatedFromRGBColorSpace
JustXYZAndGammaColorSpace vs. ComplicatedColorSpace...

https://codereview.chromium.org/2001203003/diff/80001/include/core/SkColorSpa...
File include/core/SkColorSpace.h (right):

https://codereview.chromium.org/2001203003/diff/80001/include/core/SkColorSpa...
include/core/SkColorSpace.h:69: sk_sp<SkData> writeToICC();
On 2016/05/24 16:04:46, reed1 wrote:
> On 2016/05/24 15:36:58, msarett wrote:
> > We may not need this to be a public API if/when we integrate this with
> > serialization/deserialization.
> 
> 1. should be const

Done.

> 2. no need to mention "serialization" in the comment imho

Removed.

> 3. could make it private for now, but somehow this seems sensible and so I'm
> fine if its public 

Moved to SkColorSpace_Base.  Still public so we can test.

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

https://codereview.chromium.org/2001203003/diff/80001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:214: static const uint32_t kDisplay_Profile =
SkSetFourByteTag('m', 'n', 't', 'r');
On 2016/05/24 15:57:50, scroggo wrote:
> Now that we don't have to worry about VS 2013, should we make these constexpr?

Sure sounds good to me.

https://codereview.chromium.org/2001203003/diff/80001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1004: void* profile =
sk_malloc_throw(kICCProfileSize);
On 2016/05/24 15:57:50, scroggo wrote:
> Should this be in an auto deleter of some kind?

Yeah I think that's probably safer, though at the moment I think it's not
necessary.

https://codereview.chromium.org/2001203003/diff/80001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1052: // We choose to hold onto the profile here, in
case we need to serialize again.
On 2016/05/24 15:57:50, scroggo wrote:
> On 2016/05/24 15:36:58, msarett wrote:
> > I'm not sure if this is the right decision...
> 
> Add a comment? (Or leave it out for now?)

Leaving it out for now...  That allows this function to be const, which we know
for sure that we want.

https://codereview.chromium.org/2001203003/diff/80001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/2001203003/diff/80001/tests/ColorSpaceTest.cp...
tests/ColorSpaceTest.cpp:129: sk_sp<SkColorSpace> monitorSpace =
SkColorSpace::NewICC(monitorData->data(),
On 2016/05/24 15:57:50, scroggo wrote:
> Make sure this doesn't crash if monitorData was null?

Sounds good.  Done.

[scroggo, 2016-05-24 19:12:20]

lgtm

https://codereview.chromium.org/2001203003/diff/140001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/2001203003/diff/140001/tests/ColorSpaceTest.c...
tests/ColorSpaceTest.cpp:131: return;
This should probably fail the test or print something so we don't silently stop
running the test if we delete the file.

[msarett, 2016-05-24 19:14:27]

https://codereview.chromium.org/2001203003/diff/140001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/2001203003/diff/140001/tests/ColorSpaceTest.c...
tests/ColorSpaceTest.cpp:131: return;
On 2016/05/24 19:12:20, scroggo wrote:
> This should probably fail the test or print something so we don't silently
stop
> running the test if we delete the file.

It should fail the statement above "REPORTER_ASSERT(r, monitorData);"

[scroggo, 2016-05-24 19:14:59]

https://codereview.chromium.org/2001203003/diff/140001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/2001203003/diff/140001/tests/ColorSpaceTest.c...
tests/ColorSpaceTest.cpp:131: return;
On 2016/05/24 19:14:27, msarett wrote:
> On 2016/05/24 19:12:20, scroggo wrote:
> > This should probably fail the test or print something so we don't silently
> stop
> > running the test if we delete the file.
> 
> It should fail the statement above "REPORTER_ASSERT(r, monitorData);"

D'oh! Yes it should. Nvm...

[msarett, 2016-05-24 19:31:59]

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

[commit-bot: I haz the power, 2016-05-24 19:32:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001203003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001203003/140001

[commit-bot: I haz the power, 2016-05-24 19:45:39]

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

[commit-bot: I haz the power, 2016-05-24 19:45:40]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-05-25 15:34:26]

lgtm

[msarett, 2016-05-25 15:52:43]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-05-25 15:52:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001203003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001203003/140001

[commit-bot: I haz the power, 2016-05-25 15:53:44]

Description was changed from

==========
Write ICC profiles from SkColorSpace object

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

to

==========
Write ICC profiles from SkColorSpace object

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

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

[commit-bot: I haz the power, 2016-05-25 15:53:45]

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