[Mesa-dev] [PATCH] mesa: remove unnecessary checks in _mesa_readpixels_needs_slow_path

[Anuj Phogat]

On Tue, Jun 23, 2015 at 3:34 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
> readpixels_can_use_memcpy will later call _mesa_format_matches_format_and_type
> which does much tighter checks than these to decide if we can use
> memcpy for readpixels.
>
> Also, the checks do not seem to be extensive enough anyway, since we are
> checking for signed/unsigned conversion only when the framebuffer has integers,
> but the same checks could be done for other types anyway, since as long as
> there is a signed/unsigned conversion we can't memcpy.
>
> No regressions observed on i965/llvmpipe.
> ---
> The way gallium uses _mesa_format_matches_format_and_type and
> _mesa_readpixels_needs_slow_path is a bit different, they call these
> functions to decide if they want to fallback to Mesa's implementation
> of ReadPixels, not exactly to check if we can memcpy... so it is unclear
> to me if some combinations may still need these checks to make the right
> decision. I did not see any regressions with llvmpipe at least, so I am
> assuming that they are not needed, but maybe someone wants to give this
> patch a test run on nouveau or radeon, just in case. If they are needed
> I guess the right thing would be to move the checks to st_cb_readpixels.c.
>
>  src/mesa/main/readpix.c | 16 ----------------
>  1 file changed, 16 deletions(-)
>
> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> index a3357cd..e256695 100644
> --- a/src/mesa/main/readpix.c
> +++ b/src/mesa/main/readpix.c
> @@ -128,7 +128,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);
>
> @@ -153,21 +152,6 @@ _mesa_readpixels_needs_slow_path(const struct gl_context *ctx, GLenum format,
>           return GL_TRUE;
>        }
>
> -      /* Conversion between signed and unsigned integers needs masking
> -       * (it isn't just memcpy). */
> -      srcType = _mesa_get_format_datatype(rb->Format);
> -
> -      if ((srcType == GL_INT &&
> -           (type == GL_UNSIGNED_INT ||
> -            type == GL_UNSIGNED_SHORT ||
> -            type == GL_UNSIGNED_BYTE)) ||
> -          (srcType == GL_UNSIGNED_INT &&
> -           (type == GL_INT ||
> -            type == GL_SHORT ||
> -            type == GL_BYTE))) {
> -         return GL_TRUE;
> -      }
> -
>        /* And finally, see if there are any transfer ops. */
>        return get_readpixels_transfer_ops(ctx, rb->Format, format, type,
>                                           uses_blit) != 0;
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Thanks for the patch Iago.
Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>

You might want to wait few days to hear any comments on the gallium
usage of the function. If no one objects you can push it upstream.