[Mesa-dev] [PATCH 03/11] mesa: Implement _mesa_all_varyings_in_vbos.

[Fredrik Höglund]

On Wednesday 27 July 2016, Mathias Fröhlich wrote:
> Hi,
> 
> On Thursday, June 23, 2016 16:53:59 Fredrik Höglund wrote:
> > On Friday 17 June 2016, Mathias.Froehlich at gmx.net wrote:
> > > From: Mathias Fröhlich <mathias.froehlich at web.de>
> > > 
> > > Implement the equivalent of vbo_all_varyings_in_vbos for
> > > vertex array objects.
> > > 
> > > Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> > > ---
> > >  src/mesa/main/arrayobj.c | 35 +++++++++++++++++++++++++++++++++++
> > >  src/mesa/main/arrayobj.h |  4 ++++
> > >  2 files changed, 39 insertions(+)
> > > 
> > > diff --git a/src/mesa/main/arrayobj.c b/src/mesa/main/arrayobj.c
> > > index 9c3451e..041ee63 100644
> > > --- a/src/mesa/main/arrayobj.c
> > > +++ b/src/mesa/main/arrayobj.c
> > > @@ -359,6 +359,41 @@ _mesa_update_vao_client_arrays(struct gl_context *ctx,
> > >  }
> > >  
> > >  
> > > +bool
> > > +_mesa_all_varyings_in_vbos(const struct gl_vertex_array_object *vao)
> > > +{
> > > +   /* Walk those enabled arrays that have the default vbo attached */
> > > +   GLbitfield64 mask = vao->_Enabled & ~vao->VertexAttribBufferMask;
> > > +
> > > +   while (mask) {
> > > +      /** We do not use u_bit_scan64 as we can here walk
> > > +       *  multiple attrib arrays at once
> > > +       */
> > > +      const int i = ffsll(mask) - 1;
> > > +      const struct gl_vertex_attrib_array *attrib_array =
> > > +         &vao->VertexAttrib[i];
> > > +      const struct gl_vertex_buffer_binding *buffer_binding =
> > > +         &vao->VertexBinding[attrib_array->VertexBinding];
> > > +
> > > +      /* Only enabled arrays shall appear in the _Enabled bitmask */
> > > +      assert(attrib_array->Enabled);
> > > +      /* We have already masked out vao->VertexAttribBufferMask  */
> > > +      assert(!_mesa_is_bufferobj(buffer_binding->BufferObj));
> > > +
> > > +      /* Bail out once we find the first non vbo with a non zero stride */
> > > +      if (buffer_binding->Stride != 0)
> > > +         return false;
> > 
> > I'm not sure if this is correct.  The default value for Stride is 16,
> > not 0.  The only way Stride can be zero in a binding point that doesn't
> > have a buffer object bound is if the user has explicitly called
> > glBindVertexBuffer() with both the buffer and stride parameters set
> > to zero.
> > 
> > StrideB in gl_client_array on the other hand is always zero when the array
> > is one of the currval arrays managed by the VBO context.  It is never zero
> > when the array is a user array that has been specified with gl*Pointer().
> > 
> > I think the point of vbo_all_varyings_in_vbos() is to return false if any
> > enabled array doesn't have a VBO bound, and is not one of the currval
> > arrays.
> 
> Additionally vbo_all_varyings_in_vbos() treats zero stride user
> arrays like a current vertex attribute values. Already because
> vbo_all_varyings_in_vbos() does not distinguish between a user
> zero stride array and a current attribute value which is presented likewise.
> Also I believe it's legal to call glBindVertexBuffer() with zero buffer
> and stride - or am I wrong here?
> So IMO what you write would result in a change of behavior.

The vbo_all_varyings_in_vbos() implementation predates
ARB_vertex_attrib_binding, and with the legacy gl*Pointer() API there is
no way to set StrideB to zero in an enabled array. That's why I think the
intent is to check if the array is a currval array.

(CC'ing Keith Whitwell since he wrote the original implementation of that
function)

The new ARB_vertex_attrib_binding API supports setting the stride to zero,
but it doesn't support rendering without VBO's since it provides no way to
set the VERTEX_ATTRIB_ARRAY_POINTER state.

In theory the API could be abused to force the stride to be zero in a local
memory array by first calling glVertexAttribPointer() to set the pointer
and format, and then call glBindVertexBuffer() for the corresponding
binding point with the buffer and stride parameters set to zero. This is
something I did not consider when I implemented the extension. But
the fact that you can do that may be a reason not to advertise the
extension in the compatiblity profile.

Fredrik