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 147 matching lines @@
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Creation
 ///////////////////////////////////////////////////////////////////////////////
 
 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
 //      setup the codec.
 // @param chunkReader SkPngChunkReader, for reading unknown chunks. May be NULL.
 //      If not NULL, png_ptr will hold an *unowned* pointer to it. The caller is
 //      expected to continue to own it for the lifetime of the png_ptr.
 // @param png_ptrp Optional output variable. If non-NULL, will be set to a new
 //      png_structp on success.
 // @param info_ptrp Optional output variable. If non-NULL, will be set to a new
 //      png_infop on success;
 // @param imageInfo Optional output variable. If non-NULL, will be set to
 //      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.
+// @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);
     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;
     }
 
+    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;
     }
 
     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)) {

[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;
     }
 
     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)) {

[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);
 }