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 <
jason.ekstr...@intel.com>
--Jason
On Mon, Dec 1, 2014 at 3:04 AM, Iago Toral Quiroga <ito...@igalia.com>
wrote:
> From: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
>
> 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 <sigles...@igalia.com>
> ---
> 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
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
