From d677141ab7f0e1b111189426b25be9d6efaecf84 Mon Sep 17 00:00:00 2001 From: Harald Kuhr Date: Sun, 5 Nov 2017 11:44:41 +0100 Subject: [PATCH] #386 Fixed ColorSpaces ICC profile caching bug --- .../imageio/color/ColorSpaces.java | 93 ++++++++++++------ .../imageio/color/ICCProfileSanitizer.java | 4 +- .../imageio/color/KCMSSanitizerStrategy.java | 11 ++- .../imageio/color/LCMSSanitizerStrategy.java | 2 +- .../imageio/color/ColorSpacesTest.java | 17 +++- .../color/KCMSSanitizerStrategyTest.java | 21 ++-- .../color/LCMSSanitizerStrategyTest.java | 12 +-- .../resources/profiles/adobe_rgb_1998.icc | Bin 0 -> 528 bytes .../resources/profiles/color_match_rgb.icc | Bin 0 -> 528 bytes 9 files changed, 104 insertions(+), 56 deletions(-) create mode 100644 imageio/imageio-core/src/test/resources/profiles/adobe_rgb_1998.icc create mode 100644 imageio/imageio-core/src/test/resources/profiles/color_match_rgb.icc diff --git a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/ColorSpaces.java b/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/ColorSpaces.java index 1e5337fa..7048bd66 100644 --- a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/ColorSpaces.java +++ b/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); } } diff --git a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/ICCProfileSanitizer.java b/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/ICCProfileSanitizer.java index 10352384..f97ab257 100644 --- a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/ICCProfileSanitizer.java +++ b/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) diff --git a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/KCMSSanitizerStrategy.java b/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/KCMSSanitizerStrategy.java index a4e018d7..3797605a 100644 --- a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/KCMSSanitizerStrategy.java +++ b/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) { - profile.setData(ICC_Profile.icSigHead, profileHeader); + // Special case for color profiles with rendering intent != 0, see ColorSpaces.isOffendingColorProfile method + // 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. diff --git a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/LCMSSanitizerStrategy.java b/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/LCMSSanitizerStrategy.java index d39a2ab1..8fc2978d 100644 --- a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/color/LCMSSanitizerStrategy.java +++ b/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 } diff --git a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/ColorSpacesTest.java b/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/ColorSpacesTest.java index 02290b25..d5118f2f 100644 --- a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/ColorSpacesTest.java +++ b/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); + } } diff --git a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/KCMSSanitizerStrategyTest.java b/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/KCMSSanitizerStrategyTest.java index a9345dda..cfcd8932 100644 --- a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/KCMSSanitizerStrategyTest.java +++ b/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.Mockito.verify; +import static org.mockito.Matchers.any; +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 { - new KCMSSanitizerStrategy().fixProfile(((ICC_ColorSpace) ColorSpace.getInstance(ColorSpace.CS_sRGB)).getProfile(), null); + public void testFixProfile() throws Exception { + 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)); diff --git a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/LCMSSanitizerStrategyTest.java b/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/LCMSSanitizerStrategyTest.java index ae51647c..ed7e9e5a 100644 --- a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/color/LCMSSanitizerStrategyTest.java +++ b/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); - } - - @Test - public void testFixProfileNoHeader() throws Exception { - ICC_Profile profile = mock(ICC_Profile.class); - new LCMSSanitizerStrategy().fixProfile(profile, null); - - verifyNoMoreInteractions(profile); + new LCMSSanitizerStrategy().fixProfile(null); } @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); } diff --git a/imageio/imageio-core/src/test/resources/profiles/adobe_rgb_1998.icc b/imageio/imageio-core/src/test/resources/profiles/adobe_rgb_1998.icc new file mode 100644 index 0000000000000000000000000000000000000000..5eb2bb7413522336b3abc7dcb6b00cc275343a5c GIT binary patch literal 528 zcmZQzU=nb2adKr6U|`72D=7+ccT$Lmj8b4f&%nmO%m4<7$;AbZ0RcWBPF{XqDnt}c zGBPlHyT$+{85l0>g3N-;5Xaz3E+{GiD*Xe*Mk%Sq$qWpP20(Upc}W3KoN*42Es~TC zW^Vwoi$a2&f#OGiY`%0Pb`lc12g3N-;5Xaz3E+{GiD*Xe*Mk%Sq$qWpP20(UZc}W3KoN*42Es~TC zW^Vwoi$a2&f#OGiY`%0Pb`lc12EEJjrV6b}#szukd`uN6WJVO4TzQ5lrS$at240imWTn}NX%7@llq2(ch#28I=t U3=GoO5n@Un3=D#*3=A6;0E;Y6bN~PV literal 0 HcmV?d00001