From 94e75fef79bd178f14c4bda3f0ed266d808805c2 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 11 Aug 2026 20:05:46 -0400 Subject: [PATCH] [Fix, Test] (MG_State, MG_Impl): glIsProgramPipeline answers for the first bind, not for the materialization every pipeline command now does --- .../MG_Impl/GLImpl/Drawing/GL_Drawing.cpp | 8 +++- .../Scenarios/ProgramPipelineScenario.cpp | 41 +++++++++++++++++++ MobileGL/MG_State/GLState/Core.cpp | 33 +++++++++++++-- MobileGL/MG_State/GLState/Core.h | 6 ++- .../ProgramState/ProgramPipelineObject.h | 24 ++++++++++- MobileGL/MG_Test/Program/AsyncLinkTest.cpp | 4 +- 6 files changed, 106 insertions(+), 10 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp index 493b5520..3ef42aff 100644 --- a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp +++ b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp @@ -674,6 +674,12 @@ namespace MobileGL::MG_Impl::GLImpl { // dereferences GetProgramForDraw() unconditionally, so "no current program" has to be // stopped here or it is a null dereference rather than the INVALID_OPERATION the spec // asks for - reachable through a bound pipeline that supplies no graphics stage. + // + // AFTER the argument checks, unlike the sibling draw entry points, and deliberately: + // the argument rules here are properties of the call rather than of GL state, and + // NegativeApiErrorsTest.IndirectParameterDrawsCheckBothBuffers pins the INVALID_VALUE + // they produce for a call made with no program bound. Same precedence decision, and + // the same reason, as DispatchComputeIndirect above. if (!ValidateCurrentProgramForExecution(__func__)) return; auto multiDrawElementsIndirectCount = MG_Backend::gBackendFunctionsTable.GL.MultiDrawElementsIndirectCount; if (!multiDrawElementsIndirectCount) { @@ -694,7 +700,7 @@ namespace MobileGL::MG_Impl::GLImpl { 4 * sizeof(Uint32), __func__)) { return; } - // See MultiDrawElementsIndirectCount. + // See MultiDrawElementsIndirectCount, including why this one goes last. if (!ValidateCurrentProgramForExecution(__func__)) return; auto multiDrawArraysIndirectCount = MG_Backend::gBackendFunctionsTable.GL.MultiDrawArraysIndirectCount; if (!multiDrawArraysIndirectCount) { diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ProgramPipelineScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ProgramPipelineScenario.cpp index 40d2379d..752212c9 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ProgramPipelineScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ProgramPipelineScenario.cpp @@ -133,6 +133,47 @@ void main() { gl_Position = i_position; } } // namespace + // The root cause of the cluster, stated as the two halves it actually has. + // + // Half one: glGenProgramPipelines only reserves a name, and every pipeline command used to + // demand a materialized object - so the spec's own call order (stages attached BEFORE the + // first bind, GL 4.6 core 7.4) was rejected with GL_INVALID_OPERATION and the stages were + // never recorded. Half two is the trap that fix walks into: the object now appears the + // moment anything needs somewhere to put state, so "the object exists" stops being the + // right answer for glIsProgramPipeline, which the spec ties to the first BIND. A pure + // query must not turn a reserved name into a program pipeline either. + TEST_F(ProgramPipelineScenario, AReservedNameTakesStateBeforeItIsAProgramPipeline) { + if (!Ready()) return; + + const GLuint vs = MakeSeparable(GL_VERTEX_SHADER, kSeparableVS); + if (vs == 0) return; + const GLuint pipeline = MakePipeline(); + ASSERT_NE(pipeline, 0u); + EXPECT_EQ(glIsProgramPipeline(pipeline), GL_FALSE) << "a merely reserved name is not a pipeline yet"; + + // A query answers out of default state - and leaves the name exactly as it found it. + GLint validateStatus = -1; + glGetProgramPipelineiv(pipeline, GL_VALIDATE_STATUS, &validateStatus); + EXPECT_EQ(FirstGLError(), 0u) << "querying a reserved pipeline name must not be an error"; + EXPECT_EQ(validateStatus, 0) << "a pipeline that was never validated reports VALIDATE_STATUS 0"; + EXPECT_EQ(glIsProgramPipeline(pipeline), GL_FALSE) << "a pure query must not create the object"; + + // ...and glUseProgramStages RECORDS the stage on the reserved name rather than + // rejecting it, which is the whole defect: without this the pipeline stayed empty. + glUseProgramStages(pipeline, GL_VERTEX_SHADER_BIT, vs); + EXPECT_EQ(FirstGLError(), 0u) << "glUseProgramStages before the first bind must be accepted"; + GLint stageProgram = 0; + glGetProgramPipelineiv(pipeline, GL_VERTEX_SHADER, &stageProgram); + EXPECT_EQ(static_cast(stageProgram), vs) << "the stage program was not recorded"; + EXPECT_EQ(glIsProgramPipeline(pipeline), GL_FALSE) << "taking state is still not being bound"; + + // The bind is what the spec ties glIsProgramPipeline to. + glBindProgramPipeline(pipeline); + EXPECT_EQ(glIsProgramPipeline(pipeline), GL_TRUE); + EXPECT_EQ(FirstGLError(), 0u); + glBindProgramPipeline(0); + } + // The floor: a two-stage pipeline must paint. If this fails, nothing above it can pass, and // the eight shared conformance cases have exactly one cause. TEST_F(ProgramPipelineScenario, ATwoStagePipelinePaintsWhatItsStagesDescribe) { diff --git a/MobileGL/MG_State/GLState/Core.cpp b/MobileGL/MG_State/GLState/Core.cpp index 9b8503fe..51364dec 100644 --- a/MobileGL/MG_State/GLState/Core.cpp +++ b/MobileGL/MG_State/GLState/Core.cpp @@ -383,6 +383,15 @@ namespace MobileGL::MG_State { // // Location-by-location so that arrays are carried across whole, and via the padded // storage span so a mat3's std140 column padding travels with it. + // + // KNOWN LIMIT, inherent to flattening rather than to this copy: SSO gives each stage + // program its own storage for a uniform, so two stage programs may declare the same + // name and hold different values - but the composite is one link and has one slot for + // it. RefreshCompositeUniforms walks the stages in order, so the last graphics stage + // that declares the name wins, including when it is only holding the zero default and + // an earlier stage held a written value. Fixing it properly means mirroring only the + // uniforms a program has actually been written to, which wants a per-location dirty + // set on ProgramObject. static void MirrorUniformValues(ProgramObject& source, ProgramObject& destination) { if (!source.GetLinkStatus() || !destination.GetLinkStatus()) return; const char* sourceUbo = static_cast(source.GetUBOData()); @@ -1062,26 +1071,42 @@ namespace MobileGL::MG_State { // Program pipeline void GLContext::GenProgramPipelineNames(Uint number, Vector& pipelines) { pipelines.resize(number); - // Names only: glIsProgramPipeline must answer GL_FALSE until one is bound or created. + // Names only. The OBJECT appears as soon as a command needs somewhere to put state + // (see MaterializeProgramPipelineObject), but glIsProgramPipeline still answers + // GL_FALSE until the name is bound or created - see IsProgramPipelineObject. m_programPipelineNames.Generate(number, pipelines.data()); } void GLContext::CreateProgramPipelineObject(Uint index) { - m_programPipelines[index] = MakeShared(index); + const auto object = MakeShared(index); + // glCreateProgramPipelines makes the object outright, so it answers + // glIsProgramPipeline immediately - unlike a name that only got here through + // GenProgramPipelines plus a command that materialized it. + object->MarkEverBound(); + m_programPipelines[index] = object; } Bool GLContext::ValidateProgramPipelineName(Uint index) const { return index == 0 || m_programPipelineNames.IsValid(index); } + // glIsProgramPipeline. Materialization is NOT the test: the object now appears as soon + // as any command takes state from a reserved name, and two of those commands are the + // pure queries glGetProgramPipelineiv / glGetProgramPipelineInfoLog - so keying this on + // map membership would let merely READING a gen'd name turn it into an object. GL 4.6 + // core 7.4 gives the real rule: a GenProgramPipelines name acquires program pipeline + // state when it is first bound. Same shape as IsTransformFeedbackObject. Bool GLContext::IsProgramPipelineObject(Uint index) const { if (index == 0 || !m_programPipelineNames.IsValid(index)) return false; - return m_programPipelines.find(index) != m_programPipelines.end(); + const auto it = m_programPipelines.find(index); + return it != m_programPipelines.end() && it->second && it->second->GetEverBound(); } void GLContext::BindProgramPipelineObject(Uint index) { if (index != 0) { - MaterializeProgramPipelineObject(index); + if (const auto& object = MaterializeProgramPipelineObject(index)) { + object->MarkEverBound(); + } } m_boundProgramPipeline = index; } diff --git a/MobileGL/MG_State/GLState/Core.h b/MobileGL/MG_State/GLState/Core.h index ed497efb..1d26519a 100644 --- a/MobileGL/MG_State/GLState/Core.h +++ b/MobileGL/MG_State/GLState/Core.h @@ -457,8 +457,10 @@ namespace MobileGL { UnorderedMap m_transformFeedbackObjects; IndexGenerator m_transformFeedbackNames; Uint m_boundTransformFeedback = 0; - // Map membership IS object existence here: a pipeline has no stateful default - // object 0, so no everBound flag is needed. + // Map membership is object EXISTENCE, which is not the same as the answer + // glIsProgramPipeline gives: any command that needs somewhere to put state + // materializes a reserved name, so the object can exist well before it is + // bound. ProgramPipelineObject::everBound carries the Is* answer. UnorderedMap> m_programPipelines; IndexGenerator m_programPipelineNames; Uint m_boundProgramPipeline = 0; diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h index 102a2a1d..a4524913 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramPipelineObject.h @@ -40,6 +40,17 @@ namespace MobileGL { Uint GetExternalIndex() const { return m_externalIndex; } + // glIsProgramPipeline's answer, and NOT the same question as "does this object + // exist" (GL 4.6 core 7.4: a GenProgramPipelines name "acquires program pipeline + // state only when first bound"). The object is materialized by any of the + // commands that take state from a reserved name - including the pure queries + // glGetProgramPipelineiv and glGetProgramPipelineInfoLog, which have to answer + // out of default state without ever making the name report as an object. So + // existence is map membership and this is a separate latch, exactly as + // TransformFeedbackObject::everBound is. + Bool GetEverBound() const { return m_everBound; } + void MarkEverBound() { m_everBound = true; } + // The stages a DRAW is built from: every stage but compute. GL 4.6 core 7.4 // makes the compute stage exclusive - a program object containing a compute // shader may contain no other stage, and a pipeline's compute stage is @@ -56,7 +67,17 @@ namespace MobileGL { // GRAPHICS stages are composited into a single hidden program object, rebuilt // whenever the stage set - or any stage program's own link - changes. The // signature is what that "changes" means: a stage program's lifetime id pins the - // object and its backend state version pins the link generation. It covers + // object and its backend state version pins the link generation. + // + // The backend state version is BLUNTER than that description: glUniform1i on a + // sampler and glUniformBlockBinding bump it too, so either one throws the + // composite away and relinks it on the next draw. That is correct but slow, and + // it is a shape the SSO conformance cases hit in a loop. Narrowing it to + // GetLinkVersion() means the composite must instead pick those two up the way it + // picks up uniform values (below) - the sampler half already works that way, + // the block-binding half does not yet, which is why this still keys on the + // blunter version. + // It covers // exactly the stages the composite is built from, so attaching or relinking a // compute stage never invalidates a perfectly good graphics composite - and the // compute stage, having no composite of its own, can never collide with it. @@ -120,6 +141,7 @@ namespace MobileGL { String m_infoLog; const Uint m_externalIndex = 0; Bool m_validateStatus = false; + Bool m_everBound = false; }; } // namespace GLState } // namespace MG_State diff --git a/MobileGL/MG_Test/Program/AsyncLinkTest.cpp b/MobileGL/MG_Test/Program/AsyncLinkTest.cpp index 070b20d1..f8b3e0a9 100644 --- a/MobileGL/MG_Test/Program/AsyncLinkTest.cpp +++ b/MobileGL/MG_Test/Program/AsyncLinkTest.cpp @@ -628,8 +628,8 @@ TEST_F(AsyncLinkTest, DrawThroughAPipelineWithAPendingStageProgramJoinsFirst) { GLuint pipeline = 0; GenProgramPipelines(1, &pipeline); ASSERT_NE(pipeline, 0u); - // Bind before UseProgramStages: glGenProgramPipelines only reserves the name, and the - // first bind is what turns it into an object glUseProgramStages can find. + // Bound first only because this test draws through the pipeline; glUseProgramStages no + // longer needs it (it materializes a reserved name itself, GL 4.6 core 7.4). BindProgramPipeline(pipeline); UseProgramStages(pipeline, GL_VERTEX_SHADER_BIT, vsProgram); ASSERT_EQ(GetError(), GL_NO_ERROR);