Differentiate between sRGBGamma and 2Dot2Gamma

Differentiate between sRGBGamma and 2Dot2Gamma

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

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

[msarett, 2016-06-14 21:12:29]

Description was changed from

==========
Differentiate between sRGBGamma and 2Dot2Gamma

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

to

==========
Differentiate between sRGBGamma and 2Dot2Gamma

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

[msarett, 2016-06-14 21:30:27]

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

[msarett, 2016-06-14 21:30:28]

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

https://codereview.chromium.org/2067833003/diff/20001/include/core/SkColorSpa...
include/core/SkColorSpace.h:52: static sk_sp<SkColorSpace> NewRGB(GammaNamed
gammaNamed, const SkMatrix44& toXYZD50);
I've changed this to take a GammaNamed instead of float[3].  Trying to write
accurate, fast code for pow has made me think that we really don't want clients
to specify arbitrary floats.

I put the original constructor in SkColorSpace_Base, since it's useful for PNG.

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

https://codereview.chromium.org/2067833003/diff/20001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:27: SkColorSpace_Base::SkColorSpace_Base(GammaNamed
gammaNamed, const SkMatrix44& toXYZD50, Named named,
Now SkColorSpace's have a GammaNamed *or* the increasingly wacky SkGammas
struct.

I did this because we can't say "sRGB" with a float anymore.  But I actually
think it's logical, since we were being redundant by having a name and another
representation.

In the future, I will probably blow away SkGammas and just use a look-up table
as a fallback representation.  I *think* that's how we'll end up using all of
that info anyway.

https://codereview.chromium.org/2067833003/diff/20001/src/core/SkMipMap.cpp
File src/core/SkMipMap.cpp (right):

https://codereview.chromium.org/2067833003/diff/20001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:309: return SkColorSpace::k2Dot2Curve_GammaNamed ==
info.colorSpace()->gammaNamed() ||
Will probably want to add this type of API to SkColorSpace at some point.

[Brian Osman, 2016-06-14 21:55:37]

brianosman@google.com changed reviewers:
+ brianosman@google.com

[Brian Osman, 2016-06-14 21:55:38]

https://codereview.chromium.org/2067833003/diff/20001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/2067833003/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:234: // Default to sRGB (gamma = 2.2f) if the image has
color space information,
Now that we're distinguishing between 2.2 and sRGB, does it make sense to
default to actual sRGB in this case, rather than 2.2? (ie, return
SkColorSpace::NewRGB(kSRGB_GammaNamed, mat)) ?

[msarett, 2016-06-14 22:59:10]

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

[msarett, 2016-06-14 22:59:11]

https://codereview.chromium.org/2067833003/diff/20001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/2067833003/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:234: // Default to sRGB (gamma = 2.2f) if the image has
color space information,
On 2016/06/14 21:55:38, Brian Osman wrote:
> Now that we're distinguishing between 2.2 and sRGB, does it make sense to
> default to actual sRGB in this case, rather than 2.2? (ie, return
> SkColorSpace::NewRGB(kSRGB_GammaNamed, mat)) ?

Yes I think so!  Done.

Might make sense to default to whichever we can do faster, though I don't know
what that is yet.  If it's a tie, I'd go sRGB.

[Brian Osman, 2016-06-15 13:27:00]

Mostly just nit-picking.

https://codereview.chromium.org/2067833003/diff/60001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/2067833003/diff/60001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:241: return SkColorSpace_Base::NewRGB(gammas, mat);
Nit: Now that we're returning sRGB in the else case above, I'd move this line
into the previous 'if' block, to make the control flow symmetric/clearer.

https://codereview.chromium.org/2067833003/diff/60001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:247: // Guess a default value for cHRM?  Or should we
just give up?
As I read this code and think about it more, I get conflicted. My gut says that
we should be guessing sRGB gamut for many of these images (those that just have
gAMA chunk, and those with no information at all). Or perhaps we guess sRGB
gamut if it has gAMA, and it's nearly 2.2?

