[Mesa-dev] [PATCH v3 01/24] mesa/format_utils: Fix a bug in unorm_to_float helper function

[Michel Dänzer]

On 11.12.2014 14:50, Jason Ekstrand wrote:
> 
> On Dec 10, 2014 8:13 PM, "Michel Dänzer" <michel at daenzer.net
> <mailto:michel at daenzer.net>> wrote:
>>
>> On 11.12.2014 01:08, Jason Ekstrand wrote:
>> >
>> >
>> > On Tue, Dec 9, 2014 at 10:53 PM, Michel Dänzer <michel at daenzer.net
> <mailto:michel at daenzer.net>
>> > <mailto:michel at daenzer.net <mailto:michel at daenzer.net>>> wrote:
>> >
>> >     On 09.12.2014 21:06, Iago Toral Quiroga wrote:
>> >     > From: Jason Ekstrand <jason.ekstrand at intel.com
> <mailto:jason.ekstrand at intel.com> <mailto:jason.ekstrand at intel.com
> <mailto:jason.ekstrand at intel.com>>>
>> >     >
>> >     > This patch fixes the return of a wrong value when x is lower than
>> >     > -MAX_INT(src_bits) as the result would not be between [-1.0 1.0].
>> >     >
>> >     > v2 by Samuel Iglesias <siglesias at igalia.com
> <mailto:siglesias at igalia.com> <mailto:siglesias at igalia.com
> <mailto:siglesias at igalia.com>>>:
>> >     >     - Modify unorm_to_float() to avoid doing the division when
>> >     >       x == -MAX_INT(src_bits)
>> >     >
>> >     > Reviewed-by: Ian Romanick <ian.d.romanick at intel.com
> <mailto:ian.d.romanick at intel.com> <mailto:ian.d.romanick at intel.com
> <mailto:ian.d.romanick at intel.com>>>
>> >     > ---
>> >     >  src/mesa/main/format_utils.c | 2 +-
>> >     >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >     >
>> >     > diff --git a/src/mesa/main/format_utils.c
> b/src/mesa/main/format_utils.c
>> >     > index 93a0cea..5dd0848 100644
>> >     > --- a/src/mesa/main/format_utils.c
>> >     > +++ b/src/mesa/main/format_utils.c
>> >     > @@ -152,7 +152,7 @@ unorm_to_float(unsigned x, unsigned src_bits)
>> >     >  static inline float
>> >     >  snorm_to_float(int x, unsigned src_bits)
>> >     >  {
>> >     > -   if (x == -MAX_INT(src_bits))
>> >     > +   if (x <= -MAX_INT(src_bits))
>> >     >        return -1.0f;
>> >     >     else
>> >     >        return x * (1.0f / (float)MAX_INT(src_bits));
>> >     >
>> >
>> >     The commit log talks about unorm_to_float, but the code modifies
>> >     snorm_to_float. Also, I think something like
>> >
>> >     mesa: Fix clamping to -1.0 in snorm_to_float
>> >
>> >     would be a more useful shortlog.
>> >
>> >
>> >     BTW, don't this function and unorm_to_float also need corresponding
>> >     clamping to 1.0 for values >= MAX_INT(src_bits)?
>> >
>> >
>> > Yes and no.  Every place that we use [us]norm_to_*, we assume that the
>> > value passed in is represented by the given number of bits.  Therefore,
>> > clamping above should always be useless.  The one edge case is the
>> > signed value MIN_INT(src_bits) (or, if you prefer, -MAX_INT(src_bits) -
>> > 1) in which is representable in the given number of bits but is actually
>> > less than -1 according to the formula.  Technically, using an equality
>> > there was Ok, it's just that I had it equal to the wrong thing.
>>
>> So, is returning -1.0 for -MAX_INT(src_bits) correct then, or should the
>> test be '== MIN_INT(src_bits)' or '== -MAX_INT(src_bits) - 1' or either
>> variant using <=?
> 
> -MAX_INT(src_bits) will map to -1.0 even without the if statement (just
> look at the formula).  The problem is that, thanks to the clamping
> implicit in the conversion formulas, so should -MAX_INT(src_bits)-1. 
> Therefore, any of "< -MAX_INT(src_bits)", "== -MAX_INT(src_bits) - 1",
> or "<= -MAX_INT(src_bits)" will work.  The best (most clear) would
> probably be "< -MAX_INT(src_bits)".  If I recall correctly, I decided on
> the other simply because, while we have the branch in there anyway, we
> might as well cut the calculation when we can.

Actually, you had 'x < -MAX_INT(src_bits)' in your original patch, and I
pointed out that 'x <= -MAX_INT(src_bits)' would save the calculation in
the == -MAX_INT(src_bits) case. Sorry for coming full circle on this.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer