Fix a few vertex buffer related issues - #13732
Open
notr1ch wants to merge 2 commits into
Open
Conversation
Crash reports indicated numerous crashes in LoadVertexBufferData. Looking through the code revealed that the device's curVertexBuffer / curVertexShader / curIndexBuffer variables were not cleared when the corresponding object was destroyed, leaving them pointing to dangling memory. This also affected the device rebuild path in reverse - cur was cleared, last was not. The buffer list was also assuming that every vertex shader has vertex positions, which is not true for the triangles drawn by async sources and GPU conversion textures. As those draws never load a vertex buffer, they ended up using whatever was left by the previous draw call - incorrect but harmless if the buffer is valid, use-after-free if it is not.
We load vertex and index buffers inconsistently between libobs and the frontend - some draws load a vertex buffer without an index buffer and some draws load neither. This causes the device to use whatever the previous draw call left behind, which is not a good practice. While nothing in OBS itself binds an index buffer today, the graphics API is exposed to plugins and any source render callback could leave one bound.
notr1ch
added
kind/bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Crash reports indicated something was crashing in
LoadVertexBufferData. While I was unable to reproduce a crash, I did notice some dangling pointers if vertex buffers, index buffers or vertex shaders were freed while rendering was still active. Worse, the code assumed that every draw call would have vertex positions, which is not true for the triangles used when rendering textures for GPU conversion and async sources. Those would end up using whatever vertex buffer was last loaded into buffer list, which if since-freed could also trigger a use-after-free crash.I also noticed that we were inconsistent in our use of
gs_load_indexbuffer- many places neglected to call it before drawing, causing it to use whatever the last draw caller set. Currently there's no direct code in OBS that ever calls this with a non-NULL value, but as this is exported to the API we should always assume it can be changed between draw calls.Motivation and Context
Code correctness and hopefully crash fixes.
How Has This Been Tested?
Basic run of OBS with async texture sources, no obvious regression spotted. Unable to reproduce the crashing.
Types of changes
Checklist: