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" <n...@linux.intel.com> wrote:
>
> Jason Ekstrand <ja...@jlekstrand.net> 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
