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..23e205ebb2db463cb9beb8b1b59f807c5eaf5bf5 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -187,7 +187,8 @@ bool SkPngCodec::IsPng(const char* buf, size_t bytesRead) {
 //      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) {

[scroggo, 2016/02/24 13:58:12]

add a comment for colorSpace?

[msarett, 2016/02/24 17:32:35]

Done.

     // 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 +311,76 @@ static bool read_header(SkStream* stream, SkPngChunkReader* chunkReader,
         *numberPassesPtr = numberPasses;
     }
 
+    // iCCP variables

[scroggo, 2016/02/24 13:58:12]

Can you move these variables closer to where they're used? None of these need to
declared outside "if (colorSpace)", and with some rejiggering of your else/if
blocks we can move them even closer.

Taking this a bit further, this method is already very long. Why not make
creating the colorSpace its own method? You will only need to call it at
creation time, not at rewind time. I think it will look something like this:

SkColorSpace* read_color_space(png_ptr, info_ptr) {
  // iCCP variables
  if (PNG_INFO_iCCP == png_get_iCCP(...)) {
    return SkColorSpace::NewICC(...);
  }

  if (png_get_valid(png_ptr, info_ptr, PNG_INFO_sRGB)) {
    return SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
  }

  // cHRM variables
  if (png_get_cHRM_XYZ_fixed(...) {
    // calculations...
    return SkColorSpace::NewRGB(...);
  }

  png_fixed_point gamma;
  if (PNG_INFO_gAMA == png_get_gAMA_fixed(...)) {
    // calculations...
    return SkColorSpace::NewRGB(...);
  }

  return nullptr;

[msarett, 2016/02/24 17:32:35]

Done.

+    png_charp name;
+    int compression;
+    png_bytep profile;
+    png_uint_32 length;
+
+    // cHRM variables
+    png_fixed_point XYZ[9];
+    SkFloat3x3 toXYZD50;
+
+    // gAMA variables
+    png_fixed_point gamma;
+    SkFloat3 gammas;
+
+    if (colorSpace) {
+        // First check for an ICC profile
+        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.
+            // Could knowing the name of the profile ever be interesting?  Maybe for debugging?
+            *colorSpace = SkColorSpace::NewICC(profile, length);
+
+        // Second, check for sRGB.
+        } else if (png_get_valid(png_ptr, info_ptr, PNG_INFO_sRGB)) {
+
+            // Here we are ignoring the sRGB "intent".  Should we use the "intent"?
+            *colorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+
+        // Next, check for chromaticities.
+        } else 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
+            // these XYZ values will be D50.  In fact, libpng will set its XYZ values to D65
+            // values if it sees an sRGB chunk.
+            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 our own default.
+                // What should the default be?  Instead of choosing a default, should
+                // we just give up on the color profile?
+                gamma = 0;

[msarett, 2016/02/24 01:01:11]

I need to figure out the gamma value that means "linear".

+            }
+            gammas.fVec[0] = gammas.fVec[1] = gammas.fVec[2] = ((float) gamma) * 0.00001f;
+
+            *colorSpace = SkColorSpace::NewRGB(toXYZD50, gammas);
+
+        // Last, check for gamma.
+        } else 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?
+            for (int i = 0; i < 9; i++) {
+                toXYZD50.fMat[i] = 0.0f;

[msarett, 2016/02/24 01:01:11]

I need to figure out a default for cHRM.

+            }
+
+            // Set the gammas.
+            gammas.fVec[0] = gammas.fVec[1] = gammas.fVec[2] = ((float) gamma) * 0.00001f;
+
+            *colorSpace = 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?

[scroggo, 2016/02/24 13:58:12]

According to the W3C, "untagged" images should be treated as sRGB. I'm guessing
we can leave that decision up to the client, though. Not sure what Android wants
by default.

[msarett, 2016/02/24 17:32:35]

I'm leaving this as is, in case we want to treat the absence of a color profile
different than explicitly sRGB.  But we could easily default to sRGB.

+    }
+
     SkColorProfileType profileType = kLinear_SkColorProfileType;
     if (png_get_valid(png_ptr, info_ptr, PNG_INFO_sRGB)) {
         profileType = kSRGB_SkColorProfileType;
@@ -330,8 +401,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 +506,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)) {
         return false;
     }
 
@@ -541,8 +613,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 +680,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 +807,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)) {
         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);
 }