[Mesa-dev] [PATCH] vbo: Set FLUSH_UPDATE_CURRENT when setting vertex attibutes

[Keith Whitwell]

On Tue, 2010-09-14 at 08:18 -0700, Chia-I Wu wrote:
> 2010/9/14 Kristian Høgsberg <krh at bitplanet.net>:
> > 2010/9/14 Chia-I Wu <olvaffe at gmail.com>:
> >> 2010/9/14 Kristian Høgsberg <krh at bitplanet.net>:
> >>> 2010/9/13 keith whitwell <keith.whitwell at gmail.com>:
> >>>> Hey Kristian,
> >>>>
> >>>> The first question is whether this is necessary - from vague memory I
> >>>> have an idea that current attributes need not be updated by vertex
> >>>> buffer rendering - ie. it's optional/implementation-dependent.
> >>>>
> >>>> I assume you're concerned with the case where you have something like
> >>>>
> >>>>   // ctx->Current.Color is xyz
> >>>>
> >>>>   glDrawArrays();
> >>>>
> >>>>   // has ctx->Current.Color been updated??
> >>>>
> >>>> But assuming I'm wrong about that & we really do want to make
> >>>> DrawArrays set the current values, the patch looks good...
> >>>
> >>> No, what I'm seeing is that the code in question sets three generic
> >>> vertex attributes and then calls glDrawArrays().  The value of the
> >>> last attribute is not propagates into the shader.
> >>>
> >>> The problem is that the vertex array code keeps the values in
> >>> exec->vtx.vertex, but the implementation of glDrawArrays looks in
> >>> ctx->Current (that's what I assume, I didn't track that down).  When
> >>> the code hits a case where the size of an attribute is smaller that
> >>> what we're trying to set, it recomputes the layout of the
> >>> exec->vtx.vertex values and as a side effect copies the
> >>> exec->vtx.vertex values to ctx->Current.  Since we start out with
> >>> attrsz == 0 for all attributes, each new attribute will trigger this
> >>> recomputation and thus effectively flushes all previous values to
> >>> ctx->Current.  Which is why all but the last attribute make it to the
> >>> shader.
> >>>
> >>> Note that the ATTR macro is defined differently, depending on
> >>> FEATURE_beginend - the !FEATURE_beginend case sets the
> >>> FLUSH_UPDATE_CURRENT flag too.  I don't know why we wouldn't also set
> >>> it in the FEATURE_beginend case, not using begin/end in that case is
> >>> still an option.
> >> The way glColor4f is dispatched depends on whether it is GL or ES:
> >>
> >>  GL (with FEATURE_beginend): glColor4f -> neutral_Color4f -> vbo_Color4f
> >>  ES (w/o  FEATURE_beginend): glColor4f -> _es_Color4f -> vbo_Color4f
> >>
> >> In the former case, FLUSH_UPDATE_CURRENT should have been set by
> >> vbo_exec_BeginVertices which is called by neutral_Color4f.  In the latter case,
> >> the flag must be set in vbo_Color4f.  Could it be a bug some where in vtxfmt?
> >>
> >> One issue I noticed just last week is that the current scheme does not take
> >> into account "ES with FEATURE_beginend".  This happens with --enable-gles2
> >> build.  Since FEATURE_beginend is enabled in such build, vbo_Color4f does not
> >> set FLUSH_UPDATE_CURRENT.  Yet, an ES context does not use neutral_Color4f.
> >
> > And that's exactly the problem I have.  Setting FLUSH_UPDATE_CURRENT
> > in the ATTR macro makes sure that the FLUSH_CURRENT in
> > vbo_exec_DrawArrays (and other array draw funcs) ends up calling
> > vbo_exec_copy_to_current(), which then pushes the values into
> > ctx->Current before the draw call.  I don't see a problem with this
> > approach; in a begin/end, this flag is already set, so there's no
> > overhead, outside begin/end (ES2) it's required for correct behaviour.
> If this patch is to be applied, it makes sense to also stop setting
> FLUSH_UPDATE_CURRENT in vbo_exec_BeginVertices, and remove the
> !FEATURE_beginend case.
> 
> But then, does it make sense to remove exec vtxfmt in main/vtxfmt.c and the
> related code (a big chunk!), and define ATTR to
> 
>   #define ATTR(...) do {
>      struct vbo_exec_context *exec = &vbo_context(ctx)->exec;
> 
>      if (!exec->ctx->Driver.NeedFlush)
>         vbo_exec_BeginVertices(ctx);
> 
>      /* ... */
>   }
> 
> From what I can tell, and I am be wrong, exec vtxfmt and PRE_LOOPBACK are to
> avoid any unnecessary operation in vtxfmt functions, even as cheap as setting
> FLUSH_UPDATE_CURRENT repeatedly.  If that cannot be avoided, there seems to be
> no need to have exec vtxfmt.

This is all ancient code & getting rid of large chunks of it is probably
very appropriate...

Keith