diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index e365603d..289d2c98 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -3267,8 +3267,9 @@ void main() { } m_vertexInputStateFactory.reset(); m_xfbCounterBuffer.Destroy(); - m_xfbCounterSlotByObject.clear(); - m_xfbNextCounterSlot = 0; + m_xfbCounterSlotOwner.fill(0); + m_xfbCounterSlotLastUse.fill(0); + m_xfbCounterSlotUseSerial = 0; m_xfbCountersValid.fill(false); m_xfbLastSeenGeneration.fill(0); if (m_occlusionQueryPool != VK_NULL_HANDLE) { @@ -11199,16 +11200,66 @@ void main() { } } + // Keyed on the frontend's never-reused lifetime id, NOT on the GL name. The name is + // recycled the moment glDeleteTransformFeedbacks gives it back, so a name-keyed slot + // handed a brand-new object the counter group - and the m_xfbCountersValid / + // m_xfbLastSeenGeneration entries - of the object that died under that name. + // + // Slots are never handed back (there is no backend entry telling this renderer that a span + // closed - registering the EndTransformFeedback one would flip the "captures through its own + // driver" test FixupGsStripCaptureOrder makes of it), so once all sixteen are owned a new + // object has to take one over. The victim is chosen among owners with NO OPEN SPAN: an object + // whose span is closed, or which no longer exists at all, can never resume, so its counter + // bytes are dead. Least-recently-used ALONE would be exactly the wrong rule - GL only permits + // another object to capture while this one is PAUSED, so the paused span whose counters the + // slots exist to protect is by construction the least recently used entry. Taking a group over + // resets its counter state, because those bytes describe the previous owner's span. Uint32 VulkanRenderer::CurrentXfbCounterSlot() { - const Uint name = MG_State::pGLContext->GetBoundTransformFeedbackName(); - const auto it = m_xfbCounterSlotByObject.find(name); - if (it != m_xfbCounterSlotByObject.end()) { - return it->second; + constexpr Uint32 kNoSlot = static_cast(kXfbCounterObjectSlots); + const Uint64 identity = MG_State::pGLContext->GetBoundTransformFeedbackLifetimeId(); + MOBILEGL_ASSERT(identity != 0, + "transform feedback object reported the free-slot sentinel (0) as its identity - " + "every slot would then read as 'mine' without ever being claimed"); + Uint32 freeSlot = kNoSlot; + for (Uint32 slot = 0; slot < kNoSlot; ++slot) { + if (m_xfbCounterSlotOwner[slot] == identity) { + m_xfbCounterSlotLastUse[slot] = ++m_xfbCounterSlotUseSerial; + return slot; + } + if (m_xfbCounterSlotOwner[slot] == 0 && freeSlot == kNoSlot) { + freeSlot = slot; + } } - // Past the tracked set every object shares slot group 0. Only concurrently-paused - // spans need distinct groups, and applications do not keep sixteen of those open. - const Uint32 slot = m_xfbNextCounterSlot < kXfbCounterObjectSlots ? m_xfbNextCounterSlot++ : 0; - m_xfbCounterSlotByObject[name] = slot; + Uint32 slot = freeSlot; + if (slot == kNoSlot) { + for (Uint32 candidate = 0; candidate < kNoSlot; ++candidate) { + if (MG_State::pGLContext->HasOpenTransformFeedbackSpan(m_xfbCounterSlotOwner[candidate])) { + continue; + } + if (slot == kNoSlot || m_xfbCounterSlotLastUse[candidate] < m_xfbCounterSlotLastUse[slot]) { + slot = candidate; + } + } + } + if (slot == kNoSlot) { + // Sixteen capture spans open at once. Whatever is taken loses its resume offset and + // restarts at byte 0 of its capture buffers, which is a wrong picture rather than a + // slow one - hence a report rather than a silent choice. + MGLOG_E_ONCE("CurrentXfbCounterSlot: all %zu counter groups belong to transform feedback objects " + "with an open capture span; the least recently used one is taken over and that span " + "will restart at offset 0 instead of appending", + kXfbCounterObjectSlots); + slot = 0; + for (Uint32 candidate = 1; candidate < kNoSlot; ++candidate) { + if (m_xfbCounterSlotLastUse[candidate] < m_xfbCounterSlotLastUse[slot]) { + slot = candidate; + } + } + } + m_xfbCounterSlotOwner[slot] = identity; + m_xfbCounterSlotLastUse[slot] = ++m_xfbCounterSlotUseSerial; + m_xfbCountersValid[slot] = false; + m_xfbLastSeenGeneration[slot] = 0; return slot; } diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h index c3d30855..f8498a3a 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h @@ -675,8 +675,18 @@ namespace MobileGL::MG_Backend::DirectVulkan { // per object: one group of four slots each, handed out on first use. static constexpr SizeT kXfbCounterObjectSlots = 16; VkBufferObject m_xfbCounterBuffer; - UnorderedMap m_xfbCounterSlotByObject; - Uint32 m_xfbNextCounterSlot = 0; + // Which transform feedback object owns each slot group, by the frontend's never-reused + // lifetime id (0 = the slot is free). This used to be an UnorderedMap keyed on the GL + // NAME, which is recycled by glGenTransformFeedbacks: a deleted-and-recreated object + // inherited the dead one's slot, and since nothing ever removed an entry the map also + // grew for the life of the context. A fixed table cannot do either: a group is taken over + // only from an owner with no OPEN span (see CurrentXfbCounterSlot), so an object whose + // counters can still be resumed never loses them, and a dead object's group comes back. + Array m_xfbCounterSlotOwner{}; + // Tie-break among reclaimable groups only; never on its own, because the paused span the + // groups exist for is by construction the least recently used one. + Array m_xfbCounterSlotLastUse{}; + Uint64 m_xfbCounterSlotUseSerial = 0; // Set for a slot once a captured draw has been recorded into its span; selects // counter-buffer resume on the next captured draw of the same span. Array m_xfbCountersValid{}; diff --git a/MobileGL/MG_State/GLState/Core.cpp b/MobileGL/MG_State/GLState/Core.cpp index fd4207da..952e1da8 100644 --- a/MobileGL/MG_State/GLState/Core.cpp +++ b/MobileGL/MG_State/GLState/Core.cpp @@ -14,6 +14,8 @@ #include #include +#include + namespace MobileGL::MG_State { void Init() { MGLOG_D("Initializing MobileGL State..."); @@ -1259,6 +1261,33 @@ namespace MobileGL::MG_State { return m_renderbufferState.ValidateRenderbufferObject(index); } + Uint64 GLContext::AllocateTransformFeedbackLifetimeId() { + // Starts at 1 so a zero-initialised backend slot can never carry a live object's id. + static std::atomic nextId{1}; + return nextId.fetch_add(1, std::memory_order_relaxed); + } + + GLContext::GLContext() { + // The default transform feedback object (name 0) exists from the start of the context + // (GL 4.6 core 13.2.1), but nothing binds it, so nothing else would materialise it. + // Materialising it here is what lets GetBoundTransformFeedbackLifetimeId() be a plain + // const read instead of an operator[] insert on the draw path. + m_boundTransformFeedbackLifetimeId = m_transformFeedbackObjects[0].lifetimeId; + } + + Bool GLContext::HasOpenTransformFeedbackSpan(Uint64 lifetimeId) const { + if (lifetimeId == 0) return false; + for (const auto& [name, object] : m_transformFeedbackObjects) { + if (object.lifetimeId != lifetimeId) continue; + // The bound object's span state is live in the context; its saved copy is only + // written when a bind swaps it out. + return name == m_boundTransformFeedback ? m_transformFeedbackActive : object.active; + } + // No object carries this identity any more: it was deleted, and a deleted object can + // never resume. + return false; + } + void GLContext::SaveBoundTransformFeedbackState() { auto& object = m_transformFeedbackObjects[m_boundTransformFeedback]; for (Uint i = 0; i < MAX_TRANSFORM_FEEDBACK_BUFFERS; ++i) { @@ -1295,6 +1324,9 @@ namespace MobileGL::MG_State { m_transformFeedbackGeneration = object.generation; m_transformFeedbackCapturedVertices = object.capturedVertices; m_transformFeedbackInputPrimitives = object.inputPrimitives; + // Every route that changes which object is bound - BindTransformFeedbackObject and the + // revert a delete of the bound object performs - comes through here. + m_boundTransformFeedbackLifetimeId = object.lifetimeId; } void GLContext::GenTransformFeedbackNames(Uint number, Vector& ids) { diff --git a/MobileGL/MG_State/GLState/Core.h b/MobileGL/MG_State/GLState/Core.h index 73a8f799..3b7e4802 100644 --- a/MobileGL/MG_State/GLState/Core.h +++ b/MobileGL/MG_State/GLState/Core.h @@ -60,7 +60,7 @@ namespace MobileGL { class GLContext { public: - GLContext() = default; + GLContext(); // Error void RecordError(ErrorCode code, UniquePtr info); @@ -427,6 +427,27 @@ namespace MobileGL { void BindTransformFeedbackObject(Uint index); void MarkTransformFeedbackObjectForDeletion(Uint index); Uint GetBoundTransformFeedbackName() const { return m_boundTransformFeedback; } + // The bound object's never-reused identity, for a backend that keys a per-object + // resource on it. The NAME is not an identity: glGenTransformFeedbacks recycles a + // deleted one (LIFO), so a memo keyed on the name hands a brand-new object the dead + // one's slot. Cached rather than looked up on demand: the backend asks twice per + // captured draw, and an operator[] on m_transformFeedbackObjects would be an + // INSERT on the draw path - ska::flat_hash_map invalidates every reference into + // itself when it rehashes. The cache is refreshed by + // RestoreBoundTransformFeedbackState, which every bind (and the revert a delete + // performs) goes through, and seeded for the default object by the constructor. + // Never returns 0 - the counter starts at 1 so a zero-initialised memo slot cannot + // be mistaken for a live object. + Uint64 GetBoundTransformFeedbackLifetimeId() const { return m_boundTransformFeedbackLifetimeId; } + // Whether the object carrying this identity still has an OPEN capture span - one + // that glBeginTransformFeedback started and glEndTransformFeedback has not closed, + // paused or not. A backend that hands out a bounded set of per-object slots must + // never take one of these over: a paused span's counters are precisely what its + // resume reads, and GL only lets other objects capture WHILE it is paused, so the + // paused object is also the one that looks idle. An identity no live object + // carries any more (its object was deleted) answers false, which is what makes + // such a slot reclaimable. + Bool HasOpenTransformFeedbackSpan(Uint64 lifetimeId) const; // Vertices the object captured in its last completed span; the vertex count // glDrawTransformFeedback replays. Uint64 GetTransformFeedbackRecordedVertices(Uint index) const; @@ -514,6 +535,9 @@ namespace MobileGL { GLuint m_conditionalRenderQuery = 0; GLenum m_conditionalRenderMode = GL_NONE; + // Process-wide, never-reused. See GetBoundTransformFeedbackLifetimeId(); same + // contract as BufferObject::AllocateLifetimeId(). + static Uint64 AllocateTransformFeedbackLifetimeId(); // Everything a transform feedback object owns while it is NOT the bound one. struct TransformFeedbackObjectState { struct SavedBufferBinding { @@ -532,6 +556,10 @@ namespace MobileGL { Uint64 recordedVertices = 0; Bool hasCompletedSpan = false; Bool everBound = false; + // Assigned by the default member initialiser, so every way an object comes into + // being - operator[] materialisation, `= {}` in Gen/Create - gets a fresh one, + // and a recycled NAME never brings the dead object's id back with it. + Uint64 lifetimeId = AllocateTransformFeedbackLifetimeId(); }; void SaveBoundTransformFeedbackState(); void RestoreBoundTransformFeedbackState(); @@ -540,6 +568,10 @@ namespace MobileGL { UnorderedMap m_transformFeedbackObjects; IndexGenerator m_transformFeedbackNames; Uint m_boundTransformFeedback = 0; + // Mirror of m_transformFeedbackObjects[m_boundTransformFeedback].lifetimeId, so + // the per-draw read is a load rather than a hash lookup that could insert. + // Seeded by the constructor and rewritten by RestoreBoundTransformFeedbackState. + Uint64 m_boundTransformFeedbackLifetimeId = 0; // Map membership is object EXISTENCE, which is not the same as the answer // glIsProgramPipeline gives: any command that needs somewhere to put state // materializes a reserved name, so the object can exist well before it is diff --git a/MobileGL/MG_Test/State/CMakeLists.txt b/MobileGL/MG_Test/State/CMakeLists.txt index c04845c5..52d7cb9c 100644 --- a/MobileGL/MG_Test/State/CMakeLists.txt +++ b/MobileGL/MG_Test/State/CMakeLists.txt @@ -45,6 +45,31 @@ endif() include(GoogleTest) gtest_discover_tests(ObjectLifetimeIdTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS unit) +add_executable( + TransformFeedbackLifetimeIdTest + TransformFeedbackLifetimeIdTest.cpp +) + +target_include_directories(TransformFeedbackLifetimeIdTest PRIVATE + ${MGL_ROOT}/include + ${MGL_ROOT}/MobileGL + ${MGL_ROOT}/3rdparty/xxHash + ${MGL_ROOT}/3rdparty/Vulkan-Headers/include + ${MGL_ROOT}/3rdparty/SPIRV-Reflect +) + +target_link_libraries( + TransformFeedbackLifetimeIdTest PRIVATE + GTest::gtest_main + ${LINK_LIBRARIES} +) + +if (MSVC) + target_compile_options(TransformFeedbackLifetimeIdTest PRIVATE /Zc:preprocessor) +endif() + +gtest_discover_tests(TransformFeedbackLifetimeIdTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS unit) + add_executable( RenderStateTest RenderStateTest.cpp diff --git a/MobileGL/MG_Test/State/TransformFeedbackLifetimeIdTest.cpp b/MobileGL/MG_Test/State/TransformFeedbackLifetimeIdTest.cpp new file mode 100644 index 00000000..1b0274fa --- /dev/null +++ b/MobileGL/MG_Test/State/TransformFeedbackLifetimeIdTest.cpp @@ -0,0 +1,143 @@ +// MobileGL - MobileGL/MG_Test/State/TransformFeedbackLifetimeIdTest.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 +// +// D21 (plan B v2 §4.7.3): DirectVulkan hands every transform feedback object one of sixteen +// counter-buffer groups, and the group carries that span's resume offset. The map was keyed on +// the GL NAME, which glGenTransformFeedbacks recycles the moment the object is deleted, so an +// object created on a recycled name was served the DEAD object's group together with its +// m_xfbCountersValid / m_xfbLastSeenGeneration entries. +// +// The transform feedback object is a plain struct inside a map rather than a heap object, so the +// reuse to defend against is the NAME's, not an address's - which is why these cases live here +// and not in ObjectLifetimeIdTest.cpp with the heap-allocated object types. Keeping them in +// their own translation unit also keeps the D21 commit textually independent of the rest of the +// branch, which plan B §10.4-5 asks for so it can be cherry-picked to dev on its own. +// +// The second case pins the OTHER half of the backend contract: a bounded slot table has to be +// able to tell an object whose span is still open (and may yet resume) from one whose span is +// closed or whose object is gone. + +#include + +#include "Includes.h" +#include "Init.h" + +#include + +using namespace MobileGL; + +namespace { + + MG_State::GLState::GLContext& FreshContext() { + MobileGL::Initialize(); + MG_State::pGLContext = MakeUnique(); + return *MG_State::pGLContext; + } + +} // namespace + +TEST(TransformFeedbackLifetimeIdTest, AnObjectAtARecycledNameCarriesAFreshLifetimeId) { + auto& context = FreshContext(); + + // Before anything is bound. The default object exists from the start of the context, and the + // identity has to exist with it: a backend reading 0 here would match every FREE slot in its + // table without ever claiming one, which is the same bug this id was added to remove. + EXPECT_NE(context.GetBoundTransformFeedbackLifetimeId(), 0u) + << "the default transform feedback object has no identity until something binds it"; + + Vector names; + context.GenTransformFeedbackNames(1, names); + ASSERT_EQ(names.size(), 1u); + const Uint name = names[0]; + ASSERT_NE(name, 0u); + + context.BindTransformFeedbackObject(name); + const Uint64 firstId = context.GetBoundTransformFeedbackLifetimeId(); + EXPECT_NE(firstId, 0u) << "a live transform feedback object answered to id 0, which is the value a " + "zero-initialised backend slot already carries"; + + // The default object is a different object and must not share the id. + context.BindTransformFeedbackObject(0); + EXPECT_NE(context.GetBoundTransformFeedbackLifetimeId(), firstId) + << "the default transform feedback object shares an identity with a generated one"; + + // Deleting while bound reverts to the default object (GL 4.6 core 13.2.1), which is the + // shape the backend sees; delete from there anyway so the test does not depend on it. + context.BindTransformFeedbackObject(name); + context.MarkTransformFeedbackObjectForDeletion(name); + + Vector reborn; + context.GenTransformFeedbackNames(1, reborn); + ASSERT_EQ(reborn.size(), 1u); + if (reborn[0] != name) { + GTEST_SKIP() << "inconclusive, not proven: the name generator did not hand the deleted name back, so " + "the recycled-name case was never exercised"; + } + + context.BindTransformFeedbackObject(reborn[0]); + EXPECT_NE(context.GetBoundTransformFeedbackLifetimeId(), firstId) + << "a transform feedback object created on a recycled name reports the DEAD object's lifetime id - " + "DirectVulkan would hand it the dead span's counter slot, and with it that span's resume state"; +} + +// The predicate DirectVulkan's slot table asks before it takes a group over. The case that +// matters is the middle one: object A is PAUSED and another object is bound and capturing, so A +// looks completely idle to a least-recently-used rule while being exactly the object whose +// counters must survive. +TEST(TransformFeedbackLifetimeIdTest, APausedSpanStaysOpenWhileAnotherObjectCaptures) { + auto& context = FreshContext(); + + Vector names; + context.GenTransformFeedbackNames(2, names); + ASSERT_EQ(names.size(), 2u); + const Uint nameA = names[0]; + const Uint nameB = names[1]; + + context.BindTransformFeedbackObject(nameA); + const Uint64 idA = context.GetBoundTransformFeedbackLifetimeId(); + EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(idA)) << "an object that never began a span reads as open"; + + context.BeginTransformFeedback(GL_POINTS, nullptr); + EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idA)); + + // Pausing is what makes interleaving legal (ARB_transform_feedback2); the span is still open. + context.SetTransformFeedbackPaused(true); + EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idA)); + + // Now the shape the slot table sees: B is bound and capturing, A is paused and untouched. + context.BindTransformFeedbackObject(nameB); + const Uint64 idB = context.GetBoundTransformFeedbackLifetimeId(); + EXPECT_NE(idB, idA); + context.BeginTransformFeedback(GL_POINTS, nullptr); + EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idA)) + << "a paused span stopped reading as open the moment another object was bound - a backend " + "reclaiming slots by 'is this owner still going' would take A's counters away"; + EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idB)); + + context.EndTransformFeedback(); + EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(idB)) << "a closed span still reads as open"; + EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idA)); + + // A closes its own span; its slot becomes reclaimable. + context.BindTransformFeedbackObject(nameA); + context.EndTransformFeedback(); + EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(idA)); + + // A deleted object can never resume, so its identity must not hold a slot either. + context.BindTransformFeedbackObject(nameB); + context.BeginTransformFeedback(GL_POINTS, nullptr); + EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idB)); + context.MarkTransformFeedbackObjectForDeletion(nameB); + EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(idB)) + << "the identity of a deleted transform feedback object still claims an open span, so its counter " + "group would be pinned for the life of the context"; + + // Identities the context never issued, and the free-slot sentinel, are not open spans. + EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(0)); + EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(~0ull)); +}