#386 Fixed ColorSpaces ICC profile caching bug

imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/ColorSpaces.java

@@ -40,6 +40,8 @@ import java.awt.color.ICC_Profile;
 import java.io.IOException;
 import java.io.InputStream;
 import java.lang.ref.WeakReference;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 import java.util.Arrays;
 import java.util.Map;
 import java.util.Properties;
@@ -106,34 +108,55 @@ public final class ColorSpaces {
     public static ICC_ColorSpace createColorSpace(final ICC_Profile profile) {
         Validate.notNull(profile, "profile");
 
-        byte[] profileHeader = profile.getData(ICC_Profile.icSigHead);
+        // Fix profile before lookup/create
+        profileCleaner.fixProfile(profile);
+
+        byte[] profileHeader = getProfileHeaderWithProfileId(profile);
 
         ICC_ColorSpace cs = getInternalCS(profile.getColorSpaceType(), profileHeader);
         if (cs != null) {
             return cs;
         }
 
-        // Special case for color profiles with rendering intent != 0, see isOffendingColorProfile method
-        // NOTE: Rendering intent is really a 4 byte value, but legal values are 0-3 (ICC1v42_2006_05_1.pdf, 7.2.15, p. 19)
-        if (profileHeader[ICC_Profile.icHdrRenderingIntent] != 0) {
-            profileHeader[ICC_Profile.icHdrRenderingIntent] = 0;
-
-            // Test again if this is an internal CS
-            cs = getInternalCS(profile.getColorSpaceType(), profileHeader);
-            if (cs != null) {
-                return cs;
-            }
-
-            // Fix profile before lookup/create
-            profileCleaner.fixProfile(profile, profileHeader);
-        }
-        else {
-            profileCleaner.fixProfile(profile, null);
-        }
-
         return getCachedOrCreateCS(profile, profileHeader);
     }
 
+    private static byte[] getProfileHeaderWithProfileId(final ICC_Profile profile) {
+        byte[] header = profile.getData(ICC_Profile.icSigHead);
+
+        computeProfileIdMD5(profile, header);
+
+        return header;
+    }
+
+    private static void computeProfileIdMD5(final ICC_Profile profile, final byte[] header) {
+        // Clear out preferred CMM, platform & creator, as these does not affect the profile in any way
+        // - LCMS updates CMM + creator to "lcms" and platform to current platform
+        // - KCMS keeps the values in the file...
+        Arrays.fill(header, ICC_Profile.icHdrCmmId, ICC_Profile.icHdrCmmId + 4, (byte) 0);
+        Arrays.fill(header, ICC_Profile.icHdrPlatform, ICC_Profile.icHdrPlatform + 4, (byte) 0);
+        // + Clear out rendering intent, as this may be updated by application
+        Arrays.fill(header, ICC_Profile.icHdrRenderingIntent, ICC_Profile.icHdrRenderingIntent + 4, (byte) 0);
+        Arrays.fill(header, ICC_Profile.icHdrCreator, ICC_Profile.icHdrCreator + 4, (byte) 0);
+
+        // Clear out any existing MD5, as it is no longer correct
+        Arrays.fill(header, ICC_Profile.icHdrProfileID, ICC_Profile.icHdrProfileID + 16, (byte) 0);
+
+        // Get *entire profile data*... :-/
+        byte[] data = profile.getData();
+
+        // Update with the new header data
+        System.arraycopy(header, 0, data, 0, header.length);
+
+        // Generate new MD5 and store in header
+        try {
+            byte[] md5 = MessageDigest.getInstance("MD5").digest(data);
+            System.arraycopy(md5, 0, header, ICC_Profile.icHdrProfileID, md5.length);
+        } catch (NoSuchAlgorithmException e) {
+            throw new IllegalStateException("Missing MD5 MessageDigest");
+        }
+    }
+
     private static ICC_ColorSpace getInternalCS(final int profileCSType, final byte[] profileHeader) {
         if (profileCSType == ColorSpace.TYPE_RGB && Arrays.equals(profileHeader, sRGB.header)) {
             return (ICC_ColorSpace) ColorSpace.getInstance(ColorSpace.CS_sRGB);
@@ -186,7 +209,7 @@ public final class ColorSpaces {
     public static boolean isCS_sRGB(final ICC_Profile profile) {
         Validate.notNull(profile, "profile");
 
-        return profile.getColorSpaceType() == ColorSpace.TYPE_RGB && Arrays.equals(profile.getData(ICC_Profile.icSigHead), sRGB.header);
+        return profile.getColorSpaceType() == ColorSpace.TYPE_RGB && Arrays.equals(getProfileHeaderWithProfileId(profile), sRGB.header);
     }
 
     /**
@@ -214,10 +237,11 @@ public final class ColorSpaces {
 
         // This is particularly annoying, as the byte copying isn't really necessary,
         // except the getRenderingIntent method is package protected in java.awt.color
-        byte[] data = profile.getData(ICC_Profile.icSigHead);
+        byte[] header = profile.getData(ICC_Profile.icSigHead);
 
-        return data[ICC_Profile.icHdrRenderingIntent] != 0;
+        return header[ICC_Profile.icHdrRenderingIntent] != 0 || header[ICC_Profile.icHdrRenderingIntent + 1] != 0
-    }
+                || header[ICC_Profile.icHdrRenderingIntent + 2] != 0 || header[ICC_Profile.icHdrRenderingIntent + 3] != 0;
+        }
 
     /**
      * Tests whether an ICC color profile is valid.
@@ -275,6 +299,10 @@ public final class ColorSpaces {
                             }
                         }
 
+                        if (profile.getColorSpaceType() != ColorSpace.TYPE_RGB) {
+                            throw new IllegalStateException("Configured AdobeRGB1998 profile is not TYPE_RGB");
+                        }
+
                         adobeRGB1998 = new WeakReference<>(profile);
                     }
                 }
@@ -298,6 +326,10 @@ public final class ColorSpaces {
                             return CMYKColorSpace.getInstance();
                         }
 
+                        if (profile.getColorSpaceType() != ColorSpace.TYPE_CMYK) {
+                            throw new IllegalStateException("Configured Generic CMYK profile is not TYPE_CMYK");
+                        }
+
                         genericCMYK = new WeakReference<>(profile);
                     }
                 }
@@ -356,7 +388,7 @@ public final class ColorSpaces {
     private static final class Key {
         private final byte[] data;
 
-        public Key(byte[] data) {
+        Key(byte[] data) {
             this.data = data;
         }
 
@@ -373,26 +405,27 @@ public final class ColorSpaces {
 
     // Cache header profile data to avoid excessive array creation/copying in static inner class for on-demand lazy init
     private static class sRGB {
-        private static final byte[] header = ICC_Profile.getInstance(ColorSpace.CS_sRGB).getData(ICC_Profile.icSigHead);
+        private static final byte[] header = getProfileHeaderWithProfileId(ICC_Profile.getInstance(ColorSpace.CS_sRGB));
     }
 
     private static class CIEXYZ {
-        private static final byte[] header = ICC_Profile.getInstance(ColorSpace.CS_CIEXYZ).getData(ICC_Profile.icSigHead);
+        private static final byte[] header = getProfileHeaderWithProfileId(ICC_Profile.getInstance(ColorSpace.CS_CIEXYZ));
     }
 
     private static class PYCC {
-        private static final byte[] header = ICC_Profile.getInstance(ColorSpace.CS_PYCC).getData(ICC_Profile.icSigHead);
+        private static final byte[] header = getProfileHeaderWithProfileId(ICC_Profile.getInstance(ColorSpace.CS_PYCC));
     }
 
     private static class GRAY {
-        private static final byte[] header = ICC_Profile.getInstance(ColorSpace.CS_GRAY).getData(ICC_Profile.icSigHead);
+        private static final byte[] header = getProfileHeaderWithProfileId(ICC_Profile.getInstance(ColorSpace.CS_GRAY));
     }
 
     private static class LINEAR_RGB {
-        private static final byte[] header = ICC_Profile.getInstance(ColorSpace.CS_LINEAR_RGB).getData(ICC_Profile.icSigHead);
+        private static final byte[] header = getProfileHeaderWithProfileId(ICC_Profile.getInstance(ColorSpace.CS_LINEAR_RGB));
     }
 
     private static class Profiles {
+        // TODO: Honour java.iccprofile.path property?
         private static final Properties PROFILES = loadProfiles();
 
         private static Properties loadProfiles() {
@@ -439,7 +472,7 @@ public final class ColorSpaces {
             return profiles;
         }
 
-        public static String getPath(final String profileName) {
+        static String getPath(final String profileName) {
             return PROFILES.getProperty(profileName);
         }
     }

imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/ICCProfileSanitizer.java

@@ -12,7 +12,7 @@ import java.awt.color.ICC_Profile;
  * @version $Id: ProfileCleaner.java,v 1.0 06/01/15 harald.kuhr Exp$
  */
 interface ICCProfileSanitizer {
-    void fixProfile(ICC_Profile profile, byte[] profileHeader);
+    void fixProfile(ICC_Profile profile);
 
     class Factory {
         static ICCProfileSanitizer get() {
@@ -36,7 +36,7 @@ interface ICCProfileSanitizer {
             // sun.java2d.cmm.CMSManager (using default sun.java2d.cmm=sun.java2d.cmm.kcms.CMM)
             // sun.java2d.cmm.PCMM
             // sun.java2d.cmm.kcms.CMM implements PCMM (similar to Java 6 CMM)
-            // sun.javard.cmm.lcms.LCMS implements PCMM
+            // sun.java2d.cmm.lcms.LCMS implements PCMM
 
             // Java 8:
             // sun.java2d.cmm.CMSManager (using default sun.java2d.cmm=sun.java2d.cmm.lcms.LcmsServiceProvider)

imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/KCMSSanitizerStrategy.java

@@ -17,11 +17,16 @@ final class KCMSSanitizerStrategy implements ICCProfileSanitizer {
     /** Value used instead of 'XYZ ' in problematic Corbis RGB Profiles */
     private static final byte[] CORBIS_RGB_ALTERNATE_XYZ = new byte[] {0x17, (byte) 0xA5, 0x05, (byte) 0xB8};
 
-    public void fixProfile(final ICC_Profile profile, byte[] profileHeader) {
+    public void fixProfile(final ICC_Profile profile) {
         Validate.notNull(profile, "profile");
 
-        if (profileHeader != null) {
+        // Special case for color profiles with rendering intent != 0, see ColorSpaces.isOffendingColorProfile method
-            profile.setData(ICC_Profile.icSigHead, profileHeader);
+        // NOTE: Rendering intent is a 4 byte value, legal values are 0-3 (ICC1v42_2006_05_1.pdf, 7.2.15, p. 19)
+        byte[] header = profile.getData(ICC_Profile.icSigHead);
+        if (header[ICC_Profile.icHdrRenderingIntent] != 0 || header[ICC_Profile.icHdrRenderingIntent + 1] != 0
+                || header[ICC_Profile.icHdrRenderingIntent + 2] != 0 || header[ICC_Profile.icHdrRenderingIntent + 3] != 0) {
+            Arrays.fill(header, ICC_Profile.icHdrRenderingIntent, ICC_Profile.icHdrRenderingIntent + 4, (byte) 0);
+            profile.setData(ICC_Profile.icSigHead, header);
         }
 
         // Special handling to detect problematic Corbis RGB ICC Profile for KCMS.

imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/LCMSSanitizerStrategy.java

@@ -12,7 +12,7 @@ import java.awt.color.ICC_Profile;
  * @version $Id: LCMSProfileCleaner.java,v 1.0 06/01/15 harald.kuhr Exp$
  */
 final class LCMSSanitizerStrategy implements ICCProfileSanitizer {
-    public void fixProfile(final ICC_Profile profile, byte[] profileHeader) {
+    public void fixProfile(final ICC_Profile profile) {
         Validate.notNull(profile, "profile");
         // Let LCMS handle things internally for now
     }

imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/ColorSpacesTest.java

@@ -33,6 +33,7 @@ import org.junit.Test;
 import java.awt.color.ColorSpace;
 import java.awt.color.ICC_ColorSpace;
 import java.awt.color.ICC_Profile;
+import java.io.IOException;
 
 import static org.junit.Assert.*;
 
@@ -82,7 +83,7 @@ public class ColorSpacesTest {
 
     private ICC_Profile createBrokenProfile(ICC_Profile internal) {
         byte[] data = internal.getData();
-        data[ICC_Profile.icHdrRenderingIntent] = 1; // Intent: 1 == Relative Colormetric
+        data[ICC_Profile.icHdrRenderingIntent + 3] = 1; // Intent: 1 == Relative Colormetric
         return ICC_Profile.getInstance(data);
     }
 
@@ -185,4 +186,18 @@ public class ColorSpacesTest {
     public void testIsCS_sRGBNull() {
         ColorSpaces.isCS_sRGB(null);
     }
+
+    @Test
+    public void testEqualHeadersDifferentProfile() throws IOException {
+        // These profiles are extracted from various JPEGs, and have the exact same profile header...
+        ICC_Profile profile1 = ICC_Profile.getInstance(getClass().getResourceAsStream("/profiles/adobe_rgb_1998.icc"));
+        ICC_Profile profile2 = ICC_Profile.getInstance(getClass().getResourceAsStream("/profiles/color_match_rgb.icc"));
+
+        assertNotSame(profile1, profile2); // Sanity
+
+        ICC_ColorSpace cs1 = ColorSpaces.createColorSpace(profile1);
+        ICC_ColorSpace cs2 = ColorSpaces.createColorSpace(profile2);
+
+        assertNotSame(cs1, cs2);
+    }
 }

imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/KCMSSanitizerStrategyTest.java

@@ -9,33 +9,36 @@ import java.io.IOException;
 import java.util.Arrays;
 
 import static org.junit.Assert.assertArrayEquals;
-import static org.mockito.Mockito.mock;
+import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.verify;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.*;
 
 public class KCMSSanitizerStrategyTest {
     private static final byte[] XYZ = new byte[] {'X', 'Y', 'Z', ' '};
 
     @Test(expected = IllegalArgumentException.class)
     public void testFixProfileNullProfile() throws Exception {
-        new KCMSSanitizerStrategy().fixProfile(null, null);
+        new KCMSSanitizerStrategy().fixProfile(null);
     }
 
     @Test
-    public void testFixProfileNullHeader() throws Exception {
+    public void testFixProfile() throws Exception {
-        new KCMSSanitizerStrategy().fixProfile(((ICC_ColorSpace) ColorSpace.getInstance(ColorSpace.CS_sRGB)).getProfile(), null);
+        new KCMSSanitizerStrategy().fixProfile(((ICC_ColorSpace) ColorSpace.getInstance(ColorSpace.CS_sRGB)).getProfile());
     }
 
     @Test
     public void testFixProfileUpdateHeader() throws Exception {
+        byte[] header = new byte[128];
+        header[ICC_Profile.icHdrRenderingIntent + 3] = 1;
         ICC_Profile profile = mock(ICC_Profile.class);
-        byte[] header = new byte[0];
+        when(profile.getData(ICC_Profile.icSigHead)).thenReturn(header);
 
         // Can't test that the values are actually changed, as the LCMS-backed implementation
         // of ICC_Profile does not change based on this invocation.
-        new KCMSSanitizerStrategy().fixProfile(profile, header);
+        new KCMSSanitizerStrategy().fixProfile(profile);
 
         // Verify that the method was invoked
-        verify(profile).setData(ICC_Profile.icSigHead, header);
+        verify(profile).setData(eq(ICC_Profile.icSigHead), any(byte[].class));
     }
 
     @Test
@@ -43,7 +46,7 @@ public class KCMSSanitizerStrategyTest {
         // TODO: Consider re-writing this using mocks, to avoid dependencies on the CMS implementation
         ICC_Profile corbisRGB = ICC_Profile.getInstance(getClass().getResourceAsStream("/profiles/Corbis RGB.icc"));
 
-        new KCMSSanitizerStrategy().fixProfile(corbisRGB, null);
+        new KCMSSanitizerStrategy().fixProfile(corbisRGB);
 
         // Make sure all known affected tags have type 'XYZ '
         assertArrayEquals(XYZ, Arrays.copyOfRange(corbisRGB.getData(ICC_Profile.icSigMediaWhitePointTag), 0, 4));

imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/LCMSSanitizerStrategyTest.java

@@ -11,21 +11,13 @@ public class LCMSSanitizerStrategyTest {
 
     @Test(expected = IllegalArgumentException.class)
     public void testFixProfileNullProfile() throws Exception {
-        new LCMSSanitizerStrategy().fixProfile(null, null);
+        new LCMSSanitizerStrategy().fixProfile(null);
-    }
-
-    @Test
-    public void testFixProfileNoHeader() throws Exception {
-        ICC_Profile profile = mock(ICC_Profile.class);
-        new LCMSSanitizerStrategy().fixProfile(profile, null);
-
-        verifyNoMoreInteractions(profile);
     }
 
     @Test
     public void testFixProfile() throws Exception {
         ICC_Profile profile = mock(ICC_Profile.class);
-        new LCMSSanitizerStrategy().fixProfile(profile, new byte[0]);
+        new LCMSSanitizerStrategy().fixProfile(profile);
 
         verifyNoMoreInteractions(profile);
     }