Add comments and sanity checks to parsing of ICC headers

src/core/SkColorSpace.cpp

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkAtomics.h"
 #include "SkColorSpace.h"
 
@@ skipping 164 matching lines @@
 #define return_null(msg)                                             \
     do {                                                             \
         SkColorSpacePrintf("Invalid ICC Profile: %s.\n", (msg));     \
         return nullptr;                                              \
     } while (0)
 
 static uint16_t read_big_endian_short(const uint8_t* ptr) {
     return ptr[0] << 8 | ptr[1];
 }
 
-static uint32_t read_big_endian_int(const uint8_t* ptr) {
+static uint32_t read_big_endian_uint(const uint8_t* ptr) {
     return ptr[0] << 24 | ptr[1] << 16 | ptr[2] << 8 | ptr[3];
 }
 
+static int32_t read_big_endian_int(const uint8_t* ptr) {
+    return (int32_t) read_big_endian_uint(ptr);
+}
+
+static bool color_space_almost_equal(float a, float b) {
+    return SkTAbs(a - b) < 0.01f;
+}
+
 // This is equal to the header size according to the ICC specification (128)
 // plus the size of the tag count (4).  We include the tag count since we
 // always require it to be present anyway.
 static const size_t kICCHeaderSize = 132;
 
 // Contains a signature (4), offset (4), and size (4).
 static const size_t kICCTagTableEntrySize = 12;
 
 static const uint32_t kRGB_ColorSpace  = SkSetFourByteTag('R', 'G', 'B', ' ');
-static const uint32_t kGray_ColorSpace = SkSetFourByteTag('G', 'R', 'A', 'Y');
 
 struct ICCProfileHeader {
-    // TODO (msarett):
-    // Can we ignore less of these fields?
     uint32_t fSize;
+
+    // No reason to care about the preferred color management module (ex: Adobe, Apple, etc.).
+    // We're always going to use this one.
     uint32_t fCMMType_ignored;
+
     uint32_t fVersion;
-    uint32_t fClassProfile;
-    uint32_t fColorSpace;
+    uint32_t fProfileClass;
+    uint32_t fInputColorSpace;
     uint32_t fPCS;
     uint32_t fDateTime_ignored[3];
     uint32_t fSignature;
+
+    // Indicates the platform that this profile was created for (ex: Apple, Microsoft).  This
+    // doesn't really matter to us.
     uint32_t fPlatformTarget_ignored;
+
+    // Flags can indicate:
+    // (1) Whether this profile was embedded in a file.  This flag is consistently wrong.
+    //     Ex: The profile came from a file but indicates that it did not.
+    // (2) Whether we are allowed to use the profile independently of the color data.  If set,
+    //     this may allow us to use the embedded profile for testing separate from the original
+    //     image.
     uint32_t fFlags_ignored;
-    uint32_t fManufacturer_ignored;
+
+    // We support many output devices.  It doesn't make sense to think about the attributes of
+    // the device in the context of the image profile.
+    uint32_t fDeviceManufacturer_ignored;
     uint32_t fDeviceModel_ignored;
     uint32_t fDeviceAttributes_ignored[2];
+
     uint32_t fRenderingIntent;
-    uint32_t fIlluminantXYZ_ignored[3];
+    int32_t  fIlluminantXYZ[3];
+
+    // We don't care who created the profile.
     uint32_t fCreator_ignored;
+
+    // This is an MD5 checksum.  Could be useful for checking if profiles are equal.
     uint32_t fProfileId_ignored[4];
+
+    // Reserved for future use.
     uint32_t fReserved_ignored[7];
+
     uint32_t fTagCount;
 
     void init(const uint8_t* src, size_t len) {
         SkASSERT(kICCHeaderSize == sizeof(*this));
 
         uint32_t* dst = (uint32_t*) this;
         for (uint32_t i = 0; i < kICCHeaderSize / 4; i++, src+=4) {
-            dst[i] = read_big_endian_int(src);
+            dst[i] = read_big_endian_uint(src);
         }
     }
 
     bool valid() const {
-        // TODO (msarett):
-        // For now it's nice to fail loudly on invalid inputs.  But, can we
-        // recover from some of these errors?
-
         return_if_false(fSize >= kICCHeaderSize, "Size is too small");
 
         uint8_t majorVersion = fVersion >> 24;
         return_if_false(majorVersion <= 4, "Unsupported version");
 
+        // These are the three basic classes of profiles that we might expect to see embedded
+        // in images.  Four additional classes exist, but they generally are used as a convenient
+        // way for CMMs to store calculated transforms.
         const uint32_t kDisplay_Profile = SkSetFourByteTag('m', 'n', 't', 'r');
         const uint32_t kInput_Profile   = SkSetFourByteTag('s', 'c', 'n', 'r');
         const uint32_t kOutput_Profile  = SkSetFourByteTag('p', 'r', 't', 'r');
-        // TODO (msarett):
-        // Should we also support DeviceLink, ColorSpace, Abstract, or NamedColor?
-        return_if_false(fClassProfile == kDisplay_Profile ||
-                        fClassProfile == kInput_Profile ||
-                        fClassProfile == kOutput_Profile,
-                        "Unsupported class profile");
+        return_if_false(fProfileClass == kDisplay_Profile ||
+                        fProfileClass == kInput_Profile ||
+                        fProfileClass == kOutput_Profile,
+                        "Unsupported profile");
 
         // TODO (msarett):
-        // There are many more color spaces that we could try to support.
-        return_if_false(fColorSpace == kRGB_ColorSpace || fColorSpace == kGray_ColorSpace,
-                        "Unsupported color space");
+        // All the profiles we've tested so far use RGB as the input color space.
+        return_if_false(fInputColorSpace == kRGB_ColorSpace, "Unsupported color space");
 
+        // TODO (msarett):
+        // All the profiles we've tested so far use XYZ as the profile connection space.
         const uint32_t kXYZ_PCSSpace = SkSetFourByteTag('X', 'Y', 'Z', ' ');
-        // TODO (msarett):
-        // Can we support PCS LAB as well?
         return_if_false(fPCS == kXYZ_PCSSpace, "Unsupported PCS space");
 
         return_if_false(fSignature == SkSetFourByteTag('a', 'c', 's', 'p'), "Bad signature");
 
         // TODO (msarett):
         // Should we treat different rendering intents differently?
         // Valid rendering intents include kPerceptual (0), kRelative (1),
         // kSaturation (2), and kAbsolute (3).
         return_if_false(fRenderingIntent <= 3, "Bad rendering intent");
 
+        return_if_false(color_space_almost_equal(SkFixedToFloat(fIlluminantXYZ[0]), 0.96420f) &&
+                        color_space_almost_equal(SkFixedToFloat(fIlluminantXYZ[1]), 1.00000f) &&
+                        color_space_almost_equal(SkFixedToFloat(fIlluminantXYZ[2]), 0.82491f),
+                        "Illuminant must be D50");

[scroggo, 2016/04/27 11:55:44]

Was this intended to be more flexible in future versions?

[msarett, 2016/04/27 13:11:19]

I don't know...  It doesn't look like they are headed that way though.  In v4,
they also started requiring that the white point tag matches the D50 illuminant.
 All the sources I've seen say "ICC always means D50".

+
         return_if_false(fTagCount <= 100, "Too many tags");
 
         return true;
     }
 };
 
 struct ICCTag {
     uint32_t fSignature;
     uint32_t fOffset;
     uint32_t fLength;
 
     const uint8_t* init(const uint8_t* src) {
-        fSignature = read_big_endian_int(src);
-        fOffset = read_big_endian_int(src + 4);
-        fLength = read_big_endian_int(src + 8);
+        fSignature = read_big_endian_uint(src);
+        fOffset = read_big_endian_uint(src + 4);
+        fLength = read_big_endian_uint(src + 8);
         return src + 12;
     }
 
     bool valid(size_t len) {
         return_if_false(fOffset + fLength <= len, "Tag too large for ICC profile");
         return true;
     }
 
     const uint8_t* addr(const uint8_t* src) const {
         return src + fOffset;
@@ skipping 33 matching lines @@
 
 static const uint32_t kTAG_CurveType     = SkSetFourByteTag('c', 'u', 'r', 'v');
 static const uint32_t kTAG_ParaCurveType = SkSetFourByteTag('p', 'a', 'r', 'a');
 
 static bool load_gamma(float* gamma, const uint8_t* src, size_t len) {
     if (len < 14) {
         SkColorSpacePrintf("gamma tag is too small (%d bytes)", len);
         return false;
     }
 
-    uint32_t type = read_big_endian_int(src);
+    uint32_t type = read_big_endian_uint(src);
     switch (type) {
         case kTAG_CurveType: {
             uint32_t count = read_big_endian_int(src + 8);
             if (0 == count) {
                 return false;
             }
 
             const uint16_t* table = (const uint16_t*) (src + 12);
             if (1 == count) {
                 // Table entry is the exponent (bias 256).
@@ skipping 68 matching lines @@
                 (tags[i].fSignature >>  0) & 0xFF, tags[i].fOffset, tags[i].fLength);
 
         if (!tags[i].valid(kICCHeaderSize + len)) {
             return_null("Tag is too large to fit in ICC profile");
         }
     }
 
     // Load our XYZ and gamma matrices.
     SkFloat3x3 toXYZ;
     SkFloat3 gamma {{ 1.0f, 1.0f, 1.0f }};
-    switch (header.fColorSpace) {
+    switch (header.fInputColorSpace) {
         case kRGB_ColorSpace: {
             const ICCTag* r = ICCTag::Find(tags.get(), tagCount, kTAG_rXYZ);
             const ICCTag* g = ICCTag::Find(tags.get(), tagCount, kTAG_gXYZ);
             const ICCTag* b = ICCTag::Find(tags.get(), tagCount, kTAG_bXYZ);
             if (!r || !g || !b) {
                 return_null("Need rgb tags for XYZ space");
             }
 
             if (!load_xyz(&toXYZ.fMat[0], r->addr((const uint8_t*) base), r->fLength) ||
                 !load_xyz(&toXYZ.fMat[3], g->addr((const uint8_t*) base), g->fLength) ||
@@ skipping 103 matching lines @@
 }
 
 // D65 white point of Rec.  709 [8] are:
 //
 // D65 white-point in unit luminance XYZ = 0.9505, 1.0000, 1.0890
 //
 //          R           G           B           white
 //   x      0.640       0.300       0.150       0.3127
 //   y      0.330       0.600       0.060       0.3290
 //   z      0.030       0.100       0.790       0.3582