On Dec 3, 2014 12:41 AM, "Samuel Iglesias Gonsálvez" <<a href="mailto:siglesias@igalia.com">siglesias@igalia.com</a>> wrote: > > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > > On 02/12/14 09:35, Samuel Iglesias Gonsálvez wrote: > > On Tuesday, December 02, 2014 07:54:19 AM Samuel Iglesias Gonsálvez > > wrote: > >> On Monday, December 01, 2014 10:16:35 AM Jason Ekstrand wrote: > >>> This looks much better. Two comments though. First, I think > >>> we need to tweak the float_to_uint function to clamp above. I > >>> don't know that it properly handles values larger than MAX_UINT > >>> right now. Second, I think we may want a MIN_INT macro for > >>> this. In particular, I don't know that this is well-defined to > >>> work for dst_size == 32 because a signed shift by 31 isn't > >>> well-defined according to the C spec. > >>> > >>> With those two fixed, this one is Reviewed-by: Jason Ekstrand > >>> < <a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> --Jason > >> > >> OK, I'm going to work on it. > >> > >> Thanks, > >> > >> Sam > >> > > > > By the way, I found that float_to_uint() function is unused. So, > > instead of tweaking it, I will remove it. > > > > Sam > > > > I'm going to remove float_to_uint() as a separate commit on top of the > series as it is not used. > > If you don't have any objections to this change, I will add your > reviewed-by. > > What do you think? I think my response was unfortunately cryptic. I'm concerned that we aren't clamping float-to-uint conversions from above and were not clamping float-to-int conversions at all. I think we also had this issue in some of the autogeneration. I thought fixing float_to_uint would fix it but apparently its not being used so that would have done nothing. Sorry for the confusion, --Jason > > Sam > > >>> On Mon, Dec 1, 2014 at 3:04 AM, Iago Toral Quiroga > >>> <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> > >>> > >>> wrote: > >>>> From: Samuel Iglesias Gonsalvez <<a href="mailto:siglesias@igalia.com">siglesias@igalia.com</a>> > >>>> > >>>> Fix various conversion paths that involved integer data types > >>>> of different sizes (uint16_t to uint8_t, int16_t to uint8_t, > >>>> etc) that were not being clamped properly. > >>>> > >>>> Also, one of the paths was incorrectly assigning the value > >>>> 12, instead of 1, to the constant "one". > >>>> > >>>> v2: - Create auxiliary clamping functions and use them in all > >>>> paths that > >>>> > >>>> required clamp because of different source and destination > >>>> sizes and signed-unsigned conversions. > >>>> > >>>> Signed-off-by: Samuel Iglesias Gonsalvez > >>>> <<a href="mailto:siglesias@igalia.com">siglesias@igalia.com</a>> --- > >>>> > >>>> src/mesa/main/format_utils.c | 47 > >>>> > >>>> ++++++++++++++++++++++---------------------- > >>>> > >>>> src/mesa/main/format_utils.h | 25 +++++++++++++++++++++++ 2 > >>>> files changed, 48 insertions(+), 24 deletions(-) > >>>> > >>>> diff --git a/src/mesa/main/format_utils.c > >>>> b/src/mesa/main/format_utils.c index 41fd043..dc63e1f 100644 > >>>> --- a/src/mesa/main/format_utils.c +++ > >>>> b/src/mesa/main/format_utils.c @@ -449,7 +449,6 @@ > >>>> convert_half_float(void *void_dst, int num_dst_channels, > >>>> > >>>> } > >>>> > >>>> } > >>>> > >>>> - > >>>> > >>>> static void convert_ubyte(void *void_dst, int > >>>> num_dst_channels, > >>>> > >>>> const void *void_src, GLenum src_type, int num_src_channels, > >>>> > >>>> @@ -469,7 +468,7 @@ convert_ubyte(void *void_dst, int > >>>> num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint8_t, uint16_t, _mesa_half_to_unorm(src, > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint8_t, uint16_t, > >>>> half_to_uint(src)); + SWIZZLE_CONVERT(uint8_t, > >>>> uint16_t, _mesa_unsigned_to_unsigned(half_to_uint(src), 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_UNSIGNED_BYTE: @@ -479,35 +478,35 @@ > >>>> convert_ubyte(void *void_dst, int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint8_t, int8_t, _mesa_snorm_to_unorm(src, > >>>> 8, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint8_t, int8_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint8_t, int8_t, > >>>> _mesa_signed_to_unsigned(src, 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_UNSIGNED_SHORT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint8_t, uint16_t, _mesa_unorm_to_unorm(src, > >>>> 16, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint8_t, uint16_t, src); + > >>>> SWIZZLE_CONVERT(uint8_t, uint16_t, > >>>> _mesa_unsigned_to_unsigned(src, 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_SHORT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint8_t, int16_t, _mesa_snorm_to_unorm(src, > >>>> 16, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint8_t, int16_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint8_t, int16_t, > >>>> _mesa_signed_to_unsigned(src, 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_UNSIGNED_INT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint8_t, uint32_t, _mesa_unorm_to_unorm(src, > >>>> 32, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint8_t, uint32_t, src); + > >>>> SWIZZLE_CONVERT(uint8_t, uint32_t, > >>>> _mesa_unsigned_to_unsigned(src, 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_INT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint8_t, int32_t, _mesa_snorm_to_unorm(src, > >>>> 32, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint8_t, int32_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint8_t, int32_t, > >>>> _mesa_signed_to_unsigned(src, 8)); > >>>> > >>>> } break; > >>>> > >>>> default: @@ -542,7 +541,7 @@ convert_byte(void *void_dst, int > >>>> num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int8_t, uint8_t, _mesa_unorm_to_snorm(src, > >>>> 8, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int8_t, uint8_t, src); + > >>>> SWIZZLE_CONVERT(int8_t, uint8_t, > >>>> _mesa_unsigned_to_signed(src, 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_BYTE: @@ -552,28 +551,28 @@ convert_byte(void > >>>> *void_dst, int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int8_t, uint16_t, _mesa_unorm_to_snorm(src, > >>>> 16, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int8_t, uint16_t, src); + > >>>> SWIZZLE_CONVERT(int8_t, uint16_t, > >>>> _mesa_unsigned_to_signed(src, 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_SHORT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int8_t, int16_t, _mesa_snorm_to_snorm(src, > >>>> 16, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int8_t, int16_t, src); + > >>>> SWIZZLE_CONVERT(int8_t, int16_t, _mesa_signed_to_signed(src, > >>>> 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_UNSIGNED_INT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int8_t, uint32_t, _mesa_unorm_to_snorm(src, > >>>> 32, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int8_t, uint32_t, src); + > >>>> SWIZZLE_CONVERT(int8_t, uint32_t, > >>>> _mesa_unsigned_to_signed(src, 8)); > >>>> > >>>> } break; > >>>> > >>>> case GL_INT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int8_t, int32_t, _mesa_snorm_to_snorm(src, > >>>> 32, > >>>> > >>>> 8)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int8_t, int32_t, src); + > >>>> SWIZZLE_CONVERT(int8_t, int32_t, _mesa_signed_to_signed(src, > >>>> 8)); > >>>> > >>>> } break; > >>>> > >>>> default: @@ -615,7 +614,7 @@ convert_ushort(void *void_dst, > >>>> int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint16_t, int8_t, _mesa_snorm_to_unorm(src, > >>>> 8, > >>>> > >>>> 16)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint16_t, int8_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint16_t, int8_t, > >>>> _mesa_signed_to_unsigned(src, 16)); > >>>> > >>>> } break; > >>>> > >>>> case GL_UNSIGNED_SHORT: @@ -625,21 +624,21 @@ > >>>> convert_ushort(void *void_dst, int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint16_t, int16_t, _mesa_snorm_to_unorm(src, > >>>> 16, > >>>> > >>>> 16)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint16_t, int16_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint16_t, int16_t, > >>>> _mesa_signed_to_unsigned(src, 16)); > >>>> > >>>> } break; > >>>> > >>>> case GL_UNSIGNED_INT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint16_t, uint32_t, > >>>> _mesa_unorm_to_unorm(src, > >>>> > >>>> 32, 16)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint16_t, uint32_t, src); + > >>>> SWIZZLE_CONVERT(uint16_t, uint32_t, > >>>> _mesa_unsigned_to_unsigned(src, 16)); > >>>> > >>>> } break; > >>>> > >>>> case GL_INT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint16_t, int32_t, _mesa_snorm_to_unorm(src, > >>>> 32, > >>>> > >>>> 16)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint16_t, int32_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint16_t, int32_t, > >>>> _mesa_signed_to_unsigned(src, 16)); > >>>> > >>>> } break; > >>>> > >>>> default: @@ -688,7 +687,7 @@ convert_short(void *void_dst, > >>>> int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int16_t, uint16_t, _mesa_unorm_to_snorm(src, > >>>> 16, > >>>> > >>>> 16)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int16_t, uint16_t, src); + > >>>> SWIZZLE_CONVERT(int16_t, uint16_t, > >>>> _mesa_unsigned_to_signed(src, 16)); > >>>> > >>>> } break; > >>>> > >>>> case GL_SHORT: @@ -698,14 +697,14 @@ convert_short(void > >>>> *void_dst, int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int16_t, uint32_t, _mesa_unorm_to_snorm(src, > >>>> 32, > >>>> > >>>> 16)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int16_t, uint32_t, src); + > >>>> SWIZZLE_CONVERT(int16_t, uint32_t, > >>>> _mesa_unsigned_to_signed(src, 16)); > >>>> > >>>> } break; > >>>> > >>>> case GL_INT: if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int16_t, int32_t, _mesa_snorm_to_snorm(src, > >>>> 32, > >>>> > >>>> 16)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int16_t, int32_t, src); + > >>>> SWIZZLE_CONVERT(int16_t, int32_t, > >>>> _mesa_signed_to_signed(src, 16)); > >>>> > >>>> } break; > >>>> > >>>> default: @@ -746,7 +745,7 @@ convert_uint(void *void_dst, int > >>>> num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint32_t, int8_t, _mesa_snorm_to_unorm(src, > >>>> 8, > >>>> > >>>> 32)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint32_t, int8_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint32_t, int8_t, > >>>> _mesa_signed_to_unsigned(src, 32)); > >>>> > >>>> } break; > >>>> > >>>> case GL_UNSIGNED_SHORT: @@ -760,7 +759,7 @@ convert_uint(void > >>>> *void_dst, int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint32_t, int16_t, _mesa_snorm_to_unorm(src, > >>>> 16, > >>>> > >>>> 32)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint32_t, int16_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint32_t, int16_t, > >>>> _mesa_signed_to_unsigned(src, 32)); > >>>> > >>>> } break; > >>>> > >>>> case GL_UNSIGNED_INT: @@ -770,7 +769,7 @@ convert_uint(void > >>>> *void_dst, int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(uint32_t, int32_t, _mesa_snorm_to_unorm(src, > >>>> 32, > >>>> > >>>> 32)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(uint32_t, int32_t, (src < 0) ? 0 : > >>>> src); + SWIZZLE_CONVERT(uint32_t, int32_t, > >>>> _mesa_signed_to_unsigned(src, 32)); > >>>> > >>>> } break; > >>>> > >>>> default: @@ -784,7 +783,7 @@ convert_int(void *void_dst, int > >>>> num_dst_channels, > >>>> > >>>> const void *void_src, GLenum src_type, int num_src_channels, > >>>> const uint8_t swizzle[4], bool normalized, int count) > >>>> > >>>> { > >>>> > >>>> - const int32_t one = normalized ? INT32_MAX : 12; + > >>>> const int32_t one = normalized ? INT32_MAX : 1; > >>>> > >>>> switch (src_type) { > >>>> > >>>> case GL_FLOAT: @@ -833,7 +832,7 @@ convert_int(void > >>>> *void_dst, int num_dst_channels, > >>>> > >>>> if (normalized) { > >>>> > >>>> SWIZZLE_CONVERT(int32_t, uint32_t, _mesa_unorm_to_snorm(src, > >>>> 32, > >>>> > >>>> 32)); > >>>> > >>>> } else { > >>>> > >>>> - SWIZZLE_CONVERT(int32_t, uint32_t, src); + > >>>> SWIZZLE_CONVERT(int32_t, uint32_t, > >>>> _mesa_unsigned_to_signed(src, 32)); > >>>> > >>>> } break; > >>>> > >>>> case GL_INT: diff --git a/src/mesa/main/format_utils.h > >>>> b/src/mesa/main/format_utils.h index df53edf..6d27752 100644 > >>>> --- a/src/mesa/main/format_utils.h +++ > >>>> b/src/mesa/main/format_utils.h @@ -32,6 +32,7 @@ > >>>> > >>>> #define FORMAT_UTILS_H > >>>> > >>>> #include "imports.h" > >>>> > >>>> +#include "macros.h" > >>>> > >>>> /* Only guaranteed to work for BITS <= 32 */ #define > >>>> MAX_UINT(BITS) ((BITS) == 32 ? UINT32_MAX : ((1u << (BITS)) > >>>> - 1)) > >>>> > >>>> @@ -138,6 +139,30 @@ _mesa_snorm_to_snorm(int x, unsigned > >>>> src_bits, unsigned dst_bits) > >>>> > >>>> return x >> (src_bits - dst_bits); > >>>> > >>>> } > >>>> > >>>> +static inline unsigned +_mesa_unsigned_to_unsigned(unsigned > >>>> src, unsigned dst_size) +{ + return MIN2(src, > >>>> MAX_UINT(dst_size)); +} + +static inline int > >>>> +_mesa_unsigned_to_signed(unsigned src, unsigned dst_size) > >>>> +{ + return MIN2(src, MAX_INT(dst_size)); +} + +static > >>>> inline int +_mesa_signed_to_signed(int src, unsigned > >>>> dst_size) +{ + return CLAMP(src, -(1 << (dst_size -1)), > >>>> MAX_INT(dst_size)); +} > >>> > >>> + > >>> > >>>> +static inline unsigned +_mesa_signed_to_unsigned(int src, > >>>> unsigned dst_size) +{ + return CLAMP(src, 0, > >>>> MAX_UINT(dst_size)); +} + > >>>> > >>>> bool _mesa_format_to_array(mesa_format, GLenum *type, int > >>>> *num_components, > >>>> > >>>> uint8_t swizzle[4], bool *normalized); > >>>> > >>>> -- 1.9.1 > >>>> > >>>> _______________________________________________ mesa-dev > >>>> mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > >>>> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > >>>> > >>>> > >>>> _______________________________________________ mesa-dev > >>>> mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > >>>> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1 > > iQIcBAEBAgAGBQJUfsyEAAoJEH/0ujLxfcND0jIP/01ZjJIgQBcQ8aR1Ctq8+HUu > ie4lyaDIwNAPddDOXx7eLSLdvkjPz0gGlhZCFt21wy0jcDUbpbd6wpJI3/bYsFQF > KaeiqQE1A+Lo4MIoC+qCXIqJpMUbp0bpKup7m0ae+8CX4wIs6756Z+rkUnEqoXeG > 1YSJTOD3fFY5Iibl9+XUoS5g77Rd0xWIKDRL67MYY5wTpOBIyZQp3IZhTP1i0bVR > l+qFLZ8wSszNJHviqT3tcpdRB+cMDtXJ6oPzrO7ArrxVjHat7m5x++40B1lrprxG > BL0JdSaPGBdIM7rKWvYn6gVgwFsFvB+OIfZyzlQTWyCkmUoKWh8OkTjw0TaA8Dbc > xT+yGU8MXjaOFMH0ZJh9tZXEXZgvhu5VLsCw6Pl1RxwejdlzlB4W7dqx3NkXmy3G > JajO/1SewzgVv7yXQMeLEpspOVqykuQyxoTUZkbaib6EGzT33gVryIBO4yQIYSFr > LKEmnL4WGLN9WUr9hH2vu5vcNjSs8KfvAMVofYxU6GaB8H33y172BmXKAHBLyC6h > LlpH6aphvpGaD9P/QCttPNWx1mG+8d9T5N4eECI1kMFAze0NdlNFBVm7Hsy7oO7W > oM0MOL7JAQqCoVVu+FUfQSbmSzVjDfMon3ztMw5fcGn7cPhogZCT3Rgz31+Vxvqz > DChzSgBf/7pGjq61egN9 > =Q5KP > -----END PGP SIGNATURE-----