[Fix, Test] (MG_State, MG_Backend/DirectGLES, MG_Test): close three holes review found in the composite uniform mirror - unrecorded stage programs, byte-identical writes, and Espryt's baked image units

MobileGL/MG_State/GLState/Core.cpp

@@ -414,19 +414,39 @@ namespace MobileGL::MG_State {
         // unwritten uniform reads.
         static void MirrorUniformValues(ProgramObject& source, ProgramObject& destination) {
             if (!source.GetLinkStatus() || !destination.GetLinkStatus()) return;
-            // O(uniforms written), not O(uniforms declared). The two name lookups below are
-            // string hashes into both programs' location maps, and doing them for every active
-            // uniform of every stage on every gate trip was hundreds of them per draw on a
-            // large program. A stage nothing has been written to costs one empty() test.
-            const Vector<Uint>& writtenIndices = source.GetWrittenUniformIndices();
-            if (writtenIndices.empty()) return;
 
+            // Settle both sides' phase B BEFORE taking a reference into `source`'s artifacts
+            // below: these four getters are the join gate, and a join runs the phase-B publish.
+            // Nothing that publish does marks a uniform today, but the loop holds a reference to
+            // a Vector that a mark would push_back to, and "the replay does not mark" is not a
+            // property a future reader of this line can see.
             const char* sourceUbo = static_cast<const char*>(source.GetUBOData());
             char* destinationUbo = static_cast<char*>(destination.MapUBO());
             const SizeT sourceUboSize = source.GetUBOSize();
             const SizeT destinationUboSize = destination.GetUBOSize();
 
-            for (const Uint index : writtenIndices) {
+            // O(uniforms written), not O(uniforms declared). The two name lookups below are
+            // string hashes into both programs' location maps, and doing them for every active
+            // uniform of every stage on every gate trip was hundreds of them per draw on a
+            // large program. A stage nothing has been written to costs one empty() test.
+            //
+            // FALLBACK, and it is load-bearing rather than defensive: a program only records
+            // its writes once something asks it to be separable (ProgramObject::SetSeparable
+            // arms the latch), but glUseProgramStages here validates only LINK_STATUS - it does
+            // not reject a program that was never linked as separable, which GL 4.6 core 7.4
+            // says it should. So a plain glCreateProgram/glLinkProgram program CAN be installed
+            // as a stage, and it will have recorded nothing at all. Mirroring "only what was
+            // written" would then mirror nothing and paint the composite's defaults - a fresh
+            // regression on a shape that worked. For such a program the old full walk is exactly
+            // right: it has no dirty set to be more precise with.
+            const Bool byWriteSet = source.TracksUniformWrites();
+            const Vector<Uint>& writtenIndices = source.GetWrittenUniformIndices();
+            const Uint uniformCount = source.GetUniformCount();
+            const SizeT indexCount = byWriteSet ? writtenIndices.size() : static_cast<SizeT>(uniformCount);
+            if (indexCount == 0) return;
+
+            for (SizeT slot = 0; slot < indexCount; ++slot) {
+                const Uint index = byWriteSet ? writtenIndices[slot] : static_cast<Uint>(slot);
                 const String& name = source.GetActiveUniformName(index);
                 if (name.empty()) continue;
                 const Int sourceBase = source.GetUniformLocation(name);
@@ -447,7 +467,9 @@ namespace MobileGL::MG_State {
                     // Per ELEMENT, not per array: `arr[3] = x` must carry element 3 and leave
                     // the elements another stage owns alone. `continue`, not `break` - the
                     // written elements of an array need not be a prefix of it.
-                    if (!source.IsUniformWrittenAtLocation(static_cast<Uint>(sourceLocation))) continue;
+                    if (byWriteSet && !source.IsUniformWrittenAtLocation(static_cast<Uint>(sourceLocation))) {
+                        continue;
+                    }
                     // Stop at the end of EITHER side's array rather than walking onto the
                     // neighbouring uniform of whichever program has the shorter one.
                     if (!source.UniformLocationsAliasSameUniform(sourceBase, sourceLocation) ||

MobileGL/MG_State/GLState/ProgramState/ProgramObject.h

@@ -369,6 +369,16 @@ namespace MobileGL::MG_State::GLState {
         // the value the application wrote for `f` in another stage.
         Bool TracksUniformWrites() const { return m_tracksUniformWrites; }
 
+        // Generation of the write SET itself, as distinct from the values in it. The refresh
+        // gate (ProgramPipelineObject::ComputeUniformMirrorVersions) is otherwise built out of
+        // counters that only move when BYTES move - and a write can enlarge the set without
+        // moving a byte, because both write funnels drop a value-identical write before
+        // bumping anything. glProgramUniform1f(fs, f, 0.0f) on an `f` that already reads 0.0
+        // is exactly that: it makes the FRAGMENT stage the last written-to stage for `f`, so
+        // the composite must be re-mirrored to hand it the slot, and nothing else in the gate
+        // would have noticed.
+        Uint32 GetUniformWriteSetVersion() const { return m_uniformWriteSetVersion; }
+
         // Records that `location` has been written since the last link. Cheap and idempotent;
         // a no-op on a program that can never be a pipeline stage.
         void MarkUniformWrittenAtLocation(Uint location) {
@@ -376,13 +386,23 @@ namespace MobileGL::MG_State::GLState {
             LinkArtifacts& artifacts = Artifacts();
             if (!IsValidUniformLocation(artifacts, static_cast<Int>(location))) return;
 
+            // Sized to cover this location AND the whole location space, so a program whose
+            // highest location is written first does not reallocate on every later write, and
+            // so the subscript below needs no second guard: the vector provably contains it.
             const SizeT locationWord = location / 64u;
             if (locationWord >= artifacts.writtenUniformLocationBits.size()) {
                 artifacts.writtenUniformLocationBits.resize(
-                    static_cast<SizeT>(artifacts.maxUniformLocation) / 64u + 1u, 0u);
-                if (locationWord >= artifacts.writtenUniformLocationBits.size()) return;
+                    std::max<SizeT>(locationWord + 1u, static_cast<SizeT>(artifacts.maxUniformLocation) / 64u + 1u),
+                    0u);
+            }
+            const Uint64 locationBit = Uint64{1} << (location % 64u);
+            if ((artifacts.writtenUniformLocationBits[locationWord] & locationBit) == 0) {
+                artifacts.writtenUniformLocationBits[locationWord] |= locationBit;
+                // Only on the 0 -> 1 transition: a re-write of a location already in the set
+                // changes nothing the mirror would do differently, and moving the version for
+                // it would re-walk the set on every repeated glUniform* call.
+                ++m_uniformWriteSetVersion;
             }
-            artifacts.writtenUniformLocationBits[locationWord] |= Uint64{1} << (location % 64u);
 
             // Add the owning GL active-uniform index to the compact list, once.
             const Int tIndex = artifacts.uniformIndexInTProgram[location];
@@ -394,8 +414,7 @@ namespace MobileGL::MG_State::GLState {
             const SizeT indexWord = static_cast<SizeT>(glIndex) / 64u;
             if (indexWord >= artifacts.writtenUniformIndexBits.size()) {
                 artifacts.writtenUniformIndexBits.resize(
-                    static_cast<SizeT>(artifacts.activeUniformCount) / 64u + 1u, 0u);
-                if (indexWord >= artifacts.writtenUniformIndexBits.size()) return;
+                    std::max<SizeT>(indexWord + 1u, static_cast<SizeT>(artifacts.activeUniformCount) / 64u + 1u), 0u);
             }
             const Uint64 indexBit = Uint64{1} << (static_cast<SizeT>(glIndex) % 64u);
             if ((artifacts.writtenUniformIndexBits[indexWord] & indexBit) != 0) return;
@@ -573,8 +592,24 @@ namespace MobileGL::MG_State::GLState {
             if (Artifacts().uniformSamplerOrImageUnitIndex[location] == unit) return;
             Artifacts().uniformSamplerOrImageUnitIndex[location] = unit;
             ++m_backendStateVersion;
+            // IMAGE units get their own generation, and it is not redundant with the one
+            // above. A sampler unit is re-issued to the driver per draw as a plain
+            // glUniform1i, so a backend can honour a change without rebuilding anything; an
+            // image unit cannot be, because ES forbids glUniform1i on image uniforms - Espryt
+            // has to BAKE it into the ESSL it generates (RebindImageUniformsToFrontendUnits),
+            // which means the change is only honoured by regenerating the program. That
+            // regeneration is gated on link-shaped versions, so without a counter that moves
+            // here the new unit would never reach the driver.
+            if (const glslang::TType* type = GetUniformTType(location); type != nullptr && type->isImage()) {
+                ++m_imageUnitVersion;
+            }
         }
 
+        // Generation of the image-uniform unit assignment; see SetUniformSamplerOrImageUnitIndex.
+        // A backend that compiles the unit into its program source compares this to decide
+        // whether what it built is still describing the right binding.
+        Uint32 GetImageUnitVersion() const { return m_imageUnitVersion; }
+
         Int GetUniformSamplerOrImageUnitIndex(Uint location) const {
             return Artifacts().uniformSamplerOrImageUnitIndex[location];
         }
@@ -1197,6 +1232,11 @@ namespace MobileGL::MG_State::GLState {
         // it is a latch and not just m_separable. Outside LinkArtifacts on purpose: a relink
         // clears the write SET, but a program that was separable is still separable after it.
         Bool m_tracksUniformWrites = false;
+        // Generation counters that must NOT be reset by a link, for the same reason the memo
+        // versions above are not: a reader compares them for INEQUALITY, so a reset could make
+        // a stale cache compare equal to a fresh program. See their getters.
+        Uint32 m_uniformWriteSetVersion = 0;
+        Uint32 m_imageUnitVersion = 0;
         Bool m_validateStatus = true;
         // Mutable, like m_artifacts and for the same reason: publishing a pending link is a
         // READ-side operation (the first gated getter is what pulls the result in), and the

MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h

@@ -129,10 +129,15 @@ namespace MobileGL {
                         if (!program) continue;
                         versions[stage * 2] = (static_cast<Uint64>(program->GetBackendStateVersion()) << 32) |
                                               static_cast<Uint64>(program->GetUBOContentVersion());
-                        // Its own slot rather than folded into the pair above: the storage-block
-                        // setter moves this and NOTHING else, so a rebinding would otherwise be
-                        // invisible to the refresh gate.
-                        versions[stage * 2 + 1] = program->GetBlockBindingVersion();
+                        // Their own slot rather than folded into the pair above: the
+                        // storage-block setter moves the block-binding version and NOTHING
+                        // else, so a rebinding would otherwise be invisible to the refresh
+                        // gate - and the write-set version is the only counter that moves for
+                        // a write which ENLARGES the set without changing a byte (see
+                        // ProgramObject::GetUniformWriteSetVersion), which is what decides
+                        // which stage owns a shared name.
+                        versions[stage * 2 + 1] = (static_cast<Uint64>(program->GetBlockBindingVersion()) << 32) |
+                                                  static_cast<Uint64>(program->GetUniformWriteSetVersion());
                     }
                     return versions;
                 }