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
> <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".
>
> Signed-off-by: Samuel Iglesias Gonsalvez
> <sigles...@igalia.com>
> ---
> 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,
> 0xffff));
> }
> 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,
> 0xffff));
> }
> 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 {
> - SWIZZLE_CONVERT(int16_t, int32_t, src);
> + SWIZZLE_CONVERT(int16_t, int32_t, CLAMP(src,
> -0x8000, 0x7fff));
> }
> break;
> default:
> @@ -891,7 +892,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;
>
>
> Good catch!
>
>
>
> switch (src_type) {
> case GL_FLOAT:
> @@ -940,7 +941,7 @@ convert_int(void *void_dst, int
> num_dst_channels,
> if (normalized) {
> SWIZZLE_CONVERT(int32_t, uint32_t,
> unorm_to_snorm(src, 32, 32));
> } else {
> - SWIZZLE_CONVERT(int32_t, uint32_t, src);
> + SWIZZLE_CONVERT(int32_t, uint32_t, MIN2(src,
> 0x7fffffff));
> }
> break;
> case GL_INT:
> --
> 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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
