Review Request: KColorSpace::KHCY::KHCY(const & QColor) constructor floating point precision error

[Matthew Woehlke]

(Thank you for not quoting my e-mail address unobfuscated in message 
bodies :-).)

Benoit Jacob wrote:
> we seem to be talking of 2 different things. I
> was talking about the specific function in that specific file, where
> the color is read off the RGB components of a QColor. Let's paste the
> code:
> 
> KHCY::KHCY(const QColor& color)
> {
>     qreal r = gamma(color.redF());
>     qreal g = gamma(color.greenF());
>     qreal b = gamma(color.blueF());
>     a = color.alphaF();
> 
>     // luma component
>     y = lumag(r, g, b);
> 
>     // hue component
>     qreal p = qMax(qMax(r, g), b);
>     qreal n = qMin(qMin(r, g), b);
>     qreal d = 6.0 * (p - n);
>     if (n == p)
>         h = 0.0;
>     else if (r == p)
>         h = ((g - b) / d);
>     else if (g == p)
>         h = ((b - r) / d) + (1.0 / 3.0);
>     else
>         h = ((r - g) / d) + (2.0 / 3.0);
> 
>     // chroma component
>     if (r == g && g == b)
>         c = 0.0;
>     else
>         c = qMax( (y - n) / y, (p - y) / (1 - y) );
> }
> 
> I was talking, specifically, about the "n == p", "r==p", etc, here.

Ah. But that's different...

> Here what could easily happen is that all =='s return false (even if
> the numbers are approximately equal) [...]

Please read the code more carefully. If all of these fail (when one 
should succeed), that's strictly a compilation bug, and gcc needs to be 
hit over the head. The purpose of the comparisons is to determine how 
the branching fell when p and n were assigned. They will ALWAYS (sans 
compilation failure) be exactly equal to one of r, g, b. If you consider 
"approximately equal" here, that's similarly likely to land you on the 
wrong branch.

Using a fuzzy compare here would be just plain wrong (except maybe to 
replace 'n == p'). The "right" way would be to rewrite everything so 
that assigning p and n, and calculating h, fall into the same decision 
tree, rather than split as they are presently. (I'd even go so far as to 
/hope/ gcc would do this itself, though probably not.)

Hmm... I'll keep in mind doing that rewrite (doubt I'd get to it today), 
unless you want to take a crack at it?

> These float values are between 0 and 1, the step size between possible
> values being 2^-16 which is roughly 1e-5. The "luma" seems to be used
> only at the last line.

Currently. If QColor changes to higher precision (HDR would be best!), 
this is no longer true. In general I don't want to rewrite the code in 
any way that will make it /more/ broken w.r.t. HDR.

> Even if the small difference is an artifact of floating point
> arithmetic? Looking back at this code:

There is no arithmetic here.

> So instead, the reason why your function works is that the floating
> point numbers at hand happen to have an exact representation so that
> == works for them.

It works as long as they don't get inconsistently truncated between 96 
and 99. I can assure you r, g, b do *not* have exact representation 
(look at gamma()!). Heck, even QColor::{red,green,blue}F don't have 
exact representation.

> So coming back to your requirement to "treat values as different even
> if they differ by a small amoun". As soon as you use floating point
> numbers, the only way that you can aim for this kind of exactness is
> by using a floating point type that has significantly more precision
> than the precision of the values that you want to represent.

I'm debating. On the one hand, qFuzzyCompare should catch 
representational truncation, but at the expense of effectively 
truncating chroma once it drops below a certain point of "really small". 
And since chroma==0 represents a singularity w.r.t. hue, you lose a 
meaningful value of hue at that point.

I really don't want to start treating numbers that came in different as 
being the same.

(This brings up an interesting point; if you keep colors in HCY 
representation, that's actually less lossy than RGB, since you can keep 
a real hue value even for chroma == 0, and real chroma and hue for luma 
== 0.)

> Yes, ok, don't want to waste more of your time...

It's okay. I think there is value in understanding these things. 
Especially as I am on the fence about re-writing the hue calculation. 
Well, and on using a fuzzy compare also, I guess.

-- 
Matthew
Please do not quote my e-mail address unobfuscated in message bodies.
-- 
"Who wants to sing?" -- Orcs (Warcraft II)