8325445: [macOS] Colors are not displayed in sRGB color space #1473

beldenfox · 2024-06-07T18:29:00Z

When drawing to the screen JavaFX is producing sRGB colors but on macOS that’s not necessarily what the user is seeing. Since the pixels are not tagged as sRGB the OS is copying them unmodified to the frame buffer to be displayed in the screen’s color space. In general Mac’s don’t default to sRGB so the colors will be wrong. The fix for this is a one-liner; we just need to declare that our CALayer uses the sRGB color space so the OS will convert it to the screen’s space (presumably with a slight performance penalty).

In the reverse direction the Robot should be returning sRGB colors. The getPixelColor calls were making no conversion. The getScreenCapture calls were converting to genericRGB, not sRGB, and so the results didn’t match the getPixelColor calls. This PR fixes these bugs; getPixelColor and getScreenCapture both return sRGB.

Now that everything is working in the same space when JavaFX writes out a pixel and then reads it back in the colors should match within a limited tolerance (due to rounding issues when converting from float to int and back). But that just means the various glass code paths are using the same space to perform conversions, not that it’s sRGB. AWT is color space aware and so the automated test employs an AWT Robot to double-check the results.

I swept through the rest of the Mac glass code and found a few places where colors were being converted to deviceRGB instead of sRGB e.g. when reading colors for PlatformPreferences or creating images for drag and drop. I could not think of a good way of writing automated tests for these cases.

I started investigating this since Robot tests were failing unless the monitor’s profile was set to sRGB. Unfortunately this PR doesn’t entirely fix that. My monitor uses Display P3 and I’m still seeing failures on the SwingNodeJDialogTest. The test writes out pure BLUE colors and gets back colors with a surprising amount of red. I’ve verified that this has nothing to do with JavaFX, it happens when I use CoreGraphics to make the sRGB => Display P3 color conversions directly. I guess this is a cautionary tale about what happens when you work near the edges of a color space’s gamut.

Progress

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

Issue

JDK-8325445: [macOS] Colors are not displayed in sRGB color space (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1473

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1473.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-07T18:29:12Z

👋 Welcome back mfox! 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-06-07T18:30:24Z

@beldenfox 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:

8325445: [macOS] Colors are not displayed in sRGB color space

Reviewed-by: angorya, kcr

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 28 new commits pushed to the master branch:

f7ad5cd: 8336389: Infinite loop occurs while resolving lookups
28e3ccc: 8320232: Cells duplicated when table collapsed and expanded
0fa50cb: 8337281: build.gradle assumes all modules are named "javafx.$project"
41de020: 8336331: Doc: Clarification in AccessibleAttribute, AccessibleRole
686a396: 8337228: Eclipse: missing dependencies in systemTests-test project
cb7127d: 8337229: Missing @OVERRIDES in GlassSystemMenuShim
1bdb93c: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs
25ac6fe: 8319779: SystemMenu: memory leak due to listener never being removed
ffbd12d: 8336097: UserAgent Styles using lookups are promoted to Author level if look-up is defined in Author stylesheet
d538c31: 8336882: Convert CssStyleHelperTest to use JUnit 5
... and 18 more: https://git.openjdk.org/jfx/compare/1fb8755dc6452bdb07685c02272ecfc578fb1eba...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.

mlbridge · 2024-06-07T18:33:25Z

Webrevs

kevinrushforth · 2024-06-07T22:53:58Z

Reviewers: @kevinrushforth @arapte

/reviewers 2

openjdk · 2024-06-07T22:54:11Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

kevinrushforth · 2024-07-01T14:21:52Z

As a datapoint, we were doing some testing in the jfx-sandbox:metal branch, and had over a hundred test failures that we tracked down to the color profile not being set to sRGB (the test system had the default "Color LCD profile").

I applied the fix from this patch (and resolving a couple merge conflicts due to some Metal-specific changes in the sandbox branch), and ran one of the failing tests with the default "Color LCD" profile and it now passes.

Even if this PR doesn't fix all of the problems, it seems like a good fix to consider.

kevinrushforth · 2024-07-01T14:23:15Z

@beldenfox Can you merge in the latest master so we will see a test build on macOS / aarch64?

tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java

andy-goryachev-oracle · 2024-07-29T23:27:58Z

tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java

+
+    // The component tolerance allows one bit of rounding when writing a color
+    // out and another bit when reading it back in.
+    static final double COMPONENT_TOLERANCE = 2.0 / 255.0;


suggestion: 2.0001 / 255.0 to account for floating point errors

I'm not sure this test needs to be that precise. And I'm fine with the threshold falling below 2.0/255.0 since it should be as small as possible (I even experimented briefly with lower numbers). I haven't run into an issue on the three platforms I've run this test on.

Then the comment might not be technically correct, because it is possible to have "two bit difference" in the integer color values, yet fail due to small error intrinsic in floating point operations.

Example:

a=253 b=251 delta=0.007843137254902044 tol=0.00784313725490196 delta=0.007843137254902044 tol=0.00784313725490196 (2.0 / 255.0)

I've updated the constant as you suggested.

andy-goryachev-oracle · 2024-07-29T23:29:23Z

tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java

+    }
+
+    // We use an AWT Robot since it is color space aware and will correctly convert
+    // from the screeen's color space to sRGB.


