Add SkColorSpace to SkImageInfo

include/core/SkImageInfo.h

Index: include/core/SkImageInfo.h
diff --git a/include/core/SkImageInfo.h b/include/core/SkImageInfo.h
index 4b308c05d88ca736d69451f3b970c1bd71b30b12..af3d7393b3c51b914beaaac8a9c70c25aa47a915 100644
--- a/include/core/SkImageInfo.h
+++ b/include/core/SkImageInfo.h
@@ -8,6 +8,7 @@
 #ifndef SkImageInfo_DEFINED
 #define SkImageInfo_DEFINED
 
+#include "SkColorSpace.h"
 #include "SkMath.h"
 #include "SkRect.h"
 #include "SkSize.h"
@@ -182,7 +183,8 @@ enum SkColorProfileType {
 struct SK_API SkImageInfo {
 public:
     SkImageInfo()
-        : fWidth(0)
+        : fColorSpace(nullptr)
+        , fWidth(0)
         , fHeight(0)
         , fColorType(kUnknown_SkColorType)
         , fAlphaType(kUnknown_SkAlphaType)
@@ -191,15 +193,18 @@ public:
 
     static SkImageInfo Make(int width, int height, SkColorType ct, SkAlphaType at,
                             SkColorProfileType pt = kLinear_SkColorProfileType) {
-        return SkImageInfo(width, height, ct, at, pt);
+        return SkImageInfo(width, height, ct, at, pt, nullptr);
     }
 
+    static SkImageInfo Make(int width, int height, SkColorType ct, SkAlphaType at,
+                            sk_sp<SkColorSpace> cs);
+
     /**
      *  Sets colortype to the native ARGB32 type.
      */
     static SkImageInfo MakeN32(int width, int height, SkAlphaType at,
                                SkColorProfileType pt = kLinear_SkColorProfileType) {
-        return SkImageInfo(width, height, kN32_SkColorType, at, pt);
+        return SkImageInfo(width, height, kN32_SkColorType, at, pt, nullptr);
     }
 
     /**
@@ -207,7 +212,7 @@ public:
      */
     static SkImageInfo MakeN32Premul(int width, int height,
                                      SkColorProfileType pt = kLinear_SkColorProfileType) {
-        return SkImageInfo(width, height, kN32_SkColorType, kPremul_SkAlphaType, pt);
+        return SkImageInfo(width, height, kN32_SkColorType, kPremul_SkAlphaType, pt, nullptr);
     }
 
     /**
@@ -220,12 +225,12 @@ public:
 
     static SkImageInfo MakeA8(int width, int height) {
         return SkImageInfo(width, height, kAlpha_8_SkColorType, kPremul_SkAlphaType,
-                           kLinear_SkColorProfileType);
+                           kLinear_SkColorProfileType, nullptr);
     }
 
     static SkImageInfo MakeUnknown(int width, int height) {
         return SkImageInfo(width, height, kUnknown_SkColorType, kUnknown_SkAlphaType,
-                           kLinear_SkColorProfileType);
+                           kLinear_SkColorProfileType, nullptr);
     }
 
     static SkImageInfo MakeUnknown() {
@@ -236,6 +241,9 @@ public:
     int height() const { return fHeight; }
     SkColorType colorType() const { return fColorType; }
     SkAlphaType alphaType() const { return fAlphaType; }
+    SkColorSpace* colorSpace() const { return fColorSpace.get(); }

[scroggo, 2016/05/31 13:48:25]

Now that SkColorSpace is in include/, can this return a const ref to sk_sp?

From Herb's comment [1], it looks like the goal was to not include
SkColorSpace.h, but we still include it?

[1]
https://codereview.chromium.org/2000713003/diff/20001/include/core/SkImageInf...

[msarett, 2016/05/31 14:30:08]

Yeah I think it could.  The reason I went with a bare ptr was so the client
doesn't necessarily have to ref the color space.  I think the const ref to sk_sp
would accomplish the same thing.  I'm not sure which is better (or what is "Skia
style").  Whatever we decide here, we should do the same thing in SkCodec.h.
I'll look if we might be able to get away with a forward declare...

My read of Herb's comment was a suggestion to expose information about the gamma
rather than the entire SkColorSpace object.  I ultimately decided against this
approach for a few reasons:
(1) I was wrapping APIs that are already on SkColorSpace.
(2) What should we return when the SkColorSpace is nullptr?
(3) The SkColorSpace API is already limited.  Is there a reason to limit it
further?
(4) We can expose this object without forcing the client to ref it.

[scroggo, 2016/05/31 14:52:06]

I don't think Skia style has been updated, but my read of
https://docs.google.com/document/d/1PZRnDBwhnB_Ug6DtlkphAvJ9Ga5U3ONbSn1wf6jXX-8
tells me that a getter should return a const ref.
+1
sgtm

[reed1, 2016/05/31 22:07:00]

That doc needs updating. We have (so far at least) decided to not change what
our getters return, so I think bare-pointer is correct.

+
+    // Deprecated
     SkColorProfileType profileType() const { return fProfileType; }
 
     bool isEmpty() const { return fWidth <= 0 || fHeight <= 0; }
@@ -244,6 +252,7 @@ public:
         return SkAlphaTypeIsOpaque(fAlphaType);
     }
 
+    // Deprecated
     bool isLinear() const { return kLinear_SkColorProfileType == fProfileType; }
     bool isSRGB() const { return kSRGB_SkColorProfileType == fProfileType; }
 
@@ -255,15 +264,15 @@ public:
      *  but with the specified width and height.
      */
     SkImageInfo makeWH(int newWidth, int newHeight) const {
-        return SkImageInfo::Make(newWidth, newHeight, fColorType, fAlphaType, fProfileType);
+        return SkImageInfo(newWidth, newHeight, fColorType, fAlphaType, fProfileType, fColorSpace);
     }
 
     SkImageInfo makeAlphaType(SkAlphaType newAlphaType) const {
-        return SkImageInfo::Make(fWidth, fHeight, fColorType, newAlphaType, fProfileType);
+        return SkImageInfo(fWidth, fHeight, fColorType, newAlphaType, fProfileType, fColorSpace);
     }
     
     SkImageInfo makeColorType(SkColorType newColorType) const {
-        return SkImageInfo::Make(fWidth, fHeight, newColorType, fAlphaType, fProfileType);
+        return SkImageInfo(fWidth, fHeight, newColorType, fAlphaType, fProfileType, fColorSpace);
     }
 
     int bytesPerPixel() const { return SkColorTypeBytesPerPixel(fColorType); }
@@ -285,10 +294,14 @@ public:
     }
 
     bool operator==(const SkImageInfo& other) const {
-        return 0 == memcmp(this, &other, sizeof(other));
+        return fWidth == other.fWidth && fHeight == other.fHeight &&
+               fColorType == other.fColorType && fAlphaType == other.fAlphaType &&
+               fProfileType == other.fProfileType && fColorSpace == other.fColorSpace;

[scroggo, 2016/05/31 13:48:25]

Doesn't this check whether fColorSpace is the same object as other.fColorSpace?
Is that deliberate? It at least seems misleading. (I had to upload patch set 18
of crrev.com/1997703003 because two SkCodecs created from the same file had
different color space objects, though I would expect them to be the same.)

[msarett, 2016/05/31 14:30:08]

This is deliberate, though maybe it's the wrong decision.  We have once ptrs for
sRGB and adobe RGB (and probably will add another once ptr when we have a
commonly used linear color space).

So this will work most of the time with some false negatives.  False negatives
should be rare, since they will only occur when we are comparing two rare
profiles that happen to be identical.

Another option is to avoid false negatives by doing a deep compare here.

FWIW, I think that the problem in crrev.com/1997703003 is probably because I've
temporarily regressed our ability to recognize parametric gammas as sRGB.  I
plan to fix this.

At the very least, I think this needs a comment.

[scroggo, 2016/05/31 14:52:06]

FWIW, I don't know that my test case is a normal use case.
+1

     }
     bool operator!=(const SkImageInfo& other) const {
-        return 0 != memcmp(this, &other, sizeof(other));
+        return fWidth != other.fWidth || fHeight != other.fHeight ||
+               fColorType != other.fColorType || fAlphaType != other.fAlphaType ||
+               fProfileType != other.fProfileType || fColorSpace != other.fColorSpace;
     }
 
     void unflatten(SkReadBuffer&);
@@ -314,17 +327,29 @@ public:
         return rowBytes >= rb;
     }
 
+    void reset() {
+        fWidth = 0;
+        fHeight = 0;
+        fColorType = kUnknown_SkColorType;
+        fAlphaType = kUnknown_SkAlphaType;
+        fProfileType = kLinear_SkColorProfileType;
+        fColorSpace = nullptr;
+    }
+
     SkDEBUGCODE(void validate() const;)
 
 private:
+    sk_sp<SkColorSpace> fColorSpace;
     int                 fWidth;
     int                 fHeight;
     SkColorType         fColorType;
     SkAlphaType         fAlphaType;
     SkColorProfileType  fProfileType;
 
-    SkImageInfo(int width, int height, SkColorType ct, SkAlphaType at, SkColorProfileType pt)
-        : fWidth(width)
+    SkImageInfo(int width, int height, SkColorType ct, SkAlphaType at, SkColorProfileType pt,
+                sk_sp<SkColorSpace> cs)
+        : fColorSpace(std::move(cs))
+        , fWidth(width)
         , fHeight(height)
         , fColorType(ct)
         , fAlphaType(at)