From 500a8e462a5a0537be449edd3556acc113e99768 Mon Sep 17 00:00:00 2001 From: rereview Date: Tue, 8 Sep 2026 17:15:07 -0400 Subject: [PATCH] [Fix] (Espryt): resolve the framebuffer twin by handle, ask the applier's bound handles whether one framebuffer is bound to both bindings, and make an undescribed binding say so instead of declining in silence --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 212 ++++++++++++------ 1 file changed, 144 insertions(+), 68 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 3f19299e..94979184 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -2808,38 +2808,59 @@ namespace MobileGL::MG_Backend::DirectGLES { static Bool SyncCurrentFBOByRecord() { g_fboRecordsTrusted = false; const auto& st = MG_Pipe::MGPipeApplier(); - const MG_Pipe::MGPFramebufferState& drawRecord = BoundFramebufferRecord(FramebufferTarget::Draw); - const MG_Pipe::MGPFramebufferState& readRecord = BoundFramebufferRecord(FramebufferTarget::Read); - const Bool drawUnrecorded = MG_Pipe::MGPipeHandleIsNull(drawRecord.Fbo); - const Bool readUnrecorded = MG_Pipe::MGPipeHandleIsNull(readRecord.Fbo); - // P4a decline-site F2: S, and its HALF-DESCRIBED case is LOUD NOW (ID-19), not at - // the verification round. + const MG_Pipe::MGPFramebufferState* const drawRecord = BoundFramebufferRecord(FramebufferTarget::Draw); + const MG_Pipe::MGPFramebufferState* const readRecord = BoundFramebufferRecord(FramebufferTarget::Read); + const Bool drawUnrecorded = drawRecord == nullptr || MG_Pipe::MGPipeHandleIsNull(drawRecord->Fbo); + const Bool readUnrecorded = readRecord == nullptr || MG_Pipe::MGPipeHandleIsNull(readRecord->Fbo); + // P4a decline-site F2: S - BOTH CASES ARE LOUD, the half-described one since v2 + // (ID-19) and the NEITHER-DESCRIBED one from this verification round. // - // NEITHER target recorded is the transitional state of a tree whose client half - // has not landed - every draw on this tree is that - and it stays silent until - // the round flips it. EXACTLY ONE recorded is a different animal: an emitter - // walks {Draw, Read} together and a Target = Both record writes both, so no - // correct emitter can produce it. It is the shape a PARTIALLY LANDED emitter - // produces, it is invisible in a pixel test because the arm just declines, and it - // is precisely what this phase's integration round has to catch - so it says so, - // in the words section 8's one grep looks for, and names which target is missing - // plus the handle of the one that is not. + // EXACTLY ONE recorded: an emitter walks {Draw, Read} together and a Target = Both + // record writes both, so no correct emitter can produce it. It is the shape a + // PARTIALLY LANDED emitter produces and it is invisible in a pixel test because + // the arm just declines. + // + // NEITHER recorded WAS the transitional state of a tree whose client half had not + // landed - on this branch, before B and C were integrated, every draw was that, + // which is why v2 left it silent. Package B now emits set_framebuffer_state for + // every bound framebuffer and this function only runs with bit 9 (and therefore + // 10, 11 and 7) set, so reaching a draw with NEITHER binding described means the + // record for the bound framebuffer never arrived, or arrived at a slot whose + // generation has already moved - the applier counts that second cause separately + // in StaleFramebufferRecordLookups, so the two are told apart without a third + // message. Both are seam defects and both now say so, in the words section 8's + // one grep looks for. if (drawUnrecorded || readUnrecorded) { if (drawUnrecorded != readUnrecorded) { - const MG_Pipe::MGPFramebufferState& described = drawUnrecorded ? readRecord : drawRecord; + const MG_Pipe::MGPFramebufferState& described = *(drawUnrecorded ? readRecord : drawRecord); MGLOG_E_ONCE("A framebuffer record does not describe the binding it names: the %s " "binding has no record while %s names {slot %u, gen %u} - a " "half-described applier; running the pre-handle framebuffer sync.", drawUnrecorded ? "DRAW" : "READ", drawUnrecorded ? "READ" : "DRAW", static_cast(described.Fbo.Slot), static_cast(described.Fbo.Gen)); + } else { + MGLOG_E_ONCE("A framebuffer record does not describe the binding it names: NEITHER " + "the DRAW nor the READ binding has one while the framebuffer " + "subsystem bit is set (bound handles {slot %u, gen %u} / " + "{slot %u, gen %u}, %llu stale-generation lookup(s) so far); " + "running the pre-handle framebuffer sync.", + static_cast( + st.BoundFramebuffer[SizeT(MG_Pipe::MGPipeFramebufferTarget::Draw)].Slot), + static_cast( + st.BoundFramebuffer[SizeT(MG_Pipe::MGPipeFramebufferTarget::Draw)].Gen), + static_cast( + st.BoundFramebuffer[SizeT(MG_Pipe::MGPipeFramebufferTarget::Read)].Slot), + static_cast( + st.BoundFramebuffer[SizeT(MG_Pipe::MGPipeFramebufferTarget::Read)].Gen), + static_cast(st.StaleFramebufferRecordLookups)); } return false; } // P4a decline-site F3: S - the framebuffer seam, already loud; the round keeps this // wording VERBATIM because it is the stem the other two seams now share. - if (!FramebufferRecordMatchesBinding(FramebufferTarget::Draw, drawRecord) || - !FramebufferRecordMatchesBinding(FramebufferTarget::Read, readRecord)) { + if (!FramebufferRecordMatchesBinding(FramebufferTarget::Draw, *drawRecord) || + !FramebufferRecordMatchesBinding(FramebufferTarget::Read, *readRecord)) { MGLOG_E_ONCE("A framebuffer record does not describe the binding it names; " "running the pre-handle framebuffer sync."); return false; @@ -2850,7 +2871,7 @@ namespace MobileGL::MG_Backend::DirectGLES { for (auto& target : fboTargets) { if (SyncedFramebufferSerialIsCurrent(target, st.FramebufferSerial)) continue; - const MG_Pipe::MGPFramebufferState& record = BoundFramebufferRecord(target); + const MG_Pipe::MGPFramebufferState& record = *BoundFramebufferRecord(target); if (record.IsDefault != 0) { // The default framebuffer, said by the record rather than by comparing the // bound object against pDefaultFramebufferInfo->defaultFBO - which is one @@ -2873,39 +2894,74 @@ namespace MobileGL::MG_Backend::DirectGLES { // handle - FindByHandle(record.Fbo) - so a recycled FBO cannot be mistaken for // its predecessor here. // - // A1, RECONCILED AGAINST WHAT D ACTUALLY BUILT. The fallback below is NOT the - // one-liner this package predicted. D adds `BackendPtr* GetOrCreateByHandle( - // MGPipeHandle)` (esprytobj Managers.h ~453), which returns a POINTER rather - // than the reference GetOrCreate(StatePtr) returns, and D says the pointer form - // is deliberate. So at the rebase this becomes GetOrCreateByHandle(record.Fbo) - // PLUS A NULL CHECK on the returned pointer, and once the resolve takes the - // handle the `slot.GetBoundObject()` above it and FramebufferRecordMatchesBinding - // both go with it (A2's `#else` half never fires - D left SyncToBackend's and - // SyncReadBufferToBackend's signatures textually unchanged). + // A1, TAKEN AT THE VERIFICATION ROUND AGAINST WHAT D ACTUALLY BUILT. The twin + // is resolved BY HANDLE ONLY: `BackendPtr* GetOrCreateByHandle(MGPipeHandle)` + // (Managers.h ~453) returns a POINTER rather than the reference + // GetOrCreate(StatePtr) returns, deliberately, because it has three ways to + // decline - the legacy arm, a slot past the table's sanity bound, and a + // generation BEHIND the live entry's - and every one of them has to be visible + // here rather than answered with a parked twin. So the FindByHandle-then- + // GetOrCreate(currentFBO) pair is gone and with it the last place on this arm + // where a twin could be minted against the bound frontend ADDRESS. // - // AND THE POINTER-INVALIDATION CONTRACT HAS TO BE RE-CHECKED THERE, not just - // the call. Managers.h ~381-385 says a handle-arm result is invalidated by the - // next GetOrCreate/Find/CollectGarbage. The expression below short-circuits, so - // FindByHandle's result is never held across the fallback - but - // GetOrCreateByHandle CAN GROW THE TABLE, so whoever writes the rebase must - // keep the two out of one expression and must not hold the pointer across a - // later resolve. + // WHAT DID NOT GO, against the review's A1 sketch, and why: + // * `slot.GetBoundObject()` STAYS. A2's `#else` half never fired - D left + // SyncToBackend(SharedPtr, target) and + // SyncReadBufferToBackend(SharedPtr) textually + // unchanged - so the frontend object is still this call's ARGUMENT. Only + // the twin LOOKUP moved to the handle. + // * FramebufferRecordMatchesBinding (F3) STAYS, and the §2 table's "keep + // verbatim; it is the wording §8.2 greps" is the ruling that governs: with + // the twin now resolved from the record and configured FROM THE BOUND + // OBJECT, the identity check is the only thing standing between a + // mis-keyed record and one framebuffer's attachments written into + // another's twin. Removing it would delete the check that makes this + // rewrite safe. + // + // THE POINTER-INVALIDATION CONTRACT (Managers.h ~381-385): a handle-arm result + // is a stable array element that only a table-GROWING GetOrCreate can move. + // GetOrCreateByHandle is exactly such a call, so its result is used and dropped + // inside this iteration and never held across another registry call. auto& slot = GetFramebufferBindingSlotChecked(target); const auto& currentFBO = slot.GetBoundObject(); - // P4a decline-site F4: S - unreachable today (FramebufferRecordMatchesBinding - // maps "nothing bound" to boundIsDefault and a non-default record is already - // rejected above), and at the verification round it becomes a FULL fallback - - // g_fboRecordsTrusted = false; return false - rather than the `continue` that - // would leave the other target half-run against section 1's invariant. + // P4a decline-site F4: S - FLIPPED AT THE VERIFICATION ROUND to the FULL + // fallback the review specified. Still unreachable in practice + // (FramebufferRecordMatchesBinding maps "nothing bound" to boundIsDefault and + // a non-default record is already rejected above), but the `continue` it used + // to take would have left the other target half-run against section 1's + // invariant that a decline is a whole-arm decline. if (!currentFBO) { - MGLOG_E_ONCE("A framebuffer record names %s but no FBO is bound to it.", - target == FramebufferTarget::Read ? "READ" : "DRAW"); - continue; + MGLOG_E_ONCE("A framebuffer record does not describe the binding it names: the " + "%s record names {slot %u, gen %u} but no FBO is bound to that " + "binding; running the pre-handle framebuffer sync.", + target == FramebufferTarget::Read ? "READ" : "DRAW", + static_cast(record.Fbo.Slot), + static_cast(record.Fbo.Gen)); + g_fboRecordsTrusted = false; + return false; } - auto* twinSlot = g_backendFramebufferObjects.FindByHandle(record.Fbo); - auto& backendObj = - twinSlot ? *twinSlot : g_backendFramebufferObjects.GetOrCreate(currentFBO); + auto* const twinSlot = g_backendFramebufferObjects.GetOrCreateByHandle(record.Fbo); + // A1's third decline. Bit 9 implies bit 10 implies bit 7, so the slot tables are + // armed whenever this function runs and the legacy-arm cause cannot fire here; + // what is left is a slot past the sanity bound or a generation behind the live + // entry's, and both are seam defects - a handle the emitter minted for a + // recycled slot and never described, or one it had already retired. The whole + // arm declines rather than this one target, for F4's reason. + if (twinSlot == nullptr) { + MGLOG_E_ONCE("A framebuffer record does not describe the binding it names: the " + "%s record's handle {slot %u, gen %u} is refused by the twin slot " + "table (live generation %u); running the pre-handle framebuffer " + "sync.", + target == FramebufferTarget::Read ? "READ" : "DRAW", + static_cast(record.Fbo.Slot), + static_cast(record.Fbo.Gen), + static_cast( + g_backendFramebufferObjects.LiveGenAt(record.Fbo.Slot))); + g_fboRecordsTrusted = false; + return false; + } + auto& backendObj = *twinSlot; if (!backendObj) { backendObj = MakeShared(); } @@ -2919,16 +2975,29 @@ namespace MobileGL::MG_Backend::DirectGLES { // // MINOR-1, CORRECTED: the read buffer is applied by SyncReadBufferToBackend // FROM THE FRONTEND OBJECT, not from MGPFramebufferState::ReadSurface - this - // package reads Fbo, IsDefault, Target, DrawBuffers[] and ContentHash and - // nothing else. What the change actually buys, and it is worth having, is that - // the skip is now a FIELD TEST on record.Target instead of a pointer compare - // against whatever object the previous loop iteration happened to sync, so the - // read-buffer-shared-FBO defect class can no longer be produced by an accident - // of loop order. It is the defect expressed as a field, NOT made - // unrepresentable; reading ReadSurface here is the D-C1 endpoint and belongs - // with A1's rewrite, when the twin API takes the record. + // package reads Fbo, IsDefault, DrawBuffers[] and ContentHash and nothing else. + // + // ID-27 (wire review v2, MAJOR-1): THE QUESTION IS ASKED OF THE BOUND HANDLES, + // NOT OF THE RECORD'S STORED TARGET, and it is no longer possible to ask it any + // other way in this file. `MGPFramebufferState::Target` is the target of the + // LAST EMISSION that wrote the record, and since ID-19(c) that emission may be + // a `Named` one from any of B's sixteen DSA sites - so a framebuffer really + // bound to both bindings can carry `Target == Named` and a framebuffer bound to + // neither can carry `Target == Both` left over from when it was. The skip's + // real question is "is one object bound to both bindings", and the applier + // answers it directly: BoundFramebuffer[Draw] == BoundFramebuffer[Read]. Both + // are non-null here (F2 rejected a null on either), and a handle compares by + // {slot, gen}, so a recycled slot is not its predecessor. + // + // Reading the stored Target degraded to a redundant per-frame read-buffer sync + // rather than to a wrong picture, which is exactly why it needed replacing here + // instead of being caught by a lane: `Target != Both` takes the full + // SyncToBackend path below, which is correct and merely repeats the draw pass's + // attachment work. The wire review found it by reading the emitter. + const auto& boundHandles = st.BoundFramebuffer; if (target == FramebufferTarget::Read && - record.Target == static_cast(MG_Pipe::MGPipeFramebufferTarget::Both)) { + boundHandles[SizeT(MG_Pipe::MGPipeFramebufferTarget::Draw)] == + boundHandles[SizeT(MG_Pipe::MGPipeFramebufferTarget::Read)]) { backendObj->SyncReadBufferToBackend(currentFBO); StampSyncedFramebufferSerial(target, st.FramebufferSerial); continue; @@ -3953,15 +4022,19 @@ namespace MobileGL::MG_Backend::DirectGLES { // SyncCurrentFBO immediately before this, so the records have just been // checked against the two bindings. if (FramebufferSubsystemEnabled() && FramebufferImpl::g_fboRecordsTrusted) { - const MG_Pipe::MGPFramebufferState& record = + const MG_Pipe::MGPFramebufferState* const recordPtr = BoundFramebufferRecord(FramebufferTarget::Draw); - // P4a decline-site F7: unreachable - g_fboRecordsTrusted implies both - // handles are non-null (SyncCurrentFBOByRecord sets the latch only after - // it has rejected a null on either target, and it runs immediately before - // this in PrepareForDraw). At the verification round it becomes a - // MOBILEGL_ASSERT or is deleted; it must not become a silent decline that - // looks like a legitimate one. - if (!MG_Pipe::MGPipeHandleIsNull(record.Fbo)) { + // P4a decline-site F7: FLIPPED AT THE VERIFICATION ROUND from a silent + // decline to an assertion, because it is unreachable and a silent decline + // here is indistinguishable from a legitimate one. g_fboRecordsTrusted + // implies a record with a non-null handle on BOTH targets: + // SyncCurrentFBOByRecord sets the latch only after F2 has rejected a null + // pointer or handle on either, and it runs immediately before this in + // PrepareForDraw. Kept as a checked decline rather than deleted so a + // release build cannot dereference null if that ordering ever changes. + MOBILEGL_ASSERT(recordPtr != nullptr && !MG_Pipe::MGPipeHandleIsNull(recordPtr->Fbo)); + if (recordPtr != nullptr && !MG_Pipe::MGPipeHandleIsNull(recordPtr->Fbo)) { + const MG_Pipe::MGPFramebufferState& record = *recordPtr; if (!g_broadcastMemoHandleValid || g_broadcastMemoContentHash != record.ContentHash) { Uint enabledDrawBuffers = 0; for (Uint i = 0; i < MG_State::GLState::FramebufferObject::MAX_DRAW_BUFFERS; ++i) { @@ -4124,16 +4197,19 @@ namespace MobileGL::MG_Backend::DirectGLES { // IsDefault byte, rather than the bound object's address and a comparison against // pDefaultFramebufferInfo->defaultFBO. if (FramebufferSubsystemEnabled() && FramebufferImpl::g_fboRecordsTrusted) { - const MG_Pipe::MGPFramebufferState& record = BoundFramebufferRecord(target); + const MG_Pipe::MGPFramebufferState* const recordPtr = BoundFramebufferRecord(target); // The record's own handle is the "has this binding ever been described" test, not // FramebufferSerial - which MGPipeApplierReset advances whether or not anything // was ever emitted (see SyncCurrentFBOByRecord). The trust latch above is the // other half: it says the sync that ran a moment ago found these two records // describing these two bindings. - // P4a decline-site F5: unreachable, same argument as F7 above - the trust latch - // implies a non-null handle on both targets. Becomes a MOBILEGL_ASSERT or is - // deleted at the verification round. - if (!MG_Pipe::MGPipeHandleIsNull(record.Fbo)) { + // P4a decline-site F5: FLIPPED AT THE VERIFICATION ROUND to an assertion, same + // argument and same shape as F7 above - the trust latch implies a record with a + // non-null handle on both targets, and a silent decline here would look exactly + // like a legitimate one. + MOBILEGL_ASSERT(recordPtr != nullptr && !MG_Pipe::MGPipeHandleIsNull(recordPtr->Fbo)); + if (recordPtr != nullptr && !MG_Pipe::MGPipeHandleIsNull(recordPtr->Fbo)) { + const MG_Pipe::MGPFramebufferState& record = *recordPtr; if (record.IsDefault == 0) { auto* twinEntry = FramebufferImpl::g_backendFramebufferObjects.FindByHandle(record.Fbo); // P4a decline-site F6: S - already loud, and it KEEPS this shape at the