It seems like a balancing act between trying to be clever, and making the
behavior too much of a black box. Of course, if people are creating new content
and don't like the results - they ought to embed proper profile data in their
images. It's probably more helpful to handle the huge amount of content that
already exists and isn't being maintained by anyone - so I vote for guessing
sRGB whenever we're not sure.

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

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:102: return sk_sp<SkColorSpace>(new
SkColorSpace_Base(nullptr, gammas, toXYZD50, nullptr));
sk_make_sp ?

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:128: return sk_sp<SkColorSpace>(new
SkColorSpace_Base(gammaNamed, toXYZD50, kUnknown_Named,
sk_make_sp ?

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:894: sk_sp<SkGammas> gammas(new
SkGammas(std::move(curves[0]), std::move(curves[1]),
auto gammas = sk_make_sp(...) ?

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:896: return sk_sp<SkColorSpace>(new
SkColorSpace_Base(nullptr, std::move(gammas),
sk_make_sp ?

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:916: sk_sp<SkGammas> gammas(new
SkGammas(std::move(curves[0]), std::move(curves[1]),
auto gammas = sk_make_sp(...) ?

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:919: return sk_sp<SkColorSpace>(new
SkColorSpace_Base(colorLUT.release(),
sk_make_sp ?

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1066: // FIXME (msarett): kSRGB cannot be represented
by a value.
I know you've got a FIXME comment here, but I also assume the fall-through is
intentional, since 2.2 is the best we can do to approximate it. Add a comment to
that effect?

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1129: // FIXME (msarett): kSRGB cannot be represented
by a value.
Ditto. (Also: Isn't there a special sRGB chunk? Or is that a different format?)

[msarett, 2016-06-15 14:33:49]

https://codereview.chromium.org/2067833003/diff/60001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/2067833003/diff/60001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:241: return SkColorSpace_Base::NewRGB(gammas, mat);
On 2016/06/15 13:26:59, Brian Osman wrote:
> Nit: Now that we're returning sRGB in the else case above, I'd move this line
> into the previous 'if' block, to make the control flow symmetric/clearer.

Yup that makes a lot more sense.  Done.

https://codereview.chromium.org/2067833003/diff/60001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:247: // Guess a default value for cHRM?  Or should we
just give up?
On 2016/06/15 13:26:59, Brian Osman wrote:
> As I read this code and think about it more, I get conflicted. My gut says
that
> we should be guessing sRGB gamut for many of these images (those that just
have
> gAMA chunk, and those with no information at all). Or perhaps we guess sRGB
> gamut if it has gAMA, and it's nearly 2.2?
> 
> It seems like a balancing act between trying to be clever, and making the
> behavior too much of a black box. Of course, if people are creating new
content
> and don't like the results - they ought to embed proper profile data in their
> images. It's probably more helpful to handle the huge amount of content that
> already exists and isn't being maintained by anyone - so I vote for guessing
> sRGB whenever we're not sure.

Yeah I'd agree we should guess sRGB here.  Nobody ever seems to specify a gamma
of 1...

Done.

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

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:102: return sk_sp<SkColorSpace>(new
SkColorSpace_Base(nullptr, gammas, toXYZD50, nullptr));
On 2016/06/15 13:27:00, Brian Osman wrote:
> sk_make_sp ?

Unfortunately this doesn't work when the constructor is private...

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:128: return sk_sp<SkColorSpace>(new
SkColorSpace_Base(gammaNamed, toXYZD50, kUnknown_Named,
On 2016/06/15 13:26:59, Brian Osman wrote:
> sk_make_sp ?

Uggh also private.

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:894: sk_sp<SkGammas> gammas(new
SkGammas(std::move(curves[0]), std::move(curves[1]),
On 2016/06/15 13:27:00, Brian Osman wrote:
> auto gammas = sk_make_sp(...) ?

Finally can use sk_make_sp!  Done.

I'm not really a fan of auto though, so I'll keep the type (unless "auto" is in
the style guide, then I'll learn to like it haha).

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:896: return sk_sp<SkColorSpace>(new
SkColorSpace_Base(nullptr, std::move(gammas),
On 2016/06/15 13:27:00, Brian Osman wrote:
> sk_make_sp ?

Again with the private constructor issues.

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:916: sk_sp<SkGammas> gammas(new
SkGammas(std::move(curves[0]), std::move(curves[1]),
On 2016/06/15 13:27:00, Brian Osman wrote:
> auto gammas = sk_make_sp(...) ?

Using sk_make_sp.

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:919: return sk_sp<SkColorSpace>(new
SkColorSpace_Base(colorLUT.release(),
On 2016/06/15 13:27:00, Brian Osman wrote:
> sk_make_sp ?

Private constructor.

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1066: // FIXME (msarett): kSRGB cannot be represented
by a value.
On 2016/06/15 13:26:59, Brian Osman wrote:
> I know you've got a FIXME comment here, but I also assume the fall-through is
> intentional, since 2.2 is the best we can do to approximate it. Add a comment
to
> that effect?

Yes! Done.

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:1129: // FIXME (msarett): kSRGB cannot be represented
by a value.
On 2016/06/15 13:27:00, Brian Osman wrote:
> Ditto. (Also: Isn't there a special sRGB chunk? Or is that a different
format?)

I think the best way to represent sRGB gamma in an ICC parametric is a
parametric piecewise curve.  Though most authors just use a table.

[Brian Osman, 2016-06-15 15:01:27]

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

https://codereview.chromium.org/2067833003/diff/60001/src/core/SkColorSpace.c...
src/core/SkColorSpace.cpp:894: sk_sp<SkGammas> gammas(new
SkGammas(std::move(curves[0]), std::move(curves[1]),
On 2016/06/15 14:33:48, msarett wrote:
> On 2016/06/15 13:27:00, Brian Osman wrote:
> > auto gammas = sk_make_sp(...) ?
> 
> Finally can use sk_make_sp!  Done.
> 
> I'm not really a fan of auto though, so I'll keep the type (unless "auto" is
in
> the style guide, then I'll learn to like it haha).

No problem, I was just trying to save typing. (And my quick survey of other code
in Skia shows that it's a common pattern).

https://codereview.chromium.org/2067833003/diff/70001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/2067833003/diff/70001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:271: return nullptr;
Sorry, I was also thinking about this case. Maybe return sRGB here as well, for
consistency with all of our other fallback paths?

[msarett, 2016-06-15 15:18:43]

https://codereview.chromium.org/2067833003/diff/70001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/2067833003/diff/70001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:271: return nullptr;
On 2016/06/15 15:01:27, Brian Osman wrote:
> Sorry, I was also thinking about this case. Maybe return sRGB here as well,
for
> consistency with all of our other fallback paths?

This is handled here:
https://cs.chromium.org/chromium/src/third_party/skia/src/codec/SkPngCodec.cp...

The calls to NewICC() (and in the future maybe even NewRGB()) may fail, so I
decided to address them all at once outside this function.

But this comment is extremely misleading - I'll fix it.

[Brian Osman, 2016-06-15 15:25:02]

lgtm

[reed1, 2016-06-15 15:36:45]

should we add the helper at the same time, so we can update call-sites?

e.g.

bool SkColorSpace::gammaCloseToSRGB() const 
  -- which returns true for both enums

[msarett, 2016-06-15 17:18:03]

On 2016/06/15 15:36:45, reed1 wrote:
> should we add the helper at the same time, so we can update call-sites?
> 
> e.g.
> 
> bool SkColorSpace::gammaCloseToSRGB() const 
>   -- which returns true for both enums

Sounds good to me!  I've added a function and updated the code in SkMipMap. 
Sounds like Brian will also make use of this function.

[reed1, 2016-06-16 13:42:24]

lgtm

[msarett, 2016-06-16 13:49:53]

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

[commit-bot: I haz the power, 2016-06-16 13:49:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2067833003/110001

[commit-bot: I haz the power, 2016-06-16 13:57:07]

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

[commit-bot: I haz the power, 2016-06-16 13:57:08]

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

[msarett, 2016-06-16 14:22:06]

Patchset #8 (id:130001) has been deleted

[msarett, 2016-06-16 14:22:12]

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

[commit-bot: I haz the power, 2016-06-16 14:22:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2067833003/150001

[commit-bot: I haz the power, 2016-06-16 14:36:16]

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

[commit-bot: I haz the power, 2016-06-16 14:36:17]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-06-16 14:36:39]

The CQ bit was checked by msarett@google.com

[msarett, 2016-06-16 14:36:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianosman@google.com, reed@google.com
Link to the patchset: https://codereview.chromium.org/2067833003/#ps150001
(title: "Fix DefaultXform")

[commit-bot: I haz the power, 2016-06-16 14:36:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2067833003/150001

[commit-bot: I haz the power, 2016-06-16 14:37:45]

Description was changed from

==========
Differentiate between sRGBGamma and 2Dot2Gamma

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

to

==========
Differentiate between sRGBGamma and 2Dot2Gamma

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

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

[commit-bot: I haz the power, 2016-06-16 14:37:47]

Committed patchset #8 (id:150001) as
https://skia.googlesource.com/skia/+/c4ce6b592487305de251bbebaf8eeee38371b877