[Mesa-dev] [PATCH 09/10] vbo: fix GL_LINE_LOOP stray line bug

[Brian Paul]

On 10/18/2015 03:06 PM, Sinclair Yeh wrote:
> On Fri, Oct 16, 2015 at 03:25:16PM -0600, Brian Paul wrote:
>> When long GL_LINE_LOOP primitives don't fit in one vertex buffer they
>> have to be split across buffers.  The code to do this was basically correct
>> but drivers had to pay special attention to the _mesa_prim::begin,end flags
>> in order to draw the sections of the line loop properly.  Apparently, the
>> only drivers to do this were those using the old 'tnl' module for software
>> vertex processing.
>>
>> Now we convert the split pieces of GL_LINE_LOOP prims into GL_LINE_STRIP
>> primitives so that drivers don't have to worry about the special begin/end
>> flags.  The only time a driver will get a GL_LINE_LOOP prim is when the
>> whole thing fits in one vertex buffer.
>>
>> Most fixes bug 81174, but not completely.  There's another bug somewhere
>> in the src/gallium/auxiliary/draw/ code.  If the piglit lineloop test is
>> run with -count 4096, rendering is correct, but with -count 4097 there are
>> stray lines.  4096 is a magic number in the draw code (search for "4096").
>>
>> Also note that this does not fix long line loops in display lists.  The
>> next patch fixes that.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81174
>> ---
>>   src/mesa/vbo/vbo_context.h   |  5 ++++-
>>   src/mesa/vbo/vbo_exec_api.c  | 38 +++++++++++++++++++++++++++++++++++++-
>>   src/mesa/vbo/vbo_exec_draw.c | 12 ++++++++++++
>>   3 files changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/vbo/vbo_context.h b/src/mesa/vbo/vbo_context.h
>> index 1e85335..28f43b4 100644
>> --- a/src/mesa/vbo/vbo_context.h
>> +++ b/src/mesa/vbo/vbo_context.h
>> @@ -205,7 +205,10 @@ vbo_get_default_vals_as_union(GLenum format)
>>   static inline unsigned
>>   vbo_compute_max_verts(const struct vbo_exec_context *exec)
>>   {
>> -   return (VBO_VERT_BUFFER_SIZE - exec->vtx.buffer_used) /
>> +   /* Subtract one so we're always sure to have room for an extra
>> +    * vertex for GL_LINE_LOOP -> GL_LINE_STRIP conversion.
>> +    */
>> +   return (VBO_VERT_BUFFER_SIZE - exec->vtx.buffer_used - 1) /
>>             (exec->vtx.vertex_size * sizeof(GLfloat));
>
> We may need to check for a negaive result here just in case
> buffer_used is very close or equal to VBO_VERT_BUFFER_SIZE.

Charmaine found the bug in that function.  But I'll do some extra 
testing with the fix too.


>
>>   }
>>
>> diff --git a/src/mesa/vbo/vbo_exec_api.c b/src/mesa/vbo/vbo_exec_api.c
>> index 0116f56..3c25ec0 100644
>> --- a/src/mesa/vbo/vbo_exec_api.c
>> +++ b/src/mesa/vbo/vbo_exec_api.c
>> @@ -61,7 +61,8 @@ static void reset_attrfv( struct vbo_exec_context *exec );
>>
>>   /**
>>    * Close off the last primitive, execute the buffer, restart the
>> - * primitive.
>> + * primitive.  This is called when we fill a vertex buffer before
>> + * hitting glEnd.
>>    */
>>   static void vbo_exec_wrap_buffers( struct vbo_exec_context *exec )
>>   {
>> @@ -83,6 +84,22 @@ static void vbo_exec_wrap_buffers( struct vbo_exec_context *exec )
>>
>>         last_count = last_prim->count;
>>
>> +      /* Special handling for wrapping GL_LINE_LOOP */
>> +      if (last_prim->mode == GL_LINE_LOOP &&
>> +          last_count > 0 &&
>> +          !last_prim->end) {
>> +         /* draw this section of the incomplete line loop as a line strip */
>> +         last_prim->mode = GL_LINE_STRIP;
>> +         if (!last_prim->begin) {
>> +            /* This is not the first section of the line loop, so don't
>> +             * draw the 0th vertex.  We're saving it until we draw the
>> +             * very last section of the loop.
>> +             */
>
> I don't understand some of the prim fields, so I'm assuming "start" is
> an index into the VBO, i.e. [0 ... start ... n] where 'n' is index to
> the last vertex.

Yes.


>
> With that assumption, if we do the below, then don't we miss the
> [start -> start + 1] strip?

No, the [start] vertex is always the 0th vertex of the original loop. 
We need to ignore/skip that vertex until the very end when we draw it to 
complete the loop.

And now that I've double-checked that, I realize that the assertion I 
added in vbo_copy_vertices() is incorrect: 'start' can be zero there.

I'll fix that.

-Brian


>
>
>> +            last_prim->start++;
>> +            last_prim->count--;
>> +         }
>> +      }
>> +
>>         /* Execute the buffer and save copied vertices.
>>          */
>>         if (exec->vtx.vert_count)
>> @@ -98,6 +115,7 @@ static void vbo_exec_wrap_buffers( struct vbo_exec_context *exec )
>>
>>         if (_mesa_inside_begin_end(exec->ctx)) {
>>   	 exec->vtx.prim[0].mode = exec->ctx->Driver.CurrentExecPrimitive;
>> +	 exec->vtx.prim[0].begin = 0;
>>   	 exec->vtx.prim[0].start = 0;
>>   	 exec->vtx.prim[0].count = 0;
>>   	 exec->vtx.prim_count++;
>> @@ -827,6 +845,24 @@ static void GLAPIENTRY vbo_exec_End( void )
>>         last_prim->end = 1;
>>         last_prim->count = exec->vtx.vert_count - last_prim->start;
>>
>> +      /* Special handling for GL_LINE_LOOP */
>> +      if (last_prim->mode == GL_LINE_LOOP && last_prim->begin == 0) {
>> +         /* We're finishing drawing a line loop.  Append 0th vertex onto
>> +          * end of vertex buffer so we can draw it as a line strip.
>> +          */
>> +         const fi_type *src = exec->vtx.buffer_map;
>> +         fi_type *dst = exec->vtx.buffer_map +
>> +            exec->vtx.vert_count * exec->vtx.vertex_size;
>> +
>> +         /* copy 0th vertex to end of buffer */
>> +         memcpy(dst, src, exec->vtx.vertex_size * sizeof(fi_type));
>> +
>> +         assert(last_prim->start == 0);
>> +         last_prim->start++;  /* skip vertex0 */
>> +         /* note that last_prim->count stays unchanged */
>> +         last_prim->mode = GL_LINE_STRIP;
>> +      }
>> +
>>         try_vbo_merge(exec);
>>      }
>>
>> diff --git a/src/mesa/vbo/vbo_exec_draw.c b/src/mesa/vbo/vbo_exec_draw.c
>> index 397fc09..f91f7c8 100644
>> --- a/src/mesa/vbo/vbo_exec_draw.c
>> +++ b/src/mesa/vbo/vbo_exec_draw.c
>> @@ -110,6 +110,18 @@ vbo_copy_vertices( struct vbo_exec_context *exec )
>>   	 return 1;
>>         }
>>      case GL_LINE_LOOP:
>> +      if (last_prim->begin == 0) {
>> +         /* We're dealing with the second or later section of a split/wrapped
>> +          * GL_LINE_LOOP.  Since we're converting line loops to line strips,
>> +          * we've already increment the last_prim->start counter by one to
>> +          * skip the 0th vertex in the loop.  We need to undo that (effectively
>> +          * subtract one from last_prim->start) so that we copy the 0th vertex
>> +          * to the next vertex buffer.
>> +          */
>> +         assert(last_prim->start > 0);
>> +         src -= exec->vtx.vertex_size;
>> +      }
>> +      /* fall-through */
>>      case GL_TRIANGLE_FAN:
>>      case GL_POLYGON:
>>         if (nr == 0) {
>> --
>> 1.9.1
>>