[Fix] (Query): tie the two tessellation pipeline-statistics targets to the GL_ARB_tessellation_shader advertisement

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<GLenum, GLuint> 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
+        // A 4.6 core context ACCEPTS the nine unconditional ones at glBeginQuery - there is no query
-        // since 4.6 and there is no query by which an application could learn otherwise before
+        // by which an application could learn otherwise before calling. MobileGL instruments none of
-        // calling. MobileGL instruments none of them, and says so the way GL 4.6 core 4.2.1
+        // them, and says so the way GL 4.6 core 4.2.1 provides for: GL_QUERY_COUNTER_BITS answers
-        // provides for: GL_QUERY_COUNTER_BITS answers zero for these targets, which is the
+        // zero for these targets, which is the spec's own signal that the counter is unsupported and
-        // spec's own signal that the counter is unsupported and its results indeterminate. That
+        // its results indeterminate. That is an honest zero, not an advertised capability - the
-        // is an honest zero, not an advertised capability - the alternative, GL_INVALID_ENUM on a
+        // alternative, GL_INVALID_ENUM on a core entry point, is both non-conformant AND less
-        // core entry point, is both non-conformant AND less informative.
+        // 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;
             }

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_VERTEX_SHADER_INVOCATIONS,   GL_GEOMETRY_SHADER_INVOCATIONS,
-        GL_TESS_EVALUATION_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<const char*>(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, &current);
+            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