[Mesa-dev] [PATCH 7.1/7] mesa: Add error checking in _mesa_BlitFramebuffer() for MRTs
Kenneth Graunke
kenneth at whitecape.org
Tue Jan 8 12:00:38 PST 2013
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.phogat at 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?
> + 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) {
...
}
With those two changes, this gets a:
Reviewed-by: Kenneth Graunke <kenneth at 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");
> }
> }
>
>
More information about the mesa-dev
mailing list