Add SkColorSpace to SkImageInfo

include/core/SkImageInfo.h

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkImageInfo_DEFINED
 #define SkImageInfo_DEFINED
 
+#include "SkColorSpace.h"
 #include "SkMath.h"
 #include "SkRect.h"
 #include "SkSize.h"
 
 class SkReadBuffer;
 class SkWriteBuffer;
 
 /**
  *  Describes how to interpret the alpha component of a pixel.
  */
@@ skipping 154 matching lines @@
     kLastEnum_SkColorProfileType = kSRGB_SkColorProfileType
 };
 
 /**
  *  Describe an image's dimensions and pixel type.
  *  Used for both src images and render-targets (surfaces).
  */
 struct SK_API SkImageInfo {
 public:
     SkImageInfo()
-        : fWidth(0)
+        : fColorSpace(nullptr)
+        , fWidth(0)
         , fHeight(0)
         , fColorType(kUnknown_SkColorType)
         , fAlphaType(kUnknown_SkAlphaType)
         , fProfileType(kLinear_SkColorProfileType)
     {}
 
     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);
     }
 
     /**
      *  Sets colortype to the native ARGB32 type, and the alphatype to premul.
      */
     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);
     }
 
     /**
      *  Sets colortype to the native ARGB32 type, and the alphatype to premul.
      */
     static SkImageInfo MakeN32Premul(const SkISize& size,
                                      SkColorProfileType pt = kLinear_SkColorProfileType) {
         return MakeN32Premul(size.width(), size.height(), pt);
     }
 
     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() {
         return SkImageInfo();
     }
 
     int width() const { return fWidth; }
     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; }
 
     bool isOpaque() const {
         return SkAlphaTypeIsOpaque(fAlphaType);
     }
 
+    // Deprecated
     bool isLinear() const { return kLinear_SkColorProfileType == fProfileType; }
     bool isSRGB() const { return kSRGB_SkColorProfileType == fProfileType; }
 
     SkISize dimensions() const { return SkISize::Make(fWidth, fHeight); }
     SkIRect bounds() const { return SkIRect::MakeWH(fWidth, fHeight); }
 
     /**
      *  Return a new ImageInfo with the same colortype and alphatype as this info,
      *  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); }
 
     int shiftPerPixel() const { return SkColorTypeShiftPerPixel(fColorType); }
 
     uint64_t minRowBytes64() const {
         return sk_64_mul(fWidth, this->bytesPerPixel());
     }
 
     size_t minRowBytes() const {
         return (size_t)this->minRowBytes64();
     }
 
     size_t computeOffset(int x, int y, size_t rowBytes) const {
         SkASSERT((unsigned)x < (unsigned)fWidth);
         SkASSERT((unsigned)y < (unsigned)fHeight);
         return SkColorTypeComputeOffset(fColorType, x, y, rowBytes);
     }
 
     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&);
     void flatten(SkWriteBuffer&) const;
 
     int64_t getSafeSize64(size_t rowBytes) const {
         if (0 == fHeight) {
             return 0;
         }
         return sk_64_mul(fHeight - 1, rowBytes) + fWidth * this->bytesPerPixel();
     }
 
     size_t getSafeSize(size_t rowBytes) const {
         int64_t size = this->getSafeSize64(rowBytes);
         if (!sk_64_isS32(size)) {
             return 0;
         }
         return sk_64_asS32(size);
     }
 
     bool validRowBytes(size_t rowBytes) const {
         uint64_t rb = sk_64_mul(fWidth, this->bytesPerPixel());
         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)
         , fProfileType(pt)
     {}
 };
 
 ///////////////////////////////////////////////////////////////////////////////
 
 static inline bool SkColorAndProfileAreGammaCorrect(SkColorType ct, SkColorProfileType pt) {
     return kSRGB_SkColorProfileType == pt || kRGBA_F16_SkColorType == ct;
 }
 
 static inline bool SkImageInfoIsGammaCorrect(const SkImageInfo& info) {
     return SkColorAndProfileAreGammaCorrect(info.colorType(), info.profileType());
 }
 
 #endif