Re: [Mesa-dev] [PATCH 06/20] mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly
On Tue, 2014-11-18 at 10:34 -0800, Jason Ekstrand wrote:
> In general, I like this patch. However, if we are going to claim to
> follow the GL rule of "Colors are clampped to their representable
> range, then there still seem to be quite a few cases missing. For
> instance, when going from signed to unsigned 8-bit formats, we should
> should take a min with 0 and when converting from unsigned to signed,
> we should take a max with 0x7f. Maybe we need to add integer helper
> functions like we have for the normalized ones and use those just to
> make sure we don't miss anything.
>
OK, I'll do that for the second version.
Thanks!
Sam
> --Jason
>
>
> On Tue, Nov 18, 2014 at 12:43 AM, Iago Toral Quiroga
> wrote:
> From: Samuel Iglesias Gonsalvez
>
> 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".
>
> Signed-off-by: Samuel Iglesias Gonsalvez
>
> ---
> src/mesa/main/format_utils.c | 33
> +
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/src/mesa/main/format_utils.c
> b/src/mesa/main/format_utils.c
> index b6d0fbc..8040173 100644
> --- a/src/mesa/main/format_utils.c
> +++ b/src/mesa/main/format_utils.c
> @@ -24,6 +24,7 @@
>
> #include "format_utils.h"
> #include "glformats.h"
> +#include "macros.h"
>
> static const uint8_t map_identity[7] = { 0, 1, 2, 3, 4, 5,
> 6 };
> static const uint8_t map_3210[7] = { 3, 2, 1, 0, 4, 5, 6 };
> @@ -593,28 +594,28 @@ convert_ubyte(void *void_dst, int
> num_dst_channels,
>if (normalized) {
> SWIZZLE_CONVERT(uint8_t, uint16_t,
> unorm_to_unorm(src, 16, 8));
>} else {
> - SWIZZLE_CONVERT(uint8_t, uint16_t, src);
> + SWIZZLE_CONVERT(uint8_t, uint16_t, MIN2(src, 0xff));
>}
>break;
> case GL_SHORT:
>if (normalized) {
> SWIZZLE_CONVERT(uint8_t, int16_t,
> snorm_to_unorm(src, 16, 8));
>} else {
> - SWIZZLE_CONVERT(uint8_t, int16_t, (src < 0) ? 0 :
> src);
> + SWIZZLE_CONVERT(uint8_t, int16_t, CLAMP(src, 0,
> 0xff));
>}
>break;
> case GL_UNSIGNED_INT:
>if (normalized) {
> SWIZZLE_CONVERT(uint8_t, uint32_t,
> unorm_to_unorm(src, 32, 8));
>} else {
> - SWIZZLE_CONVERT(uint8_t, uint32_t, src);
> + SWIZZLE_CONVERT(uint8_t, uint32_t, MIN2(src, 0xff));
>}
>break;
> case GL_INT:
>if (normalized) {
> SWIZZLE_CONVERT(uint8_t, int32_t,
> snorm_to_unorm(src, 32, 8));
>} else {
> - SWIZZLE_CONVERT(uint8_t, int32_t, (src < 0) ? 0 :
> src);
> + SWIZZLE_CONVERT(uint8_t, int32_t, CLAMP(src, 0,
> 0xff));
>}
>break;
> default:
> @@ -649,7 +650,7 @@ convert_byte(void *void_dst, int
> num_dst_channels,
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, uint8_t, unorm_to_snorm(src,
> 8, 8));
>} else {
> - SWIZZLE_CONVERT(int8_t, uint8_t, src);
> + SWIZZLE_CONVERT(int8_t, uint8_t, MIN2(src, 0x7f));
>}
>break;
> case GL_BYTE:
> @@ -659,28 +660,28 @@ convert_byte(void *void_dst, int
> num_dst_channels,
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, uint16_t,
> unorm_to_snorm(src, 16, 8));
>} else {
> - SWIZZLE_CONVERT(int8_t, uint16_t, src);
> + SWIZZLE_CONVERT(int8_t, uint16_t, MIN2(src, 0x7f));
>}
>break;
> case GL_SHORT:
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, int16_t, snorm_to_snorm(src,
> 16, 8));
>} else {
> - SWIZZLE_CONVERT(int8_t, int16_t, src);
> + SWIZZLE_CONVERT(int8_t, int16_t, CLAMP(src, -0x80,
> 0x7f));
>}
>break;
> case GL_UNSIGNED_INT:
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, uint32_t,
> unorm_to_snorm(src, 32, 8));
>} else {
> - SWIZZLE_CONVERT(int8_
Re: [Mesa-dev] [PATCH 06/20] mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly
In general, I like this patch. However, if we are going to claim to follow
the GL rule of "Colors are clampped to their representable range, then
there still seem to be quite a few cases missing. For instance, when going
from signed to unsigned 8-bit formats, we should should take a min with 0
and when converting from unsigned to signed, we should take a max with
0x7f. Maybe we need to add integer helper functions like we have for the
normalized ones and use those just to make sure we don't miss anything.
--Jason
On Tue, Nov 18, 2014 at 12:43 AM, Iago Toral Quiroga
wrote:
> From: Samuel Iglesias Gonsalvez
>
> 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".
>
> Signed-off-by: Samuel Iglesias Gonsalvez
> ---
> src/mesa/main/format_utils.c | 33 +
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
> index b6d0fbc..8040173 100644
> --- a/src/mesa/main/format_utils.c
> +++ b/src/mesa/main/format_utils.c
> @@ -24,6 +24,7 @@
>
> #include "format_utils.h"
> #include "glformats.h"
> +#include "macros.h"
>
> static const uint8_t map_identity[7] = { 0, 1, 2, 3, 4, 5, 6 };
> static const uint8_t map_3210[7] = { 3, 2, 1, 0, 4, 5, 6 };
> @@ -593,28 +594,28 @@ convert_ubyte(void *void_dst, int num_dst_channels,
>if (normalized) {
> SWIZZLE_CONVERT(uint8_t, uint16_t, unorm_to_unorm(src, 16, 8));
>} else {
> - SWIZZLE_CONVERT(uint8_t, uint16_t, src);
> + SWIZZLE_CONVERT(uint8_t, uint16_t, MIN2(src, 0xff));
>}
>break;
> case GL_SHORT:
>if (normalized) {
> SWIZZLE_CONVERT(uint8_t, int16_t, snorm_to_unorm(src, 16, 8));
>} else {
> - SWIZZLE_CONVERT(uint8_t, int16_t, (src < 0) ? 0 : src);
> + SWIZZLE_CONVERT(uint8_t, int16_t, CLAMP(src, 0, 0xff));
>}
>break;
> case GL_UNSIGNED_INT:
>if (normalized) {
> SWIZZLE_CONVERT(uint8_t, uint32_t, unorm_to_unorm(src, 32, 8));
>} else {
> - SWIZZLE_CONVERT(uint8_t, uint32_t, src);
> + SWIZZLE_CONVERT(uint8_t, uint32_t, MIN2(src, 0xff));
>}
>break;
> case GL_INT:
>if (normalized) {
> SWIZZLE_CONVERT(uint8_t, int32_t, snorm_to_unorm(src, 32, 8));
>} else {
> - SWIZZLE_CONVERT(uint8_t, int32_t, (src < 0) ? 0 : src);
> + SWIZZLE_CONVERT(uint8_t, int32_t, CLAMP(src, 0, 0xff));
>}
>break;
> default:
> @@ -649,7 +650,7 @@ convert_byte(void *void_dst, int num_dst_channels,
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, uint8_t, unorm_to_snorm(src, 8, 8));
>} else {
> - SWIZZLE_CONVERT(int8_t, uint8_t, src);
> + SWIZZLE_CONVERT(int8_t, uint8_t, MIN2(src, 0x7f));
>}
>break;
> case GL_BYTE:
> @@ -659,28 +660,28 @@ convert_byte(void *void_dst, int num_dst_channels,
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, uint16_t, unorm_to_snorm(src, 16, 8));
>} else {
> - SWIZZLE_CONVERT(int8_t, uint16_t, src);
> + SWIZZLE_CONVERT(int8_t, uint16_t, MIN2(src, 0x7f));
>}
>break;
> case GL_SHORT:
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, int16_t, snorm_to_snorm(src, 16, 8));
>} else {
> - SWIZZLE_CONVERT(int8_t, int16_t, src);
> + SWIZZLE_CONVERT(int8_t, int16_t, CLAMP(src, -0x80, 0x7f));
>}
>break;
> case GL_UNSIGNED_INT:
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, uint32_t, unorm_to_snorm(src, 32, 8));
>} else {
> - SWIZZLE_CONVERT(int8_t, uint32_t, src);
> + SWIZZLE_CONVERT(int8_t, uint32_t, MIN2(src, 0x7f));
>}
>break;
> case GL_INT:
>if (normalized) {
> SWIZZLE_CONVERT(int8_t, int32_t, snorm_to_snorm(src, 32, 8));
>} else {
> - SWIZZLE_CONVERT(int8_t, int32_t, src);
> + SWIZZLE_CONVERT(int8_t, int32_t, CLAMP(src, -0x80, 0x7f));
>}
>break;
> default:
> @@ -739,14 +740,14 @@ convert_ushort(void *void_dst, int num_dst_channels,
>if (normalized) {
> SWIZZLE_CONVERT(uint16_t, uint32_t, unorm_to_unorm(src, 32, 16));
>} else {
> - SWIZZLE_CONVERT(uint16_t, uint32_t, src);
> + SWIZZLE_CONVERT(uint16_t, uint32_t, MIN2(src, 0x));
>}
>break;
> case GL_INT:
>if (normalized) {
> SWIZZLE_CONVERT(uint16_t, int32_t, snorm_to_unorm(src, 32, 16));
>} else {
> - SWIZZLE_CONVERT(uint16_t, int32_t, (src < 0) ? 0 : src);
> + SWIZZLE_CONVERT(uint16_t, int32_t, CLAMP(src, 0, 0x));
>}
>break;
> default:
> @
[Mesa-dev] [PATCH 06/20] mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly
From: Samuel Iglesias Gonsalvez
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".
Signed-off-by: Samuel Iglesias Gonsalvez
---
src/mesa/main/format_utils.c | 33 +
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index b6d0fbc..8040173 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -24,6 +24,7 @@
#include "format_utils.h"
#include "glformats.h"
+#include "macros.h"
static const uint8_t map_identity[7] = { 0, 1, 2, 3, 4, 5, 6 };
static const uint8_t map_3210[7] = { 3, 2, 1, 0, 4, 5, 6 };
@@ -593,28 +594,28 @@ convert_ubyte(void *void_dst, int num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(uint8_t, uint16_t, unorm_to_unorm(src, 16, 8));
} else {
- SWIZZLE_CONVERT(uint8_t, uint16_t, src);
+ SWIZZLE_CONVERT(uint8_t, uint16_t, MIN2(src, 0xff));
}
break;
case GL_SHORT:
if (normalized) {
SWIZZLE_CONVERT(uint8_t, int16_t, snorm_to_unorm(src, 16, 8));
} else {
- SWIZZLE_CONVERT(uint8_t, int16_t, (src < 0) ? 0 : src);
+ SWIZZLE_CONVERT(uint8_t, int16_t, CLAMP(src, 0, 0xff));
}
break;
case GL_UNSIGNED_INT:
if (normalized) {
SWIZZLE_CONVERT(uint8_t, uint32_t, unorm_to_unorm(src, 32, 8));
} else {
- SWIZZLE_CONVERT(uint8_t, uint32_t, src);
+ SWIZZLE_CONVERT(uint8_t, uint32_t, MIN2(src, 0xff));
}
break;
case GL_INT:
if (normalized) {
SWIZZLE_CONVERT(uint8_t, int32_t, snorm_to_unorm(src, 32, 8));
} else {
- SWIZZLE_CONVERT(uint8_t, int32_t, (src < 0) ? 0 : src);
+ SWIZZLE_CONVERT(uint8_t, int32_t, CLAMP(src, 0, 0xff));
}
break;
default:
@@ -649,7 +650,7 @@ convert_byte(void *void_dst, int num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(int8_t, uint8_t, unorm_to_snorm(src, 8, 8));
} else {
- SWIZZLE_CONVERT(int8_t, uint8_t, src);
+ SWIZZLE_CONVERT(int8_t, uint8_t, MIN2(src, 0x7f));
}
break;
case GL_BYTE:
@@ -659,28 +660,28 @@ convert_byte(void *void_dst, int num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(int8_t, uint16_t, unorm_to_snorm(src, 16, 8));
} else {
- SWIZZLE_CONVERT(int8_t, uint16_t, src);
+ SWIZZLE_CONVERT(int8_t, uint16_t, MIN2(src, 0x7f));
}
break;
case GL_SHORT:
if (normalized) {
SWIZZLE_CONVERT(int8_t, int16_t, snorm_to_snorm(src, 16, 8));
} else {
- SWIZZLE_CONVERT(int8_t, int16_t, src);
+ SWIZZLE_CONVERT(int8_t, int16_t, CLAMP(src, -0x80, 0x7f));
}
break;
case GL_UNSIGNED_INT:
if (normalized) {
SWIZZLE_CONVERT(int8_t, uint32_t, unorm_to_snorm(src, 32, 8));
} else {
- SWIZZLE_CONVERT(int8_t, uint32_t, src);
+ SWIZZLE_CONVERT(int8_t, uint32_t, MIN2(src, 0x7f));
}
break;
case GL_INT:
if (normalized) {
SWIZZLE_CONVERT(int8_t, int32_t, snorm_to_snorm(src, 32, 8));
} else {
- SWIZZLE_CONVERT(int8_t, int32_t, src);
+ SWIZZLE_CONVERT(int8_t, int32_t, CLAMP(src, -0x80, 0x7f));
}
break;
default:
@@ -739,14 +740,14 @@ convert_ushort(void *void_dst, int num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(uint16_t, uint32_t, unorm_to_unorm(src, 32, 16));
} else {
- SWIZZLE_CONVERT(uint16_t, uint32_t, src);
+ SWIZZLE_CONVERT(uint16_t, uint32_t, MIN2(src, 0x));
}
break;
case GL_INT:
if (normalized) {
SWIZZLE_CONVERT(uint16_t, int32_t, snorm_to_unorm(src, 32, 16));
} else {
- SWIZZLE_CONVERT(uint16_t, int32_t, (src < 0) ? 0 : src);
+ SWIZZLE_CONVERT(uint16_t, int32_t, CLAMP(src, 0, 0x));
}
break;
default:
@@ -795,7 +796,7 @@ convert_short(void *void_dst, int num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(int16_t, uint16_t, unorm_to_snorm(src, 16, 16));
} else {
- SWIZZLE_CONVERT(int16_t, uint16_t, src);
+ SWIZZLE_CONVERT(int16_t, uint16_t, (src < 0) ? 0 : src);
}
break;
case GL_SHORT:
@@ -805,14 +806,14 @@ convert_short(void *void_dst, int num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(int16_t, uint32_t, unorm_to_snorm(src, 32, 16));
} else {
- SWIZZLE_CONVERT(int16_t, uint32_t, src);
+ SWIZZLE_CONVERT(int16_t, uint32_t, MIN2(src, 0x7fff));
}
break;
case GL_INT:
if (normalized) {
SWIZZLE_CONVERT(int16_t, int32_t, snorm_to_snorm(src, 32, 16));
} else {
-
