[Mesa-dev] [PATCH 03/14] mesa: Fix conditions to test signed, unsigned integer format

[Iago Toral]

On Tue, 2015-06-16 at 11:15 -0700, Anuj Phogat wrote:
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Cc: <mesa-stable at lists.freedesktop.org>
> ---
>  src/mesa/main/readpix.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> index caa2648..a9416ef 100644
> --- a/src/mesa/main/readpix.c
> +++ b/src/mesa/main/readpix.c
> @@ -160,10 +160,12 @@ _mesa_readpixels_needs_slow_path(const struct gl_context *ctx, GLenum format,
>        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))) {

As far as I understand this code we are trying to see if we can use
memcpy to directly copy the contents of the framebuffer to the
destination buffer. In that case, as long as the src/dst types have
different sign we can't just use memcpy, right? In fact it looks like we
might need to expand the checks to include the cases where srcType is
GL_(UNSIGNED_)SHORT and GL_(UNSIGNED_)BYTE as well.

That said, I think this code is not necessary with the call to
_mesa_format_matches_format_and_type that we do immediately after this,
since that will check that the framebuffer format exactly matches the
destination format anyway, which is a much tighter check than this. In
fact, a quick piglit run without these checks does not seem to break any
tests on i965. Gallium uses these two functions in a slightly different
way in st_cb_readpixels.c though, so I wonder if their use case requires
these checks to exist in this function anyway.

Iago