[Mesa-dev] [PATCH] Fixed bug in unclamped float to ubyte conversion.

[Brian Paul]

On 06/07/2013 05:20 PM, Stéphane Marchesin wrote:
> Ping, does anyone else want to review this patch?
>
> Stéphane
>
>
> On Fri, May 10, 2013 at 3:56 PM, Manfred Ernst <ernstm at chromium.org> wrote:
>> Problem: The IEEE float optimized version of UNCLAMPED_FLOAT_TO_UBYTE in macros.h
>> computed incorrect results for inputs in the range 0x3f7f0000 (=0.99609375) to
>> 0x3f7f7f80 (=0.99803924560546875) inclusive.  0x3f7f7f80 is the IEEE float
>> value that results in 254.5 when multiplied by 255.  With rounding mode
>> "round to closest even integer", this is the largest float in the range 0.0-1.0
>> that is converted to 254 by the generic implementation of
>> UNCLAMPED_FLOAT_TO_UBYTE.  The IEEE float optimized version incorrectly defined
>> the cut-off for mapping to 255 as 0x3f7f0000 (=255.0/256.0). The same bug was
>> present in the function float_to_ubyte in u_math.h.
>>
>> Fix: The proposed fix replaces the incorrect cut-off value by 0x3f800000, which
>> is the IEEE float representation of 1.0f. 0x3f7f7f81 (or any value in between)
>> would also work, but 1.0f is probably cleaner.
>>
>> The patch does not regress piglit on llvmpipe and on i965 on sandy bridge.
>> Tested-by Stéphane Marchesin <marcheu at chromium.org>
>> Reviewed-by Stéphane Marchesin <marcheu at chromium.org>
>> ---
>>   src/gallium/auxiliary/util/u_math.h | 3 +--
>>   src/mesa/main/macros.h              | 3 +--
>>   2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/util/u_math.h b/src/gallium/auxiliary/util/u_math.h
>> index 607fbec..64d16cb 100644
>> --- a/src/gallium/auxiliary/util/u_math.h
>> +++ b/src/gallium/auxiliary/util/u_math.h
>> @@ -540,14 +540,13 @@ ubyte_to_float(ubyte ub)
>>   static INLINE ubyte
>>   float_to_ubyte(float f)
>>   {
>> -   const int ieee_0996 = 0x3f7f0000;   /* 0.996 or so */
>>      union fi tmp;
>>
>>      tmp.f = f;
>>      if (tmp.i < 0) {
>>         return (ubyte) 0;
>>      }
>> -   else if (tmp.i >= ieee_0996) {
>> +   else if (tmp.i >= 0x3f800000 /* 1.0f */) {
>>         return (ubyte) 255;
>>      }
>>      else {
>> diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
>> index ac24672..8f5b5ae 100644
>> --- a/src/mesa/main/macros.h
>> +++ b/src/mesa/main/macros.h
>> @@ -142,7 +142,6 @@ extern GLfloat _mesa_ubyte_to_float_color_tab[256];
>>    *** CLAMPED_FLOAT_TO_UBYTE: map float known to be in [0,1] to ubyte in [0,255]
>>    ***/
>>   #if defined(USE_IEEE) && !defined(DEBUG)
>> -#define IEEE_0996 0x3f7f0000   /* 0.996 or so */
>>   /* This function/macro is sensitive to precision.  Test very carefully
>>    * if you change it!
>>    */
>> @@ -152,7 +151,7 @@ extern GLfloat _mesa_ubyte_to_float_color_tab[256];
>>              __tmp.f = (F);                                              \
>>              if (__tmp.i < 0)                                            \
>>                 UB = (GLubyte) 0;                                                \
>> -           else if (__tmp.i >= IEEE_0996)                              \
>> +           else if (__tmp.i >= IEEE_ONE)                               \
>>                 UB = (GLubyte) 255;                                      \
>>              else {                                                      \
>>                 __tmp.f = __tmp.f * (255.0F/256.0F) + 32768.0F;          \
>> --
>> 1.8.2.1
>>

Reviewed-by: Brian Paul <brianp at vmware.com>