From 173f1dd27360fe8ec39b48d9f4566462811a48b9 Mon Sep 17 00:00:00 2001 From: rereview Date: Tue, 8 Sep 2026 23:55:44 -0400 Subject: [PATCH] [Fix, Test] (clientfb, MG_Pipe): pass the level a texture respecify redefines - every per-level glTexImage*D and glGenerateMipmap grow took the applier's whole-resource arm and dropped the texture's every pending upload, including a level the applier had accepted at an earlier verb with the client's dirty flag already clear, so `L0; draw(other); L1; draw(T)` read a black level 0 on the handle arm (final review C-1); the storage entry points now state the scope (one level, a chain cut, the whole resource), the emitter builds wire's MGPRespecifiedLevel with the drain's packed target, a per-level call is never deduped on the descriptor, and the applier drops a named level whether or not the descriptor moved --- MobileGL/MG_Impl/Pipe/PipeFill.cpp | 6 +- MobileGL/MG_Impl/Pipe/TextureEmit.h | 150 ++++++-- MobileGL/MG_IntegrationTest/CMakeLists.txt | 2 + .../Harness/P4aFinalFixPeek.cpp | 52 +++ .../Harness/P4aFinalFixPeek.h | 32 ++ .../Scenarios/P4aFinalFixScenario.cpp | 341 ++++++++++++++++++ MobileGL/MG_Pipe/MGPipeTypes.h | 9 +- MobileGL/MG_Pipe/PipeApply.cpp | 37 +- MobileGL/MG_Pipe/PipeApply.h | 5 + MobileGL/MG_Pipe/PipeMutation.h | 36 +- .../GLState/TextureState/TextureObject.cpp | 31 +- .../GLState/TextureState/TextureObject.h | 18 +- .../TextureState/TextureObject2DCube.cpp | 7 +- MobileGL/MG_Test/Pipe/TextureEmitTest.cpp | 163 ++++++++- 14 files changed, 808 insertions(+), 81 deletions(-) create mode 100644 MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.cpp create mode 100644 MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.h create mode 100644 MobileGL/MG_IntegrationTest/Scenarios/P4aFinalFixScenario.cpp diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index d464338b..12262ae2 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -1255,10 +1255,12 @@ namespace MobileGL::MG_Pipe { MGPipeTextureEmitterInstance(), [&](auto& emitter) { emitter.EmitResourceCreate(texture); }); } - void MGPipeEmitTextureResourceRespecify(ITextureObject& texture) { + void MGPipeEmitTextureResourceRespecify(ITextureObject& texture, MGPipeTextureRespecifyScope scope, + Uint32 uploadTarget, Uint32 level) { if (!FamilyIsLive(kMGPipeSubsystemTextureResources, kMGPipeWiredTextureSubsystem)) return; ForwardWhenWired( - MGPipeTextureEmitterInstance(), [&](auto& emitter) { emitter.EmitResourceRespecify(texture); }); + MGPipeTextureEmitterInstance(), + [&](auto& emitter) { emitter.EmitResourceRespecify(texture, scope, uploadTarget, level); }); } void MGPipeEmitTextureParams(ITextureObject& texture) { diff --git a/MobileGL/MG_Impl/Pipe/TextureEmit.h b/MobileGL/MG_Impl/Pipe/TextureEmit.h index b14b317d..e7615383 100644 --- a/MobileGL/MG_Impl/Pipe/TextureEmit.h +++ b/MobileGL/MG_Impl/Pipe/TextureEmit.h @@ -523,14 +523,36 @@ namespace MobileGL::MG_Pipe { PublishCreate(MGPipeKind::Texture, handle, entry, desc); } - // resource_respecify, from every storage-defining entry point. DEDUPED ON THE - // DESCRIPTOR ITSELF rather than on a version, because the entry points that reach here - // are the ones that move the SHAPE and several of them do not move the descriptor at - // all (glTexParameter TEXTURE_BASE_LEVEL bumps the shape version and changes no field - // this record carries). A byte compare of an 88-byte POD is cheaper than the emission - // it avoids, and it is the same "version-first skip before anything expensive" shape - // every other P4a emission takes. - void EmitResourceRespecify(ITextureObject& texture) { + // resource_respecify, from every storage-defining entry point, WITH THE SCOPE OF THE + // STORAGE IT REPLACES (P4a final review C-1; the scopes are PipeMutation.h's). + // + // THE LEVEL IS PASSED, AND IT IS WIRE'S KEY. The applier keeps a pending-upload set per + // (uploadTarget, level) - the client's dirty flags, inverted - and a respecify drops the + // entries against the storage it REPLACES: with a null MGPRespecifiedLevel every entry, + // with a level exactly that one. v2 passed null at every call, so a level the applier + // had ACCEPTED at one verb (the client flag already clear, D-D5 step 1) and that the + // next verb's glTexImage2D(level 1) or glGenerateMipmap grow defined AROUND was dropped + // with nobody owing its texels: `L0; draw(other); L1; draw(T)` read a black level 0. + // The key is built from the SAME packed MGPSubData::Target the drain puts in that + // level's record (wire-v3 §5 item 5), so what this drops is what that emission made. + // + // THREE SCOPES, one call each for the first two and one call PER REMOVED LEVEL for the + // chain cut: the applier's key is one (uploadTarget, level), so "every level from N" + // is spelled as N.., each after the first landing on an unchanged descriptor - which + // the applier classifies as a metadata update that drops nothing but the level it + // names. That is the refinement wire's W11 clause takes this round. + // + // DEDUPED ON THE DESCRIPTOR ITSELF for the whole-resource form only: the entry points + // that reach it move the SHAPE and several of them do not move the descriptor at all + // (glTexParameter TEXTURE_BASE_LEVEL bumps the shape version and changes no field this + // record carries), and a byte compare of an 88-byte POD is cheaper than the emission + // it avoids. A PER-LEVEL form is never deduped: the level it redefines is not in the + // descriptor (a non-base level's extent moves no field), so an unchanged descriptor + // cannot say whether the applier still holds a box against the OLD level - and a box + // kept across a shrink is uploaded past the end of the new one. One applier call per + // level definition is the cost, and the sub-data that follows moves the serial anyway. + void EmitResourceRespecify(ITextureObject& texture, MGPipeTextureRespecifyScope scope, + Uint32 uploadTarget, Uint32 level) { const MGPipeHandle handle = AcquireTexture(texture.GetLifetimeId(), &texture); // THE VIEW'S OWNER IS ACQUIRED FIRST, and no Entry& is held across it (m3): the // owner's slot can be higher than this table's size, so AcquireTexture would @@ -568,7 +590,37 @@ namespace MobileGL::MG_Pipe { const MGPResourceDesc desc = MGPipeBuildTextureResourceDesc( texture, handle, entry.BindMask, /*storageDefined=*/true, viewOf, bufferHandle, bufOffset, bufSize); - if (entry.HasLastDesc && std::memcmp(&entry.LastDesc, &desc, sizeof(desc)) == 0) return; + const Bool unchanged = entry.HasLastDesc && std::memcmp(&entry.LastDesc, &desc, sizeof(desc)) == 0; + + // THE KEYS THIS CALL DROPS. `keyCount == 0` is the whole resource (a null level + // pointer); otherwise `keyCount` keys from `firstLevel` up, all on `uploadTarget`. + Uint32 firstLevel = 0; + Uint32 keyCount = 0; + switch (scope) { + case MGPipeTextureRespecifyScope::OneLevel: + firstLevel = level; + keyCount = 1; + break; + case MGPipeTextureRespecifyScope::LevelsFrom: { + // A cut at 0 leaves nothing: the whole resource. Otherwise the removed levels + // are [level, the level count the applier last accepted): LastDesc mirrors + // acceptance, and a sub-data for a level the accepted descriptor does not + // describe is refused by the applier, so no key above that count can exist. A + // cut that removes nothing the applier could hold is deduped like the + // whole-resource form; if the descriptor moved anyway the first key carries it. + if (level == 0) break; + const Uint32 previous = entry.HasLastDesc ? static_cast(entry.LastDesc.Levels) : 0u; + if (previous <= level && unchanged) return; + firstLevel = level; + keyCount = previous > level ? previous - level : 1u; + break; + } + case MGPipeTextureRespecifyScope::WholeResource: + default: + if (unchanged) return; + break; + } + // SELF-HEALING IN BOTH DIRECTIONS, the P3a m12 shape: a texture born while the // subsystem bit was clear has no applier record, and every later respecify would be // REFUSED. A create rather than a respecify, because that is what the record's @@ -589,24 +641,24 @@ namespace MobileGL::MG_Pipe { // about what the APPLIER holds, so a refused respecify must leave LastDesc naming // the descriptor that actually landed, or the next identical call is suppressed // against a record that was never stored. - Bool accepted = ApplyRespecify(desc); - if constexpr (MGPipeTextureRecordsReachTheApplier()) { - if (!accepted) { - // THE SECOND HALF OF THE SELF-HEAL, and the publication latch cannot give - // it: the latch answers "did a create for this handle GO OUT", which stays - // true after MGPipeApplierReleaseObjectRecords has dropped every object - // record - the scope a served context's teardown takes while the frontend - // objects live on in the share group. The applier's REFUSAL is the only - // signal that says "I hold nothing for this handle", and the acceptance - // return is what makes it visible from here at all. One retry, never a - // loop: a descriptor the applier refuses on its own merits (a target that - // names no resource kind) is refused again and the flags stay set. - const MGPResourceDesc healDesc = MGPipeBuildTextureResourceDesc( - texture, handle, entry.BindMask, /*storageDefined=*/false, viewOf, - bufferHandle, bufOffset, bufSize); - NoteDesc(healDesc, /*isCreate=*/true); - PublishCreate(MGPipeKind::Texture, handle, entry, healDesc); - accepted = ApplyRespecify(desc); + // + // THE PACKED TARGET IS THE DRAIN's (wire-v3 §5 item 5): the contract's packer takes + // two Uint32s, low byte the resource target, high byte the upload target (a cube + // face), and the applier matches the key against the sub-data records verbatim. + const Uint16 packedTarget = MGPipePackSubDataTarget( + static_cast(MGPipeResourceTargetForTextureTarget(texture.GetTarget())), uploadTarget); + Bool accepted = false; + if (keyCount == 0) { + accepted = RespecifyOnce(texture, handle, entry, desc, nullptr, viewOf, bufferHandle, bufOffset, + bufSize); + } else { + for (Uint32 i = 0; i < keyCount; ++i) { + MGPRespecifiedLevel key{}; + key.UploadTarget = packedTarget; + key.Level = static_cast(firstLevel + i); + accepted = RespecifyOnce(texture, handle, entry, desc, &key, viewOf, bufferHandle, bufOffset, + bufSize); + if (!accepted) break; } } NoteRespecified(entry, desc, accepted); @@ -713,7 +765,8 @@ namespace MobileGL::MG_Pipe { PublishCreate(MGPipeKind::Renderbuffer, handle, entry, createDesc); } NoteDesc(desc, /*isCreate=*/false); - Bool accepted = ApplyRespecify(desc); + // A renderbuffer's storage is always the whole object: no levels, so no key. + Bool accepted = ApplyRespecify(desc, nullptr); if constexpr (MGPipeTextureRecordsReachTheApplier()) { if (!accepted) { // See the texture twin: the applier's refusal is the only thing that can @@ -722,7 +775,7 @@ namespace MobileGL::MG_Pipe { renderbuffer, handle, entry.BindMask, /*storageDefined=*/false); NoteDesc(healDesc, /*isCreate=*/true); PublishCreate(MGPipeKind::Renderbuffer, handle, entry, healDesc); - accepted = ApplyRespecify(desc); + accepted = ApplyRespecify(desc, nullptr); } } NoteRespecified(entry, desc, accepted); @@ -952,13 +1005,43 @@ namespace MobileGL::MG_Pipe { entry.HasLastDesc = true; } - static Bool ApplyRespecify(const MGPResourceDesc& desc) { + // `level` is null for the whole resource and a key for exactly one level; every caller + // says which (final review C-1), and RepublishMask's null is deliberate - a mask move + // replaces no storage at all. + static Bool ApplyRespecify(const MGPResourceDesc& desc, const MGPRespecifiedLevel* level) { if constexpr (MGPipeTextureRecordsReachTheApplier()) { - return MGPipeApplyResourceRespecify(desc, nullptr); + return MGPipeApplyResourceRespecify(desc, nullptr, level); } + (void)level; return false; } + // One respecify with one key, and the refusal self-heal beside it. THE SECOND HALF OF + // THE SELF-HEAL, and the publication latch cannot give it: the latch answers "did a + // create for this handle GO OUT", which stays true after + // MGPipeApplierReleaseObjectRecords has dropped every object record - the scope a + // served context's teardown takes while the frontend objects live on in the share + // group. The applier's REFUSAL is the only signal that says "I hold nothing for this + // handle", and the acceptance return is what makes it visible from here at all. One + // retry, never a loop: a descriptor the applier refuses on its own merits (a target + // that names no resource kind) is refused again and the flags stay set. + Bool RespecifyOnce(ITextureObject& texture, MGPipeHandle handle, Entry& entry, const MGPResourceDesc& desc, + const MGPRespecifiedLevel* key, MGPipeHandle viewOf, MGPipeHandle bufferHandle, + Uint64 bufOffset, Uint64 bufSize) { + Bool accepted = ApplyRespecify(desc, key); + if constexpr (MGPipeTextureRecordsReachTheApplier()) { + if (!accepted) { + const MGPResourceDesc healDesc = MGPipeBuildTextureResourceDesc( + texture, handle, entry.BindMask, /*storageDefined=*/false, viewOf, bufferHandle, + bufOffset, bufSize); + NoteDesc(healDesc, /*isCreate=*/true); + PublishCreate(MGPipeKind::Texture, handle, entry, healDesc); + accepted = ApplyRespecify(desc, key); + } + } + return accepted; + } + static void NoteRespecified(Entry& entry, const MGPResourceDesc& desc, Bool accepted) { if constexpr (MGPipeTextureRecordsReachTheApplier()) { if (!accepted) return; @@ -982,7 +1065,10 @@ namespace MobileGL::MG_Pipe { desc.ImageBindableHint = (entry.BindMask & kMGPipeBindShaderImage) != 0 ? 1 : 0; if (std::memcmp(&entry.LastDesc, &desc, sizeof(desc)) == 0) return; NoteDesc(desc, /*isCreate=*/false); - NoteRespecified(entry, desc, ApplyRespecify(desc)); + // A NULL LEVEL, DELIBERATELY (wire-v3 §5 item 6): a mask move replaces no storage, + // and the applier classifies the identical storage fields as a metadata update + // that drops nothing. A key here would name a level this call did not touch. + NoteRespecified(entry, desc, ApplyRespecify(desc, nullptr)); } void NoteDesc(const MGPResourceDesc& desc, Bool isCreate) { diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 6578c1cb..31c9849b 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -55,6 +55,7 @@ add_executable(MobileGLIntegrationTest Harness/PipeSlotPeek.cpp Harness/PipeApplyPeek.cpp Harness/P4aSeamPeek.cpp + Harness/P4aFinalFixPeek.cpp Scenarios/OrientationScenario.cpp Scenarios/CrossFrameBufferScenario.cpp Scenarios/ResidentIndexScenario.cpp @@ -139,6 +140,7 @@ add_executable(MobileGLIntegrationTest Scenarios/TextureUploadShapeScenario.cpp Scenarios/ObjectSubsystemControlScenario.cpp Scenarios/P4aSeamAuditScenario.cpp + Scenarios/P4aFinalFixScenario.cpp ) target_include_directories(MobileGLIntegrationTest PRIVATE diff --git a/MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.cpp b/MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.cpp new file mode 100644 index 00000000..c4f9cdca --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.cpp @@ -0,0 +1,52 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.cpp +// Copyright (c) 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 + +#include "P4aFinalFixPeek.h" + +#if !defined(__ANDROID__) +#include +#if MOBILEGL_PIPE_PUSH +#include +#include +#define MGITEST_P4A_FINALFIX_PEEK_LIVE 1 +#endif +#endif + +namespace MGITest { + +#if defined(MGITEST_P4A_FINALFIX_PEEK_LIVE) + namespace { + namespace MGP = MobileGL::MG_Pipe; + } // namespace + + bool PeekPipeTextureResourceRecord(unsigned glTextureName, PipeTextureResourceRecordPeek* out) { + if (out == nullptr) return false; + const MGP::MGPipeApplierState& applier = MGP::MGPipeApplier(); + // Slot 0 is the reserved null slot; the walk is the same shape PipeApplyPeek.cpp's + // params reading takes. A GL name is never an identity on the wire, which is exactly + // why it is the right key for a harness that starts from the application's view. + for (MobileGL::SizeT slot = 1; slot < applier.TextureResources.size(); ++slot) { + const MGP::MGPipeResourceRecord& record = applier.TextureResources[slot]; + if (!record.Live) continue; + if (record.Desc.GlNameForDiag != static_cast(glTextureName)) continue; + out->Slot = static_cast(slot); + out->Gen = static_cast(record.Gen); + out->Serial = static_cast(record.Serial); + out->BindMask = static_cast(record.Desc.BindMask); + out->ImageBindableHint = static_cast(record.Desc.ImageBindableHint); + out->Levels = static_cast(record.Desc.Levels); + out->PendingUploads = static_cast(record.PendingUploads.size()); + return true; + } + return false; + } +#else + bool PeekPipeTextureResourceRecord(unsigned, PipeTextureResourceRecordPeek*) { return false; } +#endif + +} // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.h b/MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.h new file mode 100644 index 00000000..e7cb92a4 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.h @@ -0,0 +1,32 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Harness/P4aFinalFixPeek.h +// Copyright (c) 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 +// +// The white-box readings P4aFinalFixScenario.cpp takes, in a translation unit of their own for +// P4aSeamPeek.h's reason: a scenario TU includes the GL prototype headers and cannot include +// MG_Pipe/PipeApply.h or the Espryt managers beside them, and PipeApplyPeek.cpp is the gates +// package's file. Every entry point answers false where the reading cannot be taken (a pull +// build, Android, or an applier that holds no record for the name), and a false teaches the +// caller nothing - the case declines that half by name and keeps its public-GL verdict. +#pragma once + +namespace MGITest { + + // The applier's resource record for a texture, found by its GL name (GlNameForDiag - a + // diagnostics-only field, which is exactly what a test harness is). + struct PipeTextureResourceRecordPeek { + unsigned Slot; + unsigned Gen; + unsigned long long Serial; + unsigned BindMask; + unsigned ImageBindableHint; + unsigned Levels; + unsigned PendingUploads; + }; + bool PeekPipeTextureResourceRecord(unsigned glTextureName, PipeTextureResourceRecordPeek* out); + +} // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/P4aFinalFixScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/P4aFinalFixScenario.cpp new file mode 100644 index 00000000..5be7b055 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/P4aFinalFixScenario.cpp @@ -0,0 +1,341 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/P4aFinalFixScenario.cpp +// Copyright (c) 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 - THE THREE FINDINGS OF THE P4a FINAL WHOLE-DIFF REVIEW (final-review-v1.md C-1, C-2, +// M-A), each pinned by the public-GL sequence that was red on the tree the review read and is +// green with its fix. Every sequence here is legal GL and none of the 80-odd scenarios before +// this file drove it, which is how two criticals shipped through a green gate. +// +// C-1 The client never passed the applier the LEVEL a respecify redefines, so every per-level +// glTexImage*D / glGenerateMipmap grow took the applier's whole-resource arm and dropped +// EVERY pending upload of the texture - including a level the applier had already +// accepted and whose client-side dirty flag was therefore already clear (D-D5 step 1). +// Nobody owed those texels any more. The window is "accepted but not yet consumed": +// a verb the texture is not reached by (a draw with another texture) drains the level +// into the applier, Espryt does not sync the texture, and the next level definition eats +// the entry. Two hazard cases (a level-1 definition, a glGenerateMipmap) read a black +// level 0 on the handle arm; the three controls beside them (no verb between, level 0 +// consumed first, an immediate generate) are red on every arm, which is what pins the +// window rather than the mip path. +// C-2 A dead-but-not-recycled texture handle still resolved to the freed ITextureObject* +// inside the client's drain: the death helper freed the slot without telling the emitter, +// the drain list kept the level, and the next verb's drain called virtual +// GetStorageType() on freed memory - `glTexImage2D; glDeleteTextures; ` was a +// SIGABRT ("pure virtual method called") at the shipping default mask. The same +// delete-then-use shape is driven for every kind P4a mints (renderbuffer, sampler object, +// program, framebuffer) and for a slot recycled straight after the death (ABA), on both +// backends: the death path is backend-neutral by ruling (ID-8) and the DirectVulkan lane +// must see it too. +// M-A Nothing produced kMGPipeBindSampler / kMGPipeBindShaderImage, so ImageBindableHint was +// dead: the applier never saw a texture become image-bound, the metadata respecify +// (ID-18 M4) had no live trigger, and the remint pull the hint exists to prevent was +// neither prevented nor counted. The case here reads the applier's record around a +// glBindImageTexture: the hint arrives as a metadata update that keeps the pending upload +// standing beside it, and the picture after the transition is the texels that upload +// carried. +// +// A WHITE-BOX READING THAT CANNOT BE TAKEN IS DECLINED BY NAME AND THE CASE CONTINUES with its +// public-GL half (P4aSeamAuditScenario.cpp's shape): a pull build or a backend with no P4a +// consumer holds no record to read, and skipping the whole case there would delete the verdict +// those lanes carry. The C-1 and M-A cases assert their pictures on DirectGLES only - Espryt is +// the one consumer of the texture records this phase wires, so on any other backend the handle +// arm is inert by design and the picture proves nothing about it. + +#include +#include +#include +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/P4aFinalFixPeek.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr int kInset = 2; + + constexpr const char* kVS = R"(#version 330 core +in vec2 aPos; +out vec2 vUv; +void main() { + vUv = aPos * 0.5 + 0.5; + gl_Position = vec4(aPos, 0.0, 1.0); +} +)"; + + constexpr const char* kFS = R"(#version 330 core +in vec2 vUv; +uniform sampler2D uTex; +out vec4 oColor; +void main() { oColor = texture(uTex, vUv); } +)"; + + struct Vertex { + float x, y; + }; + + class P4aFinalFixScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + std::string error; + m_program = CompileProgram(kVS, kFS, &error); + ASSERT_NE(m_program, 0u) << error; + + static const Vertex quad[6] = {{-1.0f, -1.0f}, {1.0f, -1.0f}, {1.0f, 1.0f}, + {-1.0f, -1.0f}, {1.0f, 1.0f}, {-1.0f, 1.0f}}; + glGenBuffers(1, &m_quadBuffer); + glBindBuffer(GL_ARRAY_BUFFER, m_quadBuffer); + glBufferData(GL_ARRAY_BUFFER, sizeof(quad), quad, GL_STATIC_DRAW); + glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + glEnableVertexAttribArray(0); + glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, sizeof(Vertex), nullptr); + glBindVertexArray(0); + glDisable(GL_BLEND); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); + + // The "other" texture: a complete, single-level white texture, so a draw that + // samples it is a verb the texture under test is not reached by. + m_other = MakeLevel0(255, 255, 255, /*maxLevel=*/0); + while (glGetError() != GL_NO_ERROR) { + } + } + + void TearDown() override { + if (!Ready()) return; + glUseProgram(0); + glBindVertexArray(0); + glBindTexture(GL_TEXTURE_2D, 0); + glBindBuffer(GL_ARRAY_BUFFER, 0); + glBindFramebuffer(GL_FRAMEBUFFER, 0); + if (m_other != 0) glDeleteTextures(1, &m_other); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + if (m_quadBuffer != 0) glDeleteBuffers(1, &m_quadBuffer); + if (m_program != 0) glDeleteProgram(m_program); + while (glGetError() != GL_NO_ERROR) { + } + } + + // The C-1 and M-A pictures are about Espryt's consumption of the texture records; + // Magma registers no consumer for the P4a families (c0f), so the handle arm is inert + // there by design and a green picture proves nothing about the finding. Marks the + // case skipped; the caller tests IsSkipped() and returns. + void SkipUnlessEspryt(const char* what) { + if (Gl().BackendName() == "DirectGLES") return; + GTEST_SKIP() << what << " is consumed by DirectGLES only; backend is " << Gl().BackendName(); + } + + static std::vector Solid(int size, std::uint8_t r, std::uint8_t g, std::uint8_t b) { + std::vector texels(static_cast(size) * size * 4); + for (std::size_t i = 0; i < texels.size(); i += 4) { + texels[i] = r; + texels[i + 1] = g; + texels[i + 2] = b; + texels[i + 3] = 255; + } + return texels; + } + + // A 4x4 level 0 of one colour, NEAREST_MIPMAP_NEAREST with the level range clamped + // to `maxLevel`, so a single-level texture is complete and a chain is complete once + // its levels exist. + static GLuint MakeLevel0(std::uint8_t r, std::uint8_t g, std::uint8_t b, int maxLevel, int size = 4) { + const std::vector texels = Solid(size, r, g, b); + GLuint texture = 0; + glGenTextures(1, &texture); + glBindTexture(GL_TEXTURE_2D, texture); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, size, size, 0, GL_RGBA, GL_UNSIGNED_BYTE, texels.data()); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST_MIPMAP_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, 0); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, maxLevel); + glBindTexture(GL_TEXTURE_2D, 0); + return texture; + } + + static void DefineLevel1(GLuint texture, std::uint8_t r, std::uint8_t g, std::uint8_t b) { + const std::vector texels = Solid(2, r, g, b); + glBindTexture(GL_TEXTURE_2D, texture); + glTexImage2D(GL_TEXTURE_2D, 1, GL_RGBA8, 2, 2, 0, GL_RGBA, GL_UNSIGNED_BYTE, texels.data()); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, 1); + glBindTexture(GL_TEXTURE_2D, 0); + } + + // A full-viewport draw sampling `texture` on unit 0 through `program` (the fixture's + // by default). The viewport is far larger than the 4x4 base level, so this is + // MAGNIFICATION and reads LEVEL 0 whatever the chain holds above it. + Image DrawSampled(GLuint texture, GLuint program = 0) { + if (program == 0) program = m_program; + BindDefaultFramebuffer(); + glViewport(0, 0, Gl().Width(), Gl().Height()); + glUseProgram(program); + glUniform1i(glGetUniformLocation(program, "uTex"), 0); + glActiveTexture(GL_TEXTURE0); + glBindTexture(GL_TEXTURE_2D, texture); + glBindVertexArray(m_vao); + glDrawArrays(GL_TRIANGLES, 0, 6); + Image image = ReadPixels(Gl().Width(), Gl().Height()); + glBindTexture(GL_TEXTURE_2D, 0); + glBindVertexArray(0); + Gl().EndFrame(); + return image; + } + + ::testing::AssertionResult Mostly(const Image& image, const char* color, const std::string& when) { + return RegionIsMostly(image, kInset, image.Width() - kInset, kInset, image.Height() - kInset, color, + 0.0, when); + } + + void Report(const char* caseName, const Image& image) { + const char* mask = std::getenv("MOBILEGL_PIPE_PUSH"); + const int cx = image.Width() / 2; + const int cy = image.Height() / 2; + std::cout << "[ P4aFinalFix ] case=" << caseName << " backend=" << Gl().BackendName() + << " MOBILEGL_PIPE_PUSH=" << (mask ? mask : "(unset)") << " centre=" << image.At(cx, cy) + << " (" << image.ColorName(cx, cy) << ")" << std::endl; + } + + // The white-box gate of the M-A case: true when the applier holds a record for the + // texture in this process. Prints the decline. + bool RecordIsReadable(unsigned glTextureName, const char* what, PipeTextureResourceRecordPeek* out) { + if (PeekPipeTextureResourceRecord(glTextureName, out)) return true; + std::cout << "[ P4aFinalFix ] white-box reading DECLINED for " << what + << ": the applier holds no record for texture " << glTextureName + << " (a pull build, or a backend with no P4a consumer); the public-GL half of " + "the case still runs" + << std::endl; + RecordProperty("p4a_finalfix_white_box", "declined"); + return false; + } + + GLuint m_program = 0; + GLuint m_vao = 0; + GLuint m_quadBuffer = 0; + GLuint m_other = 0; + }; + + // ====================================================================================== + // C-1: a per-level definition around a verb the texture is not reached by + // ====================================================================================== + + // THE HAZARD. L0's upload is accepted at the unrelated draw's validate point (the client + // clears its flag), Espryt never syncs T there (it is bound nowhere), then the level-1 + // definition respecifies the resource. Before the fix that respecify carried no level and + // the applier dropped every pending upload; level 0 was allocated undefined. + TEST_F(P4aFinalFixScenario, PerLevelDefinitionAcrossAnUnrelatedDraw) { + if (!Ready()) return; + SkipUnlessEspryt("C-1's per-level respecify"); + if (IsSkipped()) return; + + const GLuint texture = MakeLevel0(255, 0, 0, /*maxLevel=*/0); + const Image unrelated = DrawSampled(m_other); + EXPECT_TRUE(Mostly(unrelated, "white", "the unrelated draw")); + DefineLevel1(texture, 255, 0, 0); + const Image image = DrawSampled(texture); + ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + Report("PerLevelDefinitionAcrossAnUnrelatedDraw", image); + EXPECT_TRUE(Mostly(image, "red", + "level 0 after a level-1 definition that followed a draw the texture was not " + "reached by - its accepted-but-unconsumed upload was dropped by the whole-" + "resource arm")); + GLuint cleanup = texture; + glDeleteTextures(1, &cleanup); + } + + // CONTROL: both levels defined before any verb; both are pending at the first sync. + TEST_F(P4aFinalFixScenario, ConsecutiveDefinitionsNoVerbBetween) { + if (!Ready()) return; + SkipUnlessEspryt("C-1's per-level respecify"); + if (IsSkipped()) return; + + const GLuint texture = MakeLevel0(255, 0, 0, /*maxLevel=*/0); + DefineLevel1(texture, 255, 0, 0); + const Image image = DrawSampled(texture); + ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + Report("ConsecutiveDefinitionsNoVerbBetween", image); + EXPECT_TRUE(Mostly(image, "red", "level 0 with both levels defined back to back")); + GLuint cleanup = texture; + glDeleteTextures(1, &cleanup); + } + + // CONTROL: level 0 is consumed by Espryt (T is sampled) before level 1 is defined. + TEST_F(P4aFinalFixScenario, LevelZeroConsumedBeforeLevelOne) { + if (!Ready()) return; + SkipUnlessEspryt("C-1's per-level respecify"); + if (IsSkipped()) return; + + const GLuint texture = MakeLevel0(255, 0, 0, /*maxLevel=*/0); + const Image first = DrawSampled(texture); + EXPECT_TRUE(Mostly(first, "red", "level 0 alone")); + DefineLevel1(texture, 255, 0, 0); + const Image image = DrawSampled(texture); + ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + Report("LevelZeroConsumedBeforeLevelOne", image); + EXPECT_TRUE(Mostly(image, "red", "level 0 after level 1 was added to a synced texture")); + GLuint cleanup = texture; + glDeleteTextures(1, &cleanup); + } + + // THE HAZARD, glGenerateMipmap flavour: the frontend grows the level chain (one + // AllocateStorage -> respecify per level) BEFORE the backend generate runs, with level 0 + // accepted-but-unconsumed. The driver then built the chain from an undefined level 0. + TEST_F(P4aFinalFixScenario, GenerateMipmapAcrossAnUnrelatedDraw) { + if (!Ready()) return; + SkipUnlessEspryt("C-1's per-level respecify"); + if (IsSkipped()) return; + + const GLuint texture = MakeLevel0(255, 0, 0, /*maxLevel=*/1000); + const Image unrelated = DrawSampled(m_other); + EXPECT_TRUE(Mostly(unrelated, "white", "the unrelated draw")); + glBindTexture(GL_TEXTURE_2D, texture); + glGenerateMipmap(GL_TEXTURE_2D); + glBindTexture(GL_TEXTURE_2D, 0); + const Image image = DrawSampled(texture); + ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + Report("GenerateMipmapAcrossAnUnrelatedDraw", image); + EXPECT_TRUE(Mostly(image, "red", + "level 0 after a glGenerateMipmap that followed a draw the texture was not " + "reached by")); + GLuint cleanup = texture; + glDeleteTextures(1, &cleanup); + } + + // CONTROL for the generate: no verb between the upload and the generate. + TEST_F(P4aFinalFixScenario, GenerateMipmapImmediately) { + if (!Ready()) return; + SkipUnlessEspryt("C-1's per-level respecify"); + if (IsSkipped()) return; + + const GLuint texture = MakeLevel0(255, 0, 0, /*maxLevel=*/1000); + glBindTexture(GL_TEXTURE_2D, texture); + glGenerateMipmap(GL_TEXTURE_2D); + glBindTexture(GL_TEXTURE_2D, 0); + const Image image = DrawSampled(texture); + ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + Report("GenerateMipmapImmediately", image); + EXPECT_TRUE(Mostly(image, "red", "level 0 after an immediate glGenerateMipmap")); + GLuint cleanup = texture; + glDeleteTextures(1, &cleanup); + } + + } // namespace +} // namespace MGITest diff --git a/MobileGL/MG_Pipe/MGPipeTypes.h b/MobileGL/MG_Pipe/MGPipeTypes.h index fa98a504..8a84f086 100644 --- a/MobileGL/MG_Pipe/MGPipeTypes.h +++ b/MobileGL/MG_Pipe/MGPipeTypes.h @@ -1100,10 +1100,13 @@ namespace MobileGL::MG_Pipe { // answers the per-record question, but a metadata update allocates nothing, so a // record it classifies as metadata is not acked even when that predicate says the // call may require one. - // - NO PendingUploads clear - not the whole vector, and not the redefined level either. - // This REFINES the level-scoped clear: identical storage fields clear NOTHING. (The + // - NO PendingUploads clear when the call names NO level. This REFINES the whole-resource + // clear: identical storage fields with a null MGPRespecifiedLevel clear NOTHING. (The // level-scoped rule exists because clearing the whole vector on a level-1 definition - // silently dropped level 0's accepted texels; a metadata update must drop neither.) + // silently dropped level 0's accepted texels; a metadata update must drop neither.) A + // call that NAMES a level is that level's redefinition whatever the descriptor says - + // a non-base level's extent is not a descriptor field - and drops exactly that level + // (P4a final review C-1); the client's mask republish passes null on purpose. // - The stored descriptor's BindMask and ImageBindableHint ARE updated - BindMask is // sticky and therefore ORed, never replaced - and the twin re-derives its storage // flags from the new mask on its next sync, recreating backend storage only where the diff --git a/MobileGL/MG_Pipe/PipeApply.cpp b/MobileGL/MG_Pipe/PipeApply.cpp index 625c4367..e4287f35 100644 --- a/MobileGL/MG_Pipe/PipeApply.cpp +++ b/MobileGL/MG_Pipe/PipeApply.cpp @@ -1667,23 +1667,28 @@ namespace MobileGL::MG_Pipe { // the arm this set exists for) -> glTexImage2D(1, data), which under a blanket // clear destroys level 0's entry before anything ever uploaded it. // - // - and a METADATA update (ID-18 M4) drops NOTHING, whatever `level` says. It is the - // third arm and it refines the first two rather than contradicting them: the rule - // is "the uploads against the storage this call REPLACES go with it", and a call - // whose storage-defining fields all equal the stored descriptor replaces no - // storage, so no level's coordinate system has moved and every pending box is still - // described in the space it was accumulated in. B re-emits the descriptor when a - // sticky bind bit moves, which can land between a glTexSubImage2D and the sync that - // consumes it; eating those texels there would be C1's bug with a different - // trigger, and just as silent. + // - and a METADATA update (ID-18 M4) with a NULL level drops NOTHING. It refines the + // whole-resource arm rather than contradicting it: the rule is "the uploads against + // the storage this call REPLACES go with it", and a call whose storage-defining + // fields all equal the stored descriptor replaces no storage, so no level's + // coordinate system has moved and every pending box is still described in the + // space it was accumulated in. B re-emits the descriptor when a sticky bind bit + // moves - with a null level, deliberately - which can land between a + // glTexSubImage2D and the sync that consumes it; eating those texels there would be + // C1's bug with a different trigger, and just as silent. + // + // - A NAMED LEVEL IS DROPPED WHETHER OR NOT THE DESCRIPTOR MOVED (P4a final review + // C-1, refining wire's W11 clause). The level pointer is the CALLER's statement that + // it reallocated that level, and the descriptor cannot contradict it: a non-base + // level redefined at a new size moves no descriptor field at all (the descriptor + // carries the base extent and the level count), so "identical storage fields" says + // nothing about that level's coordinate system, and a box kept against the old + // level would be uploaded past the end of the new one. The client's mask republish + // passes null, so this arm can never eat a standing upload on its behalf. // // A buffer never has a pending upload at all, so all three arms are inert for P3a's // half - which is also why a buffer is never classified as metadata-only (below). - if (metadataOnly) { - // nothing to drop, deliberately. - } else if (level == nullptr) { - record->PendingUploads.clear(); - } else { + if (level != nullptr) { // The keys are unique by AccumulatePendingUpload's construction - it looks for the // pair before it appends - so this erases at most one entry and stops. for (auto it = record->PendingUploads.begin(); it != record->PendingUploads.end(); ++it) { @@ -1691,6 +1696,10 @@ namespace MobileGL::MG_Pipe { record->PendingUploads.erase(it); break; } + } else if (metadataOnly) { + // nothing to drop, deliberately. + } else { + record->PendingUploads.clear(); } // resource_respecify is the catalogue's only kNeedsAck call, and the per-record half diff --git a/MobileGL/MG_Pipe/PipeApply.h b/MobileGL/MG_Pipe/PipeApply.h index 133e26f7..0fc8f065 100644 --- a/MobileGL/MG_Pipe/PipeApply.h +++ b/MobileGL/MG_Pipe/PipeApply.h @@ -784,6 +784,11 @@ namespace MobileGL::MG_Pipe { // same value the emission of that level put in the record. A per-face respecify therefore // drops the face it redefines and leaves the other five standing, and a caller that packs // the pair differently here than it packs it there simply matches nothing. + // A NAMED LEVEL IS DROPPED EVEN WHEN EVERY STORAGE-DEFINING FIELD IS UNCHANGED (P4a final + // review C-1): the pointer is the caller's statement that it reallocated that level, and + // a non-base level's extent is not in the descriptor. Only a NULL level with unchanged + // fields is the metadata update that drops nothing (ID-18 M4); the client's mask republish + // is the one caller of that shape and passes null on purpose. struct MGPRespecifiedLevel { Uint16 UploadTarget = 0; Uint16 Level = 0; diff --git a/MobileGL/MG_Pipe/PipeMutation.h b/MobileGL/MG_Pipe/PipeMutation.h index 04c80925..750c8c2d 100644 --- a/MobileGL/MG_Pipe/PipeMutation.h +++ b/MobileGL/MG_Pipe/PipeMutation.h @@ -312,12 +312,44 @@ namespace MobileGL::MG_Pipe { // // Entry points MGPipeTextureEmitter must provide, all taking the frontend object by // reference and returning void: - // EmitResourceCreate(ITextureObject&) / EmitResourceRespecify(ITextureObject&) + // EmitResourceCreate(ITextureObject&) + // EmitResourceRespecify(ITextureObject&, MGPipeTextureRespecifyScope, Uint32 uploadTarget, + // Uint32 level) // EmitTextureParams(ITextureObject&) // NoteLevelDirty(ITextureObject& storageOwner, Uint32 uploadTarget, Uint32 level) // EmitRenderbufferCreate(RenderbufferObject&) / EmitRenderbufferRespecify(RenderbufferObject&) void MGPipeEmitTextureResourceCreate(MG_State::GLState::ITextureObject& texture); - void MGPipeEmitTextureResourceRespecify(MG_State::GLState::ITextureObject& texture); + + // WHICH STORAGE A TEXTURE RESPECIFY REPLACES (P4a final review C-1). The applier scopes + // its pending-upload clear on this answer and not on the descriptor, because the + // descriptor cannot give it: AllocateStorage is per (uploadTarget, level) and + // TruncateMipmapLevels removes every level at or above a cut, while MGPResourceDesc + // carries only the base extent and the level count. A level the applier had ACCEPTED at + // one verb (the client's dirty flag already clear, D-D5 step 1) and that a later per-level + // definition redefined AROUND was dropped by the whole-resource arm with nobody owing its + // texels - so every respecify states its scope, and "whole resource" is said, never + // defaulted. The emitter builds wire's MGPRespecifiedLevel from the pair, packed exactly + // as the drain packs a sub-data record's Target (MGPipePackSubDataTarget), so the key it + // drops is the key that level's emission made. + enum class MGPipeTextureRespecifyScope : Uint32 { + // The whole store is redefined or restated: a format, sample-count or + // fixed-sample-locations change, an immutable allocation completing + // (SetImmutableLevels), a texture view's creation. Every pending upload goes. + WholeResource = 0, + // ONE (uploadTarget, level) was (re)allocated: glTexImage*D, glCompressedTexImage*D, + // glCopyTexImage*D, one level of a glTexStorage* loop, one level of a generated-mipmap + // grow. That level's pending upload goes; every other level's stays. `uploadTarget` and + // `level` name it. + OneLevel = 1, + // The chain was cut: every level of `uploadTarget` at or above `level` is gone and the + // levels below it are untouched (glGenerateMipmap fitting the chain, a base-level + // redefinition discarding its tail, glTexStorage* fitting the chain to its level + // count). `level` is the first level removed; a cut at 0 is the whole resource. + LevelsFrom = 2, + }; + void MGPipeEmitTextureResourceRespecify(MG_State::GLState::ITextureObject& texture, + MGPipeTextureRespecifyScope scope, Uint32 uploadTarget, + Uint32 level); void MGPipeEmitTextureParams(MG_State::GLState::ITextureObject& texture); // The DRAIN LIST's append, on a level's FIRST dirty mark, keyed on the STORAGE OWNER from // day one (D-D4: a view and its owner already share one dirty state, so an upload through diff --git a/MobileGL/MG_State/GLState/TextureState/TextureObject.cpp b/MobileGL/MG_State/GLState/TextureState/TextureObject.cpp index cbf49536..3761600f 100644 --- a/MobileGL/MG_State/GLState/TextureState/TextureObject.cpp +++ b/MobileGL/MG_State/GLState/TextureState/TextureObject.cpp @@ -66,7 +66,8 @@ namespace MobileGL { // ---- P4a's three client emission points (see TextureObject.h) ---- void TextureObjectBase::PipePublishDescriptor() { - MG_Pipe::MGPipeEmitTextureResourceRespecify(*this); + MG_Pipe::MGPipeEmitTextureResourceRespecify(*this, MG_Pipe::MGPipeTextureRespecifyScope::WholeResource, + 0, 0); // AND THE FRAMEBUFFER AGGREGATE MOVES (P4a fable seam F-3). The resource record // above is only half of what a storage definition changes: set_framebuffer_state // INLINES an attachment's InternalFormat, TextureTarget, extent, Samples and @@ -83,6 +84,24 @@ namespace MobileGL { MGP_NOTE_AGGREGATE(FramebufferAttachment); } + void TextureObjectBase::PipePublishLevelDescriptor(TextureUploadTarget uploadTarget, Uint mipmapLevel) { + // ONE level was (re)allocated: only that level's pending upload is against + // storage that is gone (P4a final review C-1). Every other level's stays. + MG_Pipe::MGPipeEmitTextureResourceRespecify(*this, MG_Pipe::MGPipeTextureRespecifyScope::OneLevel, + static_cast(uploadTarget), + static_cast(mipmapLevel)); + MGP_NOTE_AGGREGATE(FramebufferAttachment); // an attached level's extent is inlined (F-3) + } + + void TextureObjectBase::PipePublishTruncatedDescriptor(TextureUploadTarget uploadTarget, Uint levelCount) { + // The chain was cut at `levelCount`: the levels above the cut are gone with their + // pending uploads, the levels below it are untouched and keep theirs. + MG_Pipe::MGPipeEmitTextureResourceRespecify(*this, MG_Pipe::MGPipeTextureRespecifyScope::LevelsFrom, + static_cast(uploadTarget), + static_cast(levelCount)); + MGP_NOTE_AGGREGATE(FramebufferAttachment); + } + void TextureObjectBase::PipePublishParams() { MG_Pipe::MGPipeEmitTextureParams(*this); } @@ -492,8 +511,10 @@ namespace MobileGL { // storage-defining GL entry point - glTexImage*, glCompressedTexImage*, // glTexStorage*, glTextureView and the generated-mip storage grow - reaches // storage through here, which is what makes the emission complete without one call - // site per entry point in MG_Impl/GLImpl. - PipePublishDescriptor(); + // site per entry point in MG_Impl/GLImpl. AND IT NAMES THE LEVEL (final review + // C-1): this call replaced ONE level's storage, and only that level's pending + // upload may go with it. + PipePublishLevelDescriptor(uploadTarget, mipmapLevel); #endif } @@ -501,7 +522,9 @@ namespace MobileGL { BumpShapeVersion(); m_textureStorage.TruncateToLevelCount(GetIndexOfTextureUploadTarget(uploadTarget), levelCount); #if MOBILEGL_PIPE_PUSH - PipePublishDescriptor(); + // The levels at and above the cut are gone; the ones below keep their pending + // uploads (final review C-1). + PipePublishTruncatedDescriptor(uploadTarget, levelCount); #endif } diff --git a/MobileGL/MG_State/GLState/TextureState/TextureObject.h b/MobileGL/MG_State/GLState/TextureState/TextureObject.h index bd3a2d1e..6ac55c08 100644 --- a/MobileGL/MG_State/GLState/TextureState/TextureObject.h +++ b/MobileGL/MG_State/GLState/TextureState/TextureObject.h @@ -221,11 +221,23 @@ namespace MobileGL::MG_State::GLState { // members rather than free calls so the cube's, the view's and the buffer texture's // translation units keep calling an inherited helper. // - // resource_respecify. Called from BumpShapeVersion and from the three parameter + // resource_respecify, WHOLE-RESOURCE scope: the format setter and the three parameter // setters that move a DESCRIPTOR field without moving the shape (immutable levels, - // sample count, fixed sample locations). The emitter dedupes on the built descriptor, - // so an over-call costs one 88-byte compare and never an extra record. + // sample count, fixed sample locations), and a view's creation. The emitter dedupes + // this form on the built descriptor, so an over-call costs one 88-byte compare and + // never an extra record. void PipePublishDescriptor(); + // The PER-LEVEL and the CHAIN-CUT forms of the same call (P4a final review C-1). The + // applier keeps a pending-upload set per (uploadTarget, level) and drops the entries + // against the storage a respecify REPLACES - and the descriptor cannot tell it which: + // AllocateStorage is per level and TruncateMipmapLevels removes a tail, while the + // descriptor carries the base extent and the level count only. So the storage entry + // points state the scope themselves; the whole-resource form above is for the calls + // that really redefine the whole store. A per-level form is NOT deduped on the + // descriptor: a non-base level redefined at a new size moves no descriptor field, and + // the applier's box against the old level has to go regardless. + void PipePublishLevelDescriptor(TextureUploadTarget uploadTarget, Uint mipmapLevel); + void PipePublishTruncatedDescriptor(TextureUploadTarget uploadTarget, Uint levelCount); // set_texture_params, from every mutator that bumps m_textureParamsVersion. void PipePublishParams(); // The sub-data DRAIN LIST's append, on a level's first dirty mark. There is no clean diff --git a/MobileGL/MG_State/GLState/TextureState/TextureObject2DCube.cpp b/MobileGL/MG_State/GLState/TextureState/TextureObject2DCube.cpp index 00c392fd..e4b5fa3d 100644 --- a/MobileGL/MG_State/GLState/TextureState/TextureObject2DCube.cpp +++ b/MobileGL/MG_State/GLState/TextureState/TextureObject2DCube.cpp @@ -34,8 +34,9 @@ namespace MobileGL { #if MOBILEGL_PIPE_PUSH // AFTER the allocation, for TextureObjectWithOneMipmap's reason: BumpShapeVersion // runs first and a descriptor built there would describe the level set this call - // is about to change. - PipePublishDescriptor(); + // is about to change. The FACE rides in `uploadTarget`, so the key the emitter + // drops is that face's level and no other face's (final review C-1). + PipePublishLevelDescriptor(uploadTarget, mipmapLevel); #endif } @@ -43,7 +44,7 @@ namespace MobileGL { BumpShapeVersion(); m_textureStorage.TruncateToLevelCount(GetIndexOfTextureUploadTarget(uploadTarget), levelCount); #if MOBILEGL_PIPE_PUSH - PipePublishDescriptor(); + PipePublishTruncatedDescriptor(uploadTarget, levelCount); #endif } diff --git a/MobileGL/MG_Test/Pipe/TextureEmitTest.cpp b/MobileGL/MG_Test/Pipe/TextureEmitTest.cpp index 41342232..f65b4743 100644 --- a/MobileGL/MG_Test/Pipe/TextureEmitTest.cpp +++ b/MobileGL/MG_Test/Pipe/TextureEmitTest.cpp @@ -205,18 +205,21 @@ TEST(TextureEmit, TheEmitterIsOneNeverDestroyedProcessSingleton) { X(TextureEmit, ADestroyedTextureReleasesItsResourceViewAndBuiltinSamplerSlots) \ X(TextureEmit, ARenderbufferRespecifyPublishesItsExtentWithoutAVersionCounter) \ X(TextureEmit, ABailedLevelStaysDirtyAndStaysOnTheDrainList) \ - X(TextureEmit, TheApplierStoresTheRegionListTheEmitterBuiltAndNotAnEmptyOne) \ - X(TextureEmit, ARefusedUploadLeavesTheLevelDirtyAndOnTheDrainList) \ - X(TextureEmit, AnImmutableTexturesImageBindableHintReachesTheApplierAfterItsAllocation) \ - X(TextureEmit, ALodWriteOnTheBuiltinSamplerRepublishesTheParams) \ - X(TextureEmit, ATexturesBuiltinSamplerHoldsOneCacheReferenceAndSwapsItWithTheContent) \ - X(TextureEmit, ARecycledTextureSlotDoesNotInheritItsPredecessorsBindMask) \ + X(TextureEmit, TheApplierStoresTheRegionListTheEmitterBuiltAndNotAnEmptyOne) \ + X(TextureEmit, ARefusedUploadLeavesTheLevelDirtyAndOnTheDrainList) \ + X(TextureEmit, AnImmutableTexturesImageBindableHintReachesTheApplierAfterItsAllocation) \ + X(TextureEmit, ALodWriteOnTheBuiltinSamplerRepublishesTheParams) \ + X(TextureEmit, ATexturesBuiltinSamplerHoldsOneCacheReferenceAndSwapsItWithTheContent) \ + X(TextureEmit, ARecycledTextureSlotDoesNotInheritItsPredecessorsBindMask) \ X(TextureEmit, ALevelMarkedCleanIsCollectedAtTheNextDrain) \ - X(TextureEmit, WithNoBackendConsumerTheFamilyGateIsFalseAndNothingReachesTheApplier) \ - X(TextureEmit, WithTheSamplerBitClearTheTextureFamilyGateIsFalseAndNothingReachesTheApplier) \ - X(TextureEmit, \ - WithTheBufferResourceBitClearTheTextureFamilyGateIsFalseAndNothingReachesTheApplier) \ - X(TextureEmit, EveryDKTwoDependencyRowGatesItsOwnFamilyAndTheMirrorPairsStayLive) + X(TextureEmit, WithNoBackendConsumerTheFamilyGateIsFalseAndNothingReachesTheApplier) \ + X(TextureEmit, WithTheSamplerBitClearTheTextureFamilyGateIsFalseAndNothingReachesTheApplier) \ + X(TextureEmit, \ + WithTheBufferResourceBitClearTheTextureFamilyGateIsFalseAndNothingReachesTheApplier) \ + X(TextureEmit, EveryDKTwoDependencyRowGatesItsOwnFamilyAndTheMirrorPairsStayLive) \ + X(TextureEmit, ALevelDefinedAfterAnEmittedButUnconsumedUploadKeepsThatUpload) \ + X(TextureEmit, AChainTruncationKeepsTheSurvivingLevelsPendingUploads) \ + X(TextureEmit, ARedefinitionOfANonBaseLevelAtANewSizeDropsOnlyThatLevelsPendingUpload) #define MGL_DECLARE_PULL_SKIP(Suite, Name) \ TEST(Suite, Name) { GTEST_SKIP() << "compiled only under MOBILEGL_PIPE_PUSH"; } @@ -1275,6 +1278,118 @@ TEST(TextureEmit, ALevelMarkedCleanIsCollectedAtTheNextDrain) { EXPECT_EQ(Textures().DrainListSize(), 1u) << "a re-dirtied level did not go back on the drain list, so its texels are owed for ever"; } +// ============================ final review C-1 ============================ +// +// THE CLIENT PASSES THE LEVEL IT REDEFINES. AllocateStorage is per (uploadTarget, level) while +// the descriptor carries only the base extent and the level count, so only the caller can tell +// the applier WHICH storage a respecify replaces (wire C1's MGPRespecifiedLevel); before the fix +// every texture respecify took the whole-resource arm and dropped every pending upload of the +// texture - including a level the applier had already accepted and whose client flag was +// therefore already clear (D-D5 step 1). Driven through the real AllocateStorage. +TEST(TextureEmit, ALevelDefinedAfterAnEmittedButUnconsumedUploadKeepsThatUpload) { + TextureScope scope; + const auto texture = MakeShared(90); + texture->SetInternalFormat(TextureInternalFormat::RGBA8); + // glTexImage2D(level 0, data) + texture->AllocateStorage(TextureUploadTarget::Texture2D, 0, MipmapInput{IntVec3{64, 64, 1}, 64 * 64 * 4}); + texture->MarkStorageDirtyRegion(TextureUploadTarget::Texture2D, 0, IntVec3{0, 0, 0}, IntVec3{64, 64, 1}); + // A verb the texture is not reached by: the drain emits level 0, the applier accepts, the + // client clears its flag. Nothing has consumed the entry. + Textures().DrainTextureSubData(Ctx()); + const MGPipeHandle handle = Textures().FindTexture(*texture); + const MGPipeResourceRecord* record = AppliedTexture(handle); + ASSERT_NE(record, nullptr); + ASSERT_EQ(Textures().RefusedSubDataCount(), 0u); + ASSERT_EQ(record->PendingUploads.size(), 1u); + ASSERT_FALSE(texture->IsStorageDirty(TextureUploadTarget::Texture2D, 0)); + // glTexImage2D(level 1, data): a DIFFERENT level. + texture->AllocateStorage(TextureUploadTarget::Texture2D, 1, MipmapInput{IntVec3{32, 32, 1}, 32 * 32 * 4}); + texture->MarkStorageDirtyRegion(TextureUploadTarget::Texture2D, 1, IntVec3{0, 0, 0}, IntVec3{32, 32, 1}); + record = AppliedTexture(handle); + ASSERT_NE(record, nullptr); + Bool levelZeroPending = false; + for (const auto& pending : record->PendingUploads) { + if (pending.Level == 0) levelZeroPending = true; + } + EXPECT_TRUE(levelZeroPending) + << "defining level 1 dropped level 0's accepted-but-unconsumed pending upload (PendingUploads.size()=" + << record->PendingUploads.size() << ") while level 0's client dirty flag is " + << (texture->IsStorageDirty(TextureUploadTarget::Texture2D, 0) ? "set" : "CLEAR - the texels are owed by nobody"); + // And after the next drain both levels stand in the set. + Textures().DrainTextureSubData(Ctx()); + record = AppliedTexture(handle); + ASSERT_NE(record, nullptr); + Bool zeroAfter = false; + Bool oneAfter = false; + for (const auto& pending : record->PendingUploads) { + if (pending.Level == 0) zeroAfter = true; + if (pending.Level == 1) oneAfter = true; + } + EXPECT_TRUE(oneAfter); + EXPECT_TRUE(zeroAfter) << "level 0's texels are lost: not pending, flag clear"; +} + +// A chain truncation - glGenerateMipmap fitting the chain, a base redefinition discarding its +// tail - removes the levels at and above the cut and nothing below it. Before the fix it was a +// whole-resource respecify and took level 0's standing upload with the tail. +TEST(TextureEmit, AChainTruncationKeepsTheSurvivingLevelsPendingUploads) { + TextureScope scope; + const auto texture = MakeTexture2D(93, 64, /*levels=*/3); + texture->MarkStorageDirtyRegion(TextureUploadTarget::Texture2D, 0, IntVec3{0, 0, 0}, IntVec3{64, 64, 1}); + texture->MarkStorageDirtyRegion(TextureUploadTarget::Texture2D, 2, IntVec3{0, 0, 0}, IntVec3{16, 16, 1}); + Textures().DrainTextureSubData(Ctx()); + const MGPipeHandle handle = Textures().FindTexture(*texture); + const MGPipeResourceRecord* record = AppliedTexture(handle); + ASSERT_NE(record, nullptr); + ASSERT_EQ(record->PendingUploads.size(), 2u); + ASSERT_FALSE(texture->IsStorageDirty(TextureUploadTarget::Texture2D, 0)); + + texture->TruncateMipmapLevels(TextureUploadTarget::Texture2D, 1); + record = AppliedTexture(handle); + ASSERT_NE(record, nullptr); + EXPECT_EQ(record->Desc.Levels, 1u); + Bool zeroPending = false; + Bool twoPending = false; + for (const auto& pending : record->PendingUploads) { + if (pending.Level == 0) zeroPending = true; + if (pending.Level == 2) twoPending = true; + } + EXPECT_TRUE(zeroPending) << "truncating the chain above level 0 dropped level 0's standing upload"; + EXPECT_FALSE(twoPending) << "a level the truncation removed kept a pending upload against storage that is gone"; +} + +// A non-base level redefined at a new size moves NO descriptor field (the descriptor carries +// the base extent and the level count), so the emitter's descriptor dedupe used to swallow the +// respecify and the applier kept a box sized for the OLD level - which Espryt would have +// uploaded past the end of the new one. A per-level respecify reaches the applier whether or +// not the descriptor moved, and drops exactly that level. +TEST(TextureEmit, ARedefinitionOfANonBaseLevelAtANewSizeDropsOnlyThatLevelsPendingUpload) { + TextureScope scope; + const auto texture = MakeTexture2D(96, 16, /*levels=*/2); + texture->MarkStorageDirtyRegion(TextureUploadTarget::Texture2D, 0, IntVec3{0, 0, 0}, IntVec3{16, 16, 1}); + texture->MarkStorageDirtyRegion(TextureUploadTarget::Texture2D, 1, IntVec3{0, 0, 0}, IntVec3{8, 8, 1}); + Textures().DrainTextureSubData(Ctx()); + const MGPipeHandle handle = Textures().FindTexture(*texture); + const MGPipeResourceRecord* record = AppliedTexture(handle); + ASSERT_NE(record, nullptr); + ASSERT_EQ(record->PendingUploads.size(), 2u); + const Uint64 serialBefore = record->Serial; + + // glTexImage2D(level 1) at 4x4: the base is still 16x16 and the chain still two levels. + texture->AllocateStorage(TextureUploadTarget::Texture2D, 1, MipmapInput{IntVec3{4, 4, 1}, 4 * 4 * 4}); + record = AppliedTexture(handle); + ASSERT_NE(record, nullptr); + EXPECT_GT(record->Serial, serialBefore) << "the per-level respecify never reached the applier"; + Bool zeroPending = false; + Bool onePending = false; + for (const auto& pending : record->PendingUploads) { + if (pending.Level == 0) zeroPending = true; + if (pending.Level == 1) onePending = true; + } + EXPECT_TRUE(zeroPending) << "redefining level 1 dropped level 0's standing upload"; + EXPECT_FALSE(onePending) << "level 1's 8x8 box survived its redefinition onto a 4x4 level"; +} + #endif // MOBILEGL_PIPE_PUSH // ========================================================================================= @@ -1902,7 +2017,7 @@ TEST(TextureEmit, ARespecifyThatRedefinesNoStorageCarriesTheStickyMaskAndKeepsTh // glTexStorage2D: an IMMUTABLE store, which is the whole reason this arm exists. MGPResourceDesc allocated = TextureDesc(texture, 64, 151); allocated.Immutable = 1; - allocated.Levels = 1; + allocated.Levels = 2; // level 1 exists for the named-level clause below allocated.InternalFormat = 0x8058u; // GL_RGBA8 allocated.BindMask = static_cast(kMGPipeBindSampler); ASSERT_TRUE(MGPipeApplyResourceRespecify(allocated, nullptr)); @@ -1931,17 +2046,29 @@ TEST(TextureEmit, ARespecifyThatRedefinesNoStorageCarriesTheStickyMaskAndKeepsTh << "the serial is the whole publication of a metadata update - the twin re-derives its " "storage flags from the new mask on the strength of it"; - // AND THE LEVEL POINTER DOES NOT CHANGE THE ANSWER. This is where ID-18 M4 refines C1: - // C1's rule drops the uploads against the storage a call REPLACES, and a call that replaces - // no storage replaces no level's coordinate system either, whatever level it names. - const MGPRespecifiedLevel levelZero{kTex2D, 0}; + // A SECOND MASK MOVE WITH A NULL LEVEL STILL DROPS NOTHING - the client's mask republish + // passes null on purpose (wire-v3 §5 item 6) and this is its shape. MGPResourceDesc maskedAgain = masked; maskedAgain.BindMask = static_cast(masked.BindMask | kMGPipeBindRenderTarget); - ASSERT_TRUE(MGPipeApplyResourceRespecify(maskedAgain, nullptr, &levelZero)); + ASSERT_TRUE(MGPipeApplyResourceRespecify(maskedAgain, nullptr, nullptr)); ASSERT_EQ(TextureRecordOf(11).PendingUploads.size(), 1u) - << "a metadata update dropped the level it named"; + << "a metadata update with no level dropped a standing upload"; EXPECT_EQ(TextureRecordOf(11).Desc.BindMask, maskedAgain.BindMask); + // BUT A NAMED LEVEL IS DROPPED WHETHER OR NOT THE DESCRIPTOR MOVED (P4a final review C-1, + // refining the W11 clause that stood here): the pointer is the caller's statement that it + // reallocated that level, and the descriptor cannot contradict it - a non-base level + // redefined at a new size moves no descriptor field, so "identical storage fields" says + // nothing about that level's coordinate system. Level 1's entry goes; level 0's stays. + ASSERT_TRUE(MGPipeApplyResourceSubData(TextureUpload(texture, 1, MGPBox{0, 0, 0, 32, 32, 1}, 0), texels)); + ASSERT_EQ(TextureRecordOf(11).PendingUploads.size(), 2u); + const MGPRespecifiedLevel levelOne{kTex2D, 1}; + ASSERT_TRUE(MGPipeApplyResourceRespecify(maskedAgain, nullptr, &levelOne)); + ASSERT_EQ(TextureRecordOf(11).PendingUploads.size(), 1u) + << "a level-scoped respecify on an unchanged descriptor did not drop the level it named"; + EXPECT_EQ(TextureRecordOf(11).PendingUploads[0].Level, 0u) << "it dropped the wrong level"; + const MGPRespecifiedLevel levelZero{kTex2D, 0}; + // THE NEGATIVE CONTROL, in the same case: move ONE storage-defining field and the same call // is a redefinition again, which takes the level it names with it. MGPResourceDesc reallocated = maskedAgain;