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

[Anuj Phogat]

On Tue, Dec 18, 2012 at 11:57 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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.
I added this code to correctly handle a case when color renderbuffer/texture
is attached to GL_COLOR_ATTACHMENT{i} (where i!=0). Earlier it skips color
blitting if nothing is found attached to GL_COLOR_ATTACHMENT0.

Yes, I should also include compatible_color_datatypes() check inside 'for' loop.
Consider it fixed in next version of this patch.

>>
>>         /* 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...
>
Yeah, It just fixes part of the problem. I should have at least put a 'FIXME'
comment here. These changes in swrast just addressed an assertion failure
in gles3 conform minifying_blit test. Failure was caused during stencil buffer
blitting when readAtt->Texture == drawAtt->Texture. swrast blitting still need
a fix to handle blitting to multiple color draw buffers. I'll take
care of this in
next version of this patch. Thanks for noticing this.

>> +         }
>> +      }
>> +      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.
>>
>