[Mesa-dev] [PATCH] vbo: fix another GL_LINE_LOOP bug

[Brian Paul]

On 11/04/2015 12:40 PM, Charmaine Lee wrote:
>
> I concur with Sinclair and Roland that the vbo code is quite tricky.
> I do have a question, so when the line loop spans multiple vertex buffers,
> where is 0th vertex stored exactly?

It depends.  If the line loop started in a new/empty VB, the 0th vertex 
is at the start of the buffer.  But if the line loop started later in 
the buffer (because another primitive before it), the 0th vertex will be 
at the prim->start position.

I didn't find the bug that this patch fixes earlier, because my test 
programs all hit the first case.  We have to draw at least two 
primitives to hit some of the corner cases.


> In vbo_exec_End,  you are changing src = exec->vtx.buffer_map + last_prim->start * exec->vtx.vertex_size;
> If the 0th vertex is always copied to the beginning of the buffer, why do you need this change?

To account for the second case, above.


> If it was not copied to the beginning of the buffer, couldn't it be overwritten by the subsequent vertices?

This patch actually fixes a case where subsequent vertices _were_ 
overwriting previous vertices when another primitive followed the line 
loop.  That's the change at line 846.

Thanks for looking.

-Brian

>
> -Charmaine
>
>
> ________________________________________
> From: mesa-dev <mesa-dev-bounces at lists.freedesktop.org> on behalf of Brian Paul <brianp at vmware.com>
> Sent: Saturday, October 31, 2015 6:38 AM
> To: mesa-dev at lists.freedesktop.org
> Subject: [Mesa-dev] [PATCH] vbo: fix another GL_LINE_LOOP bug
>
> Very long line loops which spanned 3 or more vertex buffers were not
> handled correctly and could result in stray lines.
>
> The piglit lineloop test draws 10000 vertices by default, and is not
> long enough to trigger this.  Even 'lineloop -count 100000' doesn't
> trigger the bug.
>
> For future reference, the issue can be reproduced by changing Mesa's
> VBO_VERT_BUFFER_SIZE to 4096 and changing the piglit lineloop test to
> use glVertex2f(), draw 3 loops instead of 1, and specifying -count
> 1023.
> ---
>   src/mesa/vbo/vbo_exec_api.c  | 11 +++++++++--
>   src/mesa/vbo/vbo_exec_draw.c |  1 +
>   2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/vbo/vbo_exec_api.c b/src/mesa/vbo/vbo_exec_api.c
> index a614b26..7534599 100644
> --- a/src/mesa/vbo/vbo_exec_api.c
> +++ b/src/mesa/vbo/vbo_exec_api.c
> @@ -114,6 +114,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].end = 0;
>           exec->vtx.prim[0].start = 0;
>           exec->vtx.prim[0].count = 0;
>           exec->vtx.prim_count++;
> @@ -846,17 +847,23 @@ static void GLAPIENTRY vbo_exec_End( void )
>            /* 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;
> +         const fi_type *src = exec->vtx.buffer_map +
> +            last_prim->start * exec->vtx.vertex_size;
>            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;
> +
> +         /* Increment the vertex count so the next primitive doesn't
> +          * overwrite the last vertex which we just added.
> +          */
> +         exec->vtx.vert_count++;
> +         exec->vtx.buffer_ptr += exec->vtx.vertex_size;
>         }
>
>         try_vbo_merge(exec);
> diff --git a/src/mesa/vbo/vbo_exec_draw.c b/src/mesa/vbo/vbo_exec_draw.c
> index ed5d9e9..0d42618 100644
> --- a/src/mesa/vbo/vbo_exec_draw.c
> +++ b/src/mesa/vbo/vbo_exec_draw.c
> @@ -117,6 +117,7 @@ vbo_copy_vertices( struct vbo_exec_context *exec )
>             * subtract one from last_prim->start) so that we copy the 0th vertex
>             * to the next vertex buffer.
>             */
> +         assert(last_prim->start > 0);
>            src -= sz;
>         }
>         /* fall-through */
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>