On Wed, Jan 9, 2013 at 1:30 AM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 01/08/2013 03:54 AM, Anuj Phogat wrote:
>>
>> This patch adds required error checking in _mesa_BlitFramebuffer() when
>> blitting to multiple color render targets. It also fixes a case when
>> blitting to a framebuffer with renderbuffer/texture attached to
>> GL_COLOR_ATTACHMENT{i} (where i!=0). Earlier it skips color blitting if
>> nothing is found attached to GL_COLOR_ATTACHMENT0.
>>
>> V2: Fixed a case when number of draw buffer attachments are zero.
>> V3: Do compatible_color_datatypes() and compatible_resolve_formats()
>> check for all the draw renderbuffers in fbobject.c. Fix debug
>> code at bottom of _mesa_BlitFramebuffer() to handle MRTs. Combine
>> error checking code for linear blits with other color blit error
>> checking.
>>
>> Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com>
>> ---
>> src/mesa/main/fbobject.c | 115
>> +++++++++++++++++++++++++++-------------------
>> 1 files changed, 68 insertions(+), 47 deletions(-)
>>
>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>> index 517bf13..fe9043c 100644
>> --- a/src/mesa/main/fbobject.c
>> +++ b/src/mesa/main/fbobject.c
>> @@ -2788,7 +2788,6 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0,
>> GLint srcX1, GLint srcY1,
>> GL_DEPTH_BUFFER_BIT |
>> GL_STENCIL_BUFFER_BIT);
>> const struct gl_framebuffer *readFb, *drawFb;
>> - const struct gl_renderbuffer *colorReadRb, *colorDrawRb;
>> GET_CURRENT_CONTEXT(ctx);
>>
>> ASSERT_OUTSIDE_BEGIN_END(ctx);
>> @@ -2843,8 +2842,10 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0,
>> GLint srcX1, GLint srcY1,
>>
>> /* get color read/draw renderbuffers */
>> if (mask & GL_COLOR_BUFFER_BIT) {
>> - colorReadRb = readFb->_ColorReadBuffer;
>> - colorDrawRb = drawFb->_ColorDrawBuffers[0];
>> + const GLuint numColorDrawBuffers =
>> ctx->DrawBuffer->_NumColorDrawBuffers;
>> + const struct gl_renderbuffer *colorReadRb =
>> readFb->_ColorReadBuffer;
>> + const struct gl_renderbuffer *colorDrawRb = NULL;
>> + GLuint i;
>>
>> /* From the EXT_framebuffer_object spec:
>> *
>> @@ -2852,19 +2853,50 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0,
>> GLint srcX1, GLint srcY1,
>> * the read and draw framebuffers, the corresponding bit is
>> silently
>> * ignored."
>> */
>> - if ((colorReadRb == NULL) || (colorDrawRb == NULL)) {
>> - colorReadRb = colorDrawRb = NULL;
>> - mask &= ~GL_COLOR_BUFFER_BIT;
>> + if (!colorReadRb || numColorDrawBuffers == 0) {
>> + mask &= ~GL_COLOR_BUFFER_BIT;
>> }
>> - else if (!compatible_color_datatypes(colorReadRb->Format,
>> - colorDrawRb->Format)) {
>> - _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glBlitFramebufferEXT(color buffer datatypes
>> mismatch)");
>> - return;
>> + else {
>> + for (i = 0; i < numColorDrawBuffers; i++) {
>> + colorDrawRb = ctx->DrawBuffer->_ColorDrawBuffers[i];
>> + if (!colorDrawRb)
>> + continue;
>> +
>> + if (!compatible_color_datatypes(colorReadRb->Format,
>> + colorDrawRb->Format)) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glBlitFramebufferEXT(color buffer datatypes
>> mismatch)");
>> + return;
>> + }
>> + /* extra checks for multisample copies... */
>> + if (readFb->Visual.samples > 0 || drawFb->Visual.samples > 0)
>> {
>> + /* color formats must match */
>> + if (!compatible_resolve_formats(colorReadRb, colorDrawRb))
>> {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glBlitFramebufferEXT(bad src/dst multisample
>> pixel formats)");
>> + return;
>> + }
>> + }
>> + }
>> + }
>> +
>> + if (colorDrawRb == NULL) {
>> + colorReadRb = NULL;
>> + mask &= ~GL_COLOR_BUFFER_BIT;
>> + }
>
>
> I don't think this if-statement should exist. There are two ways to get
> here. First is via:
>
> if (!colorReadRb || numColorDrawBuffers == 0) {
> mask &= ~GL_COLOR_BUFFER_BIT;
> } else {
> handles the case of no draw buffers. mask already gets updated. In the
> "else" case, we hit the loop. Substituting the post-loop value for
> colorDrawRb, this block becomes equivalent to:
>
> if (ctx->DrawBuffer->_ColorDrawBuffers[numColorDrawBuffers-1] == NULL) {
> colorReadRb = NULL;
> mask &= ~GL_COLOR_BUFFER_BIT;
> }
>
> and that looks really sketchy - why is the last element special?
>
This doesn't always test for last element. It tests for last non-null element.
It was used to verify that we have atleast one valid draw renderbuffer. But
you are right it is already tested by numColorDrawBuffers == 0. I'll omit this
if statement before pushing upstream.
>> + if (filter == GL_LINEAR && (mask & GL_COLOR_BUFFER_BIT)) {
>> + /* 3.1 spec, page 199:
>> + * "Calling BlitFramebuffer will result in an INVALID_OPERATION
>> error
>> + * if filter is LINEAR and read buffer contains integer data."
>> + */
>> + GLenum type = _mesa_get_format_datatype(colorReadRb->Format);
>> + if (type == GL_INT || type == GL_UNSIGNED_INT) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "glBlitFramebufferEXT(integer color type)");
>> + return;
>> + }
>> }
>
>
> I would like to see this block moved into the } else { block above. That
> way, you already know that colorReadRb exists and (mask &
> GL_COLOR_BUFFER_BIT) is true, so you can simply change this to:
>
> if (filter == GL_LINEAR) {
> ...
> }
>
yes. I'll do that.
> With those two changes, this gets a:
> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
>
>
>> - }
>> - else {
>> - colorReadRb = colorDrawRb = NULL;
>> }
>>
>> if (mask & GL_STENCIL_BUFFER_BIT) {
>> @@ -2952,28 +2984,6 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0,
>> GLint srcX1, GLint srcY1,
>> "glBlitFramebufferEXT(bad src/dst multisample region
>> sizes)");
>> return;
>> }
>> -
>> - /* color formats must match */
>> - if (colorReadRb &&
>> - colorDrawRb &&
>> - !compatible_resolve_formats(colorReadRb, colorDrawRb)) {
>> - _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glBlitFramebufferEXT(bad src/dst multisample pixel
>> formats)");
>> - return;
>> - }
>> - }
>> -
>> - if (filter == GL_LINEAR && (mask & GL_COLOR_BUFFER_BIT)) {
>> - /* 3.1 spec, page 199:
>> - * "Calling BlitFramebuffer will result in an INVALID_OPERATION
>> error
>> - * if filter is LINEAR and read buffer contains integer data."
>> - */
>> - GLenum type = _mesa_get_format_datatype(colorReadRb->Format);
>> - if (type == GL_INT || type == GL_UNSIGNED_INT) {
>> - _mesa_error(ctx, GL_INVALID_OPERATION,
>> - "glBlitFramebufferEXT(integer color type)");
>> - return;
>> - }
>> }
>>
>> if (!ctx->Extensions.EXT_framebuffer_blit) {
>> @@ -2983,6 +2993,10 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0,
>> GLint srcX1, GLint srcY1,
>>
>> /* Debug code */
>> if (DEBUG_BLIT) {
>> + const struct gl_renderbuffer *colorReadRb =
>> readFb->_ColorReadBuffer;
>> + const struct gl_renderbuffer *colorDrawRb = NULL;
>> + GLuint i = 0;
>> +
>> printf("glBlitFramebuffer(%d, %d, %d, %d, %d, %d, %d, %d,"
>> " 0x%x, 0x%x)\n",
>> srcX0, srcY0, srcX1, srcY1,
>> @@ -3004,18 +3018,25 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0,
>> GLint srcX1, GLint srcY1,
>> }
>> printf("\n");
>>
>> - att = find_attachment(drawFb, colorDrawRb);
>> - printf(" Dst FBO %u RB %u (%dx%d) ",
>> - drawFb->Name, colorDrawRb->Name,
>> - colorDrawRb->Width, colorDrawRb->Height);
>> - if (att && att->Texture) {
>> - printf("Tex %u tgt 0x%x level %u face %u",
>> - att->Texture->Name,
>> - att->Texture->Target,
>> - att->TextureLevel,
>> - att->CubeMapFace);
>> + /* Print all active color render buffers */
>> + for (i = 0; i < ctx->DrawBuffer->_NumColorDrawBuffers; i++) {
>> + colorDrawRb = ctx->DrawBuffer->_ColorDrawBuffers[i];
>> + if (!colorDrawRb)
>> + continue;
>> +
>> + att = find_attachment(drawFb, colorDrawRb);
>> + printf(" Dst FBO %u RB %u (%dx%d) ",
>> + drawFb->Name, colorDrawRb->Name,
>> + colorDrawRb->Width, colorDrawRb->Height);
>> + if (att && att->Texture) {
>> + printf("Tex %u tgt 0x%x level %u face %u",
>> + att->Texture->Name,
>> + att->Texture->Target,
>> + att->TextureLevel,
>> + att->CubeMapFace);
>> + }
>> + printf("\n");
>> }
>> - printf("\n");
>> }
>> }
>>
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
