[Mesa-dev] [PATCH v2 08/23] mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly
Samuel Iglesias Gonsálvez
siglesias at igalia.com
Tue Dec 2 00:35:09 PST 2014
On Tuesday, December 02, 2014 07:54:19 AM Samuel Iglesias Gonsálvez wrote:
> On Monday, December 01, 2014 10:16:35 AM Jason Ekstrand wrote:
> > 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.ekstrand at intel.com>
> > --Jason
>
> OK, I'm going to work on it.
>
> Thanks,
>
> Sam
>
By the way, I found that float_to_uint() function is unused. So, instead of
tweaking it, I will remove it.
Sam
> > On Mon, Dec 1, 2014 at 3:04 AM, Iago Toral Quiroga <itoral at igalia.com>
> >
> > wrote:
> > > From: Samuel Iglesias Gonsalvez <siglesias at 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 <siglesias at 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 at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141202/88f81800/attachment-0001.sig>
More information about the mesa-dev
mailing list