<div dir="ltr"><div>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. </div><div>With those two fixed, this one is Reviewed-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> </div>--Jason <div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Dec 1, 2014 at 3:04 AM, Iago Toral Quiroga <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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)); +} </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +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" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div></div>