[Mesa-dev] [PATCH 6/6] mesa: Fix framebuffer blitting to GL_COLOR_ATTACHMENTi when i!=0

[Kenneth Graunke]

On 12/17/2012 07:35 PM, Anuj Phogat wrote:
> This patch fixes a case when blitting to a framebuffer with
> renderbuffers/textures attached to GL_COLOR_ATTACHMENT{1, 2, ...}.
> Earlier we were incorrectly blitting to GL_COLOR_ATTACHMENT0 by default.
>
> It also fixes a blitting case when drawAttachment->Texture ==
> readAttachment->Texture. This was causing an assertion failure in intel's
> i965 drivers (intel_miptree_attach_map()) with gles3 conformance test case:
> framebuffer_blit_functionality_minifying_blit
>
> V2: Fixed a case when number of draw buffer attachments are zero.
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
>   src/mesa/main/fbobject.c |   14 +++++++++++++-
>   src/mesa/swrast/s_blit.c |   33 ++++++++++++++++++++++++++-------
>   2 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index d87239e..f4e427b 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -2818,8 +2818,20 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1,
>
>      /* get color read/draw renderbuffers */
>      if (mask & GL_COLOR_BUFFER_BIT) {
> +      const  GLuint numColorDrawBuffers =
> +         ctx->DrawBuffer->_NumColorDrawBuffers;
>         colorReadRb = readFb->_ColorReadBuffer;
> -      colorDrawRb = drawFb->_ColorDrawBuffers[0];
> +
> +      if (numColorDrawBuffers > 0) {
> +         for (int i = 0; i < numColorDrawBuffers; i++) {
> +            if (ctx->DrawBuffer->_ColorDrawBuffers[i] != NULL) {
> +               colorDrawRb = ctx->DrawBuffer->_ColorDrawBuffers[i];
> +	       break;
> +	    }
> +         }
> +      }
> +      else
> +         colorDrawRb = NULL;

This looks wrong to me.  You're looping through all the draw buffers, 
advancing the pointer until you find the first drawbuffer, then 
stopping...but you're not doing any checks.

I believe you actually want to check the read buffer against *each* of 
the draw buffers, i.e.

          colorReadRb = readFb->_ColorReadBuffer;
          for (int i = 0; i < numColorDrawBuffers; i++) {
             if (ctx->DrawBuffer->_ColorDrawBuffers[i] == NULL) {
                continue;

                /* From the EXT_framebuffer_object spec: ... */
                if (colorReadRb == NULL || colorDrawRb == NULL) {
                    colorReadRb = colorDrawRb = NULL;
                    mask &= ~GL_COLOR_BUFFER_BIT;
                    break;
                } else if (!compatible_color_datatypes(...)) {
                    ...error...
                    return;
                }
             }
          }

It looks like the compatible_resolve_formats check (below) should also 
be in this loop, so it happens on a per-buffer basis.

I believe my interpretation of checking the read buffer against *all* 
color draw buffers is correct.  For example, this spec text:

"The read buffer contains signed integer values and **any** draw buffer
  does not contain signed integer values."

...really seems to imply that we should be applying these error checks 
to each of the draw buffers in turn.

>
>         /* From the EXT_framebuffer_object spec:
>          *
> diff --git a/src/mesa/swrast/s_blit.c b/src/mesa/swrast/s_blit.c
> index b0c56a4..c579572 100644
> --- a/src/mesa/swrast/s_blit.c
> +++ b/src/mesa/swrast/s_blit.c
> @@ -111,6 +111,9 @@ blit_nearest(struct gl_context *ctx,
>                GLbitfield buffer)
>   {
>      struct gl_renderbuffer *readRb, *drawRb;
> +   struct gl_renderbuffer_attachment *readAtt, *drawAtt;
> +   struct gl_framebuffer *readFb = ctx->ReadBuffer;
> +   struct gl_framebuffer *drawFb = ctx->DrawBuffer;
>
>      const GLint srcWidth = ABS(srcX1 - srcX0);
>      const GLint dstWidth = ABS(dstX1 - dstX0);
> @@ -146,8 +149,18 @@ blit_nearest(struct gl_context *ctx,
>
>      switch (buffer) {
>      case GL_COLOR_BUFFER_BIT:
> -      readRb = ctx->ReadBuffer->_ColorReadBuffer;
> -      drawRb = ctx->DrawBuffer->_ColorDrawBuffers[0];
> +      readAtt = &readFb->Attachment[readFb->_ColorReadBufferIndex];
> +      for (int i = 0; i < drawFb->_NumColorDrawBuffers; i++) {
> +          int idx = drawFb->_ColorDrawBufferIndexes[i];
> +          if (idx != -1) {
> +             drawAtt = &drawFb->Attachment[idx];
> +          }
> +          else {
> +	     continue;

if (idx == -1)
    continue;

drawAtt = &drawFb->Attachment[idx];

but again, I'm skeptical here since this loop finds the last draw 
attachment and doesn't do anything with the prior ones...

> +	  }
> +      }
> +      readRb = readFb->_ColorReadBuffer;
> +      drawRb = drawAtt->Renderbuffer;
>
>         if (readRb->Format == drawRb->Format) {
>   	 mode = DIRECT;
> @@ -159,8 +172,10 @@ blit_nearest(struct gl_context *ctx,
>
>         break;
>      case GL_DEPTH_BUFFER_BIT:
> -      readRb = ctx->ReadBuffer->Attachment[BUFFER_DEPTH].Renderbuffer;
> -      drawRb = ctx->DrawBuffer->Attachment[BUFFER_DEPTH].Renderbuffer;
> +      readAtt = &readFb->Attachment[BUFFER_DEPTH];
> +      drawAtt = &drawFb->Attachment[BUFFER_DEPTH];
> +      readRb = readAtt->Renderbuffer;
> +      drawRb = drawAtt->Renderbuffer;
>
>         /* Note that for depth/stencil, the formats of src/dst must match.  By
>          * using the core helpers for pack/unpack, we avoid needing to handle
> @@ -175,8 +190,10 @@ blit_nearest(struct gl_context *ctx,
>         pixelSize = 4;
>         break;
>      case GL_STENCIL_BUFFER_BIT:
> -      readRb = ctx->ReadBuffer->Attachment[BUFFER_STENCIL].Renderbuffer;
> -      drawRb = ctx->DrawBuffer->Attachment[BUFFER_STENCIL].Renderbuffer;
> +      readAtt = &readFb->Attachment[BUFFER_STENCIL];
> +      drawAtt = &drawFb->Attachment[BUFFER_STENCIL];
> +      readRb = readAtt->Renderbuffer;
> +      drawRb = drawAtt->Renderbuffer;
>         mode = UNPACK_S;
>         pixelSize = 1;
>         break;
> @@ -208,7 +225,9 @@ blit_nearest(struct gl_context *ctx,
>         return;
>      }
>
> -   if (readRb == drawRb) {
> +   if ((readRb == drawRb) ||
> +       (readAtt->Texture && drawAtt->Texture &&
> +        (readAtt->Texture == drawAtt->Texture))) {
>         /* map whole buffer for read/write */
>         /* XXX we could be clever and just map the union region of the
>          * source and dest rects.
>