8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS #18097

dmarkov20 · 2024-03-03T09:01:49Z

Updated several tests to avoid potential failure with recent LCMS update and non-LCMS generated profile.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS (Bug - P3)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18097/head:pull/18097
$ git checkout pull/18097

Update a local copy of the PR:
$ git checkout pull/18097
$ git pull https://git.openjdk.org/jdk.git pull/18097/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18097

View PR using the GUI difftool:
$ git pr show -t 18097

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18097.diff

Webrev

Link to Webrev Comment

…les were generated by LCMS

bridgekeeper · 2024-03-03T09:02:21Z

👋 Welcome back dmarkov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-03-03T09:03:08Z

@dmarkov20 The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-03-03T09:06:01Z

Webrevs

01: Full - Incremental (89557165)
00: Full (27d436fb)

aivanov-jdk

The fix is to always use the accuracy which is used for the LCMS library, without trying to detect what colour profile is used. Right?

openjdk · 2024-03-04T16:15:31Z

@dmarkov20 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS

Reviewed-by: aivanov, serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 87 new commits pushed to the master branch:

63dd6d1: 8327387: G1: Refactor region liveness processing after completion of concurrent marking
f2b5ffd: 8326578: Clean up networking properties documentation
ca5ca85: 8325651: C2 SuperWord: refactor the dependency graph
d451f81: 8326381: com.sun.net.httpserver.HttpsParameters and SSLStreams incorrectly handle needClientAuth and wantClientAuth
243cb09: 8327389: Remove use of HOTSPOT_BUILD_USER
2d4c757: 8327041: Incorrect lane size references in avx512 instructions.
761ed25: 8327138: Clean up status management in cdsConfig.hpp and CDS.java
53628f2: 8327492: Remove applet usage and update DisposeInActionEventTest.html
2627470: 8325567: jspawnhelper without args fails with segfault
a6dc4bc: 8326332: Unclosed inline tags cause misalignment in summary tables
... and 77 more: https://git.openjdk.org/jdk/compare/37e01efb7831e29068f834fe0629595cb721b90d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mrserb · 2024-03-05T00:58:42Z

test/jdk/sun/java2d/cmm/ColorConvertOp/ColConvCCMTest.java

-        (isOpenProfile() ? 215.0 : 45.5), // PYCC
-        (isOpenProfile() ? 56.0 : 47.5) // CIEXYZ
+        215.0,      // PYCC
+        56.0        // CIEXYZ


All this change will delete all information about "old and good" values and start to use lcms thresholds for all profiles? I think it should be the opposite - use the good data for all except lcms(or any other not that good as jdk8 profiles), like we do it now.

Actually the purpose of these particular tests is validation of the quality of the built-in color profiles.

I agree, I reverted almost all changes except the ones in tolerance for non-LCMS profiles. That values need to be updated to make the test pass on JDKs (e.g. JDK 8u, etc) where non-LCMS profiles are used

Could you please clarify what is the root cause of the problem. I assume you did not update the profiles itself, so what is the problem in the new lcms library?

It used to be OK until recent LCMS update where the CMM started to keep original profile ID instead of writing ‘lcms’ to returned header.

Do you mean that previously we always use lcms thresholds even for kcms related profiles?

As far as I know there is no any issues with the new LCMS library. There was a bug in LCMS: it used to set ‘lcms’ as profile ID to the header. As a result we used lcms threshold even for non-LCMS (kcms, etc.) profiles in the test.
In LCMS 2.16 the problem with profile ID was fixed, see https://github.com/openjdk/jdk/pull/17382/files#diff-738d14c3b278e6d7297dbc4943e90cef33fc04d61eba085218a1229c92ea9a33R941
And the tests started failing for non-LCMS profiles.

aivanov-jdk · 2024-03-11T11:00:38Z

It's not mentioned here, however, I think it's relevant to the discussion: the updated tests pass when the non-LCMS profiles from Java 8 are used.

dmarkov20 · 2024-03-12T10:08:40Z

/integrate

openjdk · 2024-03-12T10:09:03Z

Going to push as commit 1f43fa0.
Since your change was applied there have been 104 commits pushed to the master branch:

013aff8: 8326606: Test javax/swing/text/BoxView/6494356/bug6494356.java performs a synchronization on a value based class
b92440f: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage
139681a: 8326497: Window.toFront() fails for iconified windows on Linux
9f7aff4: 8327788: G1: Improve concurrent reference processing documentation
251347b: 8326139: C2 SuperWord: split packs (match use/def packs, implemented, mutual independence)
e21da4c: 8327486: java/util/Properties/PropertiesStoreTest.java fails "Text 'xxx' could not be parsed at index 20" after 8174269
4d6235e: 8327716: RISC-V: Change type of vector_length param of several assembler functions from int to uint
586396c: 8327705: Remove mention of "applet" from java.text package description
41450e9: 8314250: CDS dump error message: Invoker type parameter must start and end with Object: L3I_L
0a6e64e: 8327424: ProblemList serviceability/sa/TestJmapCore.java on all platforms with ZGC
... and 94 more: https://git.openjdk.org/jdk/compare/37e01efb7831e29068f834fe0629595cb721b90d...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-12T10:09:14Z

@dmarkov20 Pushed as commit 1f43fa0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profi…

27d436f

…les were generated by LCMS

openjdk bot added the rfr Pull request is ready for review label Mar 3, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Mar 3, 2024

aivanov-jdk approved these changes Mar 4, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 4, 2024

mrserb reviewed Mar 5, 2024

View reviewed changes

Tolerance update

8955716

mrserb approved these changes Mar 8, 2024

View reviewed changes

aivanov-jdk approved these changes Mar 11, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 12, 2024

openjdk bot closed this Mar 12, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS #18097

8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS #18097

dmarkov20 commented Mar 3, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 3, 2024

openjdk bot commented Mar 3, 2024

mlbridge bot commented Mar 3, 2024 •

edited

Loading

aivanov-jdk left a comment

openjdk bot commented Mar 4, 2024 •

edited

Loading

mrserb Mar 5, 2024

mrserb Mar 5, 2024

dmarkov20 Mar 7, 2024

mrserb Mar 8, 2024 •

edited

Loading

dmarkov20 Mar 8, 2024

aivanov-jdk commented Mar 11, 2024

dmarkov20 commented Mar 12, 2024

openjdk bot commented Mar 12, 2024

openjdk bot commented Mar 12, 2024

8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS #18097

8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS #18097

Conversation

dmarkov20 commented Mar 3, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 3, 2024

openjdk bot commented Mar 3, 2024

mlbridge bot commented Mar 3, 2024 • edited Loading

Webrevs

aivanov-jdk left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 4, 2024 • edited Loading

mrserb Mar 5, 2024

Choose a reason for hiding this comment

mrserb Mar 5, 2024

Choose a reason for hiding this comment

dmarkov20 Mar 7, 2024

Choose a reason for hiding this comment

mrserb Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

dmarkov20 Mar 8, 2024

Choose a reason for hiding this comment

aivanov-jdk commented Mar 11, 2024

dmarkov20 commented Mar 12, 2024

openjdk bot commented Mar 12, 2024

openjdk bot commented Mar 12, 2024

dmarkov20 commented Mar 3, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 3, 2024 •

edited

Loading

openjdk bot commented Mar 4, 2024 •

edited

Loading

mrserb Mar 8, 2024 •

edited

Loading