[Mesa-dev] [PATCH 2/4] mesa: Draw Indirect is not allowed when no vertex array binding exists.

[Lofstedt, Marta]

> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> Behalf Of Tapani Pälli
> Sent: Wednesday, October 21, 2015 1:25 PM
> To: Marek Olšák
> Cc: mesa-dev at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH 2/4] mesa: Draw Indirect is not allowed
> when no vertex array binding exists.
> 
> On 10/21/2015 01:41 PM, Marek Olšák wrote:
> > On Wed, Oct 21, 2015 at 7:16 AM, Tapani Pälli <tapani.palli at intel.com>
> wrote:
> >> On 10/20/2015 08:54 PM, Marek Olšák wrote:
> >>> On Tue, Oct 20, 2015 at 4:19 PM, Marta Lofstedt
> >>> <marta.lofstedt at linux.intel.com> wrote:
> >>>> From: Marta Lofstedt <marta.lofstedt at intel.com>
> >>>>
> >>>> OpenGL ES 3.1 spec. section 10.5:
> >>>> "An INVALID_OPERATION error is generated if zero is bound to
> >>>> VERTEX_ARRAY_BINDING, DRAW_INDIRECT_BUFFER or to any enabled
> vertex
> >>>> array."
> >>>>
> >>>> Signed-off-by: Marta Lofstedt <marta.lofstedt at linux.intel.com>
> >>>> ---
> >>>>    src/mesa/main/api_validate.c | 14 ++++++++++++++
> >>>>    1 file changed, 14 insertions(+)
> >>>>
> >>>> diff --git a/src/mesa/main/api_validate.c
> >>>> b/src/mesa/main/api_validate.c index c5628f5..7062cbd 100644
> >>>> --- a/src/mesa/main/api_validate.c
> >>>> +++ b/src/mesa/main/api_validate.c
> >>>> @@ -711,6 +711,20 @@ valid_draw_indirect(struct gl_context *ctx,
> >>>>          return GL_FALSE;
> >>>>       }
> >>>>
> >>>> +   /*
> >>>> +    * OpenGL ES 3.1 spec. section 10.5:
> >>>> +    * "An INVALID_OPERATION error is generated if zero is bound to
> >>>> +    * VERTEX_ARRAY_BINDING, DRAW_INDIRECT_BUFFER or to any
> enabled
> >>>> +    * vertex array."
> >>>> +    * OpenGL 4.5 spec. section 10.4:
> >>>> +    * "An INVALID_OPERATION error is generated if  zero is bound to
> >>>> +    * DRAW_INDIRECT_BUFFER, or if  no element array buffer is bound"
> >>>> +    */
> >>>> +   if (!_mesa_is_bufferobj(ctx->Array.ArrayBufferObj)) {
> >>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
> >>>> +                  "%s(No VBO is bound)", name);
> >>>> +   }
> >>> NAK.
> >>>
> >>> VERTEX_ARRAY_BINDING is a VAO. Array.ArrayBufferObj is from
> glBindBuffer.
> >>
> >> This check is valid, it is not against VERTEX_ARRAY_BINDING. Note
> >> "any enabled vertex array", we hit this weird situation when client
> >> has a VAO bound and has enabled vertex attrib array but has not bound
> any VBO to it.
> > No, it's invalid. The check has absolutely nothing to do with enabled
> > vertex arrays and draw calls. Absolutely nothing. glBindBuffer changes
> > a latched state, which means it doesn't do anything by itself, it only
> > affects other functions that change states. The functions affected by
> > glBindBuffer(GL_ARRAY_BUFFER, ..) are glVertexAttribPointer, etc. not
> > glDraw*. If you called glBindBuffer(GL_ARRAY_BUFFER, ..) right before
> > a Draw call, it wouldn't do anything to vertex arrays and buffers, but
> > it would pass the check.
> 
> OK my understanding was that reason why this change fixes the bug is that
> ctx->Array.ArrayBufferObj is 0 for the default VAO and never 0 when vertex
> array buffer binding has been set, and this would happen when we would
> have an VBO bound. I will spend some more time to understand this.

If you have access to the CTS it is these tests that this fixed:
ES31-CTS.draw_indirect.negative-noVBO-arrays
ES31-CTS.draw_indirect.negative-noVBO-elements

My understanding is as Tapanis above, I was trying to come up with a method
of not needing to loop through the VertexAttribPointers.
Also, I have mis-quoted the spec. I should have only quoted the:
 "or any enabled vertex arrays" and limit to gles 3.1.

> 
> > Now, where does this patch check "enabled vertex arrays"? Nowhere. It
> > doesn't check VERTEX_ARRAY_BINDING, it doesn't check
> > DRAW_INDIRECT_BUFFER, and it doesn't check enabled vertex arrays. That
> > whole comment is completely useless there.
> >
> > Sorry if I'm too direct, but you should really think more before
> > making such statements and giving Reviewed-by.
> 
> // Tapani
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev