On 11.12.2014 14:50, Jason Ekstrand wrote: > > On Dec 10, 2014 8:13 PM, "Michel Dänzer" <mic...@daenzer.net > <mailto:mic...@daenzer.net>> wrote: >> >> On 11.12.2014 01:08, Jason Ekstrand wrote: >> > >> > >> > On Tue, Dec 9, 2014 at 10:53 PM, Michel Dänzer <mic...@daenzer.net > <mailto:mic...@daenzer.net> >> > <mailto:mic...@daenzer.net <mailto:mic...@daenzer.net>>> wrote: >> > >> > On 09.12.2014 21:06, Iago Toral Quiroga wrote: >> > > From: Jason Ekstrand <jason.ekstr...@intel.com > <mailto:jason.ekstr...@intel.com> <mailto:jason.ekstr...@intel.com > <mailto:jason.ekstr...@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 <sigles...@igalia.com > <mailto:sigles...@igalia.com> <mailto:sigles...@igalia.com > <mailto:sigles...@igalia.com>>>: >> > > - Modify unorm_to_float() to avoid doing the division when >> > > x == -MAX_INT(src_bits) >> > > >> > > Reviewed-by: Ian Romanick <ian.d.roman...@intel.com > <mailto:ian.d.roman...@intel.com> <mailto:ian.d.roman...@intel.com > <mailto:ian.d.roman...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev