TMI-41: Better handling of ICC Color Profiles. Now using different strategies to "sanitize" profiles, depending on the Color Management System in use.

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

@@ -70,13 +70,10 @@ import java.util.Properties;
  * @version $Id: ColorSpaces.java,v 1.0 24.01.11 17.51 haraldk Exp$
  */
 public final class ColorSpaces {
+    final static boolean DEBUG = "true".equalsIgnoreCase(System.getProperty("com.twelvemonkeys.imageio.color.debug"));
 
-    private final static boolean DEBUG = "true".equalsIgnoreCase(System.getProperty("com.twelvemonkeys.imageio.color.debug"));
+    /** We need special ICC profile handling for KCMS vs LCMS. Delegate to specific strategy. */
-
+    private final static ICCProfileSanitizer profileCleaner = ICCProfileSanitizer.Factory.get();
-    // OpenJDK 7 seems to handle non-perceptual rendering intents gracefully, so we don't need to fiddle with the profiles.
-    // However, the later Oracle distribute JDK seems to include the color management code that has the known bugs...
-    private final static boolean JDK_HANDLES_RENDERING_INTENTS =
-            SystemUtil.isClassAvailable("java.lang.invoke.CallSite") && !SystemUtil.isClassAvailable("sun.java2d.cmm.kcms.CMM");
 
     // NOTE: java.awt.color.ColorSpace.CS_* uses 1000-1004, we'll use 5000+ to not interfere with future additions
 
@@ -86,9 +83,6 @@ public final class ColorSpaces {
     /** A best-effort "generic" CMYK color space. Either read from disk or built-in. */
     public static final int CS_GENERIC_CMYK = 5001;
 
-    /** 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};
-
     // Weak references to hold the color spaces while cached
     private static WeakReference<ICC_Profile> adobeRGB1998 = new WeakReference<ICC_Profile>(null);
     private static WeakReference<ICC_Profile> genericCMYK = new WeakReference<ICC_Profile>(null);
@@ -129,52 +123,16 @@ public final class ColorSpaces {
                 return cs;
             }
 
-            // NOTE: The intent change breaks JDK7: Seems to be a bug in ICC_Profile.getData/setData,
+            // Fix profile before lookup/create
-            // as calling it with unchanged header data, still breaks when creating new ICC_ColorSpace...
+            profileCleaner.fixProfile(profile, profileHeader);
-            // However, we simply skip that, as JDK7 handles the rendering intents already.
-            if (!JDK_HANDLES_RENDERING_INTENTS) {
-                // Fix profile before lookup/create
-                profile.setData(ICC_Profile.icSigHead, profileHeader);
-            }
         }
-
+        else {
-        // Special handling to detect problematic Corbis RGB ICC Profile.
+            profileCleaner.fixProfile(profile, null);
-        // This makes sure tags that are expected to be of type 'XYZ ' really have this expected type.
-        // Should leave other ICC profiles unchanged.
-        if (!JDK_HANDLES_RENDERING_INTENTS && fixProfileXYZTag(profile, ICC_Profile.icSigMediaWhitePointTag)) {
-            fixProfileXYZTag(profile, ICC_Profile.icSigRedColorantTag);
-            fixProfileXYZTag(profile, ICC_Profile.icSigGreenColorantTag);
-            fixProfileXYZTag(profile, ICC_Profile.icSigBlueColorantTag);
         }
 
         return getCachedOrCreateCS(profile, profileHeader);
     }
 
-    /**
-     * Fixes problematic 'XYZ ' tags in Corbis RGB profile.
-     *
-     * @return {@code true} if found and fixed, otherwise {@code false} for short-circuiting
-     * to avoid unnecessary array copying.
-     */
-    private static boolean fixProfileXYZTag(final ICC_Profile profile, final int tagSignature) {
-        // TODO: This blows up on OpenJDK... Bug?
-        byte[] data = profile.getData(tagSignature);
-
-        // The CMM expects 0x64 65 73 63 ('XYZ ') but is 0x17 A5 05 B8..?
-        if (data != null && Arrays.equals(Arrays.copyOfRange(data, 0, 4), CORBIS_RGB_ALTERNATE_XYZ)) {
-            data[0] = 'X';
-            data[1] = 'Y';
-            data[2] = 'Z';
-            data[3] = ' ';
-
-            profile.setData(tagSignature, data);
-
-            return true;
-        }
-
-        return false;
-    }
-
     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);
@@ -414,6 +372,7 @@ public final class ColorSpaces {
 
         private static Properties loadProfiles(final Platform.OperatingSystem os) {
             Properties systemDefaults;
+
             try {
                 systemDefaults = SystemUtil.loadProperties(ColorSpaces.class, "com/twelvemonkeys/imageio/color/icc_profiles_" + os.id());
             }
@@ -422,6 +381,7 @@ public final class ColorSpaces {
                         "Warning: Could not load system default ICC profile locations from %s, will use bundled fallback profiles.\n",
                         ignore.getMessage()
                 );
+
                 if (DEBUG) {
                     ignore.printStackTrace();
                 }

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

@@ -0,0 +1,52 @@
+package com.twelvemonkeys.imageio.color;
+
+import com.twelvemonkeys.lang.SystemUtil;
+
+import java.awt.color.ICC_Profile;
+
+/**
+ * ProfileCleaner.
+ *
+ * @author <a href="mailto:harald.kuhr@gmail.com">Harald Kuhr</a>
+ * @author last modified by $Author: harald.kuhr$
+ * @version $Id: ProfileCleaner.java,v 1.0 06/01/15 harald.kuhr Exp$
+ */
+interface ICCProfileSanitizer {
+    void fixProfile(ICC_Profile profile, byte[] profileHeader);
+
+    static class Factory {
+        static ICCProfileSanitizer get() {
+            // Strategy pattern:
+            //  - KCMSSanitizerStrategy - Current behaviour, default for Java 1.6 and Oracle JRE 1.7
+            //  - LCMSSanitizerStrategy - New behaviour, default for OpenJDK 1.7 and all java 1.8
+            //              (unless KCMS switch -Dsun.java2d.cmm=sun.java2d.cmm.kcms.KcmsServiceProvider present)
+            // TODO: Allow user-specific strategy selection, should heuristics not work..?
+            // -Dcom.twelvemonkeys.imageio.color.ICCProfileSanitizer=com.foo.bar.FooCMSSanitizer
+
+            ICCProfileSanitizer instance;
+
+            // Explicit System properties
+            if ("sun.java2d.cmm.kcms.KcmsServiceProvider".equals(System.getProperty("sun.java2d.cmm")) && SystemUtil.isClassAvailable("sun.java2d.cmm.kcms.CMM")) {
+                instance = new KCMSSanitizerStrategy();
+            }
+            else if ("sun.java2d.cmm.lcms.LcmsServiceProvider".equals(System.getProperty("sun.java2d.cmm")) && SystemUtil.isClassAvailable("sun.java2d.cmm.lcms.LCMS")) {
+                instance = new LCMSSanitizerStrategy();
+            }
+            // Default for Java 1.8+ or OpenJDK 1.7+ (no KCMS available)
+            else if (SystemUtil.isClassAvailable("java.util.stream.Stream")
+                    || SystemUtil.isClassAvailable("java.lang.invoke.CallSite") && !SystemUtil.isClassAvailable("sun.java2d.cmm.kcms.CMM")) {
+                instance = new LCMSSanitizerStrategy();
+            }
+            // Default for all Java versions <= 1.7
+            else {
+                instance = new KCMSSanitizerStrategy();
+            }
+
+            if (ColorSpaces.DEBUG) {
+                System.out.println("ICC ProfileCleaner instance: " + instance.getClass().getName());
+            }
+
+            return instance;
+        }
+    }
+}

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

@@ -0,0 +1,61 @@
+package com.twelvemonkeys.imageio.color;
+
+import com.twelvemonkeys.lang.Validate;
+
+import java.awt.color.ICC_Profile;
+import java.util.Arrays;
+
+/**
+ * KCMSProfileCleaner.
+ *
+ * @author <a href="mailto:harald.kuhr@gmail.com">Harald Kuhr</a>
+ * @author last modified by $Author: harald.kuhr$
+ * @version $Id: KCMSProfileCleaner.java,v 1.0 06/01/15 harald.kuhr Exp$
+ */
+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) {
+        Validate.notNull(profile, "profile");
+
+        if (profileHeader != null) {
+            profile.setData(ICC_Profile.icSigHead, profileHeader);
+        }
+
+        // Special handling to detect problematic Corbis RGB ICC Profile for KCMS.
+        // This makes sure tags that are expected to be of type 'XYZ ' really have this expected type.
+        // Should leave other ICC profiles unchanged.
+        if (fixProfileXYZTag(profile, ICC_Profile.icSigMediaWhitePointTag)) {
+            fixProfileXYZTag(profile, ICC_Profile.icSigRedColorantTag);
+            fixProfileXYZTag(profile, ICC_Profile.icSigGreenColorantTag);
+            fixProfileXYZTag(profile, ICC_Profile.icSigBlueColorantTag);
+        }
+    }
+
+    /**
+     * Fixes problematic 'XYZ ' tags in Corbis RGB profile.
+     *
+     * @return {@code true} if found and fixed, otherwise {@code false} for short-circuiting
+     * to avoid unnecessary array copying.
+     */
+    private static boolean fixProfileXYZTag(final ICC_Profile profile, final int tagSignature) {
+        byte[] data = profile.getData(tagSignature);
+
+        // The CMM expects 0x64 65 73 63 ('XYZ ') but is 0x17 A5 05 B8..?
+        if (data != null && Arrays.equals(Arrays.copyOfRange(data, 0, 4), CORBIS_RGB_ALTERNATE_XYZ)) {
+            data[0] = 'X';
+            data[1] = 'Y';
+            data[2] = 'Z';
+            data[3] = ' ';
+
+            profile.setData(tagSignature, data);
+
+            return true;
+        }
+
+        return false;
+    }
+
+}

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

@@ -0,0 +1,19 @@
+package com.twelvemonkeys.imageio.color;
+
+import com.twelvemonkeys.lang.Validate;
+
+import java.awt.color.ICC_Profile;
+
+/**
+ * LCMSProfileCleaner.
+ *
+ * @author <a href="mailto:harald.kuhr@gmail.com">Harald Kuhr</a>
+ * @author last modified by $Author: harald.kuhr$
+ * @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) {
+        Validate.notNull(profile, "profile");
+        // Let LCMS handle things internally for now
+    }
+}

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

@@ -33,8 +33,6 @@ 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 java.util.Arrays;
 
 import static org.junit.Assert.*;
 
@@ -47,8 +45,6 @@ import static org.junit.Assert.*;
  */
 public class ColorSpacesTest {
 
-    private static final byte[] XYZ = new byte[] {'X', 'Y', 'Z', ' '};
-
     @Test
     public void testCreateColorSpaceFromKnownProfileReturnsInternalCS_sRGB() {
         ICC_Profile profile = ICC_Profile.getInstance(ColorSpace.CS_sRGB);
@@ -189,19 +185,4 @@ public class ColorSpacesTest {
     public void testIsCS_sRGBNull() {
         ColorSpaces.isCS_sRGB(null);
     }
-
-    @Test
-    public void testCorbisRGBSpecialHandling() throws IOException {
-        ICC_Profile corbisRGB = ICC_Profile.getInstance(getClass().getResourceAsStream("/profiles/Corbis RGB.icc"));
-        ICC_ColorSpace colorSpace = ColorSpaces.createColorSpace(corbisRGB);
-
-        assertNotNull(colorSpace);
-
-        // Make sure all known affected tags have type 'XYZ '
-        ICC_Profile profile = colorSpace.getProfile();
-        assertArrayEquals(XYZ, Arrays.copyOfRange(profile.getData(ICC_Profile.icSigMediaWhitePointTag), 0, 4));
-        assertArrayEquals(XYZ, Arrays.copyOfRange(profile.getData(ICC_Profile.icSigRedColorantTag), 0, 4));
-        assertArrayEquals(XYZ, Arrays.copyOfRange(profile.getData(ICC_Profile.icSigGreenColorantTag), 0, 4));
-        assertArrayEquals(XYZ, Arrays.copyOfRange(profile.getData(ICC_Profile.icSigBlueColorantTag), 0, 4));
-    }
 }

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

@@ -0,0 +1,54 @@
+package com.twelvemonkeys.imageio.color;
+
+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 java.util.Arrays;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+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);
+    }
+
+    @Test
+    public void testFixProfileNullHeader() throws Exception {
+        new KCMSSanitizerStrategy().fixProfile(((ICC_ColorSpace) ColorSpace.getInstance(ColorSpace.CS_sRGB)).getProfile(), null);
+    }
+
+    @Test
+    public void testFixProfileUpdateHeader() throws Exception {
+        ICC_Profile profile = mock(ICC_Profile.class);
+        byte[] header = new byte[0];
+
+        // 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);
+
+        // Verify that the method was invoked
+        verify(profile).setData(ICC_Profile.icSigHead, header);
+    }
+
+    @Test
+    public void testFixProfileCorbisRGB() throws IOException {
+        // 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);
+
+        // Make sure all known affected tags have type 'XYZ '
+        assertArrayEquals(XYZ, Arrays.copyOfRange(corbisRGB.getData(ICC_Profile.icSigMediaWhitePointTag), 0, 4));
+        assertArrayEquals(XYZ, Arrays.copyOfRange(corbisRGB.getData(ICC_Profile.icSigRedColorantTag), 0, 4));
+        assertArrayEquals(XYZ, Arrays.copyOfRange(corbisRGB.getData(ICC_Profile.icSigGreenColorantTag), 0, 4));
+        assertArrayEquals(XYZ, Arrays.copyOfRange(corbisRGB.getData(ICC_Profile.icSigBlueColorantTag), 0, 4));
+    }
+}

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

@@ -0,0 +1,32 @@
+package com.twelvemonkeys.imageio.color;
+
+import org.junit.Test;
+
+import java.awt.color.ICC_Profile;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+
+public class LCMSSanitizerStrategyTest {
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testFixProfileNullProfile() throws Exception {
+        new LCMSSanitizerStrategy().fixProfile(null, 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]);
+
+        verifyNoMoreInteractions(profile);
+    }
+}