From e87063e90c6b6fa941aa419ec5a41cd184046bcf Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 21 Jul 2026 19:03:42 -0400 Subject: [PATCH] [Fix] (DirectGLES): route default-framebuffer binds through the FBO-binding shadow - the raw glBindFramebuffer(0) in BindCurrentFBO/SyncAndBindFramebufferObject left the shadow claiming the previous user FBO, false-skipping its next re-bind and letting scoped guards restore a stale binding (caught by adversarial review); also scrub buffer-binding shadows when VAO client-attribute staging buffers are deleted, include cube-map arrays in the pack-image-params gate, and make the delete-recording test hook assertion-unwind safe --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 18 ++++++++------ MobileGL/MG_Backend/DirectGLES/Managers.cpp | 11 +++++++++ MobileGL/MG_Backend/DirectGLES/Managers.h | 4 ++++ MobileGL/MG_Test/SanityTest.cpp | 24 +++++++++++++++++-- 4 files changed, 48 insertions(+), 9 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index d8d6ef46..5c36ffe0 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -1002,8 +1002,10 @@ namespace MobileGL::MG_Backend::DirectGLES { } } else { MGLOG_D("Binding default framebuffer as %s FBO", (target == FramebufferTarget::Read ? "READ" : "DRAW")); - g_GLESFuncs.glBindFramebuffer(target == FramebufferTarget::Draw ? GL_DRAW_FRAMEBUFFER : GL_READ_FRAMEBUFFER, - 0); + // Through the shadow: a raw bind here would leave the shadow claiming + // the previous user FBO, false-skipping its next re-bind. + FramebufferImpl::BindFramebufferId( + target == FramebufferTarget::Draw ? GL_DRAW_FRAMEBUFFER : GL_READ_FRAMEBUFFER, 0); } } @@ -1013,8 +1015,8 @@ namespace MobileGL::MG_Backend::DirectGLES { ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif if (!framebuffer || framebuffer == MG_Impl::GLImpl::FramebufferImpl::pDefaultFramebufferInfo->defaultFBO) { - g_GLESFuncs.glBindFramebuffer(target == FramebufferTarget::Draw ? GL_DRAW_FRAMEBUFFER : GL_READ_FRAMEBUFFER, - 0); + FramebufferImpl::BindFramebufferId( + target == FramebufferTarget::Draw ? GL_DRAW_FRAMEBUFFER : GL_READ_FRAMEBUFFER, 0); return; } @@ -3993,9 +3995,11 @@ namespace MobileGL::MG_Backend::DirectGLES { // accepted and repacks on the CPU. if (convertible) { // GL_PACK_IMAGE_HEIGHT/GL_PACK_SKIP_IMAGES only apply to 3D/array image - // readbacks; 2D targets must ignore them (GL 3.3 section 6.1.4). - const Bool applyPackImageParams = - backendAttachTarget == GL_TEXTURE_3D || backendAttachTarget == GL_TEXTURE_2D_ARRAY; + // readbacks (cube-map arrays address as arrays); 2D targets must ignore + // them (GL 3.3 section 6.1.4). + const Bool applyPackImageParams = backendAttachTarget == GL_TEXTURE_3D || + backendAttachTarget == GL_TEXTURE_2D_ARRAY || + backendAttachTarget == GL_TEXTURE_CUBE_MAP_ARRAY; // 3D/array images read back every slice, but the FBO path can only read one layer: // multi-slice reads are served from the CPU shadow (slice-major, tight layout). const GLsizei sliceCount = std::max(size.z(), 1); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index a372dd13..969244a5 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -863,6 +863,16 @@ namespace MobileGL::MG_Backend::DirectGLES { g_boundPixelUnpackBufferKnown = false; } + void NoteBufferIdDeleted(Uint id) { + if (id == 0) { + return; + } + if (g_boundArrayBufferKnown && g_boundArrayBufferId == id) { + InvalidateArrayBufferBindingCache(); + } + ScrubBufferBindingShadowsForId(id); + } + namespace { // Shadow of the GL indexed buffer bindings so redundant glBindBufferBase/Range // (same index + id + range) are skipped. isBase distinguishes a whole-buffer @@ -1202,6 +1212,7 @@ namespace MobileGL::MG_Backend::DirectGLES { } for (auto& bufferId : m_clientAttributeBufferIds) { if (bufferId != 0) { + BufferImpl::NoteBufferIdDeleted(bufferId); g_GLESFuncs.glDeleteBuffers(1, &bufferId); bufferId = 0; } diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index c05c4899..5aad4128 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -189,6 +189,10 @@ namespace MobileGL::MG_Backend::DirectGLES { void BindPixelPackBufferId(Uint id); void BindPixelUnpackBufferId(Uint id); void InvalidatePixelBufferBindingCaches(); + // A GL buffer id is being deleted by code outside BufferImpl (e.g. the VAO + // client-attribute staging buffers): scrub every buffer-binding shadow that + // could false-skip when the name is recycled. + void NoteBufferIdDeleted(Uint id); // Redundant-bind cache for INDEXED buffer bindings (glBindBufferBase/Range on // GL_UNIFORM_BUFFER / GL_SHADER_STORAGE_BUFFER): skips the GL call when the // (id, range) already at that index matches, like the array-buffer/texture/ diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index a3ba28ba..704d5d83 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -1680,13 +1680,22 @@ namespace { if (!g_deletedTextureIds) return; for (GLsizei i = 0; i < count; ++i) g_deletedTextureIds->push_back(textures[i]); } + + // Clears the recording hook even when a gtest assertion unwinds the test body + // (a dangling pointer to the dead stack vector would corrupt later tests). + struct ScopedDeletedTextureRecording { + explicit ScopedDeletedTextureRecording(MobileGL::Vector& sink) { g_deletedTextureIds = &sink; } + ~ScopedDeletedTextureRecording() { g_deletedTextureIds = nullptr; } + ScopedDeletedTextureRecording(const ScopedDeletedTextureRecording&) = delete; + ScopedDeletedTextureRecording& operator=(const ScopedDeletedTextureRecording&) = delete; + }; } // namespace TEST(DirectGLESBackendTexture, DestructorDeletesIdAndScrubsBindingCache) { using namespace MobileGL::MG_Backend::DirectGLES; ScopedDirectGLESTextureBindings scoped; // installs glGenTextures/glBindTexture mocks + resets caches MobileGL::Vector deleted; - g_deletedTextureIds = &deleted; + ScopedDeletedTextureRecording recording(deleted); auto functions = g_GLESFuncs; functions.glDeleteTextures = SG_DeleteTextures; SetGLESFuncsTable(functions); @@ -1714,5 +1723,16 @@ TEST(DirectGLESBackendTexture, DestructorDeletesIdAndScrubsBindingCache) { --TextureImpl::g_textureContextGeneration; // restore for later tests EXPECT_EQ(deleted.size(), 1u); } - g_deletedTextureIds = nullptr; +} + +TEST(DirectGLESStateGuards, DefaultFramebufferBindGoesThroughShadow) { + using namespace MobileGL::MG_Backend::DirectGLES; + ScopedStateGuardMocks mocks; + + // The regression this guards against: binding framebuffer 0 raw while the + // shadow keeps a user-FBO id makes the next re-bind of that FBO false-skip. + FramebufferImpl::BindFramebufferId(GL_DRAW_FRAMEBUFFER, 7); + FramebufferImpl::BindFramebufferId(GL_DRAW_FRAMEBUFFER, 0); // default-FBO path must use this API + FramebufferImpl::BindFramebufferId(GL_DRAW_FRAMEBUFFER, 7); // must reach the driver again + EXPECT_EQ(mocks.log.Count("BindFramebuffer:"), 3u); }