Bug 61907 - Indirect rendering of multi-texture vertex arrays broken
Summary: Indirect rendering of multi-texture vertex arrays broken
Alias: None
Product: Mesa
Classification: Unclassified
Component: GLX (show other bugs)
Version: 9.1
Hardware: All All
: medium normal
Assignee: Matt Turner
QA Contact: mesa-dev
Depends on:
Reported: 2013-03-06 15:29 UTC by Colin McDonald
Modified: 2016-07-08 21:06 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

Updated files with fixes for mesa src/glx (100.00 KB, text/plain)
2013-03-06 15:29 UTC, Colin McDonald

Description Colin McDonald 2013-03-06 15:29:45 UTC
Created attachment 76023 [details]
Updated files with fixes for mesa src/glx

Indirect rendering of multi-texture vertex arrays is currently
broken with three separate bugs in src/glx, which I have attempted to fix
in the attached files.

Bug 1:  For each indirect context the indirect vertex array state must be
initialised by __glXInitVertexArrayState in indirect_vertex_array.c.  As
noted in the routine header it requires that the glx context has been setup
prior to the call, in order to test the server version and extensions.

Currently __glXInitVertexArrayState is called from indirect_bind_context
in indirect_glx.c, as follows:

   state = gc->client_state_private;
   if (state->array_state == NULL) {

But, the gc context is not yet usable at this stage, so the server queries
fail, and __glXInitVertexArrayState is called without the server version
and extension information it needs.  This breaks multi-texturing as
glXInitVertexArrayState doesn't get GL_MAX_TEXTURE_UNITS.  It probably also
breaks setup of other arrays: fog, secondary colour, vertex attributes.

To fix this I have moved the call to __glXInitVertexArrayState to the end
of MakeContextCurrent in glxcurrent.c, where the glx context is usable.

Bug 2.  There are a couple of typos in the op codes in
__indirect_glTexCoordPointer in indirect_vertex_array.c, resulting in the
wrong command op code being sent.  I have corrected them.

Bug 3.  There is no draw arrays protocol support for multi-texture
coordinate arrays, so it is implemented by sending batches of immediate
mode commands from emit_element_none in indirect_vertex_array.c.  This
sends the target texture unit (which has been previously setup in the
array_state header field), followed by the texture coordinates.  But for
GL_DOUBLE coordinates the texture unit must be sent *after* the texture
coordinates. This is documented in the glx protocol description, and can
also be seen in the indirect.c immediate mode commands generated from
gl_API.xml. Sending the target texture unit in the wrong place can crash
the remote X server.

To fix this required some more extensive changes to indirect_vertex_array.c
and indirect_vertex_array_priv.h, in order to remove the texture unit value
out of the array_state "header" field, and send it separately.

The modified files from src/glx are attached.  The are based on the
latest git master branch as of 6 March 2013.

I encountered these bugs with glDrawArrays/glDrawElements calls from an
OpenSceneGraph application. I have tested the fixes on Linux x86_64,
displaying on a remote Linux X server and also on a commercial Microsoft
Windows X server.
Comment 1 Colin McDonald 2013-03-06 21:57:01 UTC
I've realised that I was so focused on describing my code changes that
I omitted to adequately describe the problem symptoms, other than to say
that multi-texturing is "broken". 

OpenSceneGraph makes extensive use off glDrawArrays and glDrawElements.
Arrays of texture coordinates are set-up using glTexCoordPointer, for
the active texture unit selected with glClientActiveTexture. For texture
units > 0, indirect rendering to a remote display results in those
texture coordinates being either ignored, or corrupting the output of
the unit 0 texture. This is because of the __glXInitVertexArrayState
initialisation problem, which fails to get GL_MAX_TEXTURE_UNITS, and
consequently rejects all texture units > 0. 

The incorrect op codes and the wrong output order for double texture
coords have the potential to cause protocol errors and/or crash the
remote display server, but we don't actually get that because of
multi-texture output being ignored due to the first problem. It is only
when the first problem is fixed that the others become apparent.
Comment 2 Brian Paul 2013-03-06 23:41:03 UTC
Just a quick note- it would be great to have a piglit test that exercises this issue (if there isn't one already).
Comment 3 Colin McDonald 2013-03-11 17:02:29 UTC
The piglit test tests/texturing/tex-skipped-unit.c demonstrates the
problem, as it uses texture unit 1 with glTexCoordPointer.

Using direct rendering, test passes.

$ piglit-run.py --tests=skip tests/all.tests  results/tex
[Mon Mar 11 16:28:02 2013] ::  running :: spec/!OpenGL 1.2/tex-skipped-unit
[Mon Mar 11 16:28:02 2013] ::     pass :: spec/!OpenGL 1.2/tex-skipped-unit

Using indirect rendering, as required for a remote X display, fails:

$ piglit-run.py --tests=skip tests/all.tests  results/tex
[Mon Mar 11 16:29:04 2013] ::  running :: spec/!OpenGL 1.2/tex-skipped-unit
[Mon Mar 11 16:29:04 2013] ::     fail :: spec/!OpenGL 1.2/tex-skipped-unit

Using LIBGL patched with the given updates is OK:

$ export LD_LIBRARY_PATH=/home/patch/lib
$ piglit-run.py --tests=skip tests/all.tests results/tex
[Mon Mar 11 16:37:29 2013] ::  running :: spec/!OpenGL 1.2/tex-skipped-unit
[Mon Mar 11 16:37:29 2013] ::     pass :: spec/!OpenGL 1.2/tex-skipped-unit

I have to say that I've had all sorts of problems getting piglit to run
on my linux system.  It's a straightward Centos 6.3 installation, but
the software renderer doesn't appear to be working properly. Hardware
rendering is better. I don't know whats going on with the system, but
I don't think those problems are related to this indirect texture
protocol issue.  Hopefully you will see the same results as given above.
Comment 4 Matt Turner 2016-06-23 06:28:32 UTC
Thank you very much for the fixes. I came across one of these bugs recently, and I was pleasantly surprised to find this nice bug report complete with fixes.

I've rebased the patches onto master and confirmed they work (after fixing another problem...). I'll send them out soon. You should expect to be in the Cc list.
Comment 5 Matt Turner 2016-07-08 21:06:34 UTC
I've pushed four patches, beginning with commit d57c85c1bf77f194720a that resolve this bug.

Colin, thank you very much for tracking this down and fixing it. Sorry it took so long for someone else to notice it.

In the future, it would make others' lives easier if you attached a real patch. I had to check out Mesa from the date you mentioned, untar the files over the existing ones, make a commit, and then rebase forward multiple years.

The next major release of Mesa will contain the fixes, which I expect will be 12.1 or 13.0 depending on how GL support progresses. We could probably cherry-pick them to older stable branches if that is useful.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.