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

[Michael Kreitzer]

I considered that as well. The only thing that worries me is IEEE 754 on x86 
seems woefully unreliable for direct comparisons if you're not trying to do 
something very simple (e.g. prevent a divide by zero).

If QColor needs to compute the floating point values by normalizing integer 
values then there is at least a floating point division involved. If by some 
chance the compiler causes one to be popped off the stack into a 64bit double 
while another remains in the 80bit stack you again run into the possibility of 
precision errors (on x86 at least).  This is plausible if QColor computes the 
floating point value on the fly as floating point return values are returned 
via the fpu stack.

The method you commited is probably fine, but I'd love to see an algorithmic 
method to avoid the problem altogether. I'm just not familiar enough with this 
subject to be able to propose one.

Thanks,
Michael

On Monday 22 June 2009 3:55:51 pm Matthew Woehlke wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/868/#review1362
> -----------------------------------------------------------
>
>
> This seems overly complicated (and also wrong if >8-bit color precision is
> in play). Can the 'r', 'g', 'b' qreals be trusted, or do they have similar
> problems? If yes, I think it works (better) to simply replace the entire
> 'if (0.0 == y || 1.0 == y)' test with 'if (r == g && g == b)'?
>
> - Matthew
>
> On 2009-06-21 22:44:32, Michael Kreitzer wrote:
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://reviewboard.kde.org/r/868/
> > -----------------------------------------------------------
> >
> > (Updated 2009-06-21 22:44:32)
> >
> >
> > Review request for kdelibs.
> >
> >
> > Summary
> > -------
> >
> > The constructor KColorSpace::KHCY::KHCY(const & QColor) has a floating
> > point precision error exposed by the -ftree-pre flag in gcc 4.4 which is
> > enabled with -O2 optimizations. The result is an invalid chroma
> > calculation in the corner case of full white (1.0, 1.0, 1.0 in rgb). The
> > patch works around this by performing an integer comparison of r, g, and
> > b and setting chroma to 0.0 if all 3 are equal.
> >
> >
> > This addresses bugs 194703 and 195522.
> >     https://bugs.kde.org/show_bug.cgi?id=194703
> >     https://bugs.kde.org/show_bug.cgi?id=195522
> >
> >
> > Diffs
> > -----
> >
> >   trunk/KDE/kdelibs/kdeui/colors/kcolorspaces.cpp 985087
> >
> > Diff: http://reviewboard.kde.org/r/868/diff
> >
> >
> > Testing
> > -------
> >
> > This patch corrects the test case code in bug 195522. It also removes all
> > color errors when applied to my system wide kdelibs library.
> >
> >
> > Thanks,
> >
> > Michael