From 1e1f4bad2725acd0ac6cb3609ee06bc95564c6ad Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 22 Sep 2026 17:49:51 -0400 Subject: [PATCH] [MG_Backend, MG_IntegrationTest, MG_Remote, docs] (P7): an owner-side upload shows through a sampled texture view, because the split clean gate resolves to the storage record P3b/P4b wave 2-D package D3, slice 1. The defect package D2 found with its new TextureViewAliasScenario gate and excluded by name because the fix is outside D2's footprint. THE HOLE. Once a glTextureView had been SAMPLED, a later glTexSubImage through the OWNER's own name never became visible through the view, on inproc, spawn and tcp, green under monolith. The client half was never at fault: the case's own emit-counter assertions pass on the red run, so the records were emitted and the applier accepted them. A view owns no texels and the client keeps ONE emission cursor per storage - an upload through a view's name is remapped onto its storage owner before it is emitted (MG_Impl/Pipe/TextureEmit.h) - so every resource_subdata for either name is keyed on the OWNER and ApplyTextureUpload moves the OWNER's Serial and PendingUploads alone. BackendTextureObject::IsDrawSyncCleanByRecord read the VIEW's Serial and PendingUploads, which nothing an upload does can move, so the gate answered CLEAN for every owner write after the view's first sample and the sync that would have drained the owner's pending uploads never ran. THE FIX, AND WHY THIS ONE. The gate's two STORAGE clauses are asked of the STORAGE record - this record for a texture that owns its texels, the record Desc.ViewOf names for a view - through the new PipeTextureStorageRecordForRecord. The three PARAMETER clauses stay the view's own record's, because a view has its own texture parameters and its own built-in sampler, which is the whole reason the second name exists. SyncTextureViewToBackendByRecord stamps that same storage serial at both its arms, so the stamp and the gate name one quantity. This is the split equivalent of the monolith gate's GetContentVersion(), which TextureObjectView forwards to its owner (TextureObjectView.cpp:100-102). THE ALTERNATIVE WAS REJECTED. A reverse index owner->views, so an owner-side resource_subdata bumps every viewing record's Serial, is the other shape D2 named. It is wrong here on three counts: it puts new mutable state in MG_Pipe's applier, which both backends read, to answer a question one backend's gate asks; it makes ApplyTextureUpload write a record other than the one the call names, which breaks the applier's own serial discipline; and its index has to be pruned on view death or an owner upload bumps a recycled slot's Serial - a silent cross-texture hazard with no test that would catch it. The resolve direction needs no state, no pruning, and is checked at every use: PipeTextureRecordForHandle tests Live and Gen. DELETION ORDERING, WHICH DECIDED IT. A view holds a STRONG SharedPtr to its storage owner (TextureObjectView::m_storageOwner) and the client emits resource_destroy from the DESTRUCTOR, not from glDeleteTextures - "the last SharedPtr to this object dropping, not the glDelete* that only marks the name and leaves a still-bound object very much alive" (TextureObject.cpp:63). So an owner's record outlives every record that views it whatever order the application deletes the two names in, and the forward edge cannot dangle. The walk checks anyway, and a null answer is NOT clean - the direction that re-syncs. VIEWS OF VIEWS. ONE hop always reaches storage: glTextureView composes a view-of-a-view onto the ROOT at creation (the spec's additive min-level rule), the client's emitter says so and TextureObjectView's invariant is that its owner is never a view. The walk is transitive anyway and BOUNDED at four hops, because this side may not depend on a client invariant to terminate; a cyclic chain the wire could forge ends as a null answer, which is "not clean", not a refusal. No new abort site and no new Fatal (rule I): fatal_census abort_sites stays 79. RED-ONCE (rule J), the case D2 excluded, with the exclusion lifted and nothing else changed, at D2's head a7c06f4f: ctest -R 'DirectGLES\.(Split|Spawn|Tcp)\.TextureViewAliasScenario' -j 3 -> 79% tests passed, 3 tests failed out of 14 4042 - DirectGLES.Split.TextureViewAliasScenario.TheOwnerUploadDrainRunsAgainAfterADrawBoundary (Failed) 4046 - DirectGLES.Spawn.TextureViewAliasScenario.TheOwnerUploadDrainRunsAgainAfterADrawBoundary (Failed) 4050 - DirectGLES.Tcp.TextureViewAliasScenario.TheOwnerUploadDrainRunsAgainAfterADrawBoundary (Failed) the first owner write, through the view: 11408 of 11408 pixels disagree; first at (2, 2) is rgba(50,160,30,255), expected rgba(0,0,255,255) +/- 2 the second owner write, through the view: 11408 of 11408 pixels disagree; first at (2, 2) is rgba(50,160,30,255), expected rgba(255,255,0,255) +/- 2 Only the PIXEL channel is red; the drain's emit counters pass, which is what places the defect on the server. The monolith control DirectGLES.TextureViewAliasScenario.* is 4/4 in the same run. With the fix: 14/14, 100%. CONTRACT-P5E.md 5.2's clean gate is amended to state the rule the code now implements, and says out loud that the rec->Serial form it carried through P5e was wrong rather than restating it. The CMake exclusion is lifted; the scenario registers all four cases on all three arms again (G14: the name never left any lane). G1: .text 0xa52203, nm --defined-only 30570 symbols, 0 added / 0 removed, -D 0/0. Every line touched in Managers.cpp is inside #if MOBILEGL_PIPE_PUSH (the gate and the resolver) or #if MOBILEGL_BUILD_DISAGGREGATED (the view sync), and Managers.h's declaration is inside the PIPE_PUSH block, so the pull build's preprocessed text is unchanged. --- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 46 +++++++++++++++++++-- MobileGL/MG_Backend/DirectGLES/Managers.h | 38 +++++++++++++++++ MobileGL/MG_IntegrationTest/CMakeLists.txt | 39 +++++++---------- MobileGL/MG_Remote/CONTRACT-P5E.md | 31 +++++++++++++- 4 files changed, 124 insertions(+), 30 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 0cb77673..6b18f809 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -4566,6 +4566,24 @@ namespace MobileGL::MG_Backend::DirectGLES { return PipeRecordAt(MG_Pipe::MGPipeApplier().RenderbufferResources, res); } + // P3b/P4b (wave 2-D, package D3). See the declaration for why a view's storage clauses are + // asked of another record, why null means "ask again" and why the deletion order cannot + // strand this walk. + const MG_Pipe::MGPipeResourceRecord* PipeTextureStorageRecordForRecord( + const MG_Pipe::MGPipeResourceRecord& record) { + // ONE hop is the shape the client emits and the second is already generous; the bound is + // here so a cyclic chain is a null answer rather than a hang, not because a real chain + // is ever this long. + constexpr Uint32 kViewChainBound = 4; + const MG_Pipe::MGPipeResourceRecord* storage = &record; + for (Uint32 hop = 0; hop < kViewChainBound; ++hop) { + if (MG_Pipe::MGPipeHandleIsNull(storage->Desc.ViewOf)) return storage; + storage = PipeTextureRecordForHandle(storage->Desc.ViewOf); + if (storage == nullptr) return nullptr; + } + return nullptr; + } + MobileGL::TextureTarget PipeTextureTargetForHandle(MG_Pipe::MGPipeHandle res) { const auto* record = PipeTextureRecordForHandle(res); if (record == nullptr) return MobileGL::TextureTarget::Unknown; @@ -7517,11 +7535,19 @@ namespace MobileGL::MG_Backend::DirectGLES { MGLOG_D("Storage texture of view %u has no ES name yet.", record.Desc.GlNameForDiag); return; } + // P3b/P4b D3: THE SERIAL THIS TWIN STAMPS IS THE STORAGE'S, not this view's, because + // the gate that reads it back (IsDrawSyncCleanByRecord) now asks the same record. It + // is read AFTER the storage sync above - which has just consumed the owner's pending + // uploads - and taken by VALUE, because the applier's vector may move under any later + // call and a Uint64 cannot dangle. A storage record that has gone answers 0, which + // the gate reads as "never clean": the direction that re-syncs. + const MG_Pipe::MGPipeResourceRecord* storageRecord = PipeTextureStorageRecordForRecord(record); + const Uint64 storageSerial = storageRecord != nullptr ? storageRecord->Serial : 0; if (m_isInitialized && m_viewSourceBackendTextureId == storageBackendTextureId) { // The steady case, and the only one this arm has to be fast at: one record read, // one integer compare and the serial stamp that keeps the aggregate clean gate // (IsDrawSyncCleanByRecord) answering true for the next draw. - m_syncedResourceSerial = record.Serial; + m_syncedResourceSerial = storageSerial; return; } // The view window is a sampler-view CSO, created even when this @@ -7563,7 +7589,8 @@ namespace MobileGL::MG_Backend::DirectGLES { m_prevTextureInfo = {static_cast(record.Desc.InternalFormat), record.Desc.Width, record.Desc.Height, record.Desc.Depth, view.NumLevels, 0, record.Desc.Samples, record.Desc.FixedSampleLocations != 0}; - m_syncedResourceSerial = record.Serial; + // The storage's, for the steady arm's reason above (P3b/P4b D3). + m_syncedResourceSerial = storageSerial; } #endif @@ -15515,9 +15542,20 @@ namespace MobileGL::MG_Backend::DirectGLES { Bool BackendTextureObject::IsDrawSyncCleanByRecord(MG_Pipe::MGPipeHandle res, const MG_Pipe::MGPipeResourceRecord& record) const { (void)res; + // P3b/P4b D3: THE TWO STORAGE CLAUSES ARE ASKED OF THE STORAGE RECORD, which is this + // record for every texture that owns its texels and the record named by Desc.ViewOf + // for a glTextureView. A view's own Serial is moved by nothing an upload does - the + // client keys every resource_subdata on the storage owner, and the applier moves that + // record's Serial and PendingUploads alone - so reading the view's here answered + // "clean" for every owner write after the view's first sample. The three PARAMETER + // clauses below stay this record's: a view has its own texture parameters and its own + // built-in sampler, which is the whole point of the name existing. See + // PipeTextureStorageRecordForRecord and CONTRACT-P5E.md §5.2. + const MG_Pipe::MGPipeResourceRecord* storage = PipeTextureStorageRecordForRecord(record); + if (storage == nullptr) return false; // SyncMipmapsToBackend's own early-out, verbatim. - if (!m_isInitialized || m_syncedResourceSerial == 0 || m_syncedResourceSerial != record.Serial || - !record.PendingUploads.empty()) { + if (!m_isInitialized || m_syncedResourceSerial == 0 || m_syncedResourceSerial != storage->Serial || + !storage->PendingUploads.empty()) { return false; } // SyncTextureParamsToBackend's (ResolvePushedTextureParams' gate). diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 812d9c0d..21e61029 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -770,6 +770,44 @@ namespace MobileGL::MG_Backend::DirectGLES { // composite: this reader hides the split behind one lookup, exactly as the wire does. const MG_Pipe::MGPipeShaderCsoRecord* PipeShaderCsoRecordForHandle(MG_Pipe::MGPipeHandle cso); + // P3b/P4b (wave 2-D, package D3): THE STORAGE RECORD BEHIND A TEXTURE RECORD - the record of + // the texture it views for a glTextureView, and itself for every other texture. + // + // WHY THERE IS A SECOND READER. A view owns no texels, and the client keeps ONE emission + // cursor per storage: an upload through a view's own name is remapped onto its storage owner + // before it is emitted (MG_Impl/Pipe/TextureEmit.h, `ViewOf names the storage owner, and ONE + // HOP always reaches storage`), so every resource_subdata for either name is keyed on the + // OWNER and the applier moves the OWNER's Serial and PendingUploads alone + // (MG_Pipe/PipeApply.cpp, ApplyTextureUpload). A view therefore has a Serial that no upload + // ever moves, and the two STORAGE clauses of its draw-path clean gate have to be asked of + // this record instead of its own - which is what the monolith gate gets for free, because + // GetContentVersion() through a view is forwarded to the owner + // (MG_State/GLState/TextureState/TextureObjectView.cpp:100-102). CONTRACT-P5E.md §5.2 states + // the rule; without it a view that has been sampled once never sees another owner write. + // + // NULL IS "ASK AGAIN", NEVER "CLEAN". Every caller treats a null answer as a reason to RUN + // the sync, which is the direction that re-uploads rather than the direction that shows stale + // texels, and it raises nothing: a missing record is PipeTextureRecordForHandle's own + // null-on-miss answer one level up, not a protocol refusal. + // + // BOUNDED, THOUGH ONE HOP IS ALWAYS ENOUGH. glTextureView composes a view-of-a-view onto the + // ROOT at creation - the spec's additive min-level rule - and TextureObjectView's invariant + // is that its owner "is never a view itself", so Desc.ViewOf already names storage. The walk + // is written transitively anyway and gives up after a fixed number of hops, because THIS side + // may not depend on a client invariant to terminate: a chain the wire could make cyclic ends + // as a null answer rather than as a hang. + // + // DELETION ORDERING IS SAFE BY CONSTRUCTION, AND CHECKED ANYWAY. A view holds a strong + // SharedPtr to its storage owner (TextureObjectView::m_storageOwner) and the client emits + // resource_destroy from the DESTRUCTOR - "the last SharedPtr to this object dropping, not the + // glDelete* that only marks the name and leaves a still-bound object very much alive" + // (MG_State/GLState/TextureState/TextureObject.cpp:63) - so an owner's record cannot die + // while a record that views it is live, however the application orders its glDeleteTextures. + // If one ever did, PipeTextureRecordForHandle tests Live and Gen, so a recycled slot answers + // null rather than a stranger's record. + const MG_Pipe::MGPipeResourceRecord* PipeTextureStorageRecordForRecord( + const MG_Pipe::MGPipeResourceRecord& record); + // P5e (fb): the texture's own TARGET, from its descriptor. The image-unit bind needs it // (glBindImageTexture's layered/format rules are per target) and MGPImageView has no room // for it - 24 bytes, no pad - so it is read off the resource record instead of being added diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 1c741550..a4907006 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -2651,34 +2651,25 @@ if (MOBILEGL_BUILD_DISAGGREGATED) # TextureViewUsable() rather than failing - which is what the Tcp arm did until the fixture # entry below got the same knob. # - # ONE CASE IS EXCLUDED BY NAME, and it is a defect this package FOUND rather than a lane - # problem. TheOwnerUploadDrainRunsAgainAfterADrawBoundary is red on all three transports and - # green under monolith: + # ALL FOUR CASES RUN ON ALL THREE ARMS. TheOwnerUploadDrainRunsAgainAfterADrawBoundary was + # EXCLUDED BY NAME here by package D2, which found the defect but could not fix it inside its + # own footprint: # # after a view has been SAMPLED once, a later glTexSubImage through the OWNER's own name # never becomes visible through the view. # - # The client half is provably correct - the case's own counter assertions pass, so the - # records were emitted and the applier accepted them. The server's per-texture clean gate - # BackendTextureObject::IsDrawSyncCleanByRecord (Managers.cpp:15433) compares the VIEW's - # record.Serial and record.PendingUploads, and an owner-side resource_subdata moves only the - # OWNER's (MG_Pipe/PipeApply.cpp:1175). Nothing anywhere propagates a serial bump from a - # storage owner to the records that view it: MGPResourceDesc::ViewOf is a forward edge with - # no reverse index, and the applier reads it only in the descriptor-equality helper - # (PipeApply.cpp:666). Under monolith the same gate reads GetContentVersion() THROUGH the - # view, which TextureObjectView forwards to the owner (TextureObjectView.cpp:100-102) - the - # forwarding the split gate has no equivalent of. CONTRACT-P5E.md:387-395 states the split - # gate in full and has the same hole, so the implementation is faithful to the contract. - # - # NOT EXCLUDED FROM THE MONOLITH LANES, where it is green and is the regression net for the - # forwarding that works. Listed in notes/p34b/split-scenario-census.md and - # notes/p34b/espryt-d2.md rather than deleted (G14). The fix is a one-line-shaped change in - # MG_Pipe/PipeApply.cpp with an O(#texture records) scan behind it, or a reverse index in - # MGPipeResourceRecord - an owner's decision, not this package's, and it is outside D2's - # declared footprint. - mgl_itest_register_split_arms( - "TextureViewAliasScenario.*-TextureViewAliasScenario.TheOwnerUploadDrainRunsAgainAfterADrawBoundary" - "") + # Package D3 fixed it and this is its red-once (rule J): the case is red on Split, Spawn AND + # Tcp at D2's head a7c06f4f with this exclusion lifted and nothing else changed, and green on + # all three with the fix. The client half was never at fault - the case's own counter + # assertions pass on the red run, so the records were emitted and the applier accepted them. + # The server's per-texture clean gate BackendTextureObject::IsDrawSyncCleanByRecord + # (MG_Backend/DirectGLES/Managers.cpp) compared the VIEW's record.Serial and + # record.PendingUploads, while an owner-side resource_subdata moves only the OWNER's + # (MG_Pipe/PipeApply.cpp ApplyTextureUpload). The gate now RESOLVES THROUGH Desc.ViewOf to the + # storage owner's record for those two clauses, which is the split equivalent of the monolith + # gate's GetContentVersion() forwarding (TextureObjectView.cpp:100-102). CONTRACT-P5E.md §5.2 + # states the rule the code now implements. + mgl_itest_register_split_arms("TextureViewAliasScenario.*" "") # ---- D2: THE UPLOAD-SHAPE GOLD, UNDER ALL THREE TRANSPORTS ------------------------------ # diff --git a/MobileGL/MG_Remote/CONTRACT-P5E.md b/MobileGL/MG_Remote/CONTRACT-P5E.md index cfb8f4a9..7b99f510 100644 --- a/MobileGL/MG_Remote/CONTRACT-P5E.md +++ b/MobileGL/MG_Remote/CONTRACT-P5E.md @@ -388,11 +388,38 @@ vertex-elements CSO stays identity-addressed. Every draw-path entry is `SyncTextureToBackendByHandle(h, imageBindable)`: record first, twin by `GetOrCreateByHandle`, three syncs `(h, *rec)`; the by-value twin copy + second `Find` (`DirectGLES.cpp:1745-1755, 1781-1805`) go with the map arm. Clean = `m_isInitialized && -m_syncedResourceSerial == rec->Serial && rec->PendingUploads.empty() && m_syncedParamsSerial == +m_syncedResourceSerial == storage->Serial && storage->PendingUploads.empty() && m_syncedParamsSerial == rec->ParamsSerial && !rec->Params.ForceResync && !m_forceTextureParamsResync && m_syncedBuiltinSampler == rec->Params.BuiltinSampler && m_syncedBuiltinSamplerSerial == SamplerCso(rec->Params.BuiltinSampler)->Serial && !rec->Params.SamplerResync && !m_forceSamplerResync -&& rec->Desc.StorageKind == Mipmap` — no new state. The four in-body live reads (`GetTarget` at +&& rec->Desc.StorageKind == Mipmap` — no new state. + +**`storage` is the STORAGE RECORD, which is `rec` itself for a texture that owns its texels and +the record `rec->Desc.ViewOf` names for a `glTextureView`** (P3b/P4b wave 2-D package D3; +`Managers.cpp PipeTextureStorageRecordForRecord`). This clause was `rec->Serial` / +`rec->PendingUploads` through P5e and **that was wrong**, which is why it is corrected here rather +than restated: a view owns no texels and the client keeps ONE emission cursor per storage — an +upload through a view's own name is remapped onto its owner before it is emitted +(`MG_Impl/Pipe/TextureEmit.h`) — so every `resource_subdata` for either name is keyed on the OWNER +and `ApplyTextureUpload` moves the OWNER's `Serial` and `PendingUploads` alone. A view's own +`Serial` is moved by nothing an upload does, so a gate that read it answered CLEAN for every owner +write after the view's first sample and the view went on sampling the texels it was minted with. +The monolith gate has this for free: `GetContentVersion()` through a view is forwarded to the owner +(`TextureObjectView.cpp:100-102`). The three PARAMETER clauses stay `rec`'s — a view has its own +texture parameters and its own built-in sampler, which is the whole reason the second name exists — +as does `Desc.StorageKind`. The resolution is transitive and BOUNDED (one hop always reaches +storage, because `glTextureView` composes a view-of-a-view onto the root at creation, but this side +may not depend on a client invariant to terminate), and a storage record that cannot be resolved is +**not clean**: the direction that re-syncs, not a refusal, and it raises nothing. +`SyncTextureViewToBackendByRecord` stamps that same storage serial — or 0, which reads as never +clean — at both its arms, so the stamp and the gate name one quantity. Deletion ordering cannot +strand the walk: a view holds a strong reference to its storage owner and the client emits +`resource_destroy` from the DESTRUCTOR rather than from `glDeleteTextures` +(`TextureObject.cpp:63`), so an owner's record outlives every record that views it whatever order +the application deletes the two names in; `Live`/`Gen` are checked anyway, so a recycled slot +answers null rather than a stranger's record. + +The four in-body live reads (`GetTarget` at `Managers.cpp:8876-8877, 8620-8621`; `IsTextureView` `:7060`; the TexBuffer backing `:8223-8231`) read `Desc.Target / Desc.ViewOf / Desc.BufferForTexBuffer`; `GetExternalIndex` in logs becomes `Desc.GlNameForDiag`. The unit work list is `{Res, backend}` keyed `(ContextSerial,