On Thu, 23 Apr 2026 17:37:01 GMT, Andy Goryachev <[email protected]> wrote:

>> 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.

My main point is that it's a big commitment to review this code. Even if you're 
familiar with the color conversions and clustering algorithms involved (I'm 
not) it's just a lot of code to go over.

It's helpful to have vetted sample code to compare to so thanks for the pointer 
to the w3 page on color conversions. Were you working with a similar reference 
algorithm for the k-means clustering code?

> > 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 MathWorks example you pointed to seems to be clustering in 2D using only 
the chroma components. When I searched for this most sample code was doing the 
same thing and handling luminance separately. However according to Wikipedia 
(which is, of course, infallible) there's precedent for performing clustering 
with luminance included.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/2068#discussion_r3133640080

Reply via email to