From 31252cf0da7d8d391bcb24164f2d8ecab5409ab0 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 27 Aug 2026 03:18:14 -0400 Subject: [PATCH] [Fix] (Tessellation): bound the pass-through control-stage cache and stop baking "draw nothing" for levels GL clamps --- MobileGL/MG_Backend/DirectGLES/Utils.cpp | 18 +-------- .../DirectVulkan/Renderer/ProgramFactory.cpp | 37 ++++++++++-------- .../DirectVulkan/Renderer/ProgramFactory.h | 15 +++++--- .../Backend/DirectGLES/EsslShaderPassTest.cpp | 38 +++++++++++++------ .../Pipeline/PassthroughTessControlTest.cpp | 8 ++-- .../ShaderTranspiler/ShaderCompiler.cpp | 28 ++++++++++++++ .../MG_Util/ShaderTranspiler/ShaderCompiler.h | 12 ++++++ 7 files changed, 104 insertions(+), 52 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index 2ca43e94..8914be6b 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -835,20 +835,6 @@ namespace MobileGL::MG_Backend::DirectGLES { return std::nullopt; } - namespace { - // A GLSL float literal for a tessellation level. Always spelled with a decimal point, - // because an integral value written without one is an INT literal and - // `gl_TessLevelOuter[0] = 1;` does not compile. A non-finite value is baked as 0.0: - // glPatchParameterfv accepts any float, and a level that is not a positive number - // discards the patch - which is what a NaN level does in GL too - whereas emitting - // "nan" would make the synthesized stage fail to compile and take the whole program - // down with it. - String TessLevelLiteral(Float value) { - if (!std::isfinite(value)) return "0.0"; - return std::format("{:.6f}", value); - } - } // namespace - String BuildPassthroughTessControlEssl(const Uint esslVersion, const Uint patchVertices, const String& inPerVertexMembers, const String& outPerVertexMembers, @@ -885,11 +871,11 @@ namespace MobileGL::MG_Backend::DirectGLES { source += " gl_out[gl_InvocationID].gl_Position = gl_in[gl_InvocationID].gl_Position;\n"; for (Uint i = 0; i < 4; ++i) { source += " gl_TessLevelOuter[" + std::to_string(i) + - "] = " + TessLevelLiteral(defaultOuterLevel[i]) + ";\n"; + "] = " + MG_Util::ShaderTranspiler::TessellationLevelLiteral(defaultOuterLevel[i]) + ";\n"; } for (Uint i = 0; i < 2; ++i) { source += " gl_TessLevelInner[" + std::to_string(i) + - "] = " + TessLevelLiteral(defaultInnerLevel[i]) + ";\n"; + "] = " + MG_Util::ShaderTranspiler::TessellationLevelLiteral(defaultInnerLevel[i]) + ";\n"; } source += "}\n"; return source; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index 64c2bd59..c3e60c07 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -3597,19 +3597,6 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } - namespace { - // A GLSL float literal for a tessellation level. Always spelled with a decimal point, - // because an integral value written without one is an INT literal and - // `gl_TessLevelOuter[0] = 1;` does not compile. A non-finite value is baked as 0.0: - // glPatchParameterfv accepts any float, and a level that is not a positive number - // discards the patch - which is what a NaN level does in GL too - whereas emitting "nan" - // would make the synthesized stage fail to compile and take the whole program down. - String TessLevelLiteral(Float value) { - if (!std::isfinite(value)) return "0.0"; - return std::format("{:.6f}", value); - } - } // namespace - Uint64 ProgramFactory::ComputePassthroughTessControlKey(Uint32 patchVertices, const FloatVec4& defaultOuterLevel, const FloatVec2& defaultInnerLevel) { @@ -3683,11 +3670,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { source += " gl_out[gl_InvocationID].gl_Position = gl_in[gl_InvocationID].gl_Position;\n"; for (Uint32 i = 0; i < 4; ++i) { source += " gl_TessLevelOuter[" + std::to_string(i) + - "] = " + TessLevelLiteral(defaultOuterLevel[i]) + ";\n"; + "] = " + MG_Util::ShaderTranspiler::TessellationLevelLiteral(defaultOuterLevel[i]) + ";\n"; } for (Uint32 i = 0; i < 2; ++i) { source += " gl_TessLevelInner[" + std::to_string(i) + - "] = " + TessLevelLiteral(defaultInnerLevel[i]) + ";\n"; + "] = " + MG_Util::ShaderTranspiler::TessellationLevelLiteral(defaultInnerLevel[i]) + ";\n"; } source += "}\n"; return source; @@ -3707,6 +3694,26 @@ namespace MobileGL::MG_Backend::DirectVulkan { return cached->second; } + // The key stopped being bounded when the levels joined it: patchVertices alone could only + // take 32 values, but six unclamped application floats can take any number, and an + // application that ramps a level per frame would retain one VkShaderModule per frame for + // the lifetime of the device. Flushed wholesale rather than aged: a module is not + // referenced by the pipelines built from it (Vulkan copies what it needs at + // vkCreateGraphicsPipelines), everything here runs on the GL thread, and an application + // that can overflow this cap is already recompiling every frame - so the flush costs it + // nothing it was not paying anyway. + if (m_passthroughTessControlStages.size() >= kMaxPassthroughTessControlStages) { + MGLOG_D("ProgramFactory: flushing %zu pass-through tessellation control stages; the application has " + "used more than %zu distinct (patch size, default level) combinations", + m_passthroughTessControlStages.size(), kMaxPassthroughTessControlStages); + for (auto& entry : m_passthroughTessControlStages) { + if (entry.second.module != VK_NULL_HANDLE) { + vkDestroyShaderModule(m_device, entry.second.module, nullptr); + } + } + m_passthroughTessControlStages.clear(); + } + VkPipelineShaderStageCreateInfo stage{VK_STRUCTURE_TYPE_PIPELINE_SHADER_STAGE_CREATE_INFO}; stage.stage = VK_SHADER_STAGE_TESSELLATION_CONTROL_BIT; stage.module = VK_NULL_HANDLE; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index b8a594d9..a8f9d0b5 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -570,11 +570,16 @@ namespace MobileGL::MG_Backend::DirectVulkan { IEvictionObserver* m_evictionObserver = nullptr; // Pass-through tessellation control stages by the identity of what was compiled into // them - the input patch size and the six default tessellation levels, folded into one - // 64-bit key by PassthroughTessControlKey below (the levels are float state, so the map - // cannot simply be keyed on the patch size any more). Never evicted: a handful of entries - // exist for the lifetime of the device, and every pipeline ever built from one keeps - // referencing its module. A failed build is cached as VK_NULL_HANDLE so a broken generator - // costs one compile, not one per draw. + // 64-bit key by ComputePassthroughTessControlKey (the levels are float state, so the map + // cannot simply be keyed on the patch size any more). A failed build is cached as + // VK_NULL_HANDLE so a broken generator costs one compile, not one per draw. + // + // Hard-capped, because the key is application-controlled: glPatchParameterfv clamps + // nothing, so an application that recomputes a level per frame mints a new key per frame. + // Reaching the cap destroys every module and starts over (see the flush in + // GetOrCreatePassthroughTessControlStage); the cap is far above what any program that + // holds its levels still will ever need. + static constexpr SizeT kMaxPassthroughTessControlStages = 64; UnorderedMap m_passthroughTessControlStages; static inline XXH64_state_t* m_hashState = XXH64_createState(); }; diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index cd1292fe..9cc17c62 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -1314,7 +1314,7 @@ TEST(PassthroughTessControlEsslTest, DeclaresThePatchSizeAndWritesEveryTessLevel // legal and ignored, and it saves the generator from having to know the domain. for (const char* level : {"gl_TessLevelOuter[0]", "gl_TessLevelOuter[1]", "gl_TessLevelOuter[2]", "gl_TessLevelOuter[3]", "gl_TessLevelInner[0]", "gl_TessLevelInner[1]"}) { - EXPECT_TRUE(Contains(out, String(level) + " = 1.000000;")) << level << "\n" << out; + EXPECT_TRUE(Contains(out, String(level) + " = 1.0;")) << level << "\n" << out; } // Nothing redeclared when the neighbours redeclared nothing - the driver's own built-in // gl_in/gl_out is then what both sides agree on, and redeclaring is what would break it. @@ -1327,12 +1327,12 @@ TEST(PassthroughTessControlEsslTest, DeclaresThePatchSizeAndWritesEveryTessLevel TEST(PassthroughTessControlEsslTest, BakesTheDefaultTessLevelsIn) { const String out = BuildPassthroughTessControlEssl(320, 4, "", "", FloatVec4(2.0f, 3.0f, 4.0f, 5.0f), FloatVec2(6.5f, 7.25f)); - EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[0] = 2.000000;")) << out; - EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[1] = 3.000000;")) << out; - EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[2] = 4.000000;")) << out; - EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[3] = 5.000000;")) << out; - EXPECT_TRUE(Contains(out, "gl_TessLevelInner[0] = 6.500000;")) << out; - EXPECT_TRUE(Contains(out, "gl_TessLevelInner[1] = 7.250000;")) << out; + EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[0] = 2.0;")) << out; + EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[1] = 3.0;")) << out; + EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[2] = 4.0;")) << out; + EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[3] = 5.0;")) << out; + EXPECT_TRUE(Contains(out, "gl_TessLevelInner[0] = 6.5;")) << out; + EXPECT_TRUE(Contains(out, "gl_TessLevelInner[1] = 7.25;")) << out; } // Every level literal carries a decimal point even when the value is integral: ESSL reads @@ -1344,22 +1344,36 @@ TEST(PassthroughTessControlEsslTest, SpellsIntegralLevelsAsFloatLiterals) { EXPECT_FALSE(Contains(out, "= 2;")) << out; } -// glPatchParameterfv accepts any float, NaN and infinity included. Emitting "nan" would make the -// synthesized stage fail to compile; 0.0 is the honest stand-in, because a level that is not a -// positive number discards the patch in GL too. -TEST(PassthroughTessControlEsslTest, NonFiniteLevelsBecomeZero) { +// glPatchParameterfv accepts any float, NaN and infinity included, and GL 4.6 core 11.2.2 +// discards a patch ONLY when a relevant outer level is <= 0 - everything else is clamped into +// [1, MAX_TESS_GEN_LEVEL]. So the three non-finite inputs do not share one answer: NaN is +// unspecified and 0.0 is the safe reading, -inf really does discard, and +inf must tessellate at +// the maximum. Baking 0.0 for +inf inverted "as finely as possible" into "draw nothing". +TEST(PassthroughTessControlEsslTest, NonFiniteLevelsFollowTheDiscardRule) { const Float notANumber = std::numeric_limits::quiet_NaN(); const Float infinity = std::numeric_limits::infinity(); const String out = BuildPassthroughTessControlEssl(320, 4, "", "", - FloatVec4(notANumber, infinity, 1.0f, 1.0f), + FloatVec4(notANumber, -infinity, infinity, 1.0f), FloatVec2(notANumber, 1.0f)); EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[0] = 0.0;")) << out; EXPECT_TRUE(Contains(out, "gl_TessLevelOuter[1] = 0.0;")) << out; + EXPECT_FALSE(Contains(out, "gl_TessLevelOuter[2] = 0.0;")) + << "a positive infinity clamps to GL_MAX_TESS_GEN_LEVEL, not to a discarded patch" << out; EXPECT_TRUE(Contains(out, "gl_TessLevelInner[0] = 0.0;")) << out; EXPECT_FALSE(Contains(out, "nan")) << out; EXPECT_FALSE(Contains(out, "inf")) << out; } +// A level below the old six-decimal format's resolution is still a POSITIVE level, which GL clamps +// to 1 and draws; rendering it as "0.000000" discarded the patch instead. +TEST(PassthroughTessControlEsslTest, TinyPositiveLevelsDoNotFlushToZero) { + const String out = BuildPassthroughTessControlEssl(320, 4, "", "", + FloatVec4(1e-7f, 1.0f, 1.0f, 1.0f), + FloatVec2(1.0f, 1.0f)); + EXPECT_FALSE(Contains(out, "gl_TessLevelOuter[0] = 0.0;")) << out; + EXPECT_FALSE(Contains(out, "gl_TessLevelOuter[0] = 0.000000;")) << out; +} + // ES 3.1 reaches tessellation only through the extension; the caller has already established // that the driver runs the evaluation stage at all, so the only question is the spelling. TEST(PassthroughTessControlEsslTest, RequestsTheExtensionBelowEs32) { diff --git a/MobileGL/MG_Test/Pipeline/PassthroughTessControlTest.cpp b/MobileGL/MG_Test/Pipeline/PassthroughTessControlTest.cpp index 5ba66d4d..fea1e2ec 100644 --- a/MobileGL/MG_Test/Pipeline/PassthroughTessControlTest.cpp +++ b/MobileGL/MG_Test/Pipeline/PassthroughTessControlTest.cpp @@ -179,10 +179,10 @@ TEST_F(PassthroughTessControlTest, BakesTheDefaultTessLevelsInAndKeysOnThem) { const FloatVec4 outer(2.0f, 3.0f, 4.0f, 5.0f); const FloatVec2 inner(6.5f, 7.25f); const String source = ProgramFactory::BuildPassthroughTessControlSource(4, outer, inner); - EXPECT_NE(source.find("gl_TessLevelOuter[0] = 2.000000;"), String::npos) << source; - EXPECT_NE(source.find("gl_TessLevelOuter[3] = 5.000000;"), String::npos) << source; - EXPECT_NE(source.find("gl_TessLevelInner[0] = 6.500000;"), String::npos) << source; - EXPECT_NE(source.find("gl_TessLevelInner[1] = 7.250000;"), String::npos) << source; + EXPECT_NE(source.find("gl_TessLevelOuter[0] = 2.0;"), String::npos) << source; + EXPECT_NE(source.find("gl_TessLevelOuter[3] = 5.0;"), String::npos) << source; + EXPECT_NE(source.find("gl_TessLevelInner[0] = 6.5;"), String::npos) << source; + EXPECT_NE(source.find("gl_TessLevelInner[1] = 7.25;"), String::npos) << source; const Uint64 defaultKey = ProgramFactory::ComputePassthroughTessControlKey(4, kDefaultOuter, kDefaultInner); diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index f0f2455c..a5d71789 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -12,6 +12,10 @@ #include "ShaderCompiler.h" +#include + +#include + #include "SpirvPasses/EliminateFloatEqualsZeroPass.h" #include "SpirvPasses/FlattenInterfaceStructPass.h" #include "SpirvPasses/RenameSamplerFunctionParameterPass.h" @@ -64,6 +68,30 @@ namespace MobileGL { namespace MG_Util { namespace ShaderTranspiler { + // Above every plausible GL_MAX_TESS_GEN_LEVEL (the GL core minimum is 64), so it lands on the + // same clamped result the device's own maximum would. +inf has to reach the tessellator as + // "as finely as possible", not as "discard". + static constexpr const char* kClampedHighTessLevelLiteral = "65536.0"; + + String TessellationLevelLiteral(Float value) { + // GL leaves a NaN level unspecified; 0.0 is the safe reading, and unlike "nan" it compiles. + if (std::isnan(value)) return "0.0"; + // -inf is <= 0 and discards the patch, exactly like 0.0. +inf clamps to the maximum. + if (std::isinf(value)) return value > 0.0f ? kClampedHighTessLevelLiteral : "0.0"; + + // Shortest round-trip, not a fixed six decimals: "{:.6f}" renders every level below ~5e-7 + // as "0.000000", which turns a positive level GL would clamp to 1 into a discarded patch. + String text = std::format("{}", value); + // ...but shortest round-trip spells an integral value as a bare digit sequence, which GLSL + // reads as an INT literal, so the decimal point has to be put back when nothing else marks + // the literal as floating point. + if (text.find('.') == String::npos && text.find('e') == String::npos && + text.find('E') == String::npos) { + text += ".0"; + } + return text; + } + // `env` is the compile-time backend snapshot; null means "resolve from the live // backend", which is what the standalone/test entry points do. The pipeline always // passes one, so a worker never reaches pActiveBackendObject through here. diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index f1a5ee6e..90c41c3c 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -18,6 +18,18 @@ namespace MobileGL { namespace MG_Util { namespace ShaderTranspiler { + // A GLSL float literal for a tessellation level, for the pass-through tessellation + // control stage both backends synthesize when a program has an evaluation stage and no + // control stage. Shared so the two generators cannot disagree about what a level means. + // + // GL 4.6 core 11.2.2 discards a patch only when a relevant OUTER level is <= 0; every + // other value is CLAMPED into [1, MAX_TESS_GEN_LEVEL]. So "draw nothing" is reserved + // for the values that really mean it, and everything else has to survive the trip + // through text: a shortest-round-trip spelling, because a fixed-decimal one flushes + // small positive levels to zero, and always with a '.' or an exponent, because a bare + // digit sequence is an INT literal and `gl_TessLevelOuter[0] = 1;` does not compile. + String TessellationLevelLiteral(Float value); + class ShaderCompiler { public: static Result> CompileShader(const ShaderAttrib& attrib);