Created attachment 102537 [details]
A Qt example that demonstrates the problem

Is that a regression ?

No. If you use the immediate-mode style rendering (glBegin/End), the vertices are written into a buffer. When the buffer is full, all vertices are drawn and a new buffer is allocated for the following vertex data. Guess what, you'll end up with a lot of line loops instead of just one.

That sounds like a reasonable explanation, except that the closing lines of the line loops are not always the same, they change when the same line loop is rendered again. But maybe there is some kind of ring buffer involved, that would explain it.

I had a look at the code and it looks like the problem is that
the data is collected in a VBO in file:

vbo_save_api.c

and if the VBO_SAVE_BUFFER_SIZE is exhausted, the buffer is drawn as
a line loop, even if the line loop is not yet finished.

I guess the correct fix would be to only draw a GL_LINE_STRIP for all overflow related draws, but I guess the problem is where the first vertex for line closing should be cached, since it will be gone with the first overflow.

I don't understand the code well enough to be able to fix this.

vbo_save_api.c is for display lists. Direct execution is done in vbo_exec_api.c and vbo_exec_draw.c. Other than that, your understanding is correct.

Hmm interesting. I thought more than 512 vertices would fit before it uses a new buffer (the buffer size is 64kB though I don't know what the vertex size ends up with). But if the bug is due to that I guess indeed changing the prim type (probably in vbo_exec_wrap_buffers()) and fixing up the copied vertices stuff (if it doesn't do the right thing already) would do the trick. But indeed closing the loop finally would be tricky (well I guess would need to keep the vertex around when doing the buffer wrap, then can simply emit it as another vertex last). I have no idea of the effects this has on things like line stippling but I guess it couldn't be worse than it is now...
Also, things like GL_POLYGON with fill mode line probably has the same problem and can't be fixed that way really.
Note that the draw module has its own decomposition of primitives into smaller chunks too which could also be buggy (vsplit stuff).

Line loops aren't so critical, since they are rarely used. But what about triangle strips... every split kills 2 triangles...

(In reply to comment #8)
> Line loops aren't so critical, since they are rarely used. But what about
> triangle strips... every split kills 2 triangles...

That shouldn't happen, the code is supposed to copy over the necessary two tris.

What I don't understand yet is why the effect is not stable (if my example is repainted several times, the wrap appears at different positions over time).

(In reply to comment #10)
> What I don't understand yet is why the effect is not stable (if my example
> is repainted several times, the wrap appears at different positions over
> time).

The wrap is done when the buffer is full, not at the beginning of a frame. If the buffer is half full when a frame starts, the wrap will occur earlier.

(In reply to comment #9)
> (In reply to comment #8)
> > Line loops aren't so critical, since they are rarely used. But what about
> > triangle strips... every split kills 2 triangles...
> 
> That shouldn't happen, the code is supposed to copy over the necessary two
> tris.

What about primitive winding/facing? If you wrap a triangle strip and start on an odd triangle, you'll end up with correct front/back faces. If you start on an even triangle, the facing is inverted, isn't it?

(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Line loops aren't so critical, since they are rarely used. But what about
> > > triangle strips... every split kills 2 triangles...
> > 
> > That shouldn't happen, the code is supposed to copy over the necessary two
> > tris.
> 
> What about primitive winding/facing? If you wrap a triangle strip and start
> on an odd triangle, you'll end up with correct front/back faces. If you
> start on an even triangle, the facing is inverted, isn't it?

This should also be handled correctly I believe. See the comments about "parity issue" in vbo_copy_vertices(). Essentially the code ensures the new buffer always starts on an odd triangle. (Though on a quick look I wouldn't be able to tell if it's all correct but it probably is as otherwise there'd probably have been bug reports about it.)

In my testing this seems to be *both* a core (vbo) and a draw problem.
With my piglit test the first error (without redrawing) appears with 4097 vertices - despite that the vbo code can handle 5460 (per vertex size is just 12 bytes) without splitting.

In addition to the above bug, I found that the stippling pattern is reset when the intermediate buffer is commited. Thus, GL_LINE_STRIP and GL_LINE_LOOP stipple patterns jump at the same places where the GL_LINE_LOOP bug appears. The correct solution would be to correctly restart the stipple pattern when flushing the buffer on long line loops.

Another stippling issue is that stippling does not stay at fixed places when parts of the line strip is outside of the viewport.

I'm digging into this bug because it pertains to an issue with a particular app and the VMware gallium driver.

The VBO code for splitting GL_LINE_LOOP is actually correct, I believe, but our implementations of vbo_context::draw_prims(), such as st_draw_vbo() and brw_draw_prims() are subtly broken.  And has been broken since day one!

The issue comes from the two 'begin' and 'end' flags in the _mesa_prim structure.  These flags indicate whether the primitive's vertices start at a glBegin() and whether the prim's vertices end at a glEnd().  Suppose we have a long GL_LINE_LOOP that gets broken into three pieces.  Here are the flags for the three _mesa_prims that we draw:

_mesa_prim[0].begin = 1
_mesa_prim[0].end = 0
_mesa_prim[1].begin = 0
_mesa_prim[1].end = 0
_mesa_prim[2].begin = 0
_mesa_prim[2].end = 1

For all three drawing calls, the 0th vertex in the primitive's vertex buffer will be a copy of the first glVertex() issued after glBegin.  If N is the number of vertices in the _mesa_prim:

For _mesa_prim[0] we should draw the line segments from v[0] .. V[N-1]
For _mesa_prim[1] we should draw the line segments from v[1] .. V[N-1]
For _mesa_prim[2] we should draw the line segments from v[1] .. V[N-1] and an extra line from V[N-1] to v[0]

You can see this in the old 'tnl' code's t_vb_rendertmp.h code for GL_LINE_LOOP.

Our implementations of draw_prims() ignore those flags and always draw V[0].V[N-1] so we get the stray lines that people are seeing.

Furthermore, draw_prims() is supposed to look at the 'begin' flag to know when to reset the line stipple counter.  We don't do that in the state tracker either.

I've posted a patch series that fixes this.  The basic idea is when we have to split a GL_LINE_LOOP, draw the pieces with GL_LINE_STRIP instead so that drivers don't need to worry about the 'begin' and 'end' flags (except where the stipple counter matters).  Drivers will only get a GL_LINE_LOOP when all the vertices live in one vertex buffer.

Unfortunately, after fixing the VBO code, there's still a bug somewhere in the gallium 'draw' code.  See comments in the patch series for more information.

*** Bug 28130 has been marked as a duplicate of this bug. ***

*** Bug 49779 has been marked as a duplicate of this bug. ***

(In reply to Brian Paul from comment #16)

> Unfortunately, after fixing the VBO code, there's still a bug somewhere in
> the gallium 'draw' code.  See comments in the patch series for more
> information.

I've found/fixed that (simple) bug now too.  Patch posted to mesa-dev for review.

The fixes for the VBO module are committed as of f221580937.
The fix for the gallium/draw module (for llvmpipe/softpipe) is committed as of b48e16fa2f8b96b.

Closing as fixed.

I found/fixed another long line loop issue.  Patch "vbo: fix another GL_LINE_LOOP bug" has been posted for review.