Set SkColorSpace object for PNGs and parse ICC profiles

src/codec/SkPngCodec.cpp

Index: src/codec/SkPngCodec.cpp
diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index f946876af2734dd7f660066d68efaa0e91585363..d3aa5e7af44e928c63312d371db1779e8f6739cb 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -165,6 +165,78 @@ bool SkPngCodec::IsPng(const char* buf, size_t bytesRead) {
     return !png_sig_cmp((png_bytep) buf, (png_size_t)0, bytesRead);
 }
 
+// Returns a colorSpace object that represents any color space information in
+// the encoded data.  If the encoded data contains no color space, this will
+// return NULL.
+SkColorSpace* read_color_space(png_structp png_ptr, png_infop info_ptr) {
+
+    // First check for an ICC profile
+    png_charp name;

[scroggo, 2016/02/24 18:31:36]

Will png_get_iCCP let you pass nullptr for name? It looks like we never use it.

[msarett, 2016/02/29 21:47:54]

We need to pass these in or png_get_iCCP does nothing.  I've added a comment.

+    int compression;

[scroggo, 2016/02/24 18:31:36]

Do we need this variable?

[msarett, 2016/02/29 21:47:55]

We need to pass these in or png_get_iCCP does nothing.  I've added a comment.

+    png_bytep profile;
+    png_uint_32 length;
+    if (PNG_INFO_iCCP == png_get_iCCP(png_ptr, info_ptr, &name, &compression, &profile,
+            &length)) {
+
+        // Compression can only ever be "deflate", so this is uninteresting.

[scroggo, 2016/02/24 18:31:36]

I didn't understand this comment the first time around. It might be clearer as
one of :

"compression"
|compression|
compression    // Note lowercase 'c', so it matches the variable name.

Although if you can do without it, I would say drop the variable entirely.

The real question here is, what does "deflate" mean to us? Is |profile|
compressed? Do we need to decompress it to use it? Or has libpng already done
that for us?

[msarett, 2016/02/29 21:47:55]

libpng has already decompressed the profile for us.  And libpng only supports
inflate/deflate decompression.  I'll improve the comment.

This variable is completely irrelevant, but we need to pass something in.

+        // Could knowing the name of the profile ever be interesting?  Maybe for debugging?

[scroggo, 2016/02/24 18:31:36]

I could see it being useful in theory. No idea how it's used in practice. Might
be more helpful if it's stored on the SkColorSpace?

[msarett, 2016/02/29 21:47:55]

Yes I think that'd be the place to put it.

+        return SkColorSpace::NewICC(profile, length);
+    }
+
+    // Second, check for sRGB.
+    if (png_get_valid(png_ptr, info_ptr, PNG_INFO_sRGB)) {
+
+        // Here we are ignoring the sRGB "intent".  Should we use the "intent"?

[scroggo, 2016/02/24 18:31:36]

FIXME/TODO? What is the "intent"?

[msarett, 2016/02/29 21:47:54]

Done.

+        return SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+    }
+
+    // Next, check for chromaticities.
+    png_fixed_point XYZ[9];
+    SkFloat3x3 toXYZD50;
+    png_fixed_point gamma;
+    SkFloat3 gammas;
+    if (png_get_cHRM_XYZ_fixed(png_ptr, info_ptr, &XYZ[0], &XYZ[1], &XYZ[2], &XYZ[3], &XYZ[4],
+            &XYZ[5], &XYZ[6], &XYZ[7], &XYZ[8])) {
+
+        // This could be wrong.  I don't see any guarantee from the PNG specification that

[scroggo, 2016/02/24 18:31:36]

FIXME? Maybe specify how you came up with this computation?

[msarett, 2016/02/29 21:47:54]

The computation isn't wrong.  What might be wrong is that we are treating
possibly non-D50 XYZ values as if they are D50.

To my knowledge, this is the best we can do.  libpng doesn't to guarantee
anything about "color temperature" D50 vs D65 etc.

+        // these XYZ values will be D50.  In fact, libpng will set its XYZ values to D65
+        // values if it sees an sRGB chunk.

[scroggo, 2016/02/24 18:31:36]

It seems like we'll only reach here if we did not see an sRGB chunk?

[msarett, 2016/02/29 21:47:54]

Yes you're right, the comment is unclear.  I'll rewrite it.

+        for (int i = 0; i < 9; i++) {
+            toXYZD50.fMat[i] = ((float) XYZ[i]) * 0.00001f;
+        }
+
+        if (!(PNG_INFO_gAMA == png_get_gAMA_fixed(png_ptr, info_ptr, &gamma))) {
+            // If the image does not specify gamma, let's choose linear.  Should we default
+            // to sRGB?  Instead of choosing a default, should we just give up on the color
+            // profile?
+            gamma = PNG_GAMMA_LINEAR;
+        }
+        gammas.fVec[0] = gammas.fVec[1] = gammas.fVec[2] = ((float) gamma) * 0.00001f;
+
+        return SkColorSpace::NewRGB(toXYZD50, gammas);
+    }
+
+    // Last, check for gamma.
+    if (PNG_INFO_gAMA == png_get_gAMA_fixed(png_ptr, info_ptr, &gamma)) {
+
+        // Guess a default value for cHRM?  Or should we just give up?
+        // Here we use the identity matrix as a default.

[scroggo, 2016/02/24 18:31:36]

Might want a method on SkFloat3x3 to set to the identity?

[msarett, 2016/02/29 21:47:55]

I'll add a FIXME.  Don't want to do this yet since I'm not sure that identity is
the right default.

[scroggo, 2016/02/29 22:18:49]

sgtm. FWIW, even if it's not the default, we could add a method - not a
constructor. SkMatrix's constructor does not initialize its fields. Clients are
expected to initialize them, possibly by calling reset() which sets to the
identity.

+        memset(toXYZD50.fMat, 0, 9 * sizeof(float));
+        toXYZD50.fMat[0] = toXYZD50.fMat[4] = toXYZD50.fMat[8] = 1.0f;
+
+        // Set the gammas.
+        gammas.fVec[0] = gammas.fVec[1] = gammas.fVec[2] = ((float) gamma) * 0.00001f;
+
+        return SkColorSpace::NewRGB(toXYZD50, gammas);
+    }
+
+    // Finally, what should we do if there is no color space information in the PNG?
+    // The specification says that this indicates "gamma is unknown" and that the
+    // "color is device dependent".  I'm assuming we can represent this with NULL.
+    // But should we guess sRGB?  Are most images intended to be sRGB?
+    return nullptr;
+}
+
 // Reads the header and initializes the output fields, if not NULL.
 //
 // @param stream Input data. Will be read to get enough information to properly
@@ -182,12 +254,17 @@ bool SkPngCodec::IsPng(const char* buf, size_t bytesRead) {
 //      bit depth of the encoded image on success.
 // @param numberPassesPtr Optional output variable. If non-NULL, will be set to
 //      the number_passes of the encoded image on success.
+// @param colorSpace Optional output variable.  If non-NULL, will be set to a
+//      colorSpace object that represents any color space information in the
+//      encoded data.  If the encoded data contains no color space, information
+//      this will be set to NULL.
 // @return true on success, in which case the caller is responsible for calling
 //      png_destroy_read_struct(png_ptrp, info_ptrp).
 //      If it returns false, the passed in fields (except stream) are unchanged.
 static bool read_header(SkStream* stream, SkPngChunkReader* chunkReader,
                         png_structp* png_ptrp, png_infop* info_ptrp,
-                        SkImageInfo* imageInfo, int* bitDepthPtr, int* numberPassesPtr) {
+                        SkImageInfo* imageInfo, int* bitDepthPtr, int* numberPassesPtr,
+                        SkColorSpace** colorSpace) {
     // The image is known to be a PNG. Decode enough to know the SkImageInfo.
     png_structp png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, nullptr,
                                                  sk_error_fn, sk_warning_fn);
@@ -310,6 +387,10 @@ static bool read_header(SkStream* stream, SkPngChunkReader* chunkReader,
         *numberPassesPtr = numberPasses;
     }
 
+    if (colorSpace) {
+        *colorSpace = read_color_space(png_ptr, info_ptr);
+    }
+
     SkColorProfileType profileType = kLinear_SkColorProfileType;
     if (png_get_valid(png_ptr, info_ptr, PNG_INFO_sRGB)) {
         profileType = kSRGB_SkColorProfileType;
@@ -330,8 +411,9 @@ static bool read_header(SkStream* stream, SkPngChunkReader* chunkReader,
 }
 
 SkPngCodec::SkPngCodec(const SkImageInfo& info, SkStream* stream, SkPngChunkReader* chunkReader,
-                       png_structp png_ptr, png_infop info_ptr, int bitDepth, int numberPasses)
-    : INHERITED(info, stream)
+                       png_structp png_ptr, png_infop info_ptr, int bitDepth, int numberPasses,
+                       SkColorSpace* colorSpace)
+    : INHERITED(info, stream, colorSpace)
     , fPngChunkReader(SkSafeRef(chunkReader))
     , fPng_ptr(png_ptr)
     , fInfo_ptr(info_ptr)
@@ -434,7 +516,7 @@ bool SkPngCodec::onRewind() {
     png_structp png_ptr;
     png_infop info_ptr;
     if (!read_header(this->stream(), fPngChunkReader.get(), &png_ptr, &info_ptr,
-                     nullptr, nullptr, nullptr)) {
+                     nullptr, nullptr, nullptr, nullptr)) {

[scroggo, 2016/02/24 18:31:36]

You can leave off this last nullptr if you call read_color_space directly inside
NewFromStream.

[msarett, 2016/02/29 21:47:55]

Done.

         return false;
     }
 
@@ -541,8 +623,9 @@ uint32_t SkPngCodec::onGetFillValue(SkColorType colorType) const {
 class SkPngScanlineDecoder : public SkPngCodec {
 public:
     SkPngScanlineDecoder(const SkImageInfo& srcInfo, SkStream* stream,
-            SkPngChunkReader* chunkReader, png_structp png_ptr, png_infop info_ptr, int bitDepth)
-        : INHERITED(srcInfo, stream, chunkReader, png_ptr, info_ptr, bitDepth, 1)
+            SkPngChunkReader* chunkReader, png_structp png_ptr, png_infop info_ptr, int bitDepth,
+            SkColorSpace* colorSpace)
+        : INHERITED(srcInfo, stream, chunkReader, png_ptr, info_ptr, bitDepth, 1, colorSpace)
         , fSrcRow(nullptr)
     {}
 
@@ -607,8 +690,9 @@ class SkPngInterlacedScanlineDecoder : public SkPngCodec {
 public:
     SkPngInterlacedScanlineDecoder(const SkImageInfo& srcInfo, SkStream* stream,
             SkPngChunkReader* chunkReader, png_structp png_ptr, png_infop info_ptr,
-            int bitDepth, int numberPasses)
-        : INHERITED(srcInfo, stream, chunkReader, png_ptr, info_ptr, bitDepth, numberPasses)
+            int bitDepth, int numberPasses, SkColorSpace* colorSpace)
+        : INHERITED(srcInfo, stream, chunkReader, png_ptr, info_ptr, bitDepth, numberPasses,
+                    colorSpace)
         , fHeight(-1)
         , fCanSkipRewind(false)
     {
@@ -733,17 +817,19 @@ SkCodec* SkPngCodec::NewFromStream(SkStream* stream, SkPngChunkReader* chunkRead
     SkImageInfo imageInfo;
     int bitDepth;
     int numberPasses;
+    SkColorSpace* colorSpace = nullptr;
 
     if (!read_header(stream, chunkReader, &png_ptr, &info_ptr, &imageInfo, &bitDepth,
-                     &numberPasses)) {
+                     &numberPasses, &colorSpace)) {

[scroggo, 2016/02/24 18:31:36]

I think read_color_space looks like a good standalone method. But I don't see
any reason to call it from inside read_header, Instead, I think you should do
the following:

if (!read_header(...)) {
  return nullptr;
}

SkColorSpace* colorSpace = read_color_space(&png_ptr, &info_ptr);

[msarett, 2016/02/29 21:47:54]

Done.

         return nullptr;
     }
 
     if (1 == numberPasses) {
         return new SkPngScanlineDecoder(imageInfo, streamDeleter.detach(), chunkReader,
-                                        png_ptr, info_ptr, bitDepth);
+                                        png_ptr, info_ptr, bitDepth, colorSpace);
     }
 
     return new SkPngInterlacedScanlineDecoder(imageInfo, streamDeleter.detach(), chunkReader,
-                                              png_ptr, info_ptr, bitDepth, numberPasses);
+                                              png_ptr, info_ptr, bitDepth, numberPasses,
+                                              colorSpace);
 }