[Mesa-dev] [PATCH 09/21] mesa: add validate_stencil_buffer() helper

[Timothy Arceri]

On 01/06/17 23:04, Samuel Pitoiset wrote:
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
> ---
>   src/mesa/main/blit.c | 111 +++++++++++++++++++++++++++------------------------
>   1 file changed, 58 insertions(+), 53 deletions(-)
> 
> diff --git a/src/mesa/main/blit.c b/src/mesa/main/blit.c
> index 2c0300eab3..207ce7d501 100644
> --- a/src/mesa/main/blit.c
> +++ b/src/mesa/main/blit.c
> @@ -178,6 +178,62 @@ is_valid_blit_filter(const struct gl_context *ctx, GLenum filter)
>   
>   
>   static void
> +validate_stencil_buffer(struct gl_context *ctx, struct gl_framebuffer *readFb,
> +                        struct gl_framebuffer *drawFb, GLbitfield *mask,
> +                        bool no_error, const char *func)

I'd really really like to avoid passing no_error everywhere. Not only 
does it result in less organized code, it potentially adds unnecessary 
branches in both the no_error and regular code paths.

I think I'd rather if this and the following patches were structured 
something like:

blit_framebuffer(...)
{
    ...

    if (readRb == NULL || drawRb == NULL) {
       *mask &= ~GL_STENCIL_BUFFER_BIT;
    } else if (!no_error) {
       if (!validate_stencil_buffer(...))
          return;
    }

    ...
}

This makes more sense to me also as the first check isn't technically a 
validation but rather a case we ignore.

With that there should be minimal code in the no_error code path and no 
reason for not marking blit_framebuffer() as ALWAYS_INLINE.

To avoid all the error code being inline twice into the 
blit_framebuffer() callers you could simply wrap it in a static function 
e.g.


void static
blit_framebuffer_err(...) {
    /* We are wrapping the err variant of the always inlined
     * blit_framebuffer() to avoid inlining it in every caller.
     */
    blit_framebuffer(...);
}

We could create same type of wrapper for the no_error path also but it's 
going to make less of a difference.

It might be interesting to compare the output from gcc before and after 
to see if gcc is smart enough to do this all for us but being explicit 
about it removes any doubt. I suspect gcc won't inline this code on it's 
own.

> +{
> +   struct gl_renderbuffer *readRb =
> +      readFb->Attachment[BUFFER_STENCIL].Renderbuffer;
> +   struct gl_renderbuffer *drawRb =
> +      drawFb->Attachment[BUFFER_STENCIL].Renderbuffer;
> +
> +   /* From the EXT_framebuffer_object spec:
> +    *
> +    *     "If a buffer is specified in <mask> and does not exist in both
> +    *     the read and draw framebuffers, the corresponding bit is silently
> +    *     ignored."
> +    */
> +   if (readRb == NULL || drawRb == NULL) {
> +      *mask &= ~GL_STENCIL_BUFFER_BIT;
> +   } else if (!no_error) {
> +      int read_z_bits, draw_z_bits;
> +
> +      if (_mesa_is_gles3(ctx) && (drawRb == readRb)) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION,
> +                     "%s(source and destination stencil buffer cannot be the "
> +                     "same)", func);
> +         return;
> +      }
> +
> +      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,
> +                     "%s(stencil attachment format mismatch)", func);
> +         return;
> +      }
> +
> +      read_z_bits = _mesa_get_format_bits(readRb->Format, GL_DEPTH_BITS);
> +      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 ||
> +           _mesa_get_format_datatype(readRb->Format) !=
> +           _mesa_get_format_datatype(drawRb->Format))) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION,
> +                     "%s(stencil attachment depth format mismatch)", func);
> +         return;
> +      }
> +   }
> +}
> +
> +static void
>   blit_framebuffer(struct gl_context *ctx,
>                    struct gl_framebuffer *readFb, struct gl_framebuffer *drawFb,
>                    GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1,
> @@ -317,59 +373,8 @@ blit_framebuffer(struct gl_context *ctx,
>         }
>      }
>   
> -   if (mask & GL_STENCIL_BUFFER_BIT) {
> -      struct gl_renderbuffer *readRb =
> -         readFb->Attachment[BUFFER_STENCIL].Renderbuffer;
> -      struct gl_renderbuffer *drawRb =
> -         drawFb->Attachment[BUFFER_STENCIL].Renderbuffer;
> -
> -      /* From the EXT_framebuffer_object spec:
> -       *
> -       *     "If a buffer is specified in <mask> and does not exist in both
> -       *     the read and draw framebuffers, the corresponding bit is silently
> -       *     ignored."
> -       */
> -      if ((readRb == NULL) || (drawRb == NULL)) {
> -         mask &= ~GL_STENCIL_BUFFER_BIT;
> -      }
> -      else {
> -         int read_z_bits, draw_z_bits;
> -
> -         if (_mesa_is_gles3(ctx) && (drawRb == readRb)) {
> -            _mesa_error(ctx, GL_INVALID_OPERATION,
> -                        "%s(source and destination stencil "
> -                        "buffer cannot be the same)", func);
> -            return;
> -         }
> -
> -         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,
> -                        "%s(stencil attachment format mismatch)", func);
> -            return;
> -         }
> -
> -         read_z_bits = _mesa_get_format_bits(readRb->Format, GL_DEPTH_BITS);
> -         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 ||
> -              _mesa_get_format_datatype(readRb->Format) !=
> -              _mesa_get_format_datatype(drawRb->Format))) {
> -
> -            _mesa_error(ctx, GL_INVALID_OPERATION,
> -                        "%s(stencil attachment depth format mismatch)", func);
> -            return;
> -         }
> -      }
> -   }
> +   if (mask & GL_STENCIL_BUFFER_BIT)
> +      validate_stencil_buffer(ctx, readFb, drawFb, &mask, false, func);
>   
>      if (mask & GL_DEPTH_BUFFER_BIT) {
>         struct gl_renderbuffer *readRb =
>