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

Message ID | 87r3uw7zqb.fsf@intel.com |
---|---|

State | New |

Headers | show |

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 _______________________________________________

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