Re: [Mesa-dev] [PATCH 06/20] mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly

2014-11-19 Thread Samuel Iglesias Gonsálvez 
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,

[Mesa-dev] [PATCH 06/20] mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly

2014-11-18 Thread Iago Toral Quiroga 
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, 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,

Re: [Mesa-dev] [PATCH 06/20] mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly

2014-11-18 Thread Jason Ekstrand 
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 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, 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,