From ef562ee9b5ca7e70138dd97873bfb08da07261c2 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 11 Aug 2026 03:54:05 -0400 Subject: [PATCH] [Fix, Test] (MG_Backend/DirectGLES): backend framebuffer, renderbuffer and sampler twins release their driver ids - a framebuffer per readback leaked the driver into stale pixels --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 30 +-- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 76 +++++- MobileGL/MG_Backend/DirectGLES/Managers.h | 37 ++- MobileGL/MG_IntegrationTest/CMakeLists.txt | 1 + .../Scenarios/PixelStoreSweepScenario.cpp | 249 ++++++++++++++++++ MobileGL/MG_Test/SanityTest.cpp | 161 ++++++++++- 6 files changed, 529 insertions(+), 25 deletions(-) create mode 100644 MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 85eec084..d774c356 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -1214,7 +1214,7 @@ namespace MobileGL::MG_Backend::DirectGLES { if (g_unitTextureSyncListValid && g_unitTextureSyncListContextId == keys.contextId && g_unitTextureSyncListMaxUnit == maxTouchedUnit && - g_unitTextureSyncListContextGeneration == g_textureContextGeneration && + g_unitTextureSyncListContextGeneration == g_backendContextGeneration && g_unitTextureSyncListEpoch == unitBindingsEpoch && g_unitTextureSyncListSamplingGeneration == samplingGeneration && PairingsIntact(g_unitTextureSyncList)) { @@ -1246,7 +1246,7 @@ namespace MobileGL::MG_Backend::DirectGLES { } g_unitTextureSyncListContextId = keys.contextId; g_unitTextureSyncListMaxUnit = maxTouchedUnit; - g_unitTextureSyncListContextGeneration = g_textureContextGeneration; + g_unitTextureSyncListContextGeneration = g_backendContextGeneration; g_unitTextureSyncListEpoch = unitBindingsEpoch; g_unitTextureSyncListSamplingGeneration = samplingGeneration; g_unitTextureSyncListValid = true; @@ -1274,7 +1274,7 @@ namespace MobileGL::MG_Backend::DirectGLES { g_fboTextureSyncListSlotVersion == fboSlotVersion && g_fboTextureSyncListObjectVersion == fboObjectVersion && g_fboTextureSyncListContextId == keys.contextId && - g_fboTextureSyncListContextGeneration == g_textureContextGeneration && + g_fboTextureSyncListContextGeneration == g_backendContextGeneration && PairingsIntact(g_fboTextureSyncList); if (fboListValid) { for (const auto& entry : g_fboTextureSyncList) { @@ -1302,7 +1302,7 @@ namespace MobileGL::MG_Backend::DirectGLES { g_fboTextureSyncListSlotVersion = fboSlotVersion; g_fboTextureSyncListObjectVersion = fboObjectVersion; g_fboTextureSyncListContextId = keys.contextId; - g_fboTextureSyncListContextGeneration = g_textureContextGeneration; + g_fboTextureSyncListContextGeneration = g_backendContextGeneration; } } else { g_fboTextureSyncListFbo = nullptr; @@ -2423,7 +2423,7 @@ namespace MobileGL::MG_Backend::DirectGLES { static_cast(maxTouchedUnit + 1) * sizeof(SamplerImpl::g_boundSamplersCache[0]); if (g_unitSamplerWalkValid && g_unitSamplerWalkContextId == keys.contextId && g_unitSamplerWalkEpoch == keys.unitBindingsEpoch && g_unitSamplerWalkMaxUnit == maxTouchedUnit && - g_unitSamplerWalkContextGeneration == TextureImpl::g_textureContextGeneration && + g_unitSamplerWalkContextGeneration == g_backendContextGeneration && std::memcmp(g_unitSamplerWalkRows.data(), SamplerImpl::g_boundSamplersCache.data(), rowBytes) == 0) { return; } @@ -2444,7 +2444,7 @@ namespace MobileGL::MG_Backend::DirectGLES { g_unitSamplerWalkContextId = keys.contextId; g_unitSamplerWalkEpoch = keys.unitBindingsEpoch; g_unitSamplerWalkMaxUnit = maxTouchedUnit; - g_unitSamplerWalkContextGeneration = TextureImpl::g_textureContextGeneration; + g_unitSamplerWalkContextGeneration = g_backendContextGeneration; std::memcpy(g_unitSamplerWalkRows.data(), SamplerImpl::g_boundSamplersCache.data(), rowBytes); g_unitSamplerWalkValid = true; } @@ -2554,7 +2554,7 @@ namespace MobileGL::MG_Backend::DirectGLES { memo.programBackendStateVersion == (currentProgram ? currentProgram->GetBackendStateVersion() : 0) && memo.programLinked == (currentProgram && currentProgram->GetLinkStatus()) && - memo.contextGeneration == TextureImpl::g_textureContextGeneration; + memo.contextGeneration == g_backendContextGeneration; // Short-circuited: the shadow compare is only meaningful once the key (and with it the // snapshotted row count) matches. if (!keysMatch || std::memcmp(memo.boundTextures.data(), TextureImpl::g_boundTexturesCache.data(), @@ -2569,7 +2569,7 @@ namespace MobileGL::MG_Backend::DirectGLES { memo.programLifetimeId = currentProgram ? currentProgram->GetLifetimeId() : 0; memo.programBackendStateVersion = currentProgram ? currentProgram->GetBackendStateVersion() : 0; memo.programLinked = currentProgram && currentProgram->GetLinkStatus(); - memo.contextGeneration = TextureImpl::g_textureContextGeneration; + memo.contextGeneration = g_backendContextGeneration; std::memcpy(memo.boundTextures.data(), TextureImpl::g_boundTexturesCache.data(), shadowBytes); memo.valid = true; } @@ -2744,7 +2744,7 @@ namespace MobileGL::MG_Backend::DirectGLES { samplerPassMemo.unitBindingsEpoch == keys.unitBindingsEpoch && samplerPassMemo.samplingGeneration == keys.samplingGeneration && samplerPassMemo.backendStateVersion == programBackendStateVersion && - samplerPassMemo.textureContextGeneration == TextureImpl::g_textureContextGeneration; + samplerPassMemo.textureContextGeneration == g_backendContextGeneration; if (samplerPassClean) { for (Uint i = 0; i < samplerPassMemo.count; ++i) { if (SamplerImpl::g_boundSamplersCache[samplerPassMemo.units[i]] != @@ -2843,7 +2843,7 @@ namespace MobileGL::MG_Backend::DirectGLES { samplerPassMemo.unitBindingsEpoch = keys.unitBindingsEpoch; samplerPassMemo.samplingGeneration = keys.samplingGeneration; samplerPassMemo.backendStateVersion = programBackendStateVersion; - samplerPassMemo.textureContextGeneration = TextureImpl::g_textureContextGeneration; + samplerPassMemo.textureContextGeneration = g_backendContextGeneration; samplerPassMemo.valid = true; } } @@ -3605,12 +3605,12 @@ namespace MobileGL::MG_Backend::DirectGLES { return false; } - if (s_resolveContextGeneration != TextureImpl::g_textureContextGeneration) { + if (s_resolveContextGeneration != g_backendContextGeneration) { // The ids belonged to a dead context; the context reclaimed them with it. s_resolveFramebuffer = 0; s_resolveRenderbuffer = 0; s_resolveFormat = 0; - s_resolveContextGeneration = TextureImpl::g_textureContextGeneration; + s_resolveContextGeneration = g_backendContextGeneration; } if (s_resolveFramebuffer == 0) { g_GLESFuncs.glGenFramebuffers(1, &s_resolveFramebuffer); @@ -3758,7 +3758,7 @@ namespace MobileGL::MG_Backend::DirectGLES { } static Bool EnsureResources() { - if (s_contextGeneration != TextureImpl::g_textureContextGeneration) { + if (s_contextGeneration != g_backendContextGeneration) { // The ids belonged to a dead context; the context reclaimed them with it. s_framebuffer = 0; s_texture = 0; @@ -3769,7 +3769,7 @@ namespace MobileGL::MG_Backend::DirectGLES { s_depthProgram = 0; s_stencilProgram = 0; s_programsFailed = false; - s_contextGeneration = TextureImpl::g_textureContextGeneration; + s_contextGeneration = g_backendContextGeneration; } if (s_programsFailed) { return false; @@ -7482,7 +7482,7 @@ namespace MobileGL::MG_Backend::DirectGLES { PixelStoreImpl::InvalidatePackStateCache(); // Texture ids belong to the dying context; wrappers destroyed later must // not glDeleteTextures a recycled name in a successor context. - ++TextureImpl::g_textureContextGeneration; + ++g_backendContextGeneration; g_backendContextOwnerThread.store(std::thread::id{}, std::memory_order_release); // Outstanding fence handles now refer to a dead context; treat them as // signaled from here on. diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index c1bfc92a..6d4c94db 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -33,6 +33,8 @@ #include namespace MobileGL::MG_Backend::DirectGLES { + Uint g_backendContextGeneration = 1; + constexpr Bool PREFER_MAP_BUFFER_RANGE_FOR_BUFFER_SYNC = false; constexpr const char* BASE_INSTANCE_UNIFORM_NAME = "mg_BaseInstance"; constexpr const char* DRAW_ID_UNIFORM_NAME = "mg_DrawID"; @@ -1646,7 +1648,7 @@ namespace MobileGL::MG_Backend::DirectGLES { ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif g_GLESFuncs.glGenTextures(1, &m_backendTextureId); - m_contextGeneration = g_textureContextGeneration; + m_contextGeneration = g_backendContextGeneration; if (m_backendTextureId == 0) { MGLOG_E("Failed to generate texture object."); MGLOG_E("ES glGetError(): %s", MG_Util::ConvertGLEnumToString(g_GLESFuncs.glGetError()).c_str()); @@ -1673,7 +1675,7 @@ namespace MobileGL::MG_Backend::DirectGLES { } } } - if (m_contextGeneration == g_textureContextGeneration && g_GLESFuncs.glDeleteTextures) { + if (m_contextGeneration == g_backendContextGeneration && g_GLESFuncs.glDeleteTextures) { g_GLESFuncs.glDeleteTextures(1, &m_backendTextureId); } m_backendTextureId = 0; @@ -1712,7 +1714,7 @@ namespace MobileGL::MG_Backend::DirectGLES { void BackendTextureObject::RecreateBackendTexture() { if (m_backendTextureId != 0) { ScratchFBOImpl::NoteTextureIdDeleted(m_backendTextureId); - if (m_contextGeneration == g_textureContextGeneration) { + if (m_contextGeneration == g_backendContextGeneration) { g_GLESFuncs.glDeleteTextures(1, &m_backendTextureId); } for (auto& unitCache : g_boundTexturesCache) { @@ -1725,7 +1727,7 @@ namespace MobileGL::MG_Backend::DirectGLES { } g_GLESFuncs.glGenTextures(1, &m_backendTextureId); - m_contextGeneration = g_textureContextGeneration; + m_contextGeneration = g_backendContextGeneration; if (m_backendTextureId == 0) { MGLOG_E("Failed to regenerate texture object."); MGLOG_E("ES glGetError(): %s", MG_Util::ConvertGLEnumToString(g_GLESFuncs.glGetError()).c_str()); @@ -3081,7 +3083,6 @@ namespace MobileGL::MG_Backend::DirectGLES { } Uint g_activeTextureUnit = 0; - Uint g_textureContextGeneration = 1; Array, MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS> g_boundTexturesCache; @@ -3100,6 +3101,7 @@ namespace MobileGL::MG_Backend::DirectGLES { m_backendColorSlots[i] = GL_COLOR_ATTACHMENT0 + i; } g_GLESFuncs.glGenFramebuffers(1, &m_backendFBOId); + m_contextGeneration = g_backendContextGeneration; if (m_backendFBOId == 0) { MGLOG_E("Failed to generate framebuffer object."); MGLOG_E("ES glGetError(): %s", MG_Util::ConvertGLEnumToString(g_GLESFuncs.glGetError()).c_str()); @@ -3108,6 +3110,22 @@ namespace MobileGL::MG_Backend::DirectGLES { } } + BackendFramebufferObject::~BackendFramebufferObject() { + if (InProcessTeardown()) { + return; // see InProcessTeardown(): the driver may be unloaded already + } + if (m_backendFBOId == 0) { + return; + } + // Scrub the binding shadow whether or not the id can still be deleted: a + // recycled name must never satisfy the shadow's dedup. + NoteFramebufferIdDeleted(m_backendFBOId); + if (m_contextGeneration == g_backendContextGeneration && g_GLESFuncs.glDeleteFramebuffers) { + g_GLESFuncs.glDeleteFramebuffers(1, &m_backendFBOId); + } + m_backendFBOId = 0; + } + void BackendFramebufferObject::Bind(FramebufferTarget target) const { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); @@ -3163,6 +3181,17 @@ namespace MobileGL::MG_Backend::DirectGLES { return g_driverFBOBindings[idx]; } + void NoteFramebufferIdDeleted(Uint id) { + if (id == 0) { + return; + } + for (SizeT idx = 0; idx < g_driverFBOBindings.size(); ++idx) { + if (g_driverFBOBindingKnown[idx] && g_driverFBOBindings[idx] == id) { + g_driverFBOBindings[idx] = 0; // glDeleteFramebuffers reverts a bound FBO to 0 + } + } + } + void InvalidateFramebufferBindingCache() { g_driverFBOBindings = {0, 0}; g_driverFBOBindingKnown = {false, false}; @@ -4570,6 +4599,7 @@ namespace MobileGL::MG_Backend::DirectGLES { ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif g_GLESFuncs.glGenSamplers(1, &m_backendSamplerId); + m_contextGeneration = g_backendContextGeneration; if (m_backendSamplerId == 0) { MGLOG_E("Failed to generate sampler object."); MGLOG_E("ES glGetError(): %s", MG_Util::ConvertGLEnumToString(g_GLESFuncs.glGetError()).c_str()); @@ -4578,6 +4608,26 @@ namespace MobileGL::MG_Backend::DirectGLES { } } + BackendSamplerObject::~BackendSamplerObject() { + if (InProcessTeardown()) { + return; // see InProcessTeardown(): the driver may be unloaded already + } + if (m_backendSamplerId == 0) { + return; + } + // Scrub the unit shadow whether or not the id can still be deleted - the next + // twin can land on this heap address and would otherwise false-skip its Bind. + for (auto& boundSampler : g_boundSamplersCache) { + if (boundSampler == this) { + boundSampler = nullptr; // glDeleteSamplers unbinds from every unit + } + } + if (m_contextGeneration == g_backendContextGeneration && g_GLESFuncs.glDeleteSamplers) { + g_GLESFuncs.glDeleteSamplers(1, &m_backendSamplerId); + } + m_backendSamplerId = 0; + } + void BackendSamplerObject::SyncToBackend( const SharedPtr& stateSamplerObject) { #ifdef TRACY_ENABLE @@ -4693,12 +4743,28 @@ namespace MobileGL::MG_Backend::DirectGLES { ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif g_GLESFuncs.glGenRenderbuffers(1, &m_backendRBOId); + m_contextGeneration = g_backendContextGeneration; if (m_backendRBOId == 0) { MGLOG_E("Failed to generate renderbuffer object."); MGLOG_E("ES glGetError(): %s", MG_Util::ConvertGLEnumToString(g_GLESFuncs.glGetError()).c_str()); } } + BackendRenderbufferObject::~BackendRenderbufferObject() { + if (InProcessTeardown()) { + return; // see InProcessTeardown(): the driver may be unloaded already + } + if (m_backendRBOId == 0) { + return; + } + // No driver-level renderbuffer-binding shadow exists (Bind() always issues the + // call), so there is nothing to scrub here - only the id to release. + if (m_contextGeneration == g_backendContextGeneration && g_GLESFuncs.glDeleteRenderbuffers) { + g_GLESFuncs.glDeleteRenderbuffers(1, &m_backendRBOId); + } + m_backendRBOId = 0; + } + void BackendRenderbufferObject::Bind() const { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 721c7687..ddaa6aed 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -36,6 +36,14 @@ namespace MobileGL::MG_Backend::DirectGLES { Bool InProcessTeardown(); void EnsureProcessTeardownSentinel(); + // Generation of the backend ES context that owns the driver ids currently handed + // out. Bumped exactly once per DestroyEGLContext. Every backend twin that owns a + // driver name (texture, framebuffer, renderbuffer, sampler) stamps this at + // construction and compares it in its destructor: a twin outliving its context + // must NOT glDelete* its id, because a successor context may already have recycled + // that name and the delete would take out a live object of the new context. + extern Uint g_backendContextGeneration; + // Which optional pieces of state a draw needs synchronized before it is issued. // Index/indirect buffer syncs and the instancing-related work are skipped for // draws that provably cannot read them. @@ -657,15 +665,20 @@ namespace MobileGL::MG_Backend::DirectGLES { MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS> g_boundTexturesCache; extern Uint g_activeTextureUnit; - // Bumped when the backend ES context is destroyed; texture ids stamped with - // an older generation belong to a dead context and must not be deleted. - extern Uint g_textureContextGeneration; } // namespace TextureImpl namespace FramebufferImpl { class BackendFramebufferObject { public: BackendFramebufferObject(); + // Deletes the driver framebuffer and scrubs the binding shadow. Without it every + // frontend glDeleteFramebuffers leaked one ES framebuffer for the process lifetime; + // an app that creates a framebuffer per readback (GL CTS packed_pixels does ~3300 + // per case) walked the driver into hundreds of megabytes of dead framebuffers and + // out of the resources a later attachment needs. + ~BackendFramebufferObject(); + BackendFramebufferObject(const BackendFramebufferObject&) = delete; + BackendFramebufferObject& operator=(const BackendFramebufferObject&) = delete; void SyncToBackend(const SharedPtr& stateFBOObject, FramebufferTarget asTarget); // Apply only this FBO's read buffer (glReadBuffer) to the backend. Split out so it can @@ -680,6 +693,7 @@ namespace MobileGL::MG_Backend::DirectGLES { private: Uint m_backendFBOId = 0; + Uint m_contextGeneration = 0; /* this will save buffers in its original form, reversion, absence or not consecutive are all allowed, as long as GL spec allows it @@ -821,6 +835,10 @@ namespace MobileGL::MG_Backend::DirectGLES { void BindFramebufferId(GLenum fbTarget, Uint id); Uint CurrentFramebufferBinding(FramebufferTarget target); void InvalidateFramebufferBindingCache(); + // A driver framebuffer id is about to be deleted: ES reverts every target that + // currently binds it to 0, so the binding shadow has to follow or the next + // BindFramebufferId(0) would be deduped away and leave the deleted name bound. + void NoteFramebufferIdDeleted(Uint id); } // namespace FramebufferImpl // Shared scratch framebuffers for the readback/copy/blit emulation paths, with a @@ -1087,12 +1105,19 @@ namespace MobileGL::MG_Backend::DirectGLES { class BackendSamplerObject { public: BackendSamplerObject(); + // Deletes the driver sampler and clears the units whose binding shadow still names + // this twin (a recycled heap address would otherwise false-skip a later Bind). + // Frontend glDeleteSamplers used to leak the backend id for the process lifetime. + ~BackendSamplerObject(); + BackendSamplerObject(const BackendSamplerObject&) = delete; + BackendSamplerObject& operator=(const BackendSamplerObject&) = delete; void SyncToBackend(const SharedPtr& stateSamplerObject); void Bind(Uint unit); Uint GetBackendSamplerId() const; private: Uint m_backendSamplerId = 0; + Uint m_contextGeneration = 0; Bool m_isInitialized = false; SamplerParameters m_cacheSamplerParameters; Uint16 m_syncedSamplerVersion = 0; @@ -1110,12 +1135,18 @@ namespace MobileGL::MG_Backend::DirectGLES { class BackendRenderbufferObject { public: BackendRenderbufferObject(); + // Deletes the driver renderbuffer; frontend glDeleteRenderbuffers used to leak it + // (with its whole image allocation) for the process lifetime. + ~BackendRenderbufferObject(); + BackendRenderbufferObject(const BackendRenderbufferObject&) = delete; + BackendRenderbufferObject& operator=(const BackendRenderbufferObject&) = delete; void SyncToBackend(const SharedPtr& stateRBOObject); Uint GetBackendRenderbufferId() const { return m_backendRBOId; } void Bind() const; private: Uint m_backendRBOId = 0; + Uint m_contextGeneration = 0; Bool m_isInitialized = false; TextureInternalFormat m_cacheInternalFormat = TextureInternalFormat::Unknown; Int m_cacheWidth = 0; diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 371d0ad7..b2ca128f 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -55,6 +55,7 @@ add_executable(MobileGLIntegrationTest Scenarios/ThreeChannelAttachmentScenario.cpp Scenarios/PipelineFailureScenario.cpp Scenarios/AdvertisedLimitsScenario.cpp + Scenarios/PixelStoreSweepScenario.cpp ) target_include_directories(MobileGLIntegrationTest PRIVATE diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp new file mode 100644 index 00000000..efc850fa --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp @@ -0,0 +1,249 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - PIXEL-STORE MODES RESTORE, and FRAMEBUFFER CHURN STAYS EXACT. +// +// Both cases here replay the shape of KHR-GL3x.packed_pixels.varied_rectangle, the single +// heaviest polluter in the GL CTS: for each of 46 (pixel-store mode, value) pairs it uploads a +// gradient into a fresh texture, attaches that texture to a FRESH framebuffer, reads it back and +// deletes both - ~3300 texture+framebuffer pairs per test case. +// +// What that found: DirectGLES had no destructor for BackendFramebufferObject (nor for the +// renderbuffer and sampler twins), so every frontend glDeleteFramebuffers leaked one driver +// framebuffer for the process lifetime. On an Adreno 830 the CTS run walked the driver to 1.2 GB +// of dead objects, and from that point on EVERY readback through a freshly attached framebuffer +// came back with someone else's pixels - which is what made ~1,500 otherwise-correct cases fail +// depending only on how much ran before them. The unit-level pin for the missing destructors is +// MG_Test/SanityTest.cpp (DirectGLESBackendFramebuffer/Renderbuffer/Sampler); this file pins the +// end-to-end behaviour they protect. +// +// The mode sweep is the second half of the same story: 46 modes are set and reset per case, so a +// mode that fails to restore is indistinguishable from the leak in a full-batch CTS run. The +// assertion here is RESTORATION - after every single mode is set and put back, a readback at +// default state must be byte-identical to one taken before the sweep ever started. +// +// Backend-agnostic on purpose: both bugs this guards against are frontend/backend bookkeeping, +// and DirectVulkan is the built-in control. + +#include +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + // Small enough that the table's row lengths (10, 15) and image heights are all >= the + // image, which is the shape the CTS uses (its gradient is 7x3). + constexpr int kTexSize = 8; + // Every buffer handed to GL is this big regardless of the image size: with row length 15, + // two skipped rows/pixels and alignment 8 the driver strides well past the natural image + // extent, and a tight buffer would be an out-of-bounds access rather than a test. (It was: + // the first version of this scenario passed its assertions and then segfaulted at + // teardown, because glReadPixels had written past a 1 KiB destination.) + constexpr std::size_t kScratchBytes = 64 * 1024; + + // Every pixel-store mode GL 4.0 has, so a reset provably covers the whole state and not + // just the subset a particular test happened to touch. + struct PixelStoreMode { + GLenum name; + GLint defaultValue; + }; + const PixelStoreMode kAllModes[] = { + {GL_UNPACK_SWAP_BYTES, 0}, {GL_UNPACK_LSB_FIRST, 0}, {GL_UNPACK_ROW_LENGTH, 0}, + {GL_UNPACK_IMAGE_HEIGHT, 0}, {GL_UNPACK_SKIP_ROWS, 0}, {GL_UNPACK_SKIP_PIXELS, 0}, + {GL_UNPACK_SKIP_IMAGES, 0}, {GL_UNPACK_ALIGNMENT, 4}, {GL_PACK_SWAP_BYTES, 0}, + {GL_PACK_LSB_FIRST, 0}, {GL_PACK_ROW_LENGTH, 0}, {GL_PACK_IMAGE_HEIGHT, 0}, + {GL_PACK_SKIP_ROWS, 0}, {GL_PACK_SKIP_PIXELS, 0}, {GL_PACK_SKIP_IMAGES, 0}, + {GL_PACK_ALIGNMENT, 4}, + }; + + // The CTS table verbatim (glcPackedPixelsTests.cpp VariedRectangleTest::iterate): 32 + // common cases plus the 14 core-only ones ES has no equivalent for and MobileGL therefore + // honours on the CPU. IMAGE_WIDTH_1/2 and IMAGE_HEIGHT_1/2 are the CTS's 10 and 15. + struct SweepCase { + GLenum mode; + GLint value; + }; + const SweepCase kSweep[] = { + {GL_UNPACK_ROW_LENGTH, 0}, {GL_UNPACK_ROW_LENGTH, 10}, {GL_UNPACK_ROW_LENGTH, 15}, + {GL_UNPACK_SKIP_ROWS, 0}, {GL_UNPACK_SKIP_ROWS, 1}, {GL_UNPACK_SKIP_ROWS, 2}, + {GL_UNPACK_SKIP_PIXELS, 0}, {GL_UNPACK_SKIP_PIXELS, 1}, {GL_UNPACK_SKIP_PIXELS, 2}, + {GL_UNPACK_ALIGNMENT, 1}, {GL_UNPACK_ALIGNMENT, 2}, {GL_UNPACK_ALIGNMENT, 4}, + {GL_UNPACK_ALIGNMENT, 8}, {GL_UNPACK_IMAGE_HEIGHT, 0}, {GL_UNPACK_IMAGE_HEIGHT, 10}, + {GL_UNPACK_IMAGE_HEIGHT, 15}, {GL_UNPACK_SKIP_IMAGES, 0}, {GL_UNPACK_SKIP_IMAGES, 1}, + {GL_UNPACK_SKIP_IMAGES, 2}, {GL_PACK_ROW_LENGTH, 0}, {GL_PACK_ROW_LENGTH, 10}, + {GL_PACK_ROW_LENGTH, 15}, {GL_PACK_SKIP_ROWS, 0}, {GL_PACK_SKIP_ROWS, 1}, + {GL_PACK_SKIP_ROWS, 2}, {GL_PACK_SKIP_PIXELS, 0}, {GL_PACK_SKIP_PIXELS, 1}, + {GL_PACK_SKIP_PIXELS, 2}, {GL_PACK_ALIGNMENT, 1}, {GL_PACK_ALIGNMENT, 2}, + {GL_PACK_ALIGNMENT, 4}, {GL_PACK_ALIGNMENT, 8}, + // core-only, no ES equivalent + {GL_UNPACK_SWAP_BYTES, GL_FALSE}, {GL_UNPACK_SWAP_BYTES, GL_TRUE}, + {GL_UNPACK_LSB_FIRST, GL_FALSE}, {GL_UNPACK_LSB_FIRST, GL_TRUE}, + {GL_PACK_SWAP_BYTES, GL_FALSE}, {GL_PACK_SWAP_BYTES, GL_TRUE}, + {GL_PACK_LSB_FIRST, GL_FALSE}, {GL_PACK_LSB_FIRST, GL_TRUE}, + {GL_PACK_IMAGE_HEIGHT, 0}, {GL_PACK_IMAGE_HEIGHT, 10}, + {GL_PACK_IMAGE_HEIGHT, 15}, {GL_PACK_SKIP_IMAGES, 0}, + {GL_PACK_SKIP_IMAGES, 1}, {GL_PACK_SKIP_IMAGES, 2}, + }; + + std::size_t ImageBytes(int size) { return static_cast(size) * size * 4; } + + // Padded to kScratchBytes so it is safe to hand to an upload running under any of the + // sweep's stride/skip settings. + std::vector MakeGradient(int size, unsigned seed) { + std::vector pixels(kScratchBytes, 0); + for (int y = 0; y < size; ++y) { + for (int x = 0; x < size; ++x) { + const std::size_t base = (static_cast(y) * size + x) * 4; + pixels[base + 0] = static_cast((x * 11 + seed) & 0xFF); + pixels[base + 1] = static_cast((y * 13 + seed) & 0xFF); + pixels[base + 2] = static_cast((x * y + seed) & 0xFF); + pixels[base + 3] = 0xFF; + } + } + return pixels; + } + + void ResetAllPixelStoreModes() { + for (const PixelStoreMode& mode : kAllModes) { + glPixelStorei(mode.name, mode.defaultValue); + } + } + + // The one operation the CTS repeats: a fresh texture, a fresh framebuffer, one readback, + // both deleted. Returns the readback; `outStatus` carries the completeness answer so a + // caller can tell an incomplete framebuffer apart from wrong pixels. + std::vector UploadAndReadBack(const std::vector& source, int size, + GLenum* outStatus) { + GLuint texture = 0; + glGenTextures(1, &texture); + glBindTexture(GL_TEXTURE_2D, texture); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, size, size, 0, GL_RGBA, GL_UNSIGNED_BYTE, source.data()); + + GLuint fbo = 0; + glGenFramebuffers(1, &fbo); + glBindFramebuffer(GL_FRAMEBUFFER, fbo); + glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0); + *outStatus = glCheckFramebufferStatus(GL_FRAMEBUFFER); + + std::vector read(kScratchBytes, 0); + if (*outStatus == GL_FRAMEBUFFER_COMPLETE) { + glReadPixels(0, 0, size, size, GL_RGBA, GL_UNSIGNED_BYTE, read.data()); + } + + glBindFramebuffer(GL_FRAMEBUFFER, 0); + glDeleteFramebuffers(1, &fbo); + glBindTexture(GL_TEXTURE_2D, 0); + glDeleteTextures(1, &texture); + return read; + } + + // Index of the first differing byte within the image, or `bytes` when they agree. + std::size_t FirstDifference(const std::vector& a, const std::vector& b, + std::size_t bytes) { + for (std::size_t i = 0; i < bytes; ++i) { + if (a[i] != b[i]) return i; + } + return bytes; + } + + class PixelStoreSweepScenario : public ScenarioTest {}; + class FramebufferChurnScenario : public ScenarioTest {}; + + } // namespace + + // Every mode in the CTS table is set, exercised and put back; the readback at default state + // afterwards must be bit-identical to the one taken before the sweep. A mode that silently + // fails to restore corrupts every later case in the batch, which is exactly how the CTS + // failures presented (the FIRST sub-case, at default state, is what failed). + TEST_F(PixelStoreSweepScenario, DefaultStateSurvivesTheFullModeSweep) { + if (!Ready()) return; + + ResetAllPixelStoreModes(); + ASSERT_EQ(FirstGLError(), 0u) << "resetting the pixel-store modes must be legal on a GL 4.0 context"; + + const std::vector gradient = MakeGradient(kTexSize, 0); + GLenum status = 0; + const std::vector baseline = UploadAndReadBack(gradient, kTexSize, &status); + ASSERT_EQ(status, static_cast(GL_FRAMEBUFFER_COMPLETE)); + ASSERT_EQ(FirstGLError(), 0u); + + const std::vector scratchSource(kScratchBytes, 0x5A); + + for (const SweepCase& sweep : kSweep) { + glPixelStorei(sweep.mode, sweep.value); + ASSERT_EQ(FirstGLError(), 0u) << "glPixelStorei(0x" << std::hex << sweep.mode << std::dec << ", " + << sweep.value << ") must be accepted"; + + // Exercise the mode: an upload and a readback that both run with it in force. + GLenum sweepStatus = 0; + (void)UploadAndReadBack(scratchSource, kTexSize, &sweepStatus); + + ResetAllPixelStoreModes(); + + GLenum afterStatus = 0; + const std::vector after = UploadAndReadBack(gradient, kTexSize, &afterStatus); + ASSERT_EQ(afterStatus, static_cast(GL_FRAMEBUFFER_COMPLETE)); + const std::size_t diff = FirstDifference(baseline, after, ImageBytes(kTexSize)); + ASSERT_EQ(diff, ImageBytes(kTexSize)) + << "default-state readback changed after setting and resetting 0x" << std::hex << sweep.mode + << std::dec << " = " << sweep.value << "; first differing byte " << diff << " (baseline " + << static_cast(baseline[diff]) << ", now " << static_cast(after[diff]) << ")"; + } + + // And the modes themselves must read back as the defaults the reset asked for. + for (const PixelStoreMode& mode : kAllModes) { + GLint value = -1; + glGetIntegerv(mode.name, &value); + EXPECT_EQ(value, mode.defaultValue) + << "pixel-store mode 0x" << std::hex << mode.name << std::dec << " did not return to its default"; + } + EXPECT_EQ(FirstGLError(), 0u); + } + + // The leak regression. Each iteration is one complete CTS inner step, and every readback has + // to be exactly the gradient THIS iteration uploaded - never the previous one's. Before the + // missing destructors were added, the driver-side framebuffer count grew without bound here. + TEST_F(FramebufferChurnScenario, RepeatedFramebufferReadbackStaysExact) { + if (!Ready()) return; + + ResetAllPixelStoreModes(); + constexpr int kSize = 8; + constexpr int kIterations = 1024; + + for (int i = 0; i < kIterations; ++i) { + // A distinct gradient per iteration: a stale attachment or a recycled driver name + // reads back the PREVIOUS iteration's image, which a constant fill could not tell + // apart from a correct read. + const std::vector gradient = MakeGradient(kSize, static_cast(i * 7 + 1)); + GLenum status = 0; + const std::vector read = UploadAndReadBack(gradient, kSize, &status); + ASSERT_EQ(status, static_cast(GL_FRAMEBUFFER_COMPLETE)) << "iteration " << i; + const std::size_t diff = FirstDifference(gradient, read, ImageBytes(kSize)); + ASSERT_EQ(diff, ImageBytes(kSize)) + << "iteration " << i << " read back a different image than it uploaded; first differing byte " + << diff << " (uploaded " << static_cast(gradient[diff]) << ", read " + << static_cast(read[diff]) << ")"; + ASSERT_EQ(FirstGLError(), 0u) << "iteration " << i; + } + } + +} // namespace MGITest diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index be60fd2e..6a7b9f3f 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -1822,13 +1822,170 @@ TEST(DirectGLESBackendTexture, DestructorDeletesIdAndScrubsBindingCache) { // A wrapper whose context died must NOT delete a foreign (recycled) name. { auto backendTexture = MobileGL::MakeShared(); - ++TextureImpl::g_textureContextGeneration; + ++g_backendContextGeneration; backendTexture.reset(); - --TextureImpl::g_textureContextGeneration; // restore for later tests + --g_backendContextGeneration; // restore for later tests EXPECT_EQ(deleted.size(), 1u); } } +// ---- DirectGLES backend twins release their driver ids -------------------------------------- +// Framebuffers, renderbuffers and samplers had no destructor at all: every frontend object the +// application deleted leaked its ES twin for the whole process lifetime. An application that +// creates a framebuffer per readback (GL CTS packed_pixels.varied_rectangle makes ~3300 of them +// per case) walked the driver into a gigabyte of dead framebuffers, and past that point every +// readback through a freshly attached framebuffer came back with stale pixels. +namespace { + struct TwinDeletionSinks { + MobileGL::Vector framebuffers; + MobileGL::Vector renderbuffers; + MobileGL::Vector samplers; + }; + + TwinDeletionSinks* g_twinDeletionSinks = nullptr; + GLuint g_nextTwinDriverId = 900; + + void TW_GenFramebuffers(GLsizei count, GLuint* ids) { + for (GLsizei i = 0; i < count; ++i) ids[i] = g_nextTwinDriverId++; + } + void TW_DeleteFramebuffers(GLsizei count, const GLuint* ids) { + if (!g_twinDeletionSinks) return; + for (GLsizei i = 0; i < count; ++i) g_twinDeletionSinks->framebuffers.push_back(ids[i]); + } + void TW_GenRenderbuffers(GLsizei count, GLuint* ids) { + for (GLsizei i = 0; i < count; ++i) ids[i] = g_nextTwinDriverId++; + } + void TW_DeleteRenderbuffers(GLsizei count, const GLuint* ids) { + if (!g_twinDeletionSinks) return; + for (GLsizei i = 0; i < count; ++i) g_twinDeletionSinks->renderbuffers.push_back(ids[i]); + } + void TW_GenSamplers(GLsizei count, GLuint* ids) { + for (GLsizei i = 0; i < count; ++i) ids[i] = g_nextTwinDriverId++; + } + void TW_DeleteSamplers(GLsizei count, const GLuint* ids) { + if (!g_twinDeletionSinks) return; + for (GLsizei i = 0; i < count; ++i) g_twinDeletionSinks->samplers.push_back(ids[i]); + } + void TW_BindFramebuffer(GLenum target, GLuint framebuffer) { + SG_Log("BindFramebuffer:" + std::to_string(target) + ":" + std::to_string(framebuffer)); + } + void TW_BindSampler(GLuint, GLuint) {} + void TW_BindRenderbuffer(GLenum, GLuint) {} + + // Installs a table that can create and destroy all three twin kinds, and unwinds it (plus the + // recording pointer) even when an assertion aborts the test body. + struct ScopedBackendTwinMocks { + ScopedBackendTwinMocks(): previousFunctions(MobileGL::MG_Backend::DirectGLES::g_GLESFuncs) { + MobileGL::MG_Backend::DirectGLES::FramebufferImpl::InvalidateFramebufferBindingCache(); + MobileGL::MG_External::GLESFunctionsTable functions{}; + functions.glGenFramebuffers = TW_GenFramebuffers; + functions.glDeleteFramebuffers = TW_DeleteFramebuffers; + functions.glBindFramebuffer = TW_BindFramebuffer; + functions.glGenRenderbuffers = TW_GenRenderbuffers; + functions.glDeleteRenderbuffers = TW_DeleteRenderbuffers; + functions.glBindRenderbuffer = TW_BindRenderbuffer; + functions.glGenSamplers = TW_GenSamplers; + functions.glDeleteSamplers = TW_DeleteSamplers; + functions.glBindSampler = TW_BindSampler; + functions.glGetError = SG_NoError; + MobileGL::MG_Backend::DirectGLES::SetGLESFuncsTable(functions); + g_twinDeletionSinks = &sinks; + g_stateGuardLog = &log; + } + + ~ScopedBackendTwinMocks() { + g_stateGuardLog = nullptr; + g_twinDeletionSinks = nullptr; + MobileGL::MG_Backend::DirectGLES::SetGLESFuncsTable(previousFunctions); + MobileGL::MG_Backend::DirectGLES::FramebufferImpl::InvalidateFramebufferBindingCache(); + } + + ScopedBackendTwinMocks(const ScopedBackendTwinMocks&) = delete; + ScopedBackendTwinMocks& operator=(const ScopedBackendTwinMocks&) = delete; + + TwinDeletionSinks sinks; + StateGuardCallLog log; + MobileGL::MG_External::GLESFunctionsTable previousFunctions; + }; +} // namespace + +TEST(DirectGLESBackendFramebuffer, DestructorDeletesIdAndScrubsBindingShadow) { + using namespace MobileGL::MG_Backend::DirectGLES; + ScopedBackendTwinMocks mocks; + + GLuint id = 0; + { + auto backendFBO = MobileGL::MakeShared(); + id = backendFBO->GetBackendFramebufferId(); + ASSERT_NE(id, 0u); + backendFBO->Bind(MobileGL::FramebufferTarget::Draw); + ASSERT_EQ(FramebufferImpl::CurrentFramebufferBinding(MobileGL::FramebufferTarget::Draw), id); + } + ASSERT_EQ(mocks.sinks.framebuffers.size(), 1u); + EXPECT_EQ(mocks.sinks.framebuffers[0], id); + // ES reverts every target bound to a deleted framebuffer to 0. The shadow has to follow, or + // the next BindFramebufferId(0) is deduped away and the driver keeps the dead name bound. + EXPECT_EQ(FramebufferImpl::CurrentFramebufferBinding(MobileGL::FramebufferTarget::Draw), 0u); + + // A twin whose context died must NOT delete a name a successor context may have recycled. + { + auto backendFBO = MobileGL::MakeShared(); + ++g_backendContextGeneration; + backendFBO.reset(); + --g_backendContextGeneration; // restore for later tests + EXPECT_EQ(mocks.sinks.framebuffers.size(), 1u); + } +} + +TEST(DirectGLESBackendRenderbuffer, DestructorDeletesId) { + using namespace MobileGL::MG_Backend::DirectGLES; + ScopedBackendTwinMocks mocks; + + GLuint id = 0; + { + auto backendRBO = MobileGL::MakeShared(); + id = backendRBO->GetBackendRenderbufferId(); + ASSERT_NE(id, 0u); + } + ASSERT_EQ(mocks.sinks.renderbuffers.size(), 1u); + EXPECT_EQ(mocks.sinks.renderbuffers[0], id); + + { + auto backendRBO = MobileGL::MakeShared(); + ++g_backendContextGeneration; + backendRBO.reset(); + --g_backendContextGeneration; + EXPECT_EQ(mocks.sinks.renderbuffers.size(), 1u); + } +} + +TEST(DirectGLESBackendSampler, DestructorDeletesIdAndScrubsUnitCache) { + using namespace MobileGL::MG_Backend::DirectGLES; + ScopedBackendTwinMocks mocks; + + GLuint id = 0; + { + auto backendSampler = MobileGL::MakeShared(); + id = backendSampler->GetBackendSamplerId(); + ASSERT_NE(id, 0u); + backendSampler->Bind(3); + ASSERT_EQ(SamplerImpl::g_boundSamplersCache[3], backendSampler.get()); + } + ASSERT_EQ(mocks.sinks.samplers.size(), 1u); + EXPECT_EQ(mocks.sinks.samplers[0], id); + // glDeleteSamplers unbinds from every unit, and the next twin can land on this heap + // address - a stale row would false-skip its Bind. + EXPECT_EQ(SamplerImpl::g_boundSamplersCache[3], nullptr); + + { + auto backendSampler = MobileGL::MakeShared(); + ++g_backendContextGeneration; + backendSampler.reset(); + --g_backendContextGeneration; + EXPECT_EQ(mocks.sinks.samplers.size(), 1u); + } +} + TEST(DirectGLESStateGuards, DefaultFramebufferBindGoesThroughShadow) { using namespace MobileGL::MG_Backend::DirectGLES; ScopedStateGuardMocks mocks;