glVertexAttribPointer size=GL_BGRA not respected

[JayFoxRox]

This is a really confusing issue because textures will be correct, the framebuffer will have color channels swapped.
glVertexAttribPointer supports GL_BGRA for type GL_UNSIGNED_BYTE, GL_INT_2_10_10_10_REV or GL_UNSIGNED_INT_2_10_10_10_REV to swap the color channels to be D3D style.
Currently apitrace seems to ignore GL_BGRA and uploads colors using size 4 which will be RGBA.
So any draw call made will have the color channels wrong - when the surface is read to the CPU and reuploaded apitrace will show the correct image from the glTexImage blob.

[jrfonseca]

Which OpenGL driver are you using?

I suspect this might be a bug in the OpenGL implementation. The wrappers need to defer glVertexAttribPointer() calls until draw time, and it reconstructs the arguments by querying the OpenGL driver via glGetVertexAttribiv(index, GL_VERTEX_ATTRIB_ARRAY_SIZE, &size), and probably it is the driver that is returning 4 instead of GL_BGRA.

[JayFoxRox]

You are absolutly right, I just verified this issue. Writing GL_BGRA will still read 4. The issue happens with both, software rendering and hw rendering.

So this is probably a Mesa issue:

Running on a Mesa DRI Mobile Intel® GM45 Express Chipset from Intel Open Source Technology Center
OpenGL version 2.1 Mesa 10.1.1 is supported

I thought apitrace was shadowing the size directly, so I didn't include it in the original post, sorry.

However, the glVertexAttribiv man page also doesn't specify that GL_BGRA can or should be returned. The spec (GL4.4 core) is not clear about it either - it just says that GL_VERTEX_ATTRIB_ARRAY_SIZE is supposed to return the number of components, the exception for GL_BGRA is not made for reads. For writes this is rather clearly defined though: number of components or GL_BGRA.
So we may not claim this is an implementation error yet.
Long story short: As the writes are described in the spec but reads don't necessarily return what was written, this is a case where apitrace should possibly shadow the parameter because there is no way it will be able to handle the draw calls properly accross all drivers otherwise at it would probably depend on unclear or undefined behaviour.

[There still seems to be a bug in Mesa which causes the normalize argument to not be ignored when using GL_BGRA as size (as accoding to spec). I should probably update Mesa, retest and possibly report that elsewhere]

[jrfonseca]

piglit has a test, bgra-vert-attrib-pointer, and I tried to trace with it Mesa (master), and it worked fine:

$ apitrace dump /home/jfonseca/work/vmware/tests/piglit/bgra-vert-attrib-pointer.trace | grep -C 3 GL_BGRA
264 glDisableVertexAttribArray(index = 1)
265 glEnableVertexAttribArray(index = 1)
266 glVertexAttribPointer(index = 0, size = 3, type = GL_FLOAT, normalized = GL_FALSE, stride = 12, pointer = blob(48))
267 glVertexAttribPointer(index = 1, size = GL_BGRA, type = GL_UNSIGNED_BYTE, normalized = GL_TRUE, stride = 4, pointer = blob(16))
268 glDrawArrays(mode = GL_TRIANGLE_STRIP, first = 0, count = 4)
270 glPopMatrix()
272 glEnable(cap = GL_BLEND)
--
284 glDisableVertexAttribArray(index = 1)
285 glEnableVertexAttribArray(index = 1)
286 glVertexAttribPointer(index = 0, size = 3, type = GL_FLOAT, normalized = GL_FALSE, stride = 12, pointer = blob(48))
287 glVertexAttribPointer(index = 1, size = GL_BGRA, type = GL_UNSIGNED_BYTE, normalized = GL_TRUE, stride = 4, pointer = blob(16))
288 glDrawArrays(mode = GL_TRIANGLE_STRIP, first = 0, count = 4)
289 glPopMatrix()
290 glPopMatrix()

[jrfonseca]

Long story short: As the writes are described in the spec but reads don't necessarily return what was written, this is a case where apitrace should possibly shadow the parameter

It's not straightforward to shadow this, as it needs to be shadowed on a per VAO (Vertex Array Object) basis.

because there is no way it will be able to handle the draw calls properly accross all drivers otherwise at it would probably depend on unclear or undefined behaviour.

There's not enough data to tell if this is just a Mesa bug, or a general ambiguity interpreting the spec. If Mesa is the only one with this bug, then it can be fixed easily enough.

There were actually many similar bugs in Mesa in respect with glGets I had to finish. Maintaing workarounds for buggy drivers in apitrace takes a toll on maintenaince, so I avoid that as much as possible.

[JayFoxRox]

I'm confused now. It seems to work in one of my applications but not in another one. One being a test I wrote to test what the read size was as you suggested that might be the problem, the other one being a system-emulator which gets thousands of GL calls from an application I did not write myself.
I didnt even check if the output was correct in the smaller test as I assumed it would be bad. Looking at it again reveals it is working properly there. The emulator on the other hand still seems to have RGB<>BGR issues.

It might be a framebuffer or texture issue then (as only about half the objects have the wrong color in the apitrace output, but they are correct when running normally. I thought I tracked it down to be a GL_BGRA problem as the problem also happens with untextured objects but it might be a different issue than I thought).

I'll close this and will reopen with a test case or trace file.

Thanks for looking into it anway 👍 . I really appreciate the time and effort you are putting into apitrace.

[JayFoxRox]

This turns out the be an apitrace issue after all.

http://www.jannikvogel.de/scratchpad/bug.zip

This is an example from the web, modified to show the bug. I added a trace file which shows the problem, it can already be seen by ust looking at it, that GL_BGRA is recorded when using an ARRAY_BUFFER - if no buffer is bound GL_BGRA is not properly recorded.

This is probably no GetVertexAttrib problem in the driver either because in both cases it will return 4 even if GL_BGRA was set. So clearly apitrace somehow shadows this for ARRAY_BUFFERs already.

The reason why your test works but my emulator doesn't is because your test uses something like an ARRAY_BUFFER while I use a simple array.
One wil trigger the bug, the other one won't.

I'll probably add a vertex cache to the emulator anyway so this issue is not too important probably, but it should still be fixed as it will result in very confusing problems.

[jrfonseca]

No, I'm pretty sure this is a driver bug, like I described on #261 (comment)

But it only happens on your system. Your test case works fine on mine (3.0 Mesa 10.1.2).

The VBO case (ARRAY_BUFFER) is not relevant. As I said in another issue, when using VBOs ApiTrace will not fiddle with glVertexAttribPointer calls at all, so everything will "just work" there, no matter how broken glGetVertexAttribiv() is.

What I still don't understand is how with two extremely close versions of Mesa one (10.1.1),shows the issue and the other (10.1.2) doesn't.

Anyway, I plan to add the test case to apitrace, and add runtime detection that will emit a warning whenever the problem is detected, pointing to this issue. If many users hit it I'll reconsider further action.

[JayFoxRox]

Oh, I didn't understand that ARRAY_BUFFER won't be using the glGetVertexAttrib at all earlier. Makes sense now. Thanks for the explanation.

I upgraded my system again and it works now (Mesa 10.1.4).
I'm not sure which change from the Mesa changelog affects this, but it works - so I'm happy.

Good idea to give a warning. I'll keep this issue opened, so you can close it when you pushed the commit which adds the warning. Thanks again!

[jrfonseca]

This was indeed fixed in 10.1.1 -> 10.1.2, http://cgit.freedesktop.org/mesa/mesa/commit/?id=b026b6bbfe3f15c8a7296ac107dc3d31f74e401e . The commit mentions Khronos OpenGL Conform Test Suite, so spite the ambiguity in the text, it looks like there is no ambiguity in the expected behavior.

[jrfonseca]

Mesa 10.1.2 had the fix for GL_VERTEX_ATTRIB_ARRAY_SIZE but GL_COLOR_ARRAY_SIZE/GL_SECONDARY_COLOR_ARRAY_SIZE was still broken. I've fixed them now -- http://cgit.freedesktop.org/mesa/mesa/commit/?id=e3e13d6b857b3083e2553457fe30ddfd4eddead4 , and also added a piglit test to catch the bug.