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

[Mathias Fröhlich]

Hi,

On Thursday, July 28, 2016 02:05:08 Fredrik Höglund wrote:
> 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)

It was indeed the first impression on my side too but then I realized that
it covers more.

> 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.

So, that's what I feared - I did not scan all the API to see that
it is possible. I did find one code path that does not guard
against zero stride. But past your comment I was getting unsure again
if some side effect guards zero stride against real VBO usage.

I would be fine to treat zero stride user arrays that are not current
variables worse than before - I also think that this is a corner case.
As indicated before, the function to test this would be even simpler
thanks to the current VAO implementation.
But why change when the equivalent implementation is so easy.
And again due to the current VAO implementation the while loop does not
walk a lot of entries and should not hurt that much.
Compared to the previous VERT_ATTRIB_MAX loop that should be an
improvement already.

Thanks for your analysis!

Regards

Mathias