Bug 81174 - Gallium: GL_LINE_LOOP broken with more than 512 points
Summary: Gallium: GL_LINE_LOOP broken with more than 512 points
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: 10.2
Hardware: Other All
: medium normal
Assignee: mesa-dev
QA Contact:
URL:
Whiteboard:
Keywords:
: 28130 49779 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-07-10 13:53 UTC by Florian Link
Modified: 2015-10-31 13:48 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
This should be a circle... (15.50 KB, image/png)
2014-07-10 13:53 UTC, Florian Link
Details
A Qt example that demonstrates the problem (6.46 KB, application/zip)
2014-07-10 13:54 UTC, Florian Link
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Link 2014-07-10 13:53:37 UTC
Created attachment 102536 [details]
This should be a circle...

The error ocurrs on Windows 7 and on Linux with Mesa softpipe and llvm.

There seem to be random lines between non-adjacent points in the line loop.
When switching to GL_LINE_STRIP, the error disappears.

Attached you find a screenshot and a Qt example program (see glwidget.cpp for the minimal GL_LINE_LOOP code).

The bug only happens when there are more than about 512 lines and there are more errors the more lines are in the loop (as seen in the screenshot).
Comment 1 Florian Link 2014-07-10 13:54:46 UTC
Created attachment 102537 [details]
A Qt example that demonstrates the problem
Comment 2 Benjamin Bellec 2014-07-10 20:23:21 UTC
Is that a regression ?
Comment 3 Marek Olšák 2014-07-11 00:42:19 UTC
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.
Comment 4 Florian Link 2014-07-11 07:38:43 UTC
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.
Comment 5 Florian Link 2014-07-11 08:44:49 UTC
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.
Comment 6 Marek Olšák 2014-07-11 16:56:08 UTC
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.
Comment 7 Roland Scheidegger 2014-07-15 00:33:40 UTC
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).
Comment 8 Marek Olšák 2014-07-15 01:12:53 UTC
Line loops aren't so critical, since they are rarely used. But what about triangle strips... every split kills 2 triangles...
Comment 9 Roland Scheidegger 2014-07-15 03:26:39 UTC
(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.
Comment 10 Florian Link 2014-07-15 05:33:30 UTC
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).
Comment 11 Marek Olšák 2014-07-15 15:16:31 UTC
(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.
Comment 12 Marek Olšák 2014-07-15 15:24:43 UTC
(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?
Comment 13 Roland Scheidegger 2014-07-15 16:20:36 UTC
(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.)
Comment 14 Roland Scheidegger 2014-07-16 00:50:06 UTC
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.
Comment 15 Florian Link 2014-09-08 11:25:46 UTC
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.
Comment 16 Brian Paul 2015-10-16 21:26:34 UTC
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.
Comment 17 Brian Paul 2015-10-17 16:13:43 UTC
*** Bug 28130 has been marked as a duplicate of this bug. ***
Comment 18 Brian Paul 2015-10-17 16:14:09 UTC
*** Bug 49779 has been marked as a duplicate of this bug. ***
Comment 19 Brian Paul 2015-10-17 18:28:41 UTC
(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.
Comment 20 Brian Paul 2015-10-21 01:16:32 UTC
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.
Comment 21 Brian Paul 2015-10-31 13:48:06 UTC
I found/fixed another long line loop issue.  Patch "vbo: fix another GL_LINE_LOOP bug" has been posted for review.


bug/show.html.tmpl processed on Jan 16, 2017 at 21:52:13.
(provided by the Example extension).