On Tue, Dec 18, 2012 at 11:57 AM, Kenneth Graunke <kenn...@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.pho...@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.
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
