On Thu, 23 Apr 2026 17:07:46 GMT, Michael Strauß <[email protected]> wrote:
>> 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). Is there any way to allow the application to control that somehow? This is complicated code that I think is not necessary in 90% of common cases. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2068#discussion_r3132647503
