Skip to content

8358623: Avoid unnecessary data copying in ICC_Profile #25650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+115 −44
Open

8358623: Avoid unnecessary data copying in ICC_Profile #25650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
426a608
Avoid arraycopy in getInstance(byte[])
mrserb Jun 4, 2025
4035c8b
Replace getData(icSigHead) by the getData(cmmProfile(), icSigHead)
mrserb Jun 4, 2025
96ad456
Eliminate possible NPE when getData is used
mrserb Jun 4, 2025
983675f
ICC -> CMM
mrserb Jun 4, 2025
137153f
Create CheckVersions.java
mrserb Jun 5, 2025
450f35e
Update CheckVersions.java
mrserb Jun 5, 2025
97d01fc
theHeader -> data
mrserb Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 36 additions & 39 deletions src/java.desktop/share/classes/java/awt/color/ICC_Profile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,31 +799,19 @@ private interface BuiltInProfile {
*/
public static ICC_Profile getInstance(byte[] data) {
ProfileDataVerifier.verify(data);
verifyHeader(data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verifyHeader(byte[] data), expects header byte array and not the entire profile array, is it technically correct to send the entire profile byte array?

Please note verifyHeader() is called from two places from getInstance(byte[] data) and setData(int tagSignature, byte[] tagData),

With the current fix, logically all the checks within verifyHeader() work (getProfileClass(data), getColorSpaceType(data) ...) because the data is extracted at specified index from the superset - entire profile byte array instead of the header byte array.

If it is decided to send only header byte array to verifyHeader() then changing the method param name to header instead of data may be clearer.

    private static void verifyHeader(byte[] header) {
        if (header == null || header.length < HEADER_SIZE) {
            throw new IllegalArgumentException("Invalid header data");
        }
        getProfileClass(header);
        getColorSpaceType(header);
        getPCSType(header);
        checkRenderingIntent(header);
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verifyHeader(byte[] data), expects header byte array and not the entire profile array, is it technically correct to send the entire profile byte array?

Validating the header directly without cloning is fine and actually better. The old code effectively skipped the "header.length < HEADER_SIZE" check because it always created a "new byte[HEADER_SIZE]".

Copy link
Contributor

@honkar-jdk honkar-jdk Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. I think renaming the param name for the following checks to something more generic such as data or profileData instead of header will cause less confusion then.

private static void checkRenderingIntent(byte[] header)
private static int getColorSpaceType(byte[] theHeader)
private static int getProfileClass(byte[] theHeader)
private static int getPCSType(byte[] theHeader)

Profile p;
try {
byte[] theHeader = new byte[HEADER_SIZE];
System.arraycopy(data, 0, theHeader, 0, HEADER_SIZE);
verifyHeader(theHeader);

Comment on lines -804 to -807
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reference to the above comment , these lines may need to be reverted as verifyHeader(byte[] header) expects the header array and not the entire profile byte array.

p = CMSManager.getModule().loadProfile(data);
} catch (CMMException c) {
throw new IllegalArgumentException("Invalid ICC Profile Data");
}

try {
if (getColorSpaceType(data) == ColorSpace.TYPE_GRAY
&& getData(p, icSigMediaWhitePointTag) != null
&& getData(p, icSigGrayTRCTag) != null) {
int type = getColorSpaceType(data);
if (type == ColorSpace.TYPE_GRAY) {
return new ICC_ProfileGray(p);
}
if (getColorSpaceType(data) == ColorSpace.TYPE_RGB
&& getData(p, icSigMediaWhitePointTag) != null
&& getData(p, icSigRedColorantTag) != null
&& getData(p, icSigGreenColorantTag) != null
&& getData(p, icSigBlueColorantTag) != null
&& getData(p, icSigRedTRCTag) != null
&& getData(p, icSigGreenTRCTag) != null
&& getData(p, icSigBlueTRCTag) != null) {
} else if (type == ColorSpace.TYPE_RGB) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above checks are moved to the corresponding constructors.

return new ICC_ProfileRGB(p);
}
} catch (CMMException c) {
Expand Down Expand Up @@ -969,7 +957,7 @@ private Profile cmmProfile() {
* @return the major version of the profile
*/
public int getMajorVersion() {
return getData(icSigHead)[8];
return getData(cmmProfile(), icSigHead)[8];
}

/**
Expand All @@ -978,7 +966,7 @@ public int getMajorVersion() {
* @return the minor version of the profile
*/
public int getMinorVersion() {
return getData(icSigHead)[9];
return getData(cmmProfile(), icSigHead)[9];
}

/**
Expand All @@ -991,12 +979,12 @@ public int getProfileClass() {
if (info != null) {
return info.profileClass;
}
byte[] theHeader = getData(icSigHead);
byte[] theHeader = getData(cmmProfile(), icSigHead);
return getProfileClass(theHeader);
}

private static int getProfileClass(byte[] theHeader) {
int theClassSig = intFromBigEndian(theHeader, icHdrDeviceClass);
private static int getProfileClass(byte[] data) {
int theClassSig = intFromBigEndian(data, icHdrDeviceClass);
return switch (theClassSig) {
case icSigInputClass -> CLASS_INPUT;
case icSigDisplayClass -> CLASS_DISPLAY;
Expand Down Expand Up @@ -1032,8 +1020,8 @@ public int getColorSpaceType() {
return getColorSpaceType(theHeader);
}

private static int getColorSpaceType(byte[] theHeader) {
int theColorSpaceSig = intFromBigEndian(theHeader, icHdrColorSpace);
private static int getColorSpaceType(byte[] data) {
int theColorSpaceSig = intFromBigEndian(data, icHdrColorSpace);
return iccCStoJCS(theColorSpaceSig);
}

Expand All @@ -1051,13 +1039,13 @@ private static int getColorSpaceType(byte[] theHeader) {
* {@code ColorSpace} class
*/
public int getPCSType() {
byte[] theHeader = getData(icSigHead);
byte[] theHeader = getData(cmmProfile(), icSigHead);
return getPCSType(theHeader);
}

private static int getPCSType(byte[] theHeader) {
int thePCSSig = intFromBigEndian(theHeader, icHdrPcs);
int theDeviceClass = intFromBigEndian(theHeader, icHdrDeviceClass);
private static int getPCSType(byte[] data) {
int thePCSSig = intFromBigEndian(data, icHdrPcs);
int theDeviceClass = intFromBigEndian(data, icHdrDeviceClass);

if (theDeviceClass == icSigLinkClass) {
return iccCStoJCS(thePCSSig);
Expand Down Expand Up @@ -1120,18 +1108,27 @@ public byte[] getData() {
* @see #setData(int, byte[])
*/
public byte[] getData(int tagSignature) {
byte[] t = getData(cmmProfile(), tagSignature);
return t != null ? t.clone() : null;
}

private static byte[] getData(Profile p, int tagSignature) {
try {
return CMSManager.getModule().getTagData(p, tagSignature);
return getData(cmmProfile(), tagSignature).clone();
} catch (CMMException c) {
return null;
}
}

/**
* Returns a particular tagged data element from the profile as a non-null
* byte array. The returned byte array is not cloned. It must not be exposed
* to or used by public APIs. It is intended strictly for internal use only.
*
* @param p the CMM profile from which to retrieve the tag data
* @param tagSignature the ICC tag signature for the data to retrieve
* @return a non-null byte array containing the tag data
* @throws CMMException if the specified tag doesn't exist
*/
static byte[] getData(Profile p, int tagSignature) {
return CMSManager.getModule().getTagData(p, tagSignature);
}

/**
* Sets a particular tagged data element in the profile from a byte array.
* The array should contain data in a format, corresponded to the
Expand Down Expand Up @@ -1183,15 +1180,15 @@ private static void verifyHeader(byte[] data) {
checkRenderingIntent(data);
}

private static void checkRenderingIntent(byte[] header) {
private static void checkRenderingIntent(byte[] data) {
int index = ICC_Profile.icHdrRenderingIntent;
/*
* ICC spec: only the least-significant 16 bits encode the rendering
* intent. The most significant 16 bits must be zero and can be ignored.
* https://www.color.org/specification/ICC.1-2022-05.pdf, section 7.2.15
*/
// Extract 16-bit unsigned rendering intent (0–65535)
int intent = (header[index + 2] & 0xff) << 8 | header[index + 3] & 0xff;
int intent = (data[index + 2] & 0xff) << 8 | data[index + 3] & 0xff;
// Only check upper bound since intent can't be negative
if (intent > icICCAbsoluteColorimetric) {
throw new IllegalArgumentException(
Expand All @@ -1212,7 +1209,7 @@ public int getNumComponents() {
if (info != null) {
return info.numComponents;
}
byte[] theHeader = getData(icSigHead);
byte[] theHeader = getData(cmmProfile(), icSigHead);
int theColorSpaceSig = intFromBigEndian(theHeader, icHdrColorSpace);
return switch (theColorSpaceSig) {
case icSigGrayData -> 1;
Expand Down Expand Up @@ -1251,7 +1248,7 @@ float[] getMediaWhitePoint() {
* encoded in an XYZType tag.
*/
final float[] getXYZTag(int tagSignature) {
byte[] theData = getData(tagSignature);
byte[] theData = getData(cmmProfile(), tagSignature);
float[] theXYZNumber = new float[3]; /* array to return */

/* convert s15Fixed16Number to float */
Expand All @@ -1275,7 +1272,7 @@ final float[] getXYZTag(int tagSignature) {
* single gamma value
*/
float getGamma(int tagSignature) {
byte[] theTRCData = getData(tagSignature);
byte[] theTRCData = getData(cmmProfile(), tagSignature);
if (intFromBigEndian(theTRCData, icCurveCount) != 1) {
throw new ProfileDataException("TRC is not a gamma");
}
Expand Down Expand Up @@ -1306,7 +1303,7 @@ float getGamma(int tagSignature) {
* table
*/
short[] getTRC(int tagSignature) {
byte[] theTRCData = getData(tagSignature);
byte[] theTRCData = getData(cmmProfile(), tagSignature);
int nElements = intFromBigEndian(theTRCData, icCurveCount);
if (nElements == 1) {
throw new ProfileDataException("TRC is not a table");
Expand Down
9 changes: 7 additions & 2 deletions src/java.desktop/share/classes/java/awt/color/ICC_ProfileGray.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -74,10 +74,15 @@ public final class ICC_ProfileGray extends ICC_Profile {
private static final long serialVersionUID = -1124721290732002649L;

/**
* Constructs a new {@code ICC_ProfileGray} from a CMM ID.
* Constructs a new {@code ICC_ProfileGray} from the specified CMM profile.
*
* @param p the CMM profile used to create this ICC profile
* @throws CMMException if the required tags are missing
*/
ICC_ProfileGray(Profile p) {
super(p);
getData(p, icSigMediaWhitePointTag);
getData(p, icSigGrayTRCTag);
}

/**
Expand Down
14 changes: 11 additions & 3 deletions src/java.desktop/share/classes/java/awt/color/ICC_ProfileRGB.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -104,12 +104,20 @@ public final class ICC_ProfileRGB extends ICC_Profile {
public static final int BLUECOMPONENT = 2;

/**
* Constructs an new {@code ICC_ProfileRGB} from a CMM ID.
* Constructs a new {@code ICC_ProfileRGB} from the specified CMM profile.
*
* @param p the CMM ID for the profile.
* @param p the CMM profile used to create this ICC profile
* @throws CMMException if the required tags are missing
*/
ICC_ProfileRGB(Profile p) {
super(p);
getData(p, icSigMediaWhitePointTag);
getData(p, icSigRedColorantTag);
getData(p, icSigGreenColorantTag);
getData(p, icSigBlueColorantTag);
getData(p, icSigRedTRCTag);
getData(p, icSigGreenTRCTag);
getData(p, icSigBlueTRCTag);
}

/**
Expand Down
61 changes: 61 additions & 0 deletions test/jdk/java/awt/color/ICC_Profile/CheckVersions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

import java.awt.color.ColorSpace;
import java.awt.color.ICC_Profile;

/**
* @test
* @bug 8358623
* @summary Verifies ICC profile version of built-in color spaces
*/
public final class CheckVersions {

public static void main(String[] args) {
test(ColorSpace.CS_CIEXYZ, 2, 3, 0);
test(ColorSpace.CS_GRAY, 2, 3, 0);
test(ColorSpace.CS_LINEAR_RGB, 2, 3, 0);
test(ColorSpace.CS_PYCC, 4, 0, 0);
test(ColorSpace.CS_sRGB, 2, 3, 0);
}

private static void test(int cs, int expMajor, int expMinor, int expPatch) {
ICC_Profile profile = ICC_Profile.getInstance(cs);

int major = profile.getMajorVersion();
int minorRaw = profile.getMinorVersion();
int minor = (minorRaw >> 4) & 0x0F;
int patch = minorRaw & 0x0F;

if (major != expMajor || minor != expMinor || patch != expPatch) {
System.err.println("Expected major: " + expMajor);
System.err.println("Expected minor: " + expMinor);
System.err.println("Expected patch: " + expPatch);

System.err.println("Actual major: " + major);
System.err.println("Actual minor: " + minor);
System.err.println("Actual patch: " + patch);
throw new RuntimeException("Test failed for ColorSpace: " + cs);
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added a test for the changed methods, since they were not covered by any jtreg tests.

Copy link
Member Author

@mrserb mrserb Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, ICC_Profile.getMinorVersion() does not return just the minor version as specified, but instead returns the raw byte, which includes both the minor and patch versions. Perhaps the specification should be updated to reflect this behavior?