Color-correct PNG images that have an sRGB chunk

Source/platform/image-decoders/png/PNGImageDecoder.cpp

Index: Source/platform/image-decoders/png/PNGImageDecoder.cpp
diff --git a/Source/platform/image-decoders/png/PNGImageDecoder.cpp b/Source/platform/image-decoders/png/PNGImageDecoder.cpp
index ad2cae077181335926b77fbfe57f3c7dee3ad011..cb949e16af3366bd6b0b61552b17b3bb4ddcab20 100644
--- a/Source/platform/image-decoders/png/PNGImageDecoder.cpp
+++ b/Source/platform/image-decoders/png/PNGImageDecoder.cpp
@@ -189,16 +189,20 @@ public:
         m_transform = 0;
     }
 
-    void createColorTransform(const ColorProfile& colorProfile, bool hasAlpha)
+    void createColorTransform(const ColorProfile& colorProfile, bool hasAlpha, bool sRGB)
     {
         clearColorTransform();
 
-        if (colorProfile.isEmpty())
+        if (colorProfile.isEmpty() && !sRGB)
             return;
         qcms_profile* deviceProfile = ImageDecoder::qcmsOutputDeviceProfile();
         if (!deviceProfile)
             return;
-        qcms_profile* inputProfile = qcms_profile_from_memory(colorProfile.data(), colorProfile.size());
+        qcms_profile* inputProfile = 0;
+        if (!colorProfile.isEmpty())

[Ken Russell (switch to Gerrit), 2014/12/03 23:50:02]

I think this should read "if (sRGB) { ... } else {
ASSERT(!colorProfile.isEmpty()); ... }" so that sRGB is preferred over other
color profiles that might be embedded in the file, per the PNG spec.

[Noel Gordon, 2014/12/05 04:12:43]

I think the spec says that a PNG should have an sRGB or an iCCP, but not both. 
Doesn't say how to tie-break, so interesting point.

The preference selection is done in readColorProfile, so if I reading that you'd
prefer sRGB, I can add it there.

Note: mozilla prefers iCCP over sRGB [1].  I don't think it matters though. 
Thoughts?

http://lxr.mozilla.org/mozilla-central/source/image/decoders/nsPNGDecoder.cpp...

[Ken Russell (switch to Gerrit), 2014/12/05 19:24:53]

I see. Thanks for clarifying that. This looks fine then.

+            inputProfile = qcms_profile_from_memory(colorProfile.data(), colorProfile.size());
+        else if (sRGB)
+            inputProfile = qcms_profile_sRGB();
         if (!inputProfile)
             return;
         // We currently only support color profiles for RGB and RGBA images.
@@ -276,7 +280,7 @@ bool PNGImageDecoder::setFailed()
 }
 
 #if USE(QCMSLIB)
-static void readColorProfile(png_structp png, png_infop info, ColorProfile& colorProfile)
+static void readColorProfile(png_structp png, png_infop info, ColorProfile& colorProfile, bool& sRGB)

[Sami, 2014/12/04 12:51:23]

Generally output parameters should be pointers rather than references.

[Noel Gordon, 2014/12/05 04:12:43]

Blink style is that output parameters are references, no?

[Sami, 2014/12/05 11:50:43]

Ah, sorry, I've spent too much time in Chromium land recently :)

 {
 #ifdef PNG_iCCP_SUPPORTED
     char* profileName;
@@ -287,8 +291,15 @@ static void readColorProfile(png_structp png, png_infop info, ColorProfile& colo
     png_bytep profile;
 #endif
     png_uint_32 profileLength;
-    if (!png_get_iCCP(png, info, &profileName, &compressionType, &profile, &profileLength))
+    if (!png_get_iCCP(png, info, &profileName, &compressionType, &profile, &profileLength)) {
+        ASSERT(!png_get_valid(png, info, PNG_INFO_iCCP));
+        // No color profile: check the sRGB chunk.
+        // FIXME: but only if no cHRM or gAMA chunk was seen?
+        // if (info->chroma_or_gamma & (PNG_INFO_cHRM | PNG_INFO_gAMA))
+        //    return;
+        sRGB = png_get_valid(png, info, PNG_INFO_sRGB);

[Sami, 2014/12/04 12:51:23]

Could we instead encode this information in |colorProfile| instead of adding a
redundant-seeming flag?

[Noel Gordon, 2014/12/05 04:12:43]

Not easily Sami. ColorProfile is not a class, it's a typedef to WTF::Vector.

[Sami, 2014/12/05 11:50:43]

Right, I was thinking it was a string identifier but looks like it's just a blob
of data. Nevermind then.

         return;
+    }
 
     // Only accept RGB color profiles from input class devices.
     bool ignoreProfile = false;
@@ -363,10 +374,11 @@ void PNGImageDecoder::headerAvailable()
         // do not similarly transform the color profile. We'd either need to transform
         // the color profile or we'd need to decode into a gray-scale image buffer and
         // hand that to CoreGraphics.
+        bool sRGB = false;
         ColorProfile colorProfile;
-        readColorProfile(png, info, colorProfile);
+        readColorProfile(png, info, colorProfile, sRGB);
         bool decodedImageHasAlpha = (colorType & PNG_COLOR_MASK_ALPHA) || trnsCount;
-        m_reader->createColorTransform(colorProfile, decodedImageHasAlpha);
+        m_reader->createColorTransform(colorProfile, decodedImageHasAlpha, sRGB);
         m_hasColorProfile = !!m_reader->colorTransform();
     }
 #endif