Looks good to me. My initial reaction was that we could simplify it but you're right about the rounding. This one gets my r-b too. On Jan 15, 2015 10:52 AM, "Neil Roberts" <<a href="mailto:neil@linux.intel.com">neil@linux.intel.com</a>> wrote: > > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > > > This looks fine to me. We should probably also do this for snorm formats. > > I don't care if that's part of this or in a separate patch. > > --Jason > > The snorm formats are a bit more fiddly because the hardware doesn't > quite seem to be doing what I'd expect. For example, when converting > from 16 bits to 8 bits we effectively want to convert from the range > [-32767,+32767] to [-127,+127]. I think that means we can just use a > formula like this: > > a = 127 * b / 32767 > > This almost works except when the result is close to halfway between two > integers the hardware seems to round in the wrong direction. For example > the input value 16642 according to the formula should be 64.5019, which > you'd hope it would round to 65. However it rounds to 64. > > We could probably bodge the formula to match what Intel hardware does > but that seems a bit cheeky considering this is in generic Mesa code. I > tried to check what NVidia is doing on my old GeForce 9400 with the > binary driver and it seems to be more or less doing the formula but > truncating the fractional part instead of rounding. That of course would > end up with completely different results again so we wouldn't be able to > match both ways of doing it. > > I made the patch below which seems to correctly implement the formula at > least for all values of converting 16-bit to 8-bit. Maybe we could just use > it and accept that it doesn't match the hardware in a few cases. I don't > think it will affect any Piglit tests which was the original incentive > to try and fix this. > > diff --git a/src/mesa/main/format_utils.h b/src/mesa/main/format_utils.h > index 2faaf76..c959b57 100644 > --- a/src/mesa/main/format_utils.h > +++ b/src/mesa/main/format_utils.h > @@ -150,8 +150,19 @@ _mesa_snorm_to_snorm(int x, unsigned src_bits, unsigned dst_bits) > return -MAX_INT(dst_bits); > else if (src_bits < dst_bits) > return EXTEND_NORMALIZED_INT(x, src_bits - 1, dst_bits - 1); > - else > - return x >> (src_bits - dst_bits); > + else { > + /* As an example, when converting from 16 bits to 8 bits, when it gets > + * here x will be in the range [-32767,+32767] and we want to map this > + * to [-127,+127]. The -32768 case will have been handled in the first > + * if-clause above. This just adds 32767 so that it is in the range > + * [0,65534], multiplies it by 254 and then divides by 65534. The > + * division is done with only positive numbers because dividing a > + * negative number has undefined rounding in C < 99. After the division > + * it is subtracted by 127 to put in back in the range [-127,127]. */ > + return (((x + MAX_INT(src_bits)) * (MAX_INT(dst_bits) * 2) + > + MAX_INT(src_bits)) / > + (MAX_INT(src_bits) * 2) - MAX_INT(dst_bits)); > + } > } > > - Neil