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

[Benoit Jacob]

First of all:

2009/6/25 Matthew Woehlke:
> Additionally, treating arbitrarily-small values as 0 would be /wrong/, since
> there is no constraint against having the not-quite-black color 1.7e-50,
> 1.4e-50, 1.9e-42. This color has a pretty strong chroma despite that it's
> luma is ludicrously small.

ah, interesting, but 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.
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.

So what you say about numbers that can be 1.7e-50, seems to be a
different thing. But actually this supports my point much better: as i
said in my previous email, in practice the above-posted code happens
to work; but on the other hand general floating point can fail in many
ways if you do exact comparisons, so i'm glad you bring up that you're
also doing general floating point stuff, as it makes this discussion
much more relevant ;)

> Benoit Jacob wrote:
>>
>> Guys, maybe I see this a bit late, but the file in SVN still uses
>> operator== between qreal's and that's a "no go".
>
> In this case I'm not sure I agree. I /want/ to treat the values as not equal
> even if they differ by a small amount;

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

    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);

Here what could easily happen is that all =='s return false (even if
the numbers are approximately equal) so you fall in the last case more
often that you should. It doesn't seem to me that that's acceptable!
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. I was saying that this is a dangerous assumption to
rely on, though i'm happy to learn that this is covered by a unit
test!

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.
You then use fuzzy compares with a precision between these two precisions.
Example: You want 10 significant digits, use double (15 significant
digits) with a fuzzy compare that uses, for example, 1e-12 (so 12
significant digits) as precision level.

> (Besides, this code is already slow, and simultaneously something you might
> want in a critical path. I'm not entirely comfortable making it even
> slower...)

Since the values above are in 0..1 and in the above code they can
really (except for y, but this doesn't appear in a comparison) be
considered 0 as soon as they are negligible when compared to 1, you
can really use the faster specialized fuzzy compares and I doubt that
that would slow down your code significantly; rather, it seems to me
that the main  room for improvement is in the conversion between
QColor's internal representation (16 bit ints, apparently not publicly
exposed) and the qreals. For that kind of reason i'm not sure if
QColor is suitable for high performance code.

> Hmm... right now you pointed out that the code as-is should always work. And
> for the moment, it /is/ working (and there is now a unit test in place to
> catch if it starts misbehaving the same way again).

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

> I'm really not sure I want to change this. Right now, the precision of
> QColor means it shouldn't ever break. Conversely, if QColor ever gets to
> where 0.9999999999, 0.9999999975, 1.0 can be represented, I'm worried that a
> fuzzy compare will wrongly consider this "pure white", which /it isn't/. (In
> fact, this color has chroma == 1.0.)

As soon as one talks about floating point, one has to distinguish
between what meaningful values can be represented, and the actual
floating point numbers used to represent them. The latter must have
significantly more significant digits than the former...

>> *** But I just want to avoid division by 0 ! So for an expression like
>> a/(b-c) the check that I want is really b==c, right ?
>
> More specifically, catch anything that will cause the result of that
> expression to be greater than one. For now. If we ever go HDR, then the
> constraints need to be re-thought. (Maybe. Actually I don't think chroma can
> exceed 1.0 unless you have "darklight", i.e. negative RGB values.)

ok, i agree that here you're not hitting that kind of issue.

>
> Additionally, treating arbitrarily-small values as 0 would be /wrong/, since
> there is no constraint against having the not-quite-black color 1.7e-50,
> 1.4e-50, 1.9e-42. This color has a pretty strong chroma despite that it's
> luma is ludicrously small.

again we're talking about different things, the comparisons weren't
done on the luma but on rgb components that came from integer
channels.


Benoit