[Mesa-dev] [PATCH 04/14] mesa: Add a mesa utility function _mesa_need_signed_unsigned_int_conversion()

[Iago Toral]

On Tue, 2015-06-16 at 11:15 -0700, Anuj Phogat wrote:
> This utility function is used in a later patch.
> 
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Cc: <mesa-stable at lists.freedesktop.org>
> ---
>  src/mesa/main/readpix.c | 32 ++++++++++++++++++--------------
>  src/mesa/main/readpix.h |  4 ++++
>  2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> index a9416ef..1038983 100644
> --- a/src/mesa/main/readpix.c
> +++ b/src/mesa/main/readpix.c
> @@ -114,6 +114,22 @@ _mesa_get_readpixels_transfer_ops(const struct gl_context *ctx,
>     return transferOps;
>  }
>  
> +bool
> +_mesa_need_signed_unsigned_int_conversion(mesa_format rbFormat,
> +                                          GLenum format, GLenum type)
> +{
> +      const GLenum srcType = _mesa_get_format_datatype(rbFormat);
> +      return (srcType == GL_INT &&
> +              _mesa_is_enum_format_integer(format) &&
> +              (type == GL_UNSIGNED_INT ||
> +               type == GL_UNSIGNED_SHORT ||
> +               type == GL_UNSIGNED_BYTE)) ||
> +             (srcType == GL_UNSIGNED_INT &&
> +              _mesa_is_enum_format_integer(format) &&
> +              (type == GL_INT ||
> +               type == GL_SHORT ||
> +               type == GL_BYTE));
> +}

I think it is better if you assign the result  of
_mesa_is_enum_format_integer(format) to a temporary instead of calling
it twice in the condition, just like you do with srcType.

>  /**
>   * Return true if memcpy cannot be used for ReadPixels.
> @@ -130,7 +146,6 @@ _mesa_readpixels_needs_slow_path(const struct gl_context *ctx, GLenum format,
>  {
>     struct gl_renderbuffer *rb =
>           _mesa_get_read_renderbuffer_for_format(ctx, format);
> -   GLenum srcType;
>  
>     assert(rb);
>  
> @@ -157,20 +172,9 @@ _mesa_readpixels_needs_slow_path(const struct gl_context *ctx, GLenum format,
>  
>        /* Conversion between signed and unsigned integers needs masking
>         * (it isn't just memcpy). */
> -      srcType = _mesa_get_format_datatype(rb->Format);
> -
> -      if ((srcType == GL_INT &&
> -           _mesa_is_enum_format_integer(format) &&
> -           (type == GL_UNSIGNED_INT ||
> -            type == GL_UNSIGNED_SHORT ||
> -            type == GL_UNSIGNED_BYTE)) ||
> -          (srcType == GL_UNSIGNED_INT &&
> -           _mesa_is_enum_format_integer(format) &&
> -           (type == GL_INT ||
> -            type == GL_SHORT ||
> -            type == GL_BYTE))) {
> +      if (_mesa_need_signed_unsigned_int_conversion(rb->Format, format,
> +                                                     type))
>           return GL_TRUE;
> -      }

You need to rebase your patch, this code does not exist any more. I
moved it to Gallium where I am not sure that your change is what they
want. You should probably just skip this part.

With these changes,
Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

>        /* And finally, see if there are any transfer ops. */
>        return _mesa_get_readpixels_transfer_ops(ctx, rb->Format, format, type,
> diff --git a/src/mesa/main/readpix.h b/src/mesa/main/readpix.h
> index f894036..a93e263 100644
> --- a/src/mesa/main/readpix.h
> +++ b/src/mesa/main/readpix.h
> @@ -46,6 +46,10 @@ _mesa_get_readpixels_transfer_ops(const struct gl_context *ctx,
>                                    GLenum format, GLenum type,
>                                    GLboolean uses_blit);
>  
> +extern bool
> +_mesa_need_signed_unsigned_int_conversion(mesa_format rbFormat,
> +                                          GLenum format, GLenum type);
> +
>  extern void
>  _mesa_readpixels(struct gl_context *ctx,
>                   GLint x, GLint y, GLsizei width, GLsizei height,