Set SkColorSpace object for PNGs and parse ICC profiles

src/codec/SkPngCodec.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkColorTable.h"
@@ skipping 169 matching lines @@
 //      reflect the properties of the encoded image on success.
 // @param bitDepthPtr Optional output variable. If non-NULL, will be set to the
 //      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.
 // @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) {

[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);
     if (!png_ptr) {
         return false;
     }
 
     AutoCleanPng autoClean(png_ptr);
 
     png_infop info_ptr = png_create_info_struct(png_ptr);
@@ skipping 102 matching lines @@
         default:
             // All the color types have been covered above.
             SkASSERT(false);
     }
 
     int numberPasses = png_set_interlace_handling(png_ptr);
     if (numberPassesPtr) {
         *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;
     }
 
     if (imageInfo) {
         *imageInfo = SkImageInfo::Make(origWidth, origHeight, colorType, alphaType, profileType);
     }
     autoClean.detach();
     if (png_ptrp) {
         *png_ptrp = png_ptr;
     }
     if (info_ptrp) {
         *info_ptrp = info_ptr;
     }
 
     return true;
 }
 
 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)
     , fSrcConfig(SkSwizzler::kUnknown)
     , fNumberPasses(numberPasses)
     , fBitDepth(bitDepth)
 {}
 
 SkPngCodec::~SkPngCodec() {
     this->destroyReadStruct();
@@ skipping 82 matching lines @@
     // This sets fPng_ptr and fInfo_ptr to nullptr. If read_header
     // succeeds, they will be repopulated, and if it fails, they will
     // remain nullptr. Any future accesses to fPng_ptr and fInfo_ptr will
     // come through this function which will rewind and again attempt
     // to reinitialize them.
     this->destroyReadStruct();
 
     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;
     }
 
     fPng_ptr = png_ptr;
     fInfo_ptr = info_ptr;
     return true;
 }
 
 SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst,
                                         size_t dstRowBytes, const Options& options,
@@ skipping 86 matching lines @@
     if (colorPtr) {
         return get_color_table_fill_value(colorType, colorPtr, 0);
     }
     return INHERITED::onGetFillValue(colorType);
 }
 
 // Subclass of SkPngCodec which supports scanline decoding
 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)
     {}
 
     Result onStartScanlineDecode(const SkImageInfo& dstInfo, const Options& options,
             SkPMColor ctable[], int* ctableCount) override {
         if (!conversion_possible(dstInfo, this->getInfo())) {
             return kInvalidConversion;
         }
 
         const Result result = this->initializeSwizzler(dstInfo, options, ctable,
@@ skipping 44 matching lines @@
     uint8_t*                    fSrcRow;
 
     typedef SkPngCodec INHERITED;
 };
 
 
 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)
     {
         SkASSERT(numberPasses != 1);
     }
 
     Result onStartScanlineDecode(const SkImageInfo& dstInfo, const Options& options,
             SkPMColor ctable[], int* ctableCount) override {
         if (!conversion_possible(dstInfo, this->getInfo())) {
             return kInvalidConversion;
@@ skipping 104 matching lines @@
     typedef SkPngCodec INHERITED;
 };
 
 SkCodec* SkPngCodec::NewFromStream(SkStream* stream, SkPngChunkReader* chunkReader) {
     SkAutoTDelete<SkStream> streamDeleter(stream);
     png_structp png_ptr;
     png_infop info_ptr;
     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);
 }