[Mesa-dev] [PATCH 1/3] mesa: Fix handling of glCopyBufferSubData() for src == dst.

[Brian Paul]

On Wed, Jan 25, 2012 at 8:51 PM, Eric Anholt <eric at anholt.net> wrote:
> Fixes piglit ARB_copy_buffer-overlap, which previously assertion failed.
> ---
>  src/mesa/main/bufferobj.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
> index 5b6db78..e4f964f 100644
> --- a/src/mesa/main/bufferobj.c
> +++ b/src/mesa/main/bufferobj.c
> @@ -526,11 +526,23 @@ _mesa_copy_buffer_subdata(struct gl_context *ctx,
>    assert(!_mesa_bufferobj_mapped(src));
>    assert(!_mesa_bufferobj_mapped(dst));
>
> -   srcPtr = ctx->Driver.MapBufferRange(ctx, readOffset, size,
> -                                       GL_MAP_READ_BIT, src);
> -   dstPtr = ctx->Driver.MapBufferRange(ctx, writeOffset, size,
> -                                       (GL_MAP_WRITE_BIT |
> -                                        GL_MAP_INVALIDATE_RANGE_BIT), dst);
> +   if (src == dst) {
> +      srcPtr = dstPtr = ctx->Driver.MapBufferRange(ctx, 0, src->Size,
> +                                                  GL_MAP_READ_BIT |
> +                                                  GL_MAP_WRITE_BIT, src);
> +
> +      if (!srcPtr)
> +        return;
> +
> +      srcPtr += readOffset;
> +      dstPtr += writeOffset;
> +   } else {
> +      srcPtr = ctx->Driver.MapBufferRange(ctx, readOffset, size,
> +                                         GL_MAP_READ_BIT, src);
> +      dstPtr = ctx->Driver.MapBufferRange(ctx, writeOffset, size,
> +                                         (GL_MAP_WRITE_BIT |
> +                                          GL_MAP_INVALIDATE_RANGE_BIT), dst);
> +   }
>
>    /* Note: the src and dst regions will never overlap.  Trying to do so
>     * would generate GL_INVALID_VALUE earlier.
> @@ -539,7 +551,8 @@ _mesa_copy_buffer_subdata(struct gl_context *ctx,
>       memcpy(dstPtr, srcPtr, size);
>
>    ctx->Driver.UnmapBuffer(ctx, src);
> -   ctx->Driver.UnmapBuffer(ctx, dst);
> +   if (dst != src)
> +      ctx->Driver.UnmapBuffer(ctx, dst);
>  }
>

Looks good to me.  Though, when srcbuf==dstbuf it wouldn't be too hard
to compute the union region to avoid mapping the whole buffer.
There's some places in the swrast code where we could do that too for
renderbuffers.

Reviewed-by: Brian Paul <brianp at vmware.com>