jfx
jfx copied to clipboard
openjdk
8325445: [macOS] Colors are not displayed in sRGB color space
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
- [x] Change must not contain extraneous whitespace
- [x] 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)
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
:wave: 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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:
- f7ad5cdccb03162dc19fffb8eed570d4fbef490c: 8336389: Infinite loop occurs while resolving lookups
- 28e3ccc7331952b4d323941c8efc97509348b1e2: 8320232: Cells duplicated when table collapsed and expanded
- 0fa50cbbd9878ca7ec0e1bfcc5567bf52e3b8db2: 8337281: build.gradle assumes all modules are named "javafx.$project"
- 41de02023571d146f392de012a55011a5f87fc9d: 8336331: Doc: Clarification in AccessibleAttribute, AccessibleRole
- 686a396c6f1a5ee0733a69a046d2131f4649eca8: 8337228: Eclipse: missing dependencies in systemTests-test project
- cb7127d4b5e0c230adced8d24e7f695c519e9082: 8337229: Missing @Overrides in GlassSystemMenuShim
- 1bdb93c792cf7c218c74ec5cacda8bac1242f73b: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs
- 25ac6fed22d0f49d01c831aaa48049c34899fe96: 8319779: SystemMenu: memory leak due to listener never being removed
- ffbd12dc01132f186908eb83144ff17c4b4bdeaf: 8336097: UserAgent Styles using lookups are promoted to Author level if look-up is defined in Author stylesheet
- d538c318fbef5198b7b0ea86df826cf4fe1adc5a: 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.
Webrevs
- 05: Full - Incremental (92b9c562)
- 04: Full - Incremental (2dad883c)
- 03: Full - Incremental (20c23759)
- 02: Full - Incremental (b6c3140b)
- 01: Full - Incremental (41e3baf9)
- 00: Full (ae06f5a4)
@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).
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.
@beldenfox Can you merge in the latest master so we will see a test build on macOS / aarch64?
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.
Going to push as commit 19d09e6c2e15683de1c69d0e841f32e337da2031.
Since your change was applied there have been 28 commits pushed to the master branch:
- f7ad5cdccb03162dc19fffb8eed570d4fbef490c: 8336389: Infinite loop occurs while resolving lookups
- 28e3ccc7331952b4d323941c8efc97509348b1e2: 8320232: Cells duplicated when table collapsed and expanded
- 0fa50cbbd9878ca7ec0e1bfcc5567bf52e3b8db2: 8337281: build.gradle assumes all modules are named "javafx.$project"
- 41de02023571d146f392de012a55011a5f87fc9d: 8336331: Doc: Clarification in AccessibleAttribute, AccessibleRole
- 686a396c6f1a5ee0733a69a046d2131f4649eca8: 8337228: Eclipse: missing dependencies in systemTests-test project
- cb7127d4b5e0c230adced8d24e7f695c519e9082: 8337229: Missing @Overrides in GlassSystemMenuShim
- 1bdb93c792cf7c218c74ec5cacda8bac1242f73b: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs
- 25ac6fed22d0f49d01c831aaa48049c34899fe96: 8319779: SystemMenu: memory leak due to listener never being removed
- ffbd12dc01132f186908eb83144ff17c4b4bdeaf: 8336097: UserAgent Styles using lookups are promoted to Author level if look-up is defined in Author stylesheet
- d538c318fbef5198b7b0ea86df826cf4fe1adc5a: 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.
@beldenfox Pushed as commit 19d09e6c2e15683de1c69d0e841f32e337da2031.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.