[Mesa-dev,v2] format_utils: Use a more precise conversion when decreasing bits

Submitted by Neil Roberts on Jan. 15, 2015, 4:52 p.m.

Details

Message ID 87r3uw7zqb.fsf@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Neil Roberts Jan. 15, 2015, 4:52 p.m.
Jason Ekstrand <jason@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.

mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Patch hide | download patch | download mbox

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
_______________________________________________

Comments

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" <neil@linux.intel.com> wrote:
>
> Jason Ekstrand <jason@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