Re: [AD] Small bugfix in hsv_to_rgb() and smaller fix in rgb_to_hsv()

[Eric Botcazou <ebotcazou@xxxxxxxxxx>]

> I decided to do this because of a bug I discovered in MSVC. I discussed it
> on allegro.cc in the following thread.
> http://www.allegro.cc/forums/view_thread.php?_id=269409 - MSVC 6 floating
> point maths bug.

Interestingly, MSVC doesn't use 'fldz' either so it ends up loading 0.0f.

> To sum up: Sometimes if the MSVC compiler runs out of FPU registers, it
> applys different rounding schemes to the numbers, and in equasions that
> rely on comparing floats for equality, or subtracting equal numbers and
> doing a test for zero to avoid a dvivde-by-zero, it produces strange
> results. I have not been able to reproduce this anomalous behaviour with
> GCC. I found this behaviour on MSVC 6 and I've been told it happens on
> MSVC7 too, but if I manually select the "improve floating point
> consistency" optimisation which isn't turned on by default in MSVC, the
> problem goes away.

This is the very classical problem of the extra-precision of the x87 FPU 
registers (which occurs with GCC too, but not on this particular example, 
and for which the cure is -ffloat-store as you discovered). Your program 
uses floats (32-bit) so each time a FP value is stored in memory, it is 
truncated to 32-bit.

Now contrary to IEEE-754 compliant FPUs (like SPARC e.g.), the x87 FPU 
doesn't feature single (32-bit) and double (64-bit) precision registers. It 
has only a stack of 8 registers whose precision is fixed once for all: 
single, double or extended (80-bit). I think MSVC uses the double-precision 
mode (GCC uses the extended precision mode). So all operations are performed 
in double precision and the result is _never_ truncated to 32-bit if it is 
kept in registers. So there is a discrepancy between values that have been 
stored in memory and values that have been kept in registers. If it happens 
that the same original value takes both paths, weird things can happen as 
you experienced.

The fix is to store the result of each FP operation in memory right after it 
was performed (hence the name -ffloat-store) but this is of course costly 
performance-wise.

> I had another look at the code, and noticed that in the hsv_to_rgb()
> function, the error does not lead to much so you could get away with
> '==0.0f' for hsv_to_rgb(). However, when I was working on my own
> rgb_to_hls() function, this error lead to out-of-range values when doing a
> divide so I used it there and thought I might port it to hsv_to_rgb(), but
> now that does seem to be a bit overkill. If you decide to get rid of the
> '< FLT_EPSILON's, then also make sure you remove the #include <float.h>

Yes, there is no division in the function so we don't need to be excessively 
careful. And loading a floating-point constant is not cheap. Therefore I 
think we'd better not change anything.

-- 
Eric Botcazou