SkColorSpace::NewICC() warn instead of fail on bad input

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 "SkColorSpace.h"
 #include "SkColorSpace_Base.h"
 #include "SkEndian.h"
@@ skipping 183 matching lines @@
 }
 
 // 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 constexpr size_t kICCHeaderSize = 132;
 
 // Contains a signature (4), offset (4), and size (4).
 static constexpr size_t kICCTagTableEntrySize = 12;
 
-static constexpr uint32_t kRGB_ColorSpace  = SkSetFourByteTag('R', 'G', 'B', ' ');
-static constexpr uint32_t kDisplay_Profile = SkSetFourByteTag('m', 'n', 't', 'r');
-static constexpr uint32_t kInput_Profile   = SkSetFourByteTag('s', 'c', 'n', 'r');
-static constexpr uint32_t kOutput_Profile  = SkSetFourByteTag('p', 'r', 't', 'r');
-static constexpr uint32_t kXYZ_PCSSpace    = SkSetFourByteTag('X', 'Y', 'Z', ' ');
-static constexpr uint32_t kACSP_Signature  = SkSetFourByteTag('a', 'c', 's', 'p');
+static constexpr uint32_t kRGB_ColorSpace     = SkSetFourByteTag('R', 'G', 'B', ' ');
+static constexpr uint32_t kDisplay_Profile    = SkSetFourByteTag('m', 'n', 't', 'r');
+static constexpr uint32_t kInput_Profile      = SkSetFourByteTag('s', 'c', 'n', 'r');
+static constexpr uint32_t kOutput_Profile     = SkSetFourByteTag('p', 'r', 't', 'r');
+static constexpr uint32_t kColorSpace_Profile = SkSetFourByteTag('s', 'p', 'a', 'c');
+static constexpr uint32_t kXYZ_PCSSpace       = SkSetFourByteTag('X', 'Y', 'Z', ' ');
+static constexpr uint32_t kACSP_Signature     = SkSetFourByteTag('a', 'c', 's', 'p');
 
 struct ICCProfileHeader {
     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 fProfileClass;
@@ skipping 42 matching lines @@
             dst[i] = read_big_endian_uint(src);
         }
     }
 
     bool valid() const {
         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
+        // These are the four basic classes of profiles that we might expect to see embedded
+        // in images.  Additional classes exist, but they generally are used as a convenient
         // way for CMMs to store calculated transforms.
         return_if_false(fProfileClass == kDisplay_Profile ||
                         fProfileClass == kInput_Profile ||
-                        fProfileClass == kOutput_Profile,
+                        fProfileClass == kOutput_Profile ||
+                        fProfileClass == kColorSpace_Profile,
                         "Unsupported profile");
 
         // TODO (msarett):
         // 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.
         return_if_false(fPCS == kXYZ_PCSSpace, "Unsupported PCS space");
 
         return_if_false(fSignature == kACSP_Signature, "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");
+        if (fRenderingIntent <= 3) {
+            // I think it makes to warn rather than fail here.  I've seen a
+            // few profiles that specify 0x01000000 as the rendering intent.
+            // It's likely that they're just confused about endianness.

[reed1, 2016/06/25 01:16:37]

What do other parsers do on these? Can you tell what Apple or Adobe do?

[msarett, 2016/07/14 13:45:04]

I'm fairly confident that this is the right decision.  My test images have
perfectly normal sRGB profiles (minus the weird rendering intent).

Safari treats these images as sRGB (but that's not too informative because
Safari also treats unmarked images as sRGB).

Linux command line tool (exiftool) parses the profile just fine.

I'm working on getting Adobe Photoshop (finally) but I don't think it'll change
my opinion.

+            SkColorSpacePrintf("Warning, bad rendering intent.\n");
+        }
 
         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");
 
         return_if_false(fTagCount <= 100, "Too many tags");
 
         return true;
     }
@@ skipping 1045 matching lines @@
 
     uint32_t profileSize = *((uint32_t*) data);
     data = SkTAddOffset<const void>(data, sizeof(uint32_t));
     length -= sizeof(uint32_t);
     if (length < profileSize) {
         return nullptr;
     }
 
     return NewICC(data, profileSize);
 }