diff --git a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp index e772e094..32ccd1bd 100644 --- a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp +++ b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp @@ -833,7 +833,8 @@ namespace MobileGL::MG_Impl::GLImpl { } auto& programObject = TryToGetProgramObject(program); if (!programObject) return; - const auto& s = programObject->GetAttachedShaders(); + // GL-visible: a shader detached since the last link is no longer attached (GL 4.6 §7.3). + const auto s = programObject->GetGLVisibleAttachedShaders(); GLsizei c = std::min((GLsizei)s.size(), maxCount); if (count) *count = c; for (GLsizei i = 0; i < c; ++i) { @@ -873,7 +874,7 @@ namespace MobileGL::MG_Impl::GLImpl { break; } case GL_ATTACHED_SHADERS: { - const auto& attachedShaders = programObject->GetAttachedShaders(); + const auto attachedShaders = programObject->GetGLVisibleAttachedShaders(); *params = (GLint)attachedShaders.size(); MGLOG_D("%s: %s = %d", __func__, MG_Util::ConvertGLEnumToString(pname).c_str(), *params); break; diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ArbShaderObjectsScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ArbShaderObjectsScenario.cpp index 86f3d4dc..1809c2f9 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ArbShaderObjectsScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ArbShaderObjectsScenario.cpp @@ -225,6 +225,53 @@ void main() { oColor = uColor; } EXPECT_EQ(FirstGLError(), 0u); } + // A detach takes effect for every query at once (GL 4.6 ¡ì7.3), even though the program keeps + // the shader for its current executable until the next link: a second detach is + // INVALID_OPERATION, and attaching it again before that link is legal and lists it once. + TEST_F(ArbShaderObjectsScenario, ADetachIsVisibleAtOnceAndAReattachWithdrawsIt) { + if (!Ready()) return; + const GLuint vs = ArbShader(GL_VERTEX_SHADER, kVertexSource); + const GLuint fs = ArbShader(GL_FRAGMENT_SHADER, kFragmentSource); + const GLuint program = glCreateProgramObjectARB(); + glAttachObjectARB(program, vs); + glAttachObjectARB(program, fs); + glLinkProgramARB(program); + GLint value = 0; + glDetachObjectARB(program, fs); + glGetProgramiv(program, GL_ATTACHED_SHADERS, &value); + EXPECT_EQ(value, 1) << "a detached shader is still counted"; + GLuint attached[4] = {}; + GLsizei count = 0; + glGetAttachedShaders(program, 4, &count, attached); + ASSERT_EQ(count, 1); + EXPECT_EQ(attached[0], vs); + glGetProgramiv(program, GL_LINK_STATUS, &value); + EXPECT_EQ(value, GL_TRUE) << "a detach must not touch the current link"; + EXPECT_EQ(FirstGLError(), 0u); + glDetachShader(program, fs); + EXPECT_EQ(FirstGLError(), static_cast(GL_INVALID_OPERATION)) << "detaching a detached shader"; + glAttachShader(program, fs); + EXPECT_EQ(FirstGLError(), 0u) << "re-attaching a shader detached since the last link"; + glGetAttachedObjectsARB(program, 4, &count, attached); + EXPECT_EQ(count, 2) << "the re-attached shader is listed exactly once"; + glAttachShader(program, fs); + EXPECT_EQ(FirstGLError(), static_cast(GL_INVALID_OPERATION)) << "attaching an attached shader"; + // The relink sees both shaders again and draws. + glLinkProgram(program); + glGetProgramiv(program, GL_LINK_STATUS, &value); + ASSERT_EQ(value, GL_TRUE); + glUseProgram(program); + glUniform4f(glGetUniformLocation(program, "uColor"), 1.0f, 0.0f, 0.0f, 1.0f); + DrawInto(); + const Image red = ReadPixels(kEdge, kEdge); + EXPECT_TRUE(RegionIsMostly(red, 0, kEdge - 1, 0, kEdge - 1, "red", 0.0, "relinked after detach + reattach")); + glUseProgram(0); + glDeleteObjectARB(program); + glDeleteObjectARB(vs); + glDeleteObjectARB(fs); + EXPECT_EQ(FirstGLError(), 0u); + } + // A program built through the extension, mixed with core calls on the same objects, draws. TEST_F(ArbShaderObjectsScenario, AnArbProgramMixedWithCoreCallsDraws) { if (!Ready()) return; diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp index ee9480f3..90c15faf 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp @@ -436,10 +436,18 @@ namespace MobileGL::MG_State::GLState { // is specified as link-then-detach and would discard its own link before anyone read it. bool ProgramObject::AttachShader(const SharedPtr& shader) { MGLOG_D("ProgramObject %u: AttachShader called for shader %p", m_externalIndex, shader.get()); - if (ShaderIsAttached(shader)) { + if (ShaderIsAttachedGLVisible(shader)) { MGLOG_D("ProgramObject %u: AttachShader - shader already attached, skipping", m_externalIndex); return false; } + if (ShaderIsAttached(shader)) { + // Detached since the last link, and attached again before the next one: GL sees it + // attached from here on, so the pending detach is withdrawn rather than the shader + // listed twice. + std::erase_if(m_detachedShaders, + [&shader](const SharedPtr& s) { return s.get() == shader.get(); }); + return true; + } m_shaders.emplace_back(shader); MGLOG_D("ProgramObject %u: AttachShader - attached successfully, total shaders now %zu", m_externalIndex, m_shaders.size()); @@ -456,7 +464,9 @@ namespace MobileGL::MG_State::GLState { SizeT ProgramObject::DetachShader(const SharedPtr& shader) { MGLOG_D("DetachShader called for shader %p from ProgramObject %u", shader.get(), m_externalIndex); - if (!ShaderIsAttached(shader)) { + // GL-visible: a shader already detached (pending the next link) is not attached, and a + // second glDetachShader of it is INVALID_OPERATION like any other unattached shader. + if (!ShaderIsAttachedGLVisible(shader)) { MGLOG_D("Shader %p is not attached to ProgramObject %u, cannot detach.", shader.get(), m_externalIndex); return 0; } @@ -665,6 +675,15 @@ namespace MobileGL::MG_State::GLState { MGLOG_D("ProgramObject %u: MarkAsDeleted - now marked deleted", m_externalIndex); } + Vector> ProgramObject::GetGLVisibleAttachedShaders() const { + Vector> visible; + visible.reserve(m_shaders.size()); + for (const auto& shader : m_shaders) { + if (ShaderIsAttachedGLVisible(shader)) visible.push_back(shader); + } + return visible; + } + Vector>& ProgramObject::GetAttachedShaders() { MGLOG_D("ProgramObject %u: GetAttachedShaders called, returning %zu shaders", m_externalIndex, m_shaders.size()); diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index 162cb201..1f9eb119 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -73,6 +73,10 @@ namespace MobileGL::MG_State::GLState { if (std::none_of(m_shaders.begin(), m_shaders.end(), matches)) return false; return std::none_of(m_detachedShaders.begin(), m_detachedShaders.end(), matches); } + // The attach list as GL sees it: the attach list minus the shaders pending detach. What + // glGetAttachedShaders and GL_ATTACHED_SHADERS answer from - the raw list keeps a detached + // shader until the next link consumes the detach. + Vector> GetGLVisibleAttachedShaders() const; bool AttachShader(const SharedPtr& shader); SizeT DetachShader(const SharedPtr& shader); SizeT RemoveShader(const SharedPtr& shader);