[Mesa-dev] [PATCH 3/3 v3] mesa: Fix GL error generation in glBlitFramebuffer()

[Anuj Phogat]

On Tue, Jan 8, 2013 at 5:25 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> From: Anuj Phogat <anuj.phogat at gmail.com>
>
> V2:
> If mask has GL_STENCIL_BUFFER_BIT set, the depth formats for readRenderBuffer
> and drawRenderBuffer must match unless one of the two buffers doesn't have
> depth, in which case it's not blitted, so the format check should be ignored.
> Same comment goes for stencil formats in depth renderbuffers if mask has
> GL_DEPTH_BUFFER_BIT set.
>
> v3 (Kayden): Refactor code to be a bit more readable.
>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/main/fbobject.c | 59 ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 15 deletions(-)
>
> Anuj,
>
> This looks correct, but the run-on conditionals are pretty hard to read...
> just too much going on in a single if-statement.
>
> How about splitting them up a bit?  Here's my proposed version.  It also
> gives slightly more descriptive messages.
>
I'm happy with the refactored code except a small mistake I explained below.
> Untested.
>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index 281cdd0..bf613ff 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -2833,14 +2833,28 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1,
>        if ((readRb == NULL) || (drawRb == NULL)) {
>          mask &= ~GL_STENCIL_BUFFER_BIT;
>        }
> -      else if (_mesa_get_format_bits(readRb->Format, GL_STENCIL_BITS) !=
> -              _mesa_get_format_bits(drawRb->Format, GL_STENCIL_BITS)) {
> -        /* There is no need to check the stencil datatype here, because
> -         * there is only one: GL_UNSIGNED_INT.
> -         */
> -         _mesa_error(ctx, GL_INVALID_OPERATION,
> -                     "glBlitFramebufferEXT(stencil buffer size mismatch)");
> -         return;
> +      else {
> +         if ((_mesa_get_format_bits(readRb->Format, GL_STENCIL_BITS) !=
> +              _mesa_get_format_bits(drawRb->Format, GL_STENCIL_BITS)) ||
> +             (_mesa_get_format_datatype(readRb->Format) !=
> +              _mesa_get_format_datatype(drawRb->Format))) {
Matching the data type is not required here as data type for stencil buffer is
always GL_UNSIGNED_INT. But, data type of GL_DEPTH24_STENCIL8 is
GL_UNSIGNED_NORMALIZED. So, matching the data types here will disallow
blitting between GL_STENCIL_INDEX8 and GL_DEPTH24_STENCIL8 which is a
valid blitting case. This caused 7 gles3 conformance blitframebuffer test cases
fail.

> +            _mesa_error(ctx, GL_INVALID_OPERATION,
> +                        "glBlitFramebuffer(stencil attachment format mismatch)");
> +            return;
> +         }
> +
> +         int read_z_bits = _mesa_get_format_bits(readRb->Format, GL_DEPTH_BITS);
> +         int draw_z_bits = _mesa_get_format_bits(drawRb->Format, GL_DEPTH_BITS);
> +
> +         /* If both buffers also have depth data, the depth formats must match
> +          * as well.  If one doesn't have depth, it's not blitted, so we should
> +          * ignore the depth format check.
> +          */
> +         if (read_z_bits > 0 && draw_z_bits > 0 && read_z_bits != draw_z_bits) {
Above mentioned data type checking should be moved here. Observed no regressions
in gles3 conformance after making this change.
> +            _mesa_error(ctx, GL_INVALID_OPERATION, "glBlitFramebuffer"
> +                        "(stencil attachment depth format mismatch)");
> +            return;
> +         }
>        }
>     }
>
> @@ -2859,13 +2873,28 @@ _mesa_BlitFramebuffer(GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1,
>        if ((readRb == NULL) || (drawRb == NULL)) {
>          mask &= ~GL_DEPTH_BUFFER_BIT;
>        }
> -      else if ((_mesa_get_format_bits(readRb->Format, GL_DEPTH_BITS) !=
> -               _mesa_get_format_bits(drawRb->Format, GL_DEPTH_BITS)) ||
> -              (_mesa_get_format_datatype(readRb->Format) !=
> -               _mesa_get_format_datatype(drawRb->Format))) {
> -         _mesa_error(ctx, GL_INVALID_OPERATION,
> -                     "glBlitFramebufferEXT(depth buffer format mismatch)");
> -         return;
> +      else {
> +         if ((_mesa_get_format_bits(readRb->Format, GL_DEPTH_BITS) !=
> +              _mesa_get_format_bits(drawRb->Format, GL_DEPTH_BITS)) ||
> +             (_mesa_get_format_datatype(readRb->Format) !=
> +              _mesa_get_format_datatype(drawRb->Format))) {
> +            _mesa_error(ctx, GL_INVALID_OPERATION,
> +                        "glBlitFramebuffer(depth attachment format mismatch)");
> +            return;
> +         }
> +
> +         int read_s_bit = _mesa_get_format_bits(readRb->Format, GL_STENCIL_BITS);
> +         int draw_s_bit = _mesa_get_format_bits(drawRb->Format, GL_STENCIL_BITS);
> +
> +         /* If both buffers also have stencil data, the stencil formats must
> +          * match as well.  If one doesn't have stencil, it's not blitted, so
> +          * we should ignore the stencil format check.
> +          */
> +         if (read_s_bit > 0 && draw_s_bit > 0 && read_s_bit != draw_s_bit) {
> +            _mesa_error(ctx, GL_INVALID_OPERATION, "glBlitFramebuffer"
> +                        "(depth attachment stencil bits mismatch)");
> +            return;
> +         }
>        }
>     }
>
> --
> 1.8.1
>