[Mesa-dev] [PATCH] mesa: Correctly use glDrawBuffers for multiple buffers and glDrawBuffer for one buffer.

[Brian Paul]

On 07/17/2014 09:21 AM, Pavel Popov wrote:
> According to spec (OpenGL 4.0 specification, pages 254-255) we have a different bits set
> for one buffer and for multiple buffers. For glDrawBuffer we may have up to four bits set
> but for glDrawBuffers we can only have one bit set.
>
> The _mesa_drawbuffers is called with ctx->Const.MaxDrawBuffers and NULL arguments when
> _mesa_update_framebuffer or _mesa_update_draw_buffers is called. In this situation realization
> for glDrawBuffers is used for any number of buffers. Even for one. But glDrawBuffer have to be
> used for one buffer instead of glDrawBuffers.
>
> Piglit test 'gl30basic' fails with assert with debug Mesa and pass with release
>      'main/buffers.c:520: _mesa_drawbuffers: Assertion `__builtin_popcount(destMask[buf]) == 1' failed.'
> Probably some other tests also can be affected.
>
> Signed-off-by: Pavel Popov <pavel.e.popov at intel.com>
> ---
>   src/mesa/main/buffers.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
> index b13a7af..a640360 100644
> --- a/src/mesa/main/buffers.c
> +++ b/src/mesa/main/buffers.c
> @@ -480,6 +480,7 @@ _mesa_drawbuffers(struct gl_context *ctx, GLuint n, const GLenum *buffers,
>      struct gl_framebuffer *fb = ctx->DrawBuffer;
>      GLbitfield mask[MAX_DRAW_BUFFERS];
>      GLuint buf;
> +   GLuint m = n;
>
>      if (!destMask) {
>         /* compute destMask values now */
> @@ -489,15 +490,17 @@ _mesa_drawbuffers(struct gl_context *ctx, GLuint n, const GLenum *buffers,
>            mask[output] = draw_buffer_enum_to_bitmask(ctx, buffers[output]);
>            ASSERT(mask[output] != BAD_MASK);
>            mask[output] &= supportedMask;
> +         if (mask[output] == 0)
> +            m--;
>         }
>         destMask = mask;
>      }
>
>      /*
> -    * If n==1, destMask[0] may have up to four bits set.
> +    * If m==1, destMask[0] may have up to four bits set.
>       * Otherwise, destMask[x] can only have one bit set.
>       */
> -   if (n == 1) {
> +   if (m == 1) {
>         GLuint count = 0, destMask0 = destMask[0];
>         while (destMask0) {
>            GLint bufIndex = ffs(destMask0) - 1;
>

Weird.  I remembered seeing this assertion a few days ago, but now I 
can't reproduce it.  I don't know what might have changed.

In any case, I don't think the patch is quite right.  If the array of 
buffers was something like {GL_NONE, GL_COLOR_ATTACHMENT1, GL_NONE, 
GL_NONE} and n=4 you'd wind up with m=1, but you'd be executing the 
wrong part of the if block.

Instead, I think you'd need to test if (m == 1 && destMask[0]) since 
it's the 0th entry that requires special validation.

And as someone else pointed out, please word-wrap the commit msg to 
70-75 chars.

-Brian