From ebff4b21f70e0f261a63aea0738e692f1f4f1290 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 27 Aug 2026 09:56:10 -0400 Subject: [PATCH] [Fix] (Query): tie the two tessellation pipeline-statistics targets to the GL_ARB_tessellation_shader advertisement --- MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp | 43 ++++++++++--- MobileGL/MG_Test/Query/QueryTest.cpp | 72 +++++++++++++++++++++- 2 files changed, 104 insertions(+), 11 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp b/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp index 703843db..bcdc199a 100644 --- a/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp +++ b/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp @@ -64,21 +64,43 @@ namespace MobileGL::MG_Impl::GLImpl { // and none of them has any state beyond "which object is counting". UnorderedMap g_activePipelineStatisticsQueryIds; + // Whether MobileGL puts GL_ARB_tessellation_shader in its extension string. Read from the + // ADVERTISED list rather than from a capability bit for the same reason + // BackendSupportsTextureViews does (GL_Texture.cpp): it makes "MobileGL claims tessellation + // support" and "the tessellation-conditional API surface is open" the same fact by + // construction, so the day a backend starts advertising the string the surface below opens + // with it and no second edit is owed. + Bool AdvertisesTessellationShaderExtension() { + const auto& activeBackendObject = MG_Backend::pActiveBackendObject; + if (!activeBackendObject) return false; + const auto& extensions = activeBackendObject->GetRendererInfo().RendererGLInfo.Extensions; + return std::find(extensions.begin(), extensions.end(), E_GL_ARB_tessellation_shader) != extensions.end(); + } + // The eleven pipeline-statistics counters (GL 4.6 core table 4.3 / ARB_pipeline_statistics_query). - // A 4.6 core context has to ACCEPT all of them at glBeginQuery - the extension is core - // since 4.6 and there is no query by which an application could learn otherwise before - // calling. MobileGL instruments none of them, and says so the way GL 4.6 core 4.2.1 - // provides for: GL_QUERY_COUNTER_BITS answers zero for these targets, which is the - // spec's own signal that the counter is unsupported and its results indeterminate. That - // is an honest zero, not an advertised capability - the alternative, GL_INVALID_ENUM on a - // core entry point, is both non-conformant AND less informative. + // A 4.6 core context ACCEPTS the nine unconditional ones at glBeginQuery - there is no query + // by which an application could learn otherwise before calling. MobileGL instruments none of + // them, and says so the way GL 4.6 core 4.2.1 provides for: GL_QUERY_COUNTER_BITS answers + // zero for these targets, which is the spec's own signal that the counter is unsupported and + // its results indeterminate. That is an honest zero, not an advertised capability - the + // alternative, GL_INVALID_ENUM on a core entry point, is both non-conformant AND less + // informative. + // + // The two TESSELLATION targets are the exception, because ARB_pipeline_statistics_query + // makes them conditional on tessellation support rather than unconditional, and the only + // thing an application (or the conformance suite) can read to decide whether an + // implementation has it is the GL_ARB_tessellation_shader string. MobileGL does not emit it + // today, so these two answer GL_INVALID_ENUM: an API surface that accepts a + // tessellation-conditional token while withholding the string that announces the condition + // is self-contradictory, and it is the contradiction the suite catches + // (KHR-GL46.pipeline_statistics_query_tests_ARB.api_coverage_unsupported_calls, whose + // support probe is gl4cPipelineStatisticsQueryTests.cpp:1166-1176). The gate is the + // advertisement itself, not a hardcoded "no", so this is one switch and not two. Bool IsPipelineStatisticsQueryTarget(GLenum target) { switch (target) { case GL_VERTICES_SUBMITTED: case GL_PRIMITIVES_SUBMITTED: case GL_VERTEX_SHADER_INVOCATIONS: - case GL_TESS_CONTROL_SHADER_PATCHES: - case GL_TESS_EVALUATION_SHADER_INVOCATIONS: case GL_GEOMETRY_SHADER_INVOCATIONS: case GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED: case GL_FRAGMENT_SHADER_INVOCATIONS: @@ -86,6 +108,9 @@ namespace MobileGL::MG_Impl::GLImpl { case GL_CLIPPING_INPUT_PRIMITIVES: case GL_CLIPPING_OUTPUT_PRIMITIVES: return true; + case GL_TESS_CONTROL_SHADER_PATCHES: + case GL_TESS_EVALUATION_SHADER_INVOCATIONS: + return AdvertisesTessellationShaderExtension(); default: return false; } diff --git a/MobileGL/MG_Test/Query/QueryTest.cpp b/MobileGL/MG_Test/Query/QueryTest.cpp index 14628147..1c84f5b6 100644 --- a/MobileGL/MG_Test/Query/QueryTest.cpp +++ b/MobileGL/MG_Test/Query/QueryTest.cpp @@ -827,10 +827,11 @@ TEST_F(QueryTest, DisableTimerQueryFeatureMatchesEnvironment) { // --------------------------------------------------------------------------------------------- TEST_F(QueryTest, PipelineStatisticsTargetsAreAcceptedAndReportZeroCounterBits) { + // The NINE unconditional targets. The two tessellation ones are conditional on tessellation + // support and have their own test below. static constexpr GLenum kTargets[] = { GL_VERTICES_SUBMITTED, GL_PRIMITIVES_SUBMITTED, - GL_VERTEX_SHADER_INVOCATIONS, GL_TESS_CONTROL_SHADER_PATCHES, - GL_TESS_EVALUATION_SHADER_INVOCATIONS, GL_GEOMETRY_SHADER_INVOCATIONS, + GL_VERTEX_SHADER_INVOCATIONS, GL_GEOMETRY_SHADER_INVOCATIONS, GL_GEOMETRY_SHADER_PRIMITIVES_EMITTED, GL_FRAGMENT_SHADER_INVOCATIONS, GL_COMPUTE_SHADER_INVOCATIONS, GL_CLIPPING_INPUT_PRIMITIVES, GL_CLIPPING_OUTPUT_PRIMITIVES, @@ -872,6 +873,73 @@ TEST_F(QueryTest, PipelineStatisticsTargetsAreAcceptedAndReportZeroCounterBits) } } +// GL_TESS_CONTROL_SHADER_PATCHES / GL_TESS_EVALUATION_SHADER_INVOCATIONS are the two +// pipeline-statistics targets ARB_pipeline_statistics_query makes CONDITIONAL on tessellation +// support, and the extension string is the only thing an application can read to decide whether +// an implementation has it. So the target and the string have to move together: accepting a +// tessellation-conditional token while withholding the string that announces the condition is +// self-contradictory, and the conformance suite catches exactly that contradiction +// (KHR-GL46.pipeline_statistics_query_tests_ARB.api_coverage_unsupported_calls demands +// GL_INVALID_ENUM for every target its own probe calls unsupported, and its probe for these two +// is `compatibility(4,0) || GL_ARB_tessellation_shader` - a CORE context fails the first half). +// +// Written against the advertisement rather than against today's answer on purpose: the day a +// backend starts emitting GL_ARB_tessellation_shader this test keeps passing and keeps pinning +// the coupling, and it fails loudly if only one of the two halves moves. +TEST_F(QueryTest, TessellationPipelineStatisticsTargetsFollowTheTessellationShaderAdvertisement) { + const auto* extensionsString = + reinterpret_cast(MG_Impl::GLImpl::GetString(GL_EXTENSIONS)); + ASSERT_NE(extensionsString, nullptr); + const Bool advertised = String(extensionsString).find("GL_ARB_tessellation_shader") != String::npos; + const GLenum expectedError = advertised ? GL_NO_ERROR : GL_INVALID_ENUM; + + static constexpr GLenum kTessTargets[] = { + GL_TESS_CONTROL_SHADER_PATCHES, + GL_TESS_EVALUATION_SHADER_INVOCATIONS, + }; + + for (const GLenum target: kTessTargets) { + GLuint id = 0; + MG_Impl::GLImpl::GenQueries(1, &id); + ASSERT_NE(id, 0u); + + MG_Impl::GLImpl::BeginQuery(target, id); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), expectedError) + << "glBeginQuery on tessellation pipeline-statistics target 0x" << std::hex << target + << " must agree with the GL_ARB_tessellation_shader advertisement"; + + if (advertised) { + MG_Impl::GLImpl::EndQuery(target); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + GLint counterBits = -1; + MG_Impl::GLImpl::GetQueryiv(target, GL_QUERY_COUNTER_BITS, &counterBits); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + EXPECT_EQ(counterBits, 0); + } else { + // Refused at glEndQuery too, not just at glBeginQuery: a target the implementation + // does not have is not half-accepted. + MG_Impl::GLImpl::EndQuery(target); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_ENUM); + // GL_QUERY_COUNTER_BITS still answers the honest zero rather than an error - the + // getter has never validated its target, and zero is what "no such counter" reads as + // (GL 4.6 core 4.2.1), so the refusal costs no information. + GLint counterBits = -1; + MG_Impl::GLImpl::GetQueryiv(target, GL_QUERY_COUNTER_BITS, &counterBits); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + EXPECT_EQ(counterBits, 0); + // And GL_CURRENT_QUERY reads as "no query" rather than tracking a slot that was + // never opened. + GLint current = -1; + MG_Impl::GLImpl::GetQueryiv(target, GL_CURRENT_QUERY, ¤t); + EXPECT_EQ(current, 0); + } + + MG_Impl::GLImpl::DeleteQueries(1, &id); + while (MG_Impl::GLImpl::GetError() != GL_NO_ERROR) { + } + } +} + // The negative half the conformance case actually asserts: an object already latched onto one // pipeline-statistics target must refuse a different one with GL_INVALID_OPERATION. This is what // per-target active slots buy - a single shared slot would have reported "a query is already