mirror of
https://github.com/MobileGL-Dev/MobileGL
synced 2026-09-10 21:28:32 +09:00
[Fix] (DirectVulkan, State): key the transform-feedback counter slots on the object's identity, not on its recycled GL name
- VulkanRenderer::CurrentXfbCounterSlot keyed m_xfbCounterSlotByObject on
GetBoundTransformFeedbackName(). glGenTransformFeedbacks hands a deleted name
straight back (IndexGenerator is LIFO) and nothing ever removed a map entry, so
a transform feedback object created on a recycled name was served the DEAD
object's counter group - and with it that group's m_xfbCountersValid and
m_xfbLastSeenGeneration entries, which are the resume/fresh decision for
vkCmdBeginTransformFeedbackEXT. This is D21 in plan B v2 4.7.3, the one entry
in that table whose today-key "guards nothing", and 10.4-5 asks for it to land
on dev on its own - hence this separate commit, kept in files no other commit
on this branch touches so the cherry-pick applies unaided.
- Frontend: TransformFeedbackObjectState gains a never-reused `lifetimeId`
through a default member initialiser, so every route into existence
(operator[] materialisation, `= {}` in GenTransformFeedbackNames and
CreateTransformFeedbackObject) mints a fresh one and a recycled name cannot
carry the dead object's id back. The allocator is the same shape as
BufferObject::AllocateLifetimeId (atomic, starts at 1 so a zeroed backend slot
is never a live object).
- The bound object's id is mirrored in m_boundTransformFeedbackLifetimeId,
refreshed by RestoreBoundTransformFeedbackState - which every bind, and the
revert that deleting the bound object performs, goes through - and seeded for
the default object by the GLContext constructor. GetBoundTransformFeedback
LifetimeId is therefore a const load. Reading it through operator[] instead
would have been an INSERT on the per-draw path, and UnorderedMap is
ska::flat_hash_map, whose rehash invalidates every reference into the
container, not just its iterators.
- Backend: the UnorderedMap is replaced by a fixed 16-entry owner table, which
fixes the second half of the same defect - the map was keyed on a value that
recycles yet was never pruned, so it grew for the life of the context. With
lifetime ids as keys a map would have grown without bound instead, so the
bounded table is required, not cosmetic.
- Slot exhaustion: past sixteen owners a group has to be taken over, and the
victim is chosen among owners with NO OPEN SPAN, which
GLContext::HasOpenTransformFeedbackSpan answers; an identity no live object
carries any more answers false, and that is what lets a dead owner's group
come back. Least-recently-used ALONE would have been exactly the wrong rule:
GL only permits another object to capture while this one is PAUSED, so the
paused span these groups exist to protect is by construction the least
recently used entry, and an LRU takeover would reset the one resume offset
that still matters. LRU is now only the tie-break among reclaimable groups.
Sixteen genuinely open spans at once is reported (MGLOG_E_ONCE) rather than
resolved silently, because whatever is taken then restarts at offset 0.
- Not done, and why: the natural place to hand a group back is
glEndTransformFeedback, but registering DirectVulkan's EndTransformFeedback
table entry would flip the test FixupGsStripCaptureOrder makes of that same
pointer (GL_Drawing.cpp:1255) to decide whether the backend already captured
in GL's vertex order, silently disabling the geometry-stage strip fixup for
DirectVulkan. Giving that discriminator a name of its own is a separate
change; until then the no-open-span rule is what keeps the table honest.
- CurrentXfbCounterSlot asserts the identity is never 0. Zero is the free-slot
sentinel, so an identity of 0 would match every free slot as "mine" without
ever claiming one - this bug reintroduced, with no symptom at the call site.
- TransformFeedbackLifetimeIdTest, in its own translation unit, pins the
frontend halves: an object created on a recycled name must not report the dead
object's id, the default object has an identity before anything binds it, and
a PAUSED span still reads as open while another object is bound and capturing
- which is the whole correctness argument for the eviction rule. The name
reuse is not simulated: the test asks the real generator and skips (loudly) if
it never recycled. Still untested: the >16-owners path itself, which needs a
backend scenario with seventeen capturing objects and there is none.
- Negative controls, each applied then reverted: making a non-bound object's
span read as closed reddens APausedSpanStaysOpenWhileAnotherObjectCaptures;
making a vanished identity read as open reddens the same case on its delete
assertion; dropping the constructor's seeding reddens
AnObjectAtARecycledNameCarriesAFreshLifetimeId.
- Tested: cmake --build build-linux -j 24 (clean, 166 targets); ctest -L unit
-j 12 -> 1386/1386 passed; ctest -L integration-gpu -> 866/866 passed
serially, and 866/866 on one of two -j 8 runs. The other -j 8 run failed
DirectGLES.PointSizeDemotionScenario.TheDemotionIsActuallyArmedWhenTheEnviron
mentPinsItOn, a member of the pre-existing parallel-ctest flake family: it
passes in isolation here, and the unmodified parent tree
(~/w7/p0-noop-wins-base) reproduces the same family under -j 8.
This commit is contained in:
@@ -14,6 +14,8 @@
|
||||
#include <MG_Util/ShaderTranspiler/CompileEnv.h>
|
||||
#include <Config.h>
|
||||
|
||||
#include <atomic>
|
||||
|
||||
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<Uint64> 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<Uint>& ids) {
|
||||
|
||||
@@ -60,7 +60,7 @@ namespace MobileGL {
|
||||
|
||||
class GLContext {
|
||||
public:
|
||||
GLContext() = default;
|
||||
GLContext();
|
||||
|
||||
// Error
|
||||
void RecordError(ErrorCode code, UniquePtr<ErrorInfo> 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<Uint, TransformFeedbackObjectState> m_transformFeedbackObjects;
|
||||
IndexGenerator<Uint> 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
|
||||
|
||||
Reference in New Issue
Block a user