Re: [PATCH] wined3d: Invalidate INDEXBUFFER device state when contents of bound index buffer is changed.
On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote:
+ if (buffer->resource.bind_count) + { + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); + } You can merge those if-conditions.
What happens exactly that makes this required?
On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote:
+ if (buffer->resource.bind_count) + { + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); + } You can merge those if-conditions.
What happens exactly that makes this required?
The bound index buffer is unloaded and the game crashes on the next draw call because we use indexed drawing with zeroish offset and GL_ELEMENT_ARRAY_BUFFER_BINDING is 0. I've sent another patch which targets this issue. The other thing is that, in theory, when a buffer is modified in the current context we should should invalidate buffer bindings for this buffer in other contexts in order to make the contents changes visible in other contexts. That's the reason why I was trying to restore device_invalidate_state() call in wined3d_buffer_load(). This would fix the regression as well. The quote from GL spec: "If the contents of an object T are changed in a context other than the cur- rent context, T must be attached or re-attached to at least one binding point in the current context, or at least one attachment point of a currently bound container object C, in order to guarantee that the new contents of T are visible in the current context."
On 31 October 2016 at 12:51, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote:
+ if (buffer->resource.bind_count) + { + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); + } You can merge those if-conditions.
What happens exactly that makes this required?
The bound index buffer is unloaded and the game crashes on the next draw call because we use indexed drawing with zeroish offset and GL_ELEMENT_ARRAY_BUFFER_BINDING is 0.
I figured it was something like that, but the question is why that happens exactly. In theory context_apply_draw_state() will load the index buffer into a specific location based on "stream_info.all_vbo", and context_update_stream_info() should make sure STATE_INDEXBUFFER is invalidated if that changes between draws. Invalidating it in buffer_unload() makes more sense, if the issue is that glDeleteBuffers() gets called for an index buffer that's still bound. (And we already have a similar invalidate for vertex buffers there.) An argument could probably be made that the invalidate belongs in delete_gl_buffer() instead of buffer_unload(), and that STATE_STREAMSRC doesn't need to be invalidated for anything other than vertex buffers.
The other thing is that, in theory, when a buffer is modified in the current context we should should invalidate buffer bindings for this buffer in other contexts in order to make the contents changes visible in other contexts. That's the reason why I was trying to restore device_invalidate_state() call in wined3d_buffer_load(). This would fix the regression as well. The quote from GL spec: "If the contents of an object T are changed in a context other than the cur- rent context, T must be attached or re-attached to at least one binding point in the current context, or at least one attachment point of a currently bound container object C, in order to guarantee that the new contents of T are visible in the current context." Yeah, but it's pretty hopeless since you'd also need to flush, and then you get into "StrictDrawOrdering" territory. This was actually the kind of thing (among others) that CSMT was intended to fix, but of course people in general care more about fps than correctness.
On Mon, Oct 31, 2016 at 1:27 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 31 October 2016 at 12:51, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote:
+ if (buffer->resource.bind_count) + { + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); + } You can merge those if-conditions.
What happens exactly that makes this required?
The bound index buffer is unloaded and the game crashes on the next draw call because we use indexed drawing with zeroish offset and GL_ELEMENT_ARRAY_BUFFER_BINDING is 0.
I figured it was something like that, but the question is why that happens exactly. In theory context_apply_draw_state() will load the index buffer into a specific location based on "stream_info.all_vbo", and context_update_stream_info() should make sure STATE_INDEXBUFFER is invalidated if that changes between draws. Invalidating it in buffer_unload() makes more sense, if the issue is that glDeleteBuffers() gets called for an index buffer that's still bound. (And we already have a similar invalidate for vertex buffers there.) An argument could probably be made that the invalidate belongs in delete_gl_buffer() instead of buffer_unload(), and that STATE_STREAMSRC doesn't need to be invalidated for anything other than vertex buffers.
Insane 2 uses a single index buffer for all draw calls when rendering game menus. This index buffer is still bound when it's unloaded. glDeleteBuffers() unbinds the deleted buffer object in the current context. After the buffer is unloaded the application still uses this deleted buffer object in other threads, but when it tries to render again in the thread in which the buffer object was deleted the crash occurs.
participants (2)
-
Henri Verbeet -
Józef Kucia