On Dec 3, 2014 12:41 AM, "Samuel Iglesias Gonsálvez" <sigles...@igalia.com>
wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> On 02/12/14 09:35, Samuel Iglesias Gonsálvez wrote:
> > 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.ekstr...@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
> >
>
> I'm going to remove float_to_uint() as a separate commit on top of the
> series as it is not used.
>
> If you don't have any objections to this change, I will add your
> reviewed-by.
>
> What do you think?
I think my response was unfortunately cryptic. I'm concerned that we
aren't clamping float-to-uint conversions from above and were not clamping
float-to-int conversions at all. I think we also had this issue in some of
the autogeneration. I thought fixing float_to_uint would fix it but
apparently its not being used so that would have done nothing.
Sorry for the confusion,
--Jason
>
> Sam
>
> >>> 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
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
>
> iQIcBAEBAgAGBQJUfsyEAAoJEH/0ujLxfcND0jIP/01ZjJIgQBcQ8aR1Ctq8+HUu
> ie4lyaDIwNAPddDOXx7eLSLdvkjPz0gGlhZCFt21wy0jcDUbpbd6wpJI3/bYsFQF
> KaeiqQE1A+Lo4MIoC+qCXIqJpMUbp0bpKup7m0ae+8CX4wIs6756Z+rkUnEqoXeG
> 1YSJTOD3fFY5Iibl9+XUoS5g77Rd0xWIKDRL67MYY5wTpOBIyZQp3IZhTP1i0bVR
> l+qFLZ8wSszNJHviqT3tcpdRB+cMDtXJ6oPzrO7ArrxVjHat7m5x++40B1lrprxG
> BL0JdSaPGBdIM7rKWvYn6gVgwFsFvB+OIfZyzlQTWyCkmUoKWh8OkTjw0TaA8Dbc
> xT+yGU8MXjaOFMH0ZJh9tZXEXZgvhu5VLsCw6Pl1RxwejdlzlB4W7dqx3NkXmy3G
> JajO/1SewzgVv7yXQMeLEpspOVqykuQyxoTUZkbaib6EGzT33gVryIBO4yQIYSFr
> LKEmnL4WGLN9WUr9hH2vu5vcNjSs8KfvAMVofYxU6GaB8H33y172BmXKAHBLyC6h
> LlpH6aphvpGaD9P/QCttPNWx1mG+8d9T5N4eECI1kMFAze0NdlNFBVm7Hsy7oO7W
> oM0MOL7JAQqCoVVu+FUfQSbmSzVjDfMon3ztMw5fcGn7cPhogZCT3Rgz31+Vxvqz
> DChzSgBf/7pGjq61egN9
> =Q5KP
> -----END PGP SIGNATURE-----
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
