On Thu, 29 May 2025 10:22:39 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
> The [next PR](https://github.com/openjdk/jdk/pull/23044) introduces several > new methods to the ICC_Profile class: > - getProfileClass(byte[]) > - getColorSpaceType(byte[]) > - getPCSType(byte[]) > - checkRenderingIntent(byte[]) > > These new methods extract data directly from the provided byte array rather > than relying on the profile instance. The first three methods essentially > duplicate the existing ones (getProfileClass(), getColorSpaceType(), > getPCSType()). > > It is possible to update implementation: > - The existing methods getColorSpaceType() and getPCSType() could delegate to > the new getColorSpaceType(byte[]) and getPCSType(byte[]) methods. > - The checkRenderingIntent(byte[]) method could be updated to report the > actual invalid intent value when an error occurs > > Tests: > - Old ValidateICCHeaderData test is update to verify the new output of the > checkRenderingIntent > - New RenderingIntentStressTest test is added to check the next part of the > icc_spec: > > * ICC spec: only the least-significant 16 bits encode the rendering > * intent. The most significant 16 bits must be zero and can be > ignored. > * See https://www.color.org/ICC1v42_2006-05.pdf, section 7.2.15. > > > > @honkar-jdk please take a look. > > Note: There is currently an inconsistency in the usage of > `getData(icSigHead)` vs `getData(cmmProfile(), icSigHead)` throughout the > codebase. I plan to address this separately. Initial testing looks good. I have added minor inline comments. src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1191: > 1189: * ICC spec: only the least-significant 16 bits encode the > rendering > 1190: * intent. The most significant 16 bits must be zero and can be > ignored. > 1191: * See https://www.color.org/ICC1v42_2006-05.pdf, section > 7.2.15. ICC Spec link in the comment refers to older one, probably update to the new version ? https://www.color.org/specification/ICC.1-2022-05.pdf , https://www.color.org/icc_specs2.xalter. test/jdk/java/awt/color/ICC_Profile/RenderingIntentStressTest.java line 95: > 93: || intent == icMediaRelativeColorimetric > 94: || intent == icSaturation || intent == > icAbsoluteColorimetric > 95: || intent == icICCAbsoluteColorimetric; I hadn't noticed it earlier but `icAbsoluteColorimetric` and `icICCAbsoluteColorimetric` (since 1.5) point to the same intent. Can it be unified or has it be maintained as two separate constants for backward compatibility ? test/jdk/java/awt/color/ICC_Profile/ValidateICCHeaderData/ValidateICCHeaderData.java line 199: > 197: if (!message.contains(": " + invalidRenderIntent)) { > 198: throw new RuntimeException("Test Failed ! Unexpected > text"); > 199: } The usual recommendation is not to check for specific exception messages but in this case it should be fine since this fix changes the exception msg and as regression test update for the fix. ------------- PR Review: https://git.openjdk.org/jdk/pull/25519#pullrequestreview-2882873809 PR Review Comment: https://git.openjdk.org/jdk/pull/25519#discussion_r2116863194 PR Review Comment: https://git.openjdk.org/jdk/pull/25519#discussion_r2116878277 PR Review Comment: https://git.openjdk.org/jdk/pull/25519#discussion_r2116872440
