diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 64f0303c..444833bb 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -2085,8 +2085,17 @@ namespace MobileGL::MG_Backend::DirectGLES { // the entry point because that one must never trigger a build (see // ShaderStorageBlockBinding below). The signature is over the values, so an // application that re-sets the same bindings every frame rebuilds nothing. + // + // The image-unit generation is a third of the same shape, and it used to be + // carried by accident: glUniform1i on an image uniform bumped the program's backend + // state version, which was in the program-pipeline composite's cache key, so a + // pipeline draw got a whole NEW composite object and therefore a fresh twin. Keying + // that cache on the link version instead (ProgramPipelineObject) removed the + // accident - and it never covered the monolithic glUseProgram path at all - so the + // dependency is stated here instead. if (!twin->GetBackendProgramId() || twin->GetSyncedLinkVersion() != currentProgram->GetLinkVersion() || + twin->GetSyncedImageUnitVersion() != currentProgram->GetImageUnitVersion() || twin->GetSnormFallbackClampOutputMask() != g_snormFallbackClampOutputMask || twin->GetUnormFallbackClampOutputMask() != g_unormFallbackClampOutputMask || twin->GetFragColorBroadcastCount() != g_fragColorBroadcastCount || diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index f6f3f0ce..2daf716d 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -4537,6 +4537,7 @@ namespace MobileGL::MG_Backend::DirectGLES { // driver's linked interface. ReseedShaderStorageBlockBindings(m_backendProgramId, *stateProgramObject); m_syncedLinkVersion = stateProgramObject->GetLinkVersion(); + m_syncedImageUnitVersion = stateProgramObject->GetImageUnitVersion(); m_isInitialized = true; MGLOG_D("Program sync completed. backend ID %u", m_backendProgramId); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 6cc9b369..594f8cc7 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -1048,6 +1048,13 @@ namespace MobileGL::MG_Backend::DirectGLES { // Frontend link version this backend program (and its resource caches) was // built from; a mismatch means every link-derived cache here is stale. Uint32 GetSyncedLinkVersion() const { return m_syncedLinkVersion; } + // Image-uniform unit generation this backend program was GENERATED against. + // Separate from the link version because it is not link state: ES forbids + // glUniform1i on an image uniform, so RebindImageUniformsToFrontendUnits bakes the + // unit into the ESSL, and a program built before glUniform1i moved that unit is as + // stale as one built before a relink - while the sampler half, which really is + // re-issued per draw, needs nothing of the sort. + Uint32 GetSyncedImageUnitVersion() const { return m_syncedImageUnitVersion; } private: void CacheResourceLocations(const SharedPtr& stateProgramObject); @@ -1079,6 +1086,7 @@ namespace MobileGL::MG_Backend::DirectGLES { Uint32 m_lastUploadedGlobalUboVersion = ~0u; BufferImpl::UboRingAllocation m_globalUboRingAllocation; Uint32 m_syncedLinkVersion = ~0u; + Uint32 m_syncedImageUnitVersion = ~0u; SamplerPassMemo m_samplerPassMemo; }; diff --git a/MobileGL/MG_State/GLState/Core.cpp b/MobileGL/MG_State/GLState/Core.cpp index 90508c5a..d4b4852b 100644 --- a/MobileGL/MG_State/GLState/Core.cpp +++ b/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& 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(source.GetUBOData()); char* destinationUbo = static_cast(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& writtenIndices = source.GetWrittenUniformIndices(); + const Uint uniformCount = source.GetUniformCount(); + const SizeT indexCount = byWriteSet ? writtenIndices.size() : static_cast(uniformCount); + if (indexCount == 0) return; + + for (SizeT slot = 0; slot < indexCount; ++slot) { + const Uint index = byWriteSet ? writtenIndices[slot] : static_cast(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(sourceLocation))) continue; + if (byWriteSet && !source.IsUniformWrittenAtLocation(static_cast(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) || diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index 5ebed93b..da40d644 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/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(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(artifacts.maxUniformLocation) / 64u + 1u, 0u); - if (locationWord >= artifacts.writtenUniformLocationBits.size()) return; + std::max(locationWord + 1u, static_cast(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(glIndex) / 64u; if (indexWord >= artifacts.writtenUniformIndexBits.size()) { artifacts.writtenUniformIndexBits.resize( - static_cast(artifacts.activeUniformCount) / 64u + 1u, 0u); - if (indexWord >= artifacts.writtenUniformIndexBits.size()) return; + std::max(indexWord + 1u, static_cast(artifacts.activeUniformCount) / 64u + 1u), 0u); } const Uint64 indexBit = Uint64{1} << (static_cast(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 diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h index 491419de..981b55ac 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h @@ -129,10 +129,15 @@ namespace MobileGL { if (!program) continue; versions[stage * 2] = (static_cast(program->GetBackendStateVersion()) << 32) | static_cast(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(program->GetBlockBindingVersion()) << 32) | + static_cast(program->GetUniformWriteSetVersion()); } return versions; } diff --git a/MobileGL/MG_Test/Program/ProgramPipelineCompositeTest.cpp b/MobileGL/MG_Test/Program/ProgramPipelineCompositeTest.cpp index c7a979a7..0d8e065a 100644 --- a/MobileGL/MG_Test/Program/ProgramPipelineCompositeTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramPipelineCompositeTest.cpp @@ -208,6 +208,87 @@ TEST_F(ProgramPipelineCompositeTest, WhenBothStagesWereWrittenTheLastGraphicsSta DeleteProgramPipelines(1, &pipeline); } +// The both-written tie again, through the case that has no BYTES to move: the fragment stage +// writes the value it was already holding. +// +// The refresh gate is built out of counters that move when bytes move (the UBO content +// version, the backend state version), and both write funnels drop a value-identical write +// before bumping either. So this write enlarges the write SET - it makes the fragment stage +// the last written-to stage for `u_shared`, which is what decides the slot - while moving +// nothing else. Without a generation on the set itself the gate never trips and the draw keeps +// the vertex stage's value. +TEST_F(ProgramPipelineCompositeTest, AValueIdenticalWriteStillTakesTheSlotForItsStage) { + const GLuint vs = MakeSeparableProgram(GL_VERTEX_SHADER, kSharedUniformVs); + const GLuint fs = MakeSeparableProgram(GL_FRAGMENT_SHADER, kSharedUniformFs); + + GLuint pipeline = 0; + GenProgramPipelines(1, &pipeline); + BindProgramPipeline(pipeline); + UseProgramStages(pipeline, GL_VERTEX_SHADER_BIT, vs); + UseProgramStages(pipeline, GL_FRAGMENT_SHADER_BIT, fs); + + const float fromVs[4] = {5.0f, 5.0f, 5.0f, 5.0f}; + ProgramUniform4fv(vs, GetUniformLocation(vs, "u_shared"), 1, fromVs); + ASSERT_EQ(ReadVec4(*DrawProgram(), "u_shared"), (std::vector{5.0f, 5.0f, 5.0f, 5.0f})); + + // The fragment program's u_shared already reads all-zero, so this write changes not one + // byte of its shadow - and must still hand it the composite's slot. + const float zeros[4] = {0.0f, 0.0f, 0.0f, 0.0f}; + ProgramUniform4fv(fs, GetUniformLocation(fs, "u_shared"), 1, zeros); + ASSERT_EQ(GetError(), GL_NO_ERROR); + EXPECT_EQ(ReadVec4(*DrawProgram(), "u_shared"), (std::vector{0.0f, 0.0f, 0.0f, 0.0f})) + << "a write that moved no bytes never reached the refresh gate"; + + BindProgramPipeline(0); + DeleteProgramPipelines(1, &pipeline); +} + +// glUseProgramStages here accepts a program that was never linked as separable (GL 4.6 core 7.4 +// says it should not, and MobileGL validates only LINK_STATUS). Such a program has recorded +// none of its writes, because nothing ever armed its tracking latch - so the mirror has to fall +// back to carrying everything rather than carrying nothing. Mirroring nothing would have been a +// fresh regression on a shape that worked before the dirty set existed. +TEST_F(ProgramPipelineCompositeTest, ANonSeparableStageProgramStillMirrorsItsUniforms) { + const char* vsSource = R"(#version 430 core +uniform vec4 u_vsOnly; +void main() { gl_Position = u_vsOnly; } +)"; + const GLuint shader = CreateShader(GL_VERTEX_SHADER); + ShaderSource(shader, 1, &vsSource, nullptr); + CompileShader(shader); + const GLuint vs = CreateProgram(); + // Deliberately NO ProgramParameteri(GL_PROGRAM_SEPARABLE): this is the shape the latch + // cannot see coming. + AttachShader(vs, shader); + LinkProgram(vs); + GLint linked = GL_FALSE; + GetProgramiv(vs, GL_LINK_STATUS, &linked); + ASSERT_EQ(linked, GL_TRUE); + + const GLuint fs = MakeSeparableProgram(GL_FRAGMENT_SHADER, kSharedUniformFs); + + GLuint pipeline = 0; + GenProgramPipelines(1, &pipeline); + BindProgramPipeline(pipeline); + UseProgramStages(pipeline, GL_VERTEX_SHADER_BIT, vs); + UseProgramStages(pipeline, GL_FRAGMENT_SHADER_BIT, fs); + ASSERT_EQ(GetError(), GL_NO_ERROR); + + const float written[4] = {3.0f, 1.0f, 4.0f, 1.0f}; + ProgramUniform4fv(vs, GetUniformLocation(vs, "u_vsOnly"), 1, written); + ASSERT_EQ(GetError(), GL_NO_ERROR); + + const auto composite = DrawProgram(); + ASSERT_NE(composite, nullptr); + EXPECT_FALSE(MG_State::pGLContext->GetProgramObject(vs)->TracksUniformWrites()) + << "this case is only meaningful while the stage program records nothing"; + EXPECT_EQ(ReadVec4(*composite, "u_vsOnly"), (std::vector{3.0f, 1.0f, 4.0f, 1.0f})) + << "a stage program with no write record must fall back to mirroring everything"; + + BindProgramPipeline(0); + DeleteProgramPipelines(1, &pipeline); +} + // glProgramUniform* addresses a program by NAME and needs neither a current program nor an // active shader program, so it is a write path that never touches the pipeline at all. It has // to record the write exactly like glUniform* does.