do we plan to fix FX robot eventually?

This PR fixes the JavaFX Robot. I give a more detailed explanation of why I'm using the AWT Robot in a comment just before the sRGBPixelTest. I've updated the comments.

andy-goryachev-oracle

The reproducer in the ticket fails without the fix and works as described on 2021 macOS 14.5 M1, using built-in retina and an external Samsung LS27A600U display (which seems to have a different color profile).

I would like to request a change for SRGBTest.COMPONENT_TOLERANCE value (or comment), please see inline comments.

andy-goryachev-oracle · 2024-08-01T17:42:16Z

modules/javafx.graphics/src/main/native-glass/mac/GlassRobot.m

+                        components[0] = (CGFloat)((color & 0x00FF0000) >> 16) / 255.0;
+                        components[1] = (CGFloat)((color & 0x0000FF00) >> 8) / 255.0;
+                        components[2] = (CGFloat)((color & 0x000000FF)) / 255.0;
+                        components[3] = (CGFloat)((color & 0xFF000000) >> 24) / 255.0;


is this line guaranteed not to produce negative values? e.g. when color = 0xffffffff

Good catch. I've updated the code to read the pixel value into an unsigned integer.

tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java

andy-goryachev-oracle · 2024-08-01T18:02:05Z

tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java

+
+    // The component tolerance allows one bit of rounding when writing a color
+    // out and another bit when reading it back in.
+    static final double COMPONENT_TOLERANCE = 2.0 / 255.0;


Then the comment might not be technically correct, because it is possible to have "two bit difference" in the integer color values, yet fail due to small error intrinsic in floating point operations.

Example:

a=253 b=251 delta=0.007843137254902044 tol=0.00784313725490196 delta=0.007843137254902044 tol=0.00784313725490196 (2.0 / 255.0)

tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java

…t handling in Robot

andy-goryachev-oracle

retested, looks good.
thank you for the meticulously commented unit tests!

kevinrushforth · 2024-08-01T23:02:41Z

I'll test the updated version. From my earlier testing, I think the following two things need to be addressed, but I'll let you know for sure:

The new system test that uses both AWT and FX robot will need a timeout value, so it won't hang due to JDK-8335468. We might separately consider skipping the test on Linux until that bug is fixed, but since that is an AWT bug, we need the timeout regardless so it doesn't hang the test run when run on a JDK without that fix.
I want to test this with the metal branch of the jfx-sandbox repo, which will require fixing a merge conflict. When I did this last time I ran into a few unexpected test failures. It's possible I made a mistake in resolving the merge conflicts, but that will need to be checked.

kevinrushforth

The code changes all look good. Everything looks good on macOS. I finished all my testing, including testing with the metal pipeline.

The only thing needed is a timeout value on one of the tests to avoid hanging on Linux when running with a Wayland display server. I'll approve once you make that change.

kevinrushforth · 2024-08-03T14:33:47Z

tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java

+    // only verify that the JavaFX renderer and JavaFX Robot can round-trip
+    // colors but they might both be working in the wrong space. We use an
+    // AWT Robot to verify that they are working in sRGB.
+    @Test


This test needs a timeout value to avoid hanging the test job on Linux with Wayland. See JDK-8335468. I recommend:

@Test(timeout = 15000)

Maybe there is a way to skip the test on Wayland until we find a proper solution?
Asking for completeness sake - simply adding a timeout works too.

Q: is this the only test that needs explicit timeout?

I don't see any way to query whether we're running under Wayland or not. That would be nice to have.

This is the only test that uses the AWT Robot on Linux. The window background test also uses the AWT Robot but only runs on Mac.

@azvegint added an isOnWayland method to SwingNodeBase.java. I'll file a new testbug to skip tests that use AWT Robot on Wayland pending the fix for JDK-8335468. As part of that, we can move isOnWayland to Util.java (and fix it to first check PlatformUtil::isLinux).

maybe we ought to fast track an isolated addition of isOnWayland() and integrate that first? what do you think?

edit: never mind, this PR is already integrated. aren't we supposed to wait 24 hrs before integrating?

I filed https://bugs.openjdk.org/browse/JDK-8337827 to track this.

maybe we ought to fast track an isolated addition of isOnWayland() and integrate that first? what do you think?

Not as part of this PR, no.

edit: never mind, this PR is already integrated. aren't we supposed to wait 24 hrs before integrating?

That's 24 hours after "rfr".

it's all good then. thanks for filing JDK-8337827

beldenfox · 2024-08-05T15:43:31Z

/integrate

openjdk · 2024-08-05T15:43:57Z

Going to push as commit 19d09e6.
Since your change was applied there have been 28 commits pushed to the master branch:

f7ad5cd: 8336389: Infinite loop occurs while resolving lookups
28e3ccc: 8320232: Cells duplicated when table collapsed and expanded
0fa50cb: 8337281: build.gradle assumes all modules are named "javafx.$project"
41de020: 8336331: Doc: Clarification in AccessibleAttribute, AccessibleRole
686a396: 8337228: Eclipse: missing dependencies in systemTests-test project
cb7127d: 8337229: Missing @OVERRIDES in GlassSystemMenuShim
1bdb93c: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs
25ac6fe: 8319779: SystemMenu: memory leak due to listener never being removed
ffbd12d: 8336097: UserAgent Styles using lookups are promoted to Author level if look-up is defined in Author stylesheet
d538c31: 8336882: Convert CssStyleHelperTest to use JUnit 5
... and 18 more: https://git.openjdk.org/jfx/compare/1fb8755dc6452bdb07685c02272ecfc578fb1eba...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-05T15:44:05Z

@beldenfox Pushed as commit 19d09e6.

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

beldenfox added 19 commits February 16, 2024 09:00

Adding explicit color space to layer

08b4031

Added test for checking consistent use of sRGB color spaces

b7b6979

New SRGB test, fixed Robot and window background

a0b18f0

Window background test only applies to Mac and Linux

7ddeb31

Restrict window background test to Mac

b135377

Platform preferences now report colors in sRGB

682aabf

Switching Pixels, Pasteboard, and offscreen drawer to sRGB

b1a101e

Single pixel Robot code is not truly single pixel (again)

c84b164

Merge remote-tracking branch 'upstream/master' into colormgmt

9f7e7a6

Correctly rounding when converting color components to bytes

117877a

Merge remote-tracking branch 'upstream/master' into colormgmt

2b8545a

We don't need to manually release the color space.

afda17a

Merge remote-tracking branch 'upstream/master' into colormgmt

6acdb53

Cleaned up SRGBTest

7c32063

Robot code is now executed inside autorelease pool

dea5138

Merge remote-tracking branch 'upstream/master' into colormgmt

74e52cc

Merge remote-tracking branch 'upstream/master' into colormgmt

0924592

Merge remote-tracking branch 'upstream/master' into colormgmt

a7204d0

Color space is two words.

ae06f5a

openjdk bot added the rfr Ready for review label Jun 7, 2024

kevinrushforth self-requested a review June 12, 2024 12:16

beldenfox added 2 commits July 1, 2024 07:40

Merge remote-tracking branch 'upstream/master' into colormgmt

41e3baf

The last deprecated declaration has been removed from GlassView3D.m

b6c3140

andy-goryachev-oracle reviewed Jul 29, 2024

View reviewed changes

tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java Show resolved Hide resolved

andy-goryachev-oracle reviewed Jul 29, 2024

View reviewed changes

Updated comments concerning the AWT Robot

20c2375

andy-goryachev-oracle reviewed Aug 1, 2024

View reviewed changes

Review feedback: floating point precision in test, ensure unsigned in…

2dad883

…t handling in Robot

andy-goryachev-oracle approved these changes Aug 1, 2024

View reviewed changes

kevinrushforth reviewed Aug 3, 2024

View reviewed changes

Added timeout to test due to potential hang when using XWayland

92b9c56

kevinrushforth approved these changes Aug 3, 2024

View reviewed changes

kevinrushforth requested a review from andy-goryachev-oracle August 3, 2024 16:32

andy-goryachev-oracle approved these changes Aug 5, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Aug 5, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 5, 2024

openjdk bot closed this Aug 5, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Aug 5, 2024

beldenfox deleted the colormgmt branch October 4, 2024 21:30

8325445: [macOS] Colors are not displayed in sRGB color space #1473

8325445: [macOS] Colors are not displayed in sRGB color space #1473

Uh oh!

Conversation

beldenfox commented Jun 7, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 7, 2024

Uh oh!

openjdk bot commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Jun 7, 2024

Uh oh!

openjdk bot commented Jun 7, 2024

Uh oh!

kevinrushforth commented Jul 1, 2024

Uh oh!

kevinrushforth commented Jul 1, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Aug 1, 2024

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

beldenfox commented Jun 7, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 7, 2024 •

edited

Loading

mlbridge bot commented Jun 7, 2024 •

edited

Loading

kevinrushforth Aug 5, 2024 •

edited

Loading

andy-goryachev-oracle Aug 5, 2024 •

edited

Loading