On Wed, 22 Apr 2026 16:20:42 GMT, Martin Fox <[email protected]> wrote:
>> Michael Strauß has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 10 additional >> commits since the last revision: >> >> - comment >> - Merge branch 'master' into feature/scene-fill >> - use ChangeListener instead of InvalidationListener >> - import >> - check FX thread >> - Update window background when scene fill changes >> - Update window background when ColorScheme changes >> - review comments >> - Clear background to dominant fill color >> - Adjust scene background to color scheme > > modules/javafx.graphics/src/main/java/com/sun/javafx/image/ImageUtils.java > line 113: > >> 111: } >> 112: >> 113: /** > > I'm onboard with the rest of this PR but this big chunk of code is above my > pay grade. Is the conversion from sRGB to CIELAB correct? Is it a correct > implementation of k-means clustering? Is it even valid to perform clustering > for CIELAB values? Rather than try to answer these questions I generally go > get a snack and put off the review for another week. Good questions. > Is the conversion from sRGB to CIELAB correct? Well, let's take [CSS Color 4](https://www.w3.org/TR/css-color-4/#predefined-to-lab-oklab), which specifies the following conversion sequence: decode sRGB -> linear RGB -> XYZ(D65) -> adapt to D50 -> Lab(D50). This implementation is a bit simpler: decode sRGB -> linear RGB -> XYZ(D65) -> Lab(D65). It skips the D65->D50 adaptation step, and computes Lab directly using D65 reference-white constants. That's not entirely correct from a standards perspective, but it is internally consistent: forward and inverse conversions both use D65. I'd say that's generally good enough for our purpose. > Is it a correct implementation of k-means clustering? Yes, it first initializes the centers with k-means++, and the optimization loop is Lloyd's algorithm: assign each point to the nearest center, replace each center by the mean of its assigned points, repeat until convergence. > Is it even valid to perform clustering for CIELAB values? It's valid and a very good idea™. [MathWorks](https://www.mathworks.com/content/dam/mathworks/tag-team/Objects/c/88360_93001v00_Color-Based_Seg_K-Means_Clustering_2016.pdf) has a clustering example where the first step is "Converting the Image from the RGB Color Space to the L*a*b* Color Space". The reason why it's a good idea is that Euclidean distance in CIELAB is roughly perceptual, much more so than RGB. And since our optimization algorithm minimizes Euclidean distance, it's a good fit for it. That being said, and looking at this code again, the cluster-selection heuristic might be a bit weak: int upper = Math.max(1, nEst / 300); clusters = Math.min(clusters, upper); This collapses to a single cluster for nEst < 600, which can probably happen in the real world for a very small background image that has two distinct regions (maybe very bright and very dark). If we only select a single cluster, we end up with muddy gray. This might not be what we want (or it might be what we want... after all, there's no _correct_ way to do this). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2068#discussion_r3132458560
