KhronosGroup / OpenGL-API

OpenGL and OpenGL ES API Issue Tracker
34 stars 5 forks source link

Ill-specified interaction of display lists and primitive restart #74

Open ianromanick opened 3 years ago

ianromanick commented 3 years ago

A few bits of the OpenGL 3.1+ specification combine to generate an interaction that may be impossible to satisfy.

Vertex array pointers are dereferenced when the commands ArrayElement, DrawArrays, DrawElements, or DrawRangeElementsare accumulated into a display list.

Based on that, what should the following code do?

    glDisable(GL_PRIMITIVE_RESTART);
    glPrimitiveRestartIndex(0x7fffffff);

    glNewList(1, GL_COMPILE);
    glBegin(GL_TRIANGLES);
    glArrayIndex(0); glArrayIndex(1); glArrayIndex(2);
    glArrayIndex(0x7fffffff);
    glArrayIndex(3); glArrayIndex(4); glArrayIndex(5);
    glEnd();
    glEndList();

    glEnable(GL_PRIMITIVE_RESTART);
    glCallList(1);

    glPrimitiveRestartIndex(0x12345678);
    glCallList(1);

The difficulty is that glArrayIndex(0x7fffffff) is supposed to dereference the vertex array at the time it is called. However, the state of GL_PRIMITIVE_RESTART and the primitive restart index aren't known until glCallList. It is impossible for the implementation to know if 0x7fffffff should start a new primitive or dereference a very large memory address.

On at least one implementation, it crashes inside glArrayIndex(0x7fffffff).

ianromanick commented 3 years ago

This would also affect GLX implementations of OpenGL 3.1+ compatibility profile due to the need for the client to dereference client-side arrays to send to the server.

NicolBolas commented 3 years ago

The interaction is not ill-specified. Since the dereferencing happened at the time of compilation, and at the time of the specification, primitive restarting was not set, the behavior is obvious: it accesses the index exactly as you specified it during compilation.

Also, the function is glArrayElement.

ianromanick commented 3 years ago

Yes, glArrayIndex should have been glArrayElement. You got me on that one.

You have chosen to interpret the spec one way. That interpretation is likely similar to the interpretation made by the vendor that crashes on array index 0x7fffffff. However, at least one other vendor does not interpret the spec that way, and that driver has very different behavior. At the point where two major vendors of desktop OpenGL drivers make dramatically different interpretations of the spec, the spec is, by definition, ill specified.

The problem is that before we removed all of the deprecated functionality from OpenGL 3.1, the primitive restart index and primitive restart control was client state. Since client state was no longer a thing in OpenGL 3.1, the restart index and the restart enable had to change.

In OpenGL 3.0 with GL_NV_primitive_restart, the interactions were all clear, and things just worked. Section 21.4 of the OpenGL 4.6 spec says (earlier versions of the spec say basically the same thing):

Doing so causes the commands within the list to be executed just as if they were given normally. The only exception pertains to commands that rely upon client state. When such a command is accumulated into the display list (that is, when issued, not when executed), the client state in effect at that time applies to the command.

It also says:

Vertex array pointers are dereferenced when the commands ArrayElement, DrawArrays, DrawElements, or DrawRangeElements are accumulated into a display list.

Calls to glClientEnable(GL_PRIMITIVE_RESTART_NV) and glPrimitiveRestartIndexNV modified client state. Those commands cannot be compiled into a display list (more on this below). As client state, this data can affect glDrawElements and friends when they are compiled into the display list. GL_NV_primitive_restart provides glPrimitiveRestartNV for this purpose. When the restart index is encountered either in a display list or when GLX protocol is used, the command glPrimitiveRestartNV is generated instead. Applying the principle of least surprise: the display list usage and the non-display list usage function the same.

Client state affects compilation of display lists, but server state affects execution of display lists. In general, server state is ignored during compilation. The commands are just stored in the list. Section 21.4 calls this out:

If mode is COMPILE, then commands are not executed as they are placed in the display list.

This prevents errors from being generated during compilation. This is why you can make a display list that just contains a bunch of called to glVertex and a trailing call to glEnd. The server state (in this case, whether or not glBegin was called) is ignored. Now GL_PRIMITIVE_RESTART is server state, but it's needed to know how to dereference the data put in the display list. Okay, maybe we could "cheat" and look at the server state while compiling the display list. That only solves part of the problem.

In addition, even in the compatibility profile specs, there is no analog to glPrimitiveRestartNV. Even if the restart index and restart enable issues were clear, the part that would defines how it actually worked in a display list was never incorporated into the core spec. Some commands, such as DrawArraysOneInstance, are defined in the spec just to be used as tools to define other functionality, so it would be perfectly normal to define glPrimitiveRestart in a similar manner.

To make matters worse, glEnable(GL_PRIMITIVE_RESTART) can be compiled into the display list. The behavior of index 0x7fffffff can change from meaning "dereference a huge pointer" to "restart the primitive" in the middle of the display list. Except... that's not possible. The glEnable isn't executed while compiling the display list, but that is when the data is dereferenced. Looking at server state doesn't help here. Somehow we'd have to modify server state too, and that's right out.

As far as I can tell from having done lots and lots of spec archaeology, changing primitive restart enable and primitive restart index to server state in compatibility profile effectively makes it impossible to both follow the letter of the spec and have primitive restart work in display lists. In my opinion, this violates the principle of least surprise. I also don't think it's what anyone intended.

NicolBolas commented 3 years ago

At the point where two major vendors of desktop OpenGL drivers make dramatically different interpretations of the spec, the spec is, by definition, ill specified.

The existence of driver bugs is not a reasonable definition of "ill specified". "Ill specified" means that the standard does not say what ought to happen. That people might fail to implement it correctly is not de facto evidence of a defect in the standard.

Your point about server state vs. client state is valid, but not the idea that driver bug == ill specified.

ianromanick commented 3 years ago

I've been actively contributing to OpenGL specs for 20 years. This is the process. Please stop being a troll.