From cc16eb832104fe970b1bf24fb0558573b0f2d84c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 10 May 2023 00:20:43 +0200 Subject: [PATCH 1/2] Shrink the GLRRenderCommand struct from 152 to 88 bytes Turns out the VR work bloated it a bit, which can't be good. Think it's fine to allocate these view matrices on the heap to get them out of the way, there won't be that crazy many per frame usually. --- Common/GPU/OpenGL/GLQueueRunner.cpp | 19 ++++++++++--------- Common/GPU/OpenGL/GLQueueRunner.h | 9 +++++++-- Common/GPU/OpenGL/GLRenderManager.cpp | 5 +++++ Common/GPU/OpenGL/GLRenderManager.h | 12 ++++++------ 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/Common/GPU/OpenGL/GLQueueRunner.cpp b/Common/GPU/OpenGL/GLQueueRunner.cpp index 80a3193e40db..3b67b6950ca4 100644 --- a/Common/GPU/OpenGL/GLQueueRunner.cpp +++ b/Common/GPU/OpenGL/GLQueueRunner.cpp @@ -1038,29 +1038,30 @@ void GLQueueRunner::PerformRenderPass(const GLRStep &step, bool first, bool last { _dbg_assert_(curProgram); if (IsMultiviewSupported()) { - int layout = GetStereoBufferIndex(c.uniformMatrix4.name); + int layout = GetStereoBufferIndex(c.uniformStereoMatrix4.name); if (layout >= 0) { int size = 2 * 16 * sizeof(float); - glBindBufferBase(GL_UNIFORM_BUFFER, layout, *c.uniformMatrix4.loc); - glBindBuffer(GL_UNIFORM_BUFFER, *c.uniformMatrix4.loc); + glBindBufferBase(GL_UNIFORM_BUFFER, layout, *c.uniformStereoMatrix4.loc); + glBindBuffer(GL_UNIFORM_BUFFER, *c.uniformStereoMatrix4.loc); void *matrices = glMapBufferRange(GL_UNIFORM_BUFFER, 0, size, GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_BUFFER_BIT); - memcpy(matrices, c.uniformMatrix4.m, size); + memcpy(matrices, c.uniformStereoMatrix4.mData, size); glUnmapBuffer(GL_UNIFORM_BUFFER); glBindBuffer(GL_UNIFORM_BUFFER, 0); } } else { - int loc = c.uniformMatrix4.loc ? *c.uniformMatrix4.loc : -1; - if (c.uniformMatrix4.name) { - loc = curProgram->GetUniformLoc(c.uniformMatrix4.name); + int loc = c.uniformStereoMatrix4.loc ? *c.uniformStereoMatrix4.loc : -1; + if (c.uniformStereoMatrix4.name) { + loc = curProgram->GetUniformLoc(c.uniformStereoMatrix4.name); } if (loc >= 0) { if (GetVRFBOIndex() == 0) { - glUniformMatrix4fv(loc, 1, false, c.uniformMatrix4.m); + glUniformMatrix4fv(loc, 1, false, c.uniformStereoMatrix4.mData); } else { - glUniformMatrix4fv(loc, 1, false, &c.uniformMatrix4.m[16]); + glUniformMatrix4fv(loc, 1, false, c.uniformStereoMatrix4.mData + 16); } } } + delete [] c.uniformStereoMatrix4.mData; CHECK_GL_ERROR_IF_DEBUG(); break; } diff --git a/Common/GPU/OpenGL/GLQueueRunner.h b/Common/GPU/OpenGL/GLQueueRunner.h index e662b43dd190..5d6a0bf75398 100644 --- a/Common/GPU/OpenGL/GLQueueRunner.h +++ b/Common/GPU/OpenGL/GLQueueRunner.h @@ -25,7 +25,7 @@ struct GLOffset2D { int x, y; }; -enum class GLRAllocType { +enum class GLRAllocType : uint8_t { NONE, NEW, ALIGNED, @@ -130,8 +130,13 @@ struct GLRRenderData { struct { const char *name; // if null, use loc const GLint *loc; - float m[32]; + float m[16]; } uniformMatrix4; + struct { + const char *name; // if null, use loc + const GLint *loc; + float *mData; // new'd, 32 entries + } uniformStereoMatrix4; struct { uint32_t clearColor; float clearZ; diff --git a/Common/GPU/OpenGL/GLRenderManager.cpp b/Common/GPU/OpenGL/GLRenderManager.cpp index 3b0acd83a71c..a28ab3e4cc3c 100644 --- a/Common/GPU/OpenGL/GLRenderManager.cpp +++ b/Common/GPU/OpenGL/GLRenderManager.cpp @@ -41,6 +41,11 @@ GLRTexture::~GLRTexture() { } } +GLRenderManager::GLRenderManager() { + // size_t sz = sizeof(GLRRenderData); + // _dbg_assert_(sz == 88); +} + GLRenderManager::~GLRenderManager() { _dbg_assert_(!run_); diff --git a/Common/GPU/OpenGL/GLRenderManager.h b/Common/GPU/OpenGL/GLRenderManager.h index f06724a6cbef..fba1ec678cab 100644 --- a/Common/GPU/OpenGL/GLRenderManager.h +++ b/Common/GPU/OpenGL/GLRenderManager.h @@ -390,10 +390,9 @@ struct GLRRenderThreadTask { // directly in the destructor. class GLRenderManager { public: - GLRenderManager() {} + GLRenderManager(); ~GLRenderManager(); - void SetInvalidationCallback(InvalidationCallback callback) { invalidationCallback_ = callback; } @@ -783,10 +782,11 @@ class GLRenderManager { _dbg_assert_(curProgram_); #endif GLRRenderData data{ GLRRenderCommand::UNIFORMSTEREOMATRIX }; - data.uniformMatrix4.name = name; - data.uniformMatrix4.loc = loc; - memcpy(&data.uniformMatrix4.m[0], left, sizeof(float) * 16); - memcpy(&data.uniformMatrix4.m[16], right, sizeof(float) * 16); + data.uniformStereoMatrix4.name = name; + data.uniformStereoMatrix4.loc = loc; + data.uniformStereoMatrix4.mData = new float[32]; + memcpy(&data.uniformStereoMatrix4.mData[0], left, sizeof(float) * 16); + memcpy(&data.uniformStereoMatrix4.mData[16], right, sizeof(float) * 16); curRenderStep_->commands.push_back(data); } From f593d65833124cf5332b8563f572ac70498307a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 10 May 2023 10:13:54 +0200 Subject: [PATCH 2/2] Avoid double-free in stereo mode --- Common/GPU/OpenGL/GLQueueRunner.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Common/GPU/OpenGL/GLQueueRunner.cpp b/Common/GPU/OpenGL/GLQueueRunner.cpp index 3b67b6950ca4..fd57be4bdc8c 100644 --- a/Common/GPU/OpenGL/GLQueueRunner.cpp +++ b/Common/GPU/OpenGL/GLQueueRunner.cpp @@ -1048,6 +1048,7 @@ void GLQueueRunner::PerformRenderPass(const GLRStep &step, bool first, bool last glUnmapBuffer(GL_UNIFORM_BUFFER); glBindBuffer(GL_UNIFORM_BUFFER, 0); } + delete[] c.uniformStereoMatrix4.mData; // We only playback once. } else { int loc = c.uniformStereoMatrix4.loc ? *c.uniformStereoMatrix4.loc : -1; if (c.uniformStereoMatrix4.name) { @@ -1060,8 +1061,12 @@ void GLQueueRunner::PerformRenderPass(const GLRStep &step, bool first, bool last glUniformMatrix4fv(loc, 1, false, c.uniformStereoMatrix4.mData + 16); } } + if (GetVRFBOIndex() == 1 || GetVRPassesCount() == 1) { + // Only delete the data if we're rendering the only or the second eye. + // If we delete during the first eye, we get a use-after-free or double delete. + delete[] c.uniformStereoMatrix4.mData; + } } - delete [] c.uniformStereoMatrix4.mData; CHECK_GL_ERROR_IF_DEBUG(); break; }