From 09449256795d0624263e86a328b1a85458ea6c34 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 03:48:53 -0400 Subject: [PATCH 01/21] [Fix] (CTS-Harness): pin device glcts surface to 256^2 rgba8888d24s8, sync qpa, classify no-log reboots as hangs --- tools/cts/scripts/run_cts.py | 34 ++++++++++++++++++++++--- tools/cts/scripts/wgl_glcts_pipeline.py | 4 ++- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/tools/cts/scripts/run_cts.py b/tools/cts/scripts/run_cts.py index 18d6e2c0..22751f57 100644 --- a/tools/cts/scripts/run_cts.py +++ b/tools/cts/scripts/run_cts.py @@ -86,6 +86,19 @@ def main(): ap.add_argument("--outdir", required=True) ap.add_argument("--device-dir", default="/data/local/tmp/mgcts") ap.add_argument("--surface", default="fbo", help="--deqp-surface-type value") + # Without an explicit size, dEQP's FboRenderContext sizes the wrapper FBO to + # GL_MAX_RENDERBUFFER_SIZE (16384^2 here) and size-derived test allocations + # explode (a 4-sample 16K depth texture alone is 4 GiB). + ap.add_argument("--surface-size", type=int, default=256, + help="--deqp-surface-width/height value") + # With DONT_CARE depth/stencil bits dEQP's FboRenderContext picks the first entry of + # its own format list, GL_DEPTH32F_STENCIL8. framebuffer_blit meanwhile hardcodes + # GL_DEPTH24_STENCIL8 for its own buffers whenever it detects an FBO surface, then + # blits depth between the two - which the spec forbids for mismatched formats, so a + # conformant driver has to fail it. Asking for a config the test agrees with avoids + # the contradiction instead of papering over it. + ap.add_argument("--gl-config-name", default="rgba8888d24s8", + help="--deqp-gl-config-name value (empty string to leave it unset)") ap.add_argument("--max-rounds", type=int, default=4000) ap.add_argument("--max-empty-streak", type=int, default=64, help="abort after this many consecutive chunks that produce no log at all") @@ -151,14 +164,22 @@ def main(): adb(args.serial, "shell", f"rm -f {dev_qpa}", timeout=60) extra_env = "".join(f"{kv} " for kv in args.env) + config_flag = ( + f"--deqp-gl-config-name={args.gl_config_name} " if args.gl_config_name else "" + ) + # The trailing sync makes the qpa durable: a hard GPU hang reboots the + # device, and f2fs rolls back unsynced writes, silently eating the log. cmd = ( f"cd {args.device_dir} && " f"MOBILEGL_BACKEND_TYPE={args.backend} LD_LIBRARY_PATH=. {extra_env}" f"./glcts --deqp-caselist-file={dev_list} " f"--deqp-surface-type={args.surface} " + f"--deqp-surface-width={args.surface_size} " + f"--deqp-surface-height={args.surface_size} " + f"{config_flag}" f"--deqp-terminate-on-device-lost=disable " f"--deqp-log-images=disable --deqp-log-shader-sources=disable " - f"--deqp-log-filename={dev_qpa} > /dev/null 2>&1; echo RC=$?" + f"--deqp-log-filename={dev_qpa} > /dev/null 2>&1; rc=$?; sync; echo RC=$rc" ) run = adb(args.serial, "shell", cmd, timeout=args.chunk_timeout) if run.returncode == 124: @@ -224,9 +245,16 @@ def main(): f"to label the rest of the suite as crashes.", file=sys.stderr) break + # No log at all. If the device rebooted, the first unrun case took the + # whole device down (a reboot can also roll back the freshly created + # qpa on f2fs) - that is a hang to quarantine, not a process crash. victim = remaining[0] - print(f"[run_cts] no output at all; recording {victim} as Crash") - crashed.append(victim) + if rebooted: + print(f"[run_cts] DEVICE HANG in {victim} (no log at all) - quarantining it") + hung.append(victim) + else: + print(f"[run_cts] no output at all; recording {victim} as Crash") + crashed.append(victim) done.add(victim) progressed = 1 diff --git a/tools/cts/scripts/wgl_glcts_pipeline.py b/tools/cts/scripts/wgl_glcts_pipeline.py index f39327eb..e043d142 100644 --- a/tools/cts/scripts/wgl_glcts_pipeline.py +++ b/tools/cts/scripts/wgl_glcts_pipeline.py @@ -46,8 +46,10 @@ DEFAULT_DEQP_ARGS = ( "--deqp-gl-context-type=wgl", "--deqp-surface-type=fbo", "--deqp-gl-config-name=rgba8888d24s8", + # Height <= 0 is DONT_CARE, which FboRenderContext resolves to + # GL_MAX_RENDERBUFFER_SIZE - a 64x16384 surface. Pin both. "--deqp-surface-width=64", - "--deqp-surface-height=-1", + "--deqp-surface-height=64", "--deqp-base-seed=3", "--deqp-visibility=hidden", "--deqp-watchdog=enable", From 392736fb6b5b876a1e6630c41c50824265cbfbe0 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 03:59:29 -0400 Subject: [PATCH 02/21] [Fix, Test] (ShaderTranspiler): rewrite float-equals-zero exactly instead of within a 1e-4 epsilon --- .../ShaderTranspiler/DemoteFloat64Test.cpp | 150 +++++++++++++++--- .../EliminateFloatEqualsZeroPass.cpp | 61 +++---- .../EliminateFloatEqualsZeroPass.h | 19 ++- 3 files changed, 182 insertions(+), 48 deletions(-) diff --git a/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp b/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp index 6128dd99..b1b6dfd0 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp +++ b/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp @@ -8,6 +8,7 @@ #include +#include #include #include @@ -484,52 +485,165 @@ TEST_F(DemoteFloat64Test, RejectsGarbageInput) { EXPECT_FALSE(ShaderCompiler::DemoteFloat64ToFloat32(notSpirv, output, true)); } -// EliminateFloatEqualsZeroPass turns a comparison against 0.0 into an epsilon test, a -// workaround for drivers whose exact float compare misbehaves. Deciding WHICH constants are -// zero used to read every float constant as though it were 32 bits wide, and on a 64-bit -// constant that reads the LOW half of the mantissa - which is zero for 1.0lf, 2.0lf, 0.5lf and -// every other round double a shader is likely to spell. Each of those was mistaken for 0.0, so -// a comparison against 1.0lf became an epsilon test against ZERO, and came out true for a -// uniform holding exactly 1.0. That is the whole of KHR-GL43.compute_shader.fp64-case2. +// EliminateFloatEqualsZeroPass re-spells a comparison against 0.0 through GLSL.std.450 FAbs, so +// that no float-equality instruction reaches a driver that gets one wrong. Deciding WHICH +// constants are zero used to read every float constant as though it were 32 bits wide, and on a +// 64-bit constant that reads the LOW half of the mantissa - which is zero for 1.0lf, 2.0lf, 0.5lf +// and every other round double a shader is likely to spell. Each of those was mistaken for 0.0, so +// a comparison against 1.0lf became a test against ZERO, and came out true for a uniform holding +// exactly 1.0. That is the whole of KHR-GL43.compute_shader.fp64-case2. +// +// The replacement itself used to be an epsilon ball, `abs(x) < 1e-4`, which called any legitimately +// small value zero: KHR-GL3x.buffer_objects.triangles computes a specular term of ~6e-5 at a large +// render target and rendered black. It is exact now - `abs(x) <= 0.0` / `abs(x) > 0.0` against the +// module's own zero constant - and the tests below pin both halves of that: only a genuine 0.0 is +// matched, and what the compare tests against is the constant the source itself spelled. // // Asserted on the optimized module rather than through a driver, because that is where the -// rewrite happens and its fingerprint there is unambiguous: the epsilon form introduces a +// rewrite happens and its fingerprint there is unambiguous: the rewrite introduces a // GLSL.std.450 FAbs, and nothing else in these shaders would. namespace { - Bool RewritesToAnEpsilonTest(const String& source) { + String OptimizedDisassembly(const String& source) { const Vector input = CompileToSpirv(GL_COMPUTE_SHADER, source); EXPECT_FALSE(input.empty()); - if (input.empty()) return false; + if (input.empty()) return {}; Vector output; EXPECT_TRUE(ShaderCompiler::SanitizeAndOptimizeBinary(input, output, true, true)); - return Disassemble(output).find("FAbs") != String::npos; + return Disassemble(output); } - String CompareAgainst(const String& type, const String& literal) { + Bool RewritesToAnAbsoluteValueTest(const String& source) { + return OptimizedDisassembly(source).find("FAbs") != String::npos; + } + + String CompareAgainstUsing(const String& type, const String& op, const String& literal) { return "#version 430 core\n" "layout(local_size_x = 1) in;\n" "buffer Result { int g_result; };\n" "uniform " + type + " g_0;\n" "void main() {\n" " g_result = 0;\n" - " if (g_0 != " + literal + ") g_result = 1;\n" + " if (g_0 " + op + " " + literal + ") g_result = 1;\n" "}\n"; } + + String CompareAgainst(const String& type, const String& literal) { + return CompareAgainstUsing(type, "!=", literal); + } + + // Every instruction of a disassembly, split into whitespace-separated tokens, so an operand can + // be identified by position instead of by a substring another opcode might also contain - + // `OpFOrdLessThan` is a prefix of `OpFOrdLessThanEqual`, and those two are the whole difference + // between the epsilon rewrite and the exact one. + Vector> TokenizedInstructions(const String& disassembly) { + Vector> instructions; + StringStream lines(disassembly); + String line; + while (std::getline(lines, line)) { + Vector tokens; + StringStream words(line); + String word; + while (words >> word) tokens.push_back(word); + instructions.push_back(tokens); + } + return instructions; + } + + // The compare the rewrite leaves behind, e.g. `%22 = OpFOrdLessThanEqual %bool %21 %float_0`, + // or an empty vector if the module has none. These four opcodes are the only ones the pass + // emits and nothing else in these shaders produces one. + Vector FindRewrittenCompare(const String& disassembly) { + for (const Vector& tokens : TokenizedInstructions(disassembly)) { + if (tokens.size() < 6 || tokens[1] != "=") continue; + if (tokens[2] == "OpFOrdLessThanEqual" || tokens[2] == "OpFUnordLessThanEqual" || + tokens[2] == "OpFOrdGreaterThan" || tokens[2] == "OpFUnordGreaterThan") { + return tokens; + } + } + return {}; + } + + // Result id of the module's 0.0 constant of the type FAbs produces - the constant the source + // itself spelled - found without assuming what the disassembler names it or how it prints the + // literal. + String FindZeroConstantId(const String& disassembly) { + const Vector> instructions = TokenizedInstructions(disassembly); + String floatTypeId; + for (const Vector& tokens : instructions) { + if (tokens.size() >= 7 && tokens[2] == "OpExtInst" && tokens[5] == "FAbs") { + floatTypeId = tokens[3]; + break; + } + } + if (floatTypeId.empty()) return {}; + + for (const Vector& tokens : instructions) { + if (tokens.size() < 5 || tokens[2] != "OpConstant" || tokens[3] != floatTypeId) continue; + char* end = nullptr; + const double value = std::strtod(tokens[4].c_str(), &end); + if (end != nullptr && *end == '\0' && value == 0.0) return tokens[0]; + } + return {}; + } + + // The shape the pass promises: the given opcode (either NaN half of it), tested against the + // module's own zero constant rather than against anything this pass invented. + void ExpectComparedAgainstModuleZero(const String& source, const String& orderedOpcode, + const String& unorderedOpcode) { + const String disassembly = OptimizedDisassembly(source); + const Vector compare = FindRewrittenCompare(disassembly); + ASSERT_FALSE(compare.empty()) << "no rewritten compare in the optimized module\n" + << disassembly; + EXPECT_TRUE(compare[2] == orderedOpcode || compare[2] == unorderedOpcode) + << "expected " << orderedOpcode << " (or its unordered twin), got " << compare[2] << "\n" + << disassembly; + + const String zeroId = FindZeroConstantId(disassembly); + ASSERT_FALSE(zeroId.empty()) << "the module has no 0.0 constant of the abs() type\n" + << disassembly; + EXPECT_EQ(compare.back(), zeroId) + << "the rewrite compares against " << compare.back() + << " instead of the module's own zero; a synthesized threshold is the epsilon bug\n" + << disassembly; + } } // namespace TEST_F(DemoteFloat64Test, AComparisonAgainstANonZeroDoubleIsLeftAlone) { - EXPECT_FALSE(RewritesToAnEpsilonTest(CompareAgainst("double", "1.0LF"))) - << "a double compared against 1.0lf was rewritten into an epsilon test against zero"; + EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("double", "1.0LF"))) + << "a double compared against 1.0lf was rewritten into a test against zero"; } TEST_F(DemoteFloat64Test, AComparisonAgainstZeroIsStillRewritten) { - EXPECT_TRUE(RewritesToAnEpsilonTest(CompareAgainst("double", "0.0LF"))) + EXPECT_TRUE(RewritesToAnAbsoluteValueTest(CompareAgainst("double", "0.0LF"))) << "the rewrite must still fire for a genuine comparison against zero"; } TEST_F(DemoteFloat64Test, TheThirtyTwoBitBehaviourIsUnchanged) { - EXPECT_FALSE(RewritesToAnEpsilonTest(CompareAgainst("float", "1.0"))) + EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("float", "1.0"))) << "a float compared against 1.0 must not be rewritten"; - EXPECT_TRUE(RewritesToAnEpsilonTest(CompareAgainst("float", "0.0"))) + EXPECT_TRUE(RewritesToAnAbsoluteValueTest(CompareAgainst("float", "0.0"))) << "the 32-bit behaviour this pass shipped with must be preserved exactly"; } + +// The pass matches ZERO, not "small". The old constant-is-zero test was `fabs(v) <= 1e-4`, so a +// float compared against exactly 1e-4 was declared a comparison against zero and rewritten into +// `abs(x) >= 1e-4` - a different question from the one the shader asked, against a constant that +// was never zero to begin with. +TEST_F(DemoteFloat64Test, AComparisonAgainstASmallNonZeroLiteralIsLeftAlone) { + EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("float", "0.0001"))) + << "a float compared against 1e-4 was treated as a comparison against zero"; + EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("double", "0.0001LF"))) + << "the 64-bit accessor must judge the constant just as exactly as the 32-bit one"; +} + +// What replaces the compare, not just that something did. Both properties here are what makes the +// rewrite exact rather than a tolerance, and neither is visible in the FAbs fingerprint above. +TEST_F(DemoteFloat64Test, TheRewriteComparesAbsAgainstTheModulesOwnZero) { + // `x == 0.0` -> `abs(x) <= 0.0`. The equality has to be INSIDE the replacement: with a strict + // `<` and no epsilon left to hide behind, +/-0 would stop comparing equal to zero. + ExpectComparedAgainstModuleZero(CompareAgainstUsing("float", "==", "0.0"), + "OpFOrdLessThanEqual", "OpFUnordLessThanEqual"); + // `x != 0.0` -> `abs(x) > 0.0`, the strict complement of the above. + ExpectComparedAgainstModuleZero(CompareAgainstUsing("float", "!=", "0.0"), "OpFOrdGreaterThan", + "OpFUnordGreaterThan"); +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp index f16de06a..d58f3f32 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp @@ -15,8 +15,6 @@ #include "source/opt/ir_builder.h" #include "source/opt/ir_context.h" #include "source/opt/module.h" -#include "source/opt/type_manager.h" -#include #include namespace MobileGL { @@ -29,7 +27,6 @@ namespace MobileGL { analysis::ConstantManager* const_mgr = context()->get_constant_mgr(); analysis::DefUseManager* def_use_mgr = context()->get_def_use_mgr(); - analysis::TypeManager* type_mgr = context()->get_type_mgr(); // 2. Import `GLSL.std.450` extension ID (for abs() func) uint32_t glsl_std_450_id = context()->get_feature_mgr()->GetExtInstImportId_GLSLstd450(); @@ -70,6 +67,10 @@ namespace MobileGL { uint32_t op2_id = inst.GetSingleWordInOperand(1); uint32_t var_id = 0; + // The zero the source spelled, reused verbatim as the right-hand side + // of the rewritten compare - so nothing has to be synthesized for a + // width this pass would have to encode by hand. + uint32_t zero_id = 0; // The constant's WIDTH decides which accessor may read it, and asking // the wrong one does not fail - it answers. @@ -78,8 +79,8 @@ namespace MobileGL { // bits. On a 64-bit constant words()[0] is the LOW half of the // mantissa, and that half is zero for every round double a shader // actually spells: 1.0lf, 2.0lf, 0.5lf, 100.0lf. Each of those - // therefore looked like 0.0 here, and `d != 1.0lf` was rewritten into - // `abs(d) >= epsilon` - which is TRUE for d == 1.0. That is the whole + // therefore looked like 0.0 here, and `d != 1.0lf` was rewritten into a + // test of `d` against ZERO - which is TRUE for d == 1.0. That is the whole // of KHR-GL43.compute_shader.fp64-case2: twelve uniforms compared // against vector and matrix constructors were untouched (a composite // is not a FloatConstant) and the one scalar comparison in the shader @@ -97,17 +98,24 @@ namespace MobileGL { const analysis::Float* floatType = floatConstant->type() != nullptr ? floatConstant->type()->AsFloat() : nullptr; if (floatType == nullptr) return false; + // Exactly zero - a near-zero constant is not a zero constant. + // `x == 1e-5` asks a different question than `x == 0.0` and must + // keep its own right-hand side. -0.0 compares equal to 0.0 here, + // which is correct: `x == -0.0` and `x == 0.0` are the same + // predicate in IEEE, and abs() maps both zeroes onto +0. switch (floatType->width()) { - case 32: return std::fabs(floatConstant->GetFloatValue()) <= K_EPSILON; - case 64: return std::fabs(floatConstant->GetDoubleValue()) <= K_EPSILON; + case 32: return floatConstant->GetFloatValue() == 0.0f; + case 64: return floatConstant->GetDoubleValue() == 0.0; default: return false; } }; if (is_float_zero(op2_id)) { var_id = op1_id; // x == 0.0 + zero_id = op2_id; } else if (is_float_zero(op1_id)) { var_id = op2_id; // 0.0 == x + zero_id = op1_id; } else { ++itInst; continue; @@ -120,12 +128,7 @@ namespace MobileGL { uint32_t float_type_id = def_use_mgr->GetDef(var_id)->type_id(); uint32_t bool_type_id = inst.type_id(); - // 2. Create constant ID for `Epsilon` - const analysis::Constant* eps_const = const_mgr->GetConstant( - type_mgr->GetType(float_type_id), {*(reinterpret_cast(&K_EPSILON))}); - uint32_t eps_id = const_mgr->GetDefiningInstruction(eps_const)->result_id(); - - // 3. Build Abs(x) inst + // 2. Build Abs(x) inst // OpExtInst %float_type %glsl_import Abs %x InstructionBuilder builder( context(), &inst, IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); @@ -140,38 +143,42 @@ namespace MobileGL { Instruction* abs_inst = builder.AddInstruction(MakeUnique( context(), spv::Op::OpExtInst, float_type_id, context()->TakeNextId(), abs_operands)); - // 4. build Abs(x) < Epsilon - // OpFOrdLessThan %bool_type %abs_val %eps - std::vector less_operands; - less_operands.push_back({SPV_OPERAND_TYPE_ID, {abs_inst->result_id()}}); - less_operands.push_back({SPV_OPERAND_TYPE_ID, {eps_id}}); + // 3. build Abs(x) <= 0.0, or Abs(x) > 0.0 for the NotEqual forms + // OpFOrdLessThanEqual %bool_type %abs_val %zero + std::vector cmp_operands; + cmp_operands.push_back({SPV_OPERAND_TYPE_ID, {abs_inst->result_id()}}); + cmp_operands.push_back({SPV_OPERAND_TYPE_ID, {zero_id}}); + // Equality is INCLUDED in the replacement, which is what makes the + // rewrite exact: |x| <= 0 is true for +0 and -0 and false for every + // other finite value, |x| > 0 is its complement. The ordered/unordered + // half of the opcode is preserved, so NaN keeps answering as it did. spv::Op replacementOp = spv::Op::OpNop; switch (inst.opcode()) { case spv::Op::OpFOrdEqual: - replacementOp = spv::Op::OpFOrdLessThan; + replacementOp = spv::Op::OpFOrdLessThanEqual; break; case spv::Op::OpFUnordEqual: - replacementOp = spv::Op::OpFUnordLessThan; + replacementOp = spv::Op::OpFUnordLessThanEqual; break; case spv::Op::OpFOrdNotEqual: - replacementOp = spv::Op::OpFOrdGreaterThanEqual; + replacementOp = spv::Op::OpFOrdGreaterThan; break; case spv::Op::OpFUnordNotEqual: - replacementOp = spv::Op::OpFUnordGreaterThanEqual; + replacementOp = spv::Op::OpFUnordGreaterThan; break; default: MOBILEGL_ASSERT(false, "Unexpected float compare opcode: %d", static_cast(inst.opcode())); break; } - Instruction* less_than_inst = builder.AddInstruction(MakeUnique( - context(), replacementOp, bool_type_id, context()->TakeNextId(), less_operands)); + Instruction* cmp_inst = builder.AddInstruction(MakeUnique( + context(), replacementOp, bool_type_id, context()->TakeNextId(), cmp_operands)); - // 5. Replaces all uses of old insn with new one - context()->ReplaceAllUsesWith(inst.result_id(), less_than_inst->result_id()); + // 4. Replaces all uses of old insn with new one + context()->ReplaceAllUsesWith(inst.result_id(), cmp_inst->result_id()); - // 6. Kill old instruction (will be cleaned up by DCE later) + // 5. Kill old instruction (will be cleaned up by DCE later) auto nextInstIt = context()->KillInst(&inst); if (nextInstIt) { itInst = nextInstIt; diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.h index 8e6ab37d..6c59a225 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.h @@ -15,15 +15,28 @@ namespace MobileGL { namespace MG_Util { namespace ShaderTranspiler { + // Keeps the driver's float-EQUALITY instruction out of the module: every scalar + // comparison against a constant 0.0 is re-spelled through GLSL.std.450 FAbs, so no + // OpFOrdEqual / OpFUnordEqual / OpFOrdNotEqual / OpFUnordNotEqual against zero ever + // reaches a shader compiler that gets exact float compare wrong. + // + // The rewrite is EXACT, not a tolerance. `x == 0.0` becomes `abs(x) <= 0.0` and + // `x != 0.0` becomes `abs(x) > 0.0`, both against the module's own zero constant: + // |x| <= 0 holds for +0 and -0 and for nothing else, so the two forms agree on every + // input, at any float width, with or without denormal flushing. The ordered/unordered + // half of the opcode is carried across unchanged, which is what keeps NaN answering + // the way it did before. + // + // It used to be an epsilon ball (abs(x) < 1e-4). That silently classified any + // legitimately small value as zero - KHR-GL3x.buffer_objects.triangles renders a + // specular term of ~6e-5 at a large render target and came out black - so the fuzz is + // gone; the reason the pass exists never needed it. class EliminateFloatEqualsZeroPass : public spvtools::opt::Pass { public: const char* name() const override { return "float-equals-zero-elimination"; } Status Process() override; static spvtools::Optimizer::PassToken CreateEliminateFloatEqualsZeroPass(); - - private: - const float K_EPSILON = 0.0001f; }; } // namespace ShaderTranspiler } // namespace MG_Util From d9abf1c2c1766103175b4b65e25a71002bd5d97f Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:07:38 -0400 Subject: [PATCH 03/21] [Fix] (DirectGLES): probe the real multisample texture sample counts instead of hardcoding one --- .../DirectGLES/BackendObject_DirectGLES.cpp | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp index 7d23f02e..1df1de0f 100644 --- a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp @@ -406,9 +406,12 @@ namespace MobileGL::MG_Backend::DirectGLES { return complete; } + // `samples` only reaches the multisample targets; every other target ignores it. The + // descending sample walk (ProbeTextureSampleCounts) reuses this whole routine rather than + // repeating the gen/bind/completeness/delete dance. Bool ProbeTexture(const MG_External::GLESFunctionsTable& gl, TextureTarget target, GLenum internalFormat, GLenum imageFormat, GLenum imageType, TextureInternalFormat logicalFormat, - Bool* outRenderable) { + Bool* outRenderable, Int samples = 1) { if (!IsGLESProbeTextureTarget(target) || !gl.glGenTextures || !gl.glBindTexture || !gl.glDeleteTextures) { return false; } @@ -428,10 +431,11 @@ namespace MobileGL::MG_Backend::DirectGLES { const Bool isMultisample = IsGLESProbeMultisampleTarget(target); if (isMultisample) { + const auto probeSamples = static_cast(std::max(samples, 1)); if (target == TextureTarget::Texture2DMultisample && gl.glTexStorage2DMultisample) { - gl.glTexStorage2DMultisample(glTarget, 1, internalFormat, 1, 1, GL_TRUE); + gl.glTexStorage2DMultisample(glTarget, probeSamples, internalFormat, 1, 1, GL_TRUE); } else if (target == TextureTarget::Texture2DMultisampleArray && gl.glTexStorage3DMultisample) { - gl.glTexStorage3DMultisample(glTarget, 1, internalFormat, 1, 1, 1, GL_TRUE); + gl.glTexStorage3DMultisample(glTarget, probeSamples, internalFormat, 1, 1, 1, GL_TRUE); } else { gl.glBindTexture(glTarget, static_cast(previousBinding)); gl.glDeleteTextures(1, &texture); @@ -527,6 +531,29 @@ namespace MobileGL::MG_Backend::DirectGLES { return sampleCounts; } + // The multisample TEXTURE twin of ProbeRenderbufferSampleCounts. It used to be a + // hardcoded {1}, which made glGetInternalformativ(GL_SAMPLES) claim a one-sample maximum + // for every format on the multisample targets even where glTexImage2DMultisample happily + // accepts four - GL 4.6 core 8.8 makes that query the definition of the maximum, so the + // two answers cannot both be right. Completeness is required at every count, exactly as + // the renderbuffer walk requires it; the caller only reaches here once the one-sample + // probe has already succeeded, so 1 terminates the list without being re-probed. + Vector ProbeTextureSampleCounts(const MG_External::GLESFunctionsTable& gl, TextureTarget target, + GLenum internalFormat, GLenum imageFormat, GLenum imageType, + TextureInternalFormat logicalFormat, Int maxSamples) { + Vector sampleCounts; + for (Int samples = std::max(maxSamples, 1); samples > 1; samples >>= 1) { + Bool renderable = false; + const Bool created = ProbeTexture(gl, target, internalFormat, imageFormat, imageType, logicalFormat, + &renderable, samples); + if (created && renderable) { + sampleCounts.push_back(samples); + } + } + sampleCounts.push_back(1); + return sampleCounts; + } + void PopulateFormatCapabilitiesImpl(const MG_External::GLESFunctionsTable& gl, const MG_External::GLESCapabilities& capabilities, FormatCapabilityCache& cache) { @@ -627,7 +654,11 @@ namespace MobileGL::MG_Backend::DirectGLES { AddFullFormatCaps(cache, targetIndex, formatIndex, BuildTextureCapsFromProbe(logicalFormat, target, nativeRenderable)); if (IsGLESProbeMultisampleTarget(target)) { - cache.SampleCounts[targetIndex][formatIndex] = {1}; + const Int maxSamples = + GetGLESFormatMaxSamples(capabilities, logicalFormat, nativeInfo.ImageFormat); + cache.SampleCounts[targetIndex][formatIndex] = ProbeTextureSampleCounts( + gl, probeTarget, nativeInfo.InternalFormat, nativeInfo.ImageFormat, + nativeInfo.ImageType, logicalFormat, maxSamples); } } shouldProbeFallback = !nativeCreated || !nativeRenderable; @@ -645,7 +676,11 @@ namespace MobileGL::MG_Backend::DirectGLES { LogGLESFormatCaveat(logicalFormat, targetIndex, fallbackInfo); } if (IsGLESProbeMultisampleTarget(target)) { - cache.SampleCounts[targetIndex][formatIndex] = {1}; + const Int maxSamples = + GetGLESFormatMaxSamples(capabilities, logicalFormat, fallbackInfo.ImageFormat); + cache.SampleCounts[targetIndex][formatIndex] = ProbeTextureSampleCounts( + gl, probeTarget, fallbackInfo.InternalFormat, fallbackInfo.ImageFormat, + fallbackInfo.ImageType, logicalFormat, maxSamples); } } } From f297af7d2b12545c03d31a4aa1d6644b0479f04b Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:10:45 -0400 Subject: [PATCH 04/21] [Fix] (GLImpl, DirectGLES, DirectVulkan): floor every advertised sample cap and clamp the realised count in the backends --- .../DirectGLES/BackendObject_DirectGLES.cpp | 24 +++++++++++ .../DirectGLES/BackendObject_DirectGLES.h | 10 +++++ MobileGL/MG_Backend/DirectGLES/Managers.cpp | 24 ++++++++--- .../Renderer/VkTextureManager.cpp | 13 ++++++ .../GLImpl/Framebuffer/GL_Framebuffer.cpp | 23 +++++++---- MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp | 20 +++++++-- MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.h | 4 ++ .../MG_Impl/GLImpl/Texture/GL_Texture.cpp | 41 +++++++++++++++++-- 8 files changed, 137 insertions(+), 22 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp index 1df1de0f..b35b82cc 100644 --- a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp @@ -8,6 +8,7 @@ #include "BackendObject_DirectGLES.h" #include "MG_Backend/BackendObject.h" +#include "MG_Backend/BackendObjects.h" #include #include #include @@ -782,6 +783,29 @@ namespace MobileGL::MG_Backend::DirectGLES { PopulateFormatCapabilitiesImpl(gl, capabilities, cache); } + Int ClampSamplesToBackendSupport(SizeT targetIndex, TextureInternalFormat logicalFormat, GLenum imageFormat, + Int samples) { + if (samples <= 1) { + return samples; + } + + Int maxSamples = 0; + const SizeT formatIndex = static_cast(logicalFormat); + if (pActiveBackendObject && targetIndex < kFormatCapabilityTargetCount && + formatIndex < kFormatCapabilityFormatCount) { + // Descending, so the head is the largest count this device actually allocated. + const Vector& probedCounts = + pActiveBackendObject->GetFormatCapabilities().SampleCounts[targetIndex][formatIndex]; + if (!probedCounts.empty()) { + maxSamples = probedCounts.front(); + } + } + if (maxSamples <= 0) { + maxSamples = GetGLESFormatMaxSamples(g_GLESCapabilities, logicalFormat, imageFormat); + } + return std::min(samples, std::max(maxSamples, 1)); + } + BackendObject_DirectGLES::~BackendObject_DirectGLES() { DestroyEGLContext(); } diff --git a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.h b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.h index 8aa4fdba..bb9a48be 100644 --- a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.h +++ b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.h @@ -18,6 +18,16 @@ namespace MobileGL::MG_Backend::DirectGLES { const MG_External::GLESCapabilities& capabilities, FormatCapabilityCache& cache); + // Clamps a requested sample count down to what the ES driver can really deliver for this + // format on this format-capability target: the probed per-format list when there is one, the + // driver's per-class GL_MAX_*_SAMPLES otherwise. The frontend deliberately validates against + // the count MobileGL advertises instead (GL_Getter's GetAdvertisedMaxSamples), which on a + // driver reporting GL_MAX_INTEGER_SAMPLES 1 is higher than the driver accepts, so every ES + // allocation call has to come through here. The shadow state keeps the requested count, so + // GL_TEXTURE_SAMPLES and framebuffer completeness still answer what the application asked for. + Int ClampSamplesToBackendSupport(SizeT targetIndex, TextureInternalFormat logicalFormat, GLenum imageFormat, + Int samples); + class BackendObject_DirectGLES : public BackendObject { public: ~BackendObject_DirectGLES() override; diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 45d32f75..a279a4ec 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -9,6 +9,7 @@ #include "Managers.h" #include "Utils.h" #include "DirectGLES.h" +#include "BackendObject_DirectGLES.h" #include #include @@ -2648,16 +2649,24 @@ namespace MobileGL::MG_Backend::DirectGLES { if (TextureImpl::IsMultisampleTextureTarget(targetInternal)) { DebugImpl::ErrorLopper::Clear(); BufferImpl::BindPixelUnpackBufferId(0); // no-op once the resting 0 state is pinned + // The frontend validates against the count MobileGL advertises, which can + // exceed what the driver takes for this format (Adreno: GL_MAX_SAMPLES 4, + // GL_MAX_INTEGER_SAMPLES 1). Clamp the ES call - and only the ES call: + // stateTextureObject keeps the requested count so GL_TEXTURE_SAMPLES and + // framebuffer completeness still report what the application asked for. + const auto backendSamples = static_cast(ClampSamplesToBackendSupport( + GetFormatCapabilityTargetIndex(targetInternal), textureMipmapObject->GetFormat(), + glFormat, static_cast(stateTextureObject->GetSamples()))); switch (targetInternal) { case TextureTarget::Texture2DMultisample: g_GLESFuncs.glTexStorage2DMultisample( - target, static_cast(stateTextureObject->GetSamples()), glInternalFormat, + target, backendSamples, glInternalFormat, static_cast(baseSize.x()), static_cast(baseSize.y()), stateTextureObject->HasFixedSampleLocations() ? GL_TRUE : GL_FALSE); break; case TextureTarget::Texture2DMultisampleArray: g_GLESFuncs.glTexStorage3DMultisample( - target, static_cast(stateTextureObject->GetSamples()), glInternalFormat, + target, backendSamples, glInternalFormat, static_cast(baseSize.x()), static_cast(baseSize.y()), static_cast(baseSize.z()), stateTextureObject->HasFixedSampleLocations() ? GL_TRUE : GL_FALSE); @@ -5621,9 +5630,14 @@ namespace MobileGL::MG_Backend::DirectGLES { TextureImpl::GenerateRenderbufferFormatInfo(internalFormat, &glInternalFormat, &glFormat, &glType); if (samples > 0) { - g_GLESFuncs.glRenderbufferStorageMultisample( - GL_RENDERBUFFER, static_cast(samples), glInternalFormat, static_cast(width), - static_cast(height)); + // Same clamp as the multisample texture path: the frontend accepts the count it + // advertised, the driver only takes the count it supports for this format, and + // the state object keeps reporting the requested one. + const auto backendSamples = static_cast(ClampSamplesToBackendSupport( + GetRenderbufferFormatCapabilityTargetIndex(), internalFormat, glFormat, samples)); + g_GLESFuncs.glRenderbufferStorageMultisample(GL_RENDERBUFFER, backendSamples, glInternalFormat, + static_cast(width), + static_cast(height)); } else { g_GLESFuncs.glRenderbufferStorage(GL_RENDERBUFFER, glInternalFormat, static_cast(width), static_cast(height)); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.cpp index 233f472d..4c5f7ac7 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.cpp @@ -1848,6 +1848,19 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } } + if (rounded == 0 && (supported & VK_SAMPLE_COUNT_1_BIT) != 0) { + // Nothing at two samples or above. Reachable because the frontend validates + // multisample allocations against the count MobileGL ADVERTISES (GL requires + // GL_MAX_SAMPLES >= 4) rather than against the device's per-format support, so + // a format this device cannot multisample at all now gets here instead of + // being refused up front. Keeping the unsupported count would hand + // vkCreateImage an invalid VkImageCreateInfo; one sample is at least a legal + // image, and the samples-08726 hazard above is the lesser of the two. + MGLOG_W_ONCE("Multisample texture format %d supports no count above one on this device; " + "backing it with a single sample", + static_cast(format)); + rounded = static_cast(VK_SAMPLE_COUNT_1_BIT); + } if (rounded != 0) { resolvedSampleCount = static_cast(rounded); } diff --git a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp index 2b2701fc..39638faa 100644 --- a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp +++ b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -617,16 +618,17 @@ namespace MobileGL::MG_Impl::GLImpl { if (MG_Backend::pActiveBackendObject == nullptr) { return std::numeric_limits::max(); } - return std::max(MG_Backend::pActiveBackendObject->GetDynamicParameters().MaxSamples, 1); + return GetAdvertisedMaxSamples(); } - // GL_MAX_SAMPLES is the ceiling over all formats; an integer format has its own, lower - // one (GL_MAX_INTEGER_SAMPLES) and GL 4.6 core 9.2.4 makes exceeding it INVALID_OPERATION. - // The multisample TEXTURE path already resolves the limit per format - // (GL_Texture.cpp, GetMaxTextureSamplesForFormat); renderbuffers only ever compared - // against GL_MAX_SAMPLES, so on a driver where the two differ - Adreno reports - // GL_MAX_SAMPLES 4 and GL_MAX_INTEGER_SAMPLES 1 - an integer renderbuffer accepted a - // sample count the format cannot deliver, and said GL_NO_ERROR about it. + // GL_MAX_SAMPLES is the ceiling over all formats; an integer format has its own + // (GL_MAX_INTEGER_SAMPLES) and GL 4.6 core 9.2.4 makes exceeding it INVALID_OPERATION. + // The multisample TEXTURE path resolves the limit per format the same way + // (GL_Texture.cpp, GetMaxSupportedTextureSamples). Both are floored to the value MobileGL + // advertises: on a driver where the two differ - Adreno reports GL_MAX_SAMPLES 4 and + // GL_MAX_INTEGER_SAMPLES 1 - rejecting the advertised count here only moves the failure + // from the driver into MobileGL, so the frontend accepts it and the backend clamps the + // count it actually hands the driver. Int GetMaxRenderbufferSamplesForFormat_State(TextureInternalFormat format) { if (MG_Backend::pActiveBackendObject == nullptr) { return std::numeric_limits::max(); @@ -645,7 +647,10 @@ namespace MobileGL::MG_Impl::GLImpl { if (!isIntegerFormat) { return GetMaxRenderbufferSamples_State(); } - return std::max(dynamicParameters.MaxIntegerSamples, 1); + // Per-format still, but never below the ceiling glGetIntegerv(GL_MAX_SAMPLES) promised: + // the driver's raw GL_MAX_INTEGER_SAMPLES stays the *backend* limit and the backend + // clamps to it, while the frontend honours what it advertised. + return std::max(dynamicParameters.MaxIntegerSamples, GetAdvertisedMaxSamples()); } Bool ValidateRenderbufferStorageSize_State(GLsizei width, GLsizei height, const char* caller) { diff --git a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp index 893f4909..091eb225 100644 --- a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp +++ b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp @@ -422,6 +422,18 @@ namespace MobileGL::MG_Impl::GLImpl { } } // namespace + // GL 4.6 core table 23.53 requires GL_MAX_SAMPLES >= 4, so the driver's value is floored + // before it is advertised. Every other multisample ceiling MobileGL advertises has to be + // floored the same way: promising 4 samples globally while answering GL_MAX_INTEGER_SAMPLES + // 1 - which is exactly what Adreno reports - makes the frontend reject the very count it + // just told the application to use. The backends clamp the realised count instead. + GLint GetAdvertisedMaxSamples() { + if (MG_Backend::pActiveBackendObject == nullptr) { + return kFrontendMaxSamples; + } + return std::max(MG_Backend::pActiveBackendObject->GetDynamicParameters().MaxSamples, kFrontendMaxSamples); + } + /* @INSERTION_POINT:FUNCTION_IMPLEMENTATION@ */ const GLubyte* GetString(GLenum name) { static String vendorString; @@ -2117,7 +2129,7 @@ namespace MobileGL::MG_Impl::GLImpl { *params = dynamicParameters.MaxClipDistances; break; case GL_MAX_COLOR_TEXTURE_SAMPLES: - *params = dynamicParameters.MaxColorTextureSamples; + *params = std::max(dynamicParameters.MaxColorTextureSamples, GetAdvertisedMaxSamples()); break; case GL_MAX_COMBINED_FRAGMENT_UNIFORM_COMPONENTS: *params = GetMaxCombinedUniformComponents(kFrontendMaxFragmentUniformComponents, @@ -2147,7 +2159,7 @@ namespace MobileGL::MG_Impl::GLImpl { *params = dynamicParameters.MaxCubeMapTextureSize; break; case GL_MAX_DEPTH_TEXTURE_SAMPLES: - *params = dynamicParameters.MaxDepthTextureSamples; + *params = std::max(dynamicParameters.MaxDepthTextureSamples, GetAdvertisedMaxSamples()); break; case GL_MAX_FRAMEBUFFER_WIDTH: *params = dynamicParameters.MaxFramebufferWidth; @@ -2174,7 +2186,7 @@ namespace MobileGL::MG_Impl::GLImpl { *params = dynamicParameters.MaxComputeImageUniforms; break; case GL_MAX_INTEGER_SAMPLES: - *params = dynamicParameters.MaxIntegerSamples; + *params = std::max(dynamicParameters.MaxIntegerSamples, GetAdvertisedMaxSamples()); break; case GL_MAX_RENDERBUFFER_SIZE: *params = dynamicParameters.MaxRenderbufferSize; @@ -2340,7 +2352,7 @@ namespace MobileGL::MG_Impl::GLImpl { : dynamicParameters.MaxDrawBuffers; break; case GL_MAX_SAMPLES: - *params = std::max(dynamicParameters.MaxSamples, kFrontendMaxSamples); + *params = GetAdvertisedMaxSamples(); break; case GL_MAX_TEXTURE_MAX_ANISOTROPY_EXT: // Float state (see GetFloatv); rounded to nearest for the integer query per GL 3.3 6.1.2. diff --git a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.h b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.h index 29df6e87..ee8faf65 100644 --- a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.h +++ b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.h @@ -24,4 +24,8 @@ namespace MobileGL::MG_Impl::GLImpl { void GetInteger64i_v(GLenum target, GLuint index, GLint64* data); GLenum GetError(); GLenum GetGraphicsResetStatus(); + // The GL_MAX_SAMPLES value MobileGL advertises, i.e. the driver's value floored to the GL + // core minimum. Frontend multisample validators have to honour this ceiling for every + // format, otherwise MobileGL rejects a sample count it advertised itself. + GLint GetAdvertisedMaxSamples(); } // namespace MobileGL::MG_Impl::GLImpl diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index f1327ef3..5e960ad9 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -474,15 +474,48 @@ namespace MobileGL::MG_Impl::GLImpl { target == TextureTarget::Texture2DMultisampleArray; } - Int GetMaxSupportedTextureSamples(TextureInternalFormat textureInternalFormat) { + // The largest count the backend actually probed for this format on this target, or 0 when + // it has no answer for the pair. Both backends build the list in descending order. + Int GetProbedMaxTextureSamples(TextureTarget textureTarget, TextureInternalFormat textureInternalFormat) { + if (MG_Backend::pActiveBackendObject == nullptr) { + return 0; + } + const SizeT targetIndex = MG_Backend::GetFormatCapabilityTargetIndex(textureTarget); + const SizeT formatIndex = static_cast(textureInternalFormat); + if (targetIndex >= MG_Backend::kFormatCapabilityTargetCount || + formatIndex >= MG_Backend::kFormatCapabilityFormatCount) { + return 0; + } + const auto& sampleCounts = + MG_Backend::pActiveBackendObject->GetFormatCapabilities().SampleCounts[targetIndex][formatIndex]; + return sampleCounts.empty() ? 0 : sampleCounts.front(); + } + + // The ceiling the frontend enforces, which must never be lower than the one MobileGL + // advertises: the CTS - and real applications - read GL_MAX_SAMPLES once and hand that + // exact count to glTexImage*Multisample for every format. Answering 4 there and then + // rejecting 4 here because the ES driver reports GL_MAX_INTEGER_SAMPLES 1 (Adreno) is a + // self-inconsistency, not a spec-mandated error. The backends clamp the count they hand + // the driver; the shadow state keeps reporting what the application asked for. + Int GetMaxSupportedTextureSamples(TextureTarget textureTarget, + TextureInternalFormat textureInternalFormat) { if (MG_Backend::pActiveBackendObject == nullptr) { return std::numeric_limits::max(); } + const Int advertisedMaxSamples = GetAdvertisedMaxSamples(); + // glGetInternalformativ(GL_SAMPLES) is answered from this very list (GetInternalformativ + // below), and GL 4.6 core 8.8 makes that query the definition of the per-format + // maximum - validating against anything else is how the two answers drifted apart. + const Int probedMaxSamples = GetProbedMaxTextureSamples(textureTarget, textureInternalFormat); + if (probedMaxSamples > 0) { + return std::max(probedMaxSamples, advertisedMaxSamples); + } + const auto& dynamicParameters = MG_Backend::pActiveBackendObject->GetDynamicParameters(); if (MG_Util::IsDepthFormatInternalFormat(textureInternalFormat) || MG_Util::IsStencilFormatInternalFormat(textureInternalFormat)) { - return std::max(dynamicParameters.MaxDepthTextureSamples, 1); + return std::max(dynamicParameters.MaxDepthTextureSamples, advertisedMaxSamples); } GLenum normalizedInternalFormat = MG_Util::ConvertTextureInternalFormatToGLEnum(textureInternalFormat); @@ -495,7 +528,7 @@ namespace MobileGL::MG_Impl::GLImpl { normalizedFormat == GL_RGB_INTEGER || normalizedFormat == GL_RGBA_INTEGER; return std::max(isIntegerFormat ? dynamicParameters.MaxIntegerSamples : dynamicParameters.MaxColorTextureSamples, - 1); + advertisedMaxSamples); } Bool ValidateTextureMultisampleStorage(TextureTarget textureTarget, GLsizei samples, GLsizei width, @@ -532,7 +565,7 @@ namespace MobileGL::MG_Impl::GLImpl { // dimensions, and GL CTS's per-case state reset (gluStateReset) clears the default // GL_TEXTURE_2D_MULTISAMPLE_ARRAY texture with glTexImage3DMultisample(..., 0, 0, 0). - const Int maxSamples = GetMaxSupportedTextureSamples(textureInternalFormat); + const Int maxSamples = GetMaxSupportedTextureSamples(textureTarget, textureInternalFormat); if (samples > maxSamples) { // GL specifies INVALID_OPERATION - not INVALID_VALUE - when the sample count // exceeds what the format supports, and the native Adreno driver agrees. From f17cb23ea3686127de078ffe7689d43abc2e484c Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:15:46 -0400 Subject: [PATCH 05/21] [Fix, Test] (GLImpl, DirectGLES): deallocate zero-sized multisample images instead of defining them --- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 62 +++++++++++++------ .../MG_Impl/GLImpl/Texture/GL_Texture.cpp | 32 ++++++++++ MobileGL/MG_Test/Texture/TextureTest.cpp | 52 ++++++++++++++++ 3 files changed, 128 insertions(+), 18 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index a279a4ec..c5d3227b 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -2657,25 +2657,51 @@ namespace MobileGL::MG_Backend::DirectGLES { const auto backendSamples = static_cast(ClampSamplesToBackendSupport( GetFormatCapabilityTargetIndex(targetInternal), textureMipmapObject->GetFormat(), glFormat, static_cast(stateTextureObject->GetSamples()))); - switch (targetInternal) { - case TextureTarget::Texture2DMultisample: - g_GLESFuncs.glTexStorage2DMultisample( - target, backendSamples, glInternalFormat, - static_cast(baseSize.x()), static_cast(baseSize.y()), - stateTextureObject->HasFixedSampleLocations() ? GL_TRUE : GL_FALSE); - break; - case TextureTarget::Texture2DMultisampleArray: - g_GLESFuncs.glTexStorage3DMultisample( - target, backendSamples, glInternalFormat, - static_cast(baseSize.x()), static_cast(baseSize.y()), - static_cast(baseSize.z()), - stateTextureObject->HasFixedSampleLocations() ? GL_TRUE : GL_FALSE); - break; - default: - MOBILEGL_ASSERT(false, "Unexpected multisample target: %d", static_cast(targetInternal)); - break; + // ES 3.1 8.19 requires width/height (and depth, for the array target) >= 1, + // so a degenerate size has nothing to allocate and must not reach the + // driver. The frontend deallocates such an image rather than defining it + // (GL 4.6 core 8.8), so this is belt and braces for any path that still + // syncs one. + const Bool hasAllocatableSize = + baseSize.x() >= 1 && baseSize.y() >= 1 && + (targetInternal != TextureTarget::Texture2DMultisampleArray || baseSize.z() >= 1); + if (!hasAllocatableSize) { + MGLOG_D("Skipping multisample storage for texture %u: degenerate size (%d, %d, %d)", + m_backendTextureId, baseSize.x(), baseSize.y(), baseSize.z()); + } else { + switch (targetInternal) { + case TextureTarget::Texture2DMultisample: + g_GLESFuncs.glTexStorage2DMultisample( + target, backendSamples, glInternalFormat, + static_cast(baseSize.x()), static_cast(baseSize.y()), + stateTextureObject->HasFixedSampleLocations() ? GL_TRUE : GL_FALSE); + break; + case TextureTarget::Texture2DMultisampleArray: + g_GLESFuncs.glTexStorage3DMultisample( + target, backendSamples, glInternalFormat, + static_cast(baseSize.x()), static_cast(baseSize.y()), + static_cast(baseSize.z()), + stateTextureObject->HasFixedSampleLocations() ? GL_TRUE : GL_FALSE); + break; + default: + MOBILEGL_ASSERT(false, "Unexpected multisample target: %d", + static_cast(targetInternal)); + break; + } + m_backendStorageImmutable = true; } - m_backendStorageImmutable = true; + // The one storage branch that cleared the ES error queue without ever + // draining it again, so anything this call raised was left for an + // unrelated later query to trip over. Paired with its two siblings now. + DebugImpl::ErrorLopper::Loop([file = __FILE__, line = __LINE__, func = __func__, target, + glInternalFormat, backendSamples](GLenum err) { + MGLOG_D("%s(%s:%d) ES error: %s. glTexStorage*Multisample: target=%s, internalformat=%s, " + "samples=%d", + func, file, line, MG_Util::ConvertGLEnumToString(err).c_str(), + MG_Util::ConvertGLEnumToString(target).c_str(), + MG_Util::ConvertGLEnumToString(glInternalFormat).c_str(), + static_cast(backendSamples)); + }); for (const auto& uploadTarget : uploadTargets) { for (SizeT level = 0; level < mipmapCount; ++level) { textureMipmapObject->MarkStorageDirty(uploadTarget, level, false); diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 5e960ad9..c35b527c 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -590,6 +590,20 @@ namespace MobileGL::MG_Impl::GLImpl { "AllocateMultisampleTextureStorage requires mipmap-backed storage"); auto* textureMipmapObject = static_cast(textureObject.get()); + // GL 4.6 core 8.8: a zero-sized image DEALLOCATES the image rather than defining an + // empty one. Only the multisample pair cares, and it cares a great deal: the CTS's + // per-case state reset clears both DEFAULT multisample textures this way on every + // texture unit, and a "defined" 0x0 default texture stops being skipped by + // IsUndefinedDefaultTexture - it then joins the per-draw sync and bind passes on + // every unit the reset touched, and reaches an ES glTexStorage*Multisample(..., 0, 0) + // that ES 3.1 8.19 makes INVALID_VALUE on every driver there is. A proxy target holds + // no image at all, only the query result, so it keeps recording what was asked for. + if ((width <= 0 || height <= 0 || depth <= 0) && + !TextureImpl::IsProxyTextureTarget(textureUploadTarget)) { + textureObject->SetInternalFormat(TextureInternalFormat::Unknown); + textureMipmapObject->TruncateMipmapLevels(textureUploadTarget, 0); + return; + } textureObject->SetInternalFormat(textureInternalFormat); textureObject->SetSamples(samples); textureObject->SetFixedSampleLocations(fixedsamplelocations == GL_TRUE); @@ -4717,6 +4731,22 @@ namespace MobileGL::MG_Impl::GLImpl { TextureStorage3D(textureObject->GetExternalIndex(), levels, internalformat, width, height, depth); } + // Unlike glTexImage*Multisample, where a zero-sized image is a legal deallocation (see + // AllocateMultisampleTextureStorage), the immutable forms take a strictly positive size: GL + // 4.6 core 8.19 makes width, height or depth < 1 INVALID_VALUE. Without this the shared + // _State helper would deallocate the image and TexStorageMultisample_State would then freeze + // the now-imageless texture as immutable. + static Bool ValidateTexStorageMultisampleSize(GLsizei width, GLsizei height, GLsizei depth, const char* caller) { + if (width >= 1 && height >= 1 && depth >= 1) { + return true; + } + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", caller, + "Immutable multisample storage requires width, height and depth >= 1.")); + return false; + } + // The multisample storage forms allocate exactly what the glTexImage*Multisample ones do, and // then freeze it: TEXTURE_IMMUTABLE_FORMAT becomes TRUE and a second call is INVALID_OPERATION // (GL 4.6 core 8.19). Only the allocation was shared before, so a multisample texture stayed @@ -4737,6 +4767,7 @@ namespace MobileGL::MG_Impl::GLImpl { const TextureTarget textureTarget = MG_Util::ConvertGLEnumToTextureTarget(target); auto& activeUnit = MG_State::pGLContext->GetTextureUnitObject(MG_State::pGLContext->GetActiveTextureUnit()); if (!ValidateTextureMutable(activeUnit.GetBindingSlot(textureTarget).GetBoundObject(), __func__)) return; + if (!ValidateTexStorageMultisampleSize(width, height, 1, __func__)) return; TexStorageMultisample_State( target, TexImage2DMultisample_State(target, samples, internalformat, width, height, fixedsamplelocations), __func__); @@ -4747,6 +4778,7 @@ namespace MobileGL::MG_Impl::GLImpl { const TextureTarget textureTarget = MG_Util::ConvertGLEnumToTextureTarget(target); auto& activeUnit = MG_State::pGLContext->GetTextureUnitObject(MG_State::pGLContext->GetActiveTextureUnit()); if (!ValidateTextureMutable(activeUnit.GetBindingSlot(textureTarget).GetBoundObject(), __func__)) return; + if (!ValidateTexStorageMultisampleSize(width, height, depth, __func__)) return; TexStorageMultisample_State(target, TexImage3DMultisample_State(target, samples, internalformat, width, height, depth, fixedsamplelocations), diff --git a/MobileGL/MG_Test/Texture/TextureTest.cpp b/MobileGL/MG_Test/Texture/TextureTest.cpp index 7d98521c..19279b0d 100644 --- a/MobileGL/MG_Test/Texture/TextureTest.cpp +++ b/MobileGL/MG_Test/Texture/TextureTest.cpp @@ -2975,6 +2975,58 @@ TEST_F(TextureTest, CtsStyleStateResetOnDefaultTexturesLeavesNoError) { EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR) << "GL_TEXTURE_2D_MULTISAMPLE_ARRAY reset failed"; } +// Clean is not enough: per GL 4.6 core 8.8 that zero-sized reset has to DEALLOCATE the image, +// not define an empty one. gluStateReset runs it on both default multisample textures on every +// texture unit of a 3.2+ context, and a default texture left 'defined' afterwards stops being +// skipped by IsUndefinedDefaultTexture - it then joins the per-draw sync and bind passes on +// every unit the reset touched and reaches an ES glTexStorage*Multisample(..., 0, 0), which ES +// 3.1 8.19 rejects on every driver. +TEST_F(TextureTest, ZeroSizedMultisampleTexImageDeallocatesTheImage) { + MG_Impl::GLImpl::ActiveTexture(GL_TEXTURE0); + + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D_MULTISAMPLE, 0); + const auto& defaultMultisample = MG_State::pGLContext->GetTextureUnitObject(0) + .GetBindingSlot(TextureTarget::Texture2DMultisample) + .GetBoundObject(); + MG_Impl::GLImpl::TexImage2DMultisample(GL_TEXTURE_2D_MULTISAMPLE, 1, GL_RGBA8, 4, 4, GL_TRUE); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + ASSERT_FALSE(MG_State::GLState::IsUndefinedDefaultTexture(defaultMultisample.get())); + + MG_Impl::GLImpl::TexImage2DMultisample(GL_TEXTURE_2D_MULTISAMPLE, 1, GL_RGBA8, 0, 0, GL_TRUE); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + EXPECT_TRUE(MG_State::GLState::IsUndefinedDefaultTexture(defaultMultisample.get())); + + // The array target's reset also passes zero LAYERS, which deallocates just the same. + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D_MULTISAMPLE_ARRAY, 0); + const auto& defaultMultisampleArray = MG_State::pGLContext->GetTextureUnitObject(0) + .GetBindingSlot(TextureTarget::Texture2DMultisampleArray) + .GetBoundObject(); + MG_Impl::GLImpl::TexImage3DMultisample(GL_TEXTURE_2D_MULTISAMPLE_ARRAY, 1, GL_RGBA8, 4, 4, 2, GL_TRUE); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + ASSERT_FALSE(MG_State::GLState::IsUndefinedDefaultTexture(defaultMultisampleArray.get())); + + MG_Impl::GLImpl::TexImage3DMultisample(GL_TEXTURE_2D_MULTISAMPLE_ARRAY, 1, GL_RGBA8, 4, 4, 0, GL_TRUE); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + EXPECT_TRUE(MG_State::GLState::IsUndefinedDefaultTexture(defaultMultisampleArray.get())); + + // The immutable forms do NOT share that leniency: GL 4.6 core 8.19 makes a size below 1 + // INVALID_VALUE, and freezing an imageless texture as immutable would be unrecoverable. + GLuint texture = 0; + MG_Impl::GLImpl::GenTextures(1, &texture); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D_MULTISAMPLE, texture); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + MG_Impl::GLImpl::TexStorage2DMultisample(GL_TEXTURE_2D_MULTISAMPLE, 1, GL_RGBA8, 0, 0, GL_TRUE); + ExpectSingleGlError(GL_INVALID_VALUE); + EXPECT_FALSE(MG_State::pGLContext->GetTextureUnitObject(0) + .GetBindingSlot(TextureTarget::Texture2DMultisample) + .GetBoundObject() + ->IsImmutable()); + + MG_Impl::GLImpl::DeleteTextures(1, &texture); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D_MULTISAMPLE, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); +} + // ---- GL CTS packed_pixels / texture_swizzle readback root-cause regressions -------------------- TEST_F(TextureTest, NormalizeLegacySizedFormatsMapToCanonicalShadowLayouts) { From c1b3b16cab18127d79e2e02339b9894c6035701f Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:21:09 -0400 Subject: [PATCH 06/21] [Fix] (DirectGLES): drain the ES error queue in ErrorLopper's non-debug arm too --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index a22ac79d..2259ebb1 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -300,10 +300,32 @@ namespace MobileGL::MG_Backend::DirectGLES { Clear(); } #else - void ErrorLopper::Loop(const std::function& func) {} - void ErrorLopper::Clear() {} - ErrorLopper::ErrorLopper() = default; - ErrorLopper::~ErrorLopper() = default; + // Error HYGIENE is not a debugging feature: every site that brackets a risky ES call with + // Clear()/Loop() relied on these to empty the driver's queue, and compiling them to + // nothing left whatever the driver raised sitting there for an unrelated later + // `glGetError() == GL_NO_ERROR` probe to read as its own failure. The callback stays + // unused because MGLOG_D is compiled out at this level, but the queue still gets drained. + // Bounded like DrainESErrors: a driver that never returns GL_NO_ERROR (a lost context is + // the usual way) must not spin here. + constexpr Int kMaxDrainedESErrors = 32; + + void ErrorLopper::Loop(const std::function& func) { + static_cast(func); + for (Int i = 0; i < kMaxDrainedESErrors && g_GLESFuncs.glGetError() != GL_NO_ERROR; ++i) { + } + } + + void ErrorLopper::Clear() { + for (Int i = 0; i < kMaxDrainedESErrors && g_GLESFuncs.glGetError() != GL_NO_ERROR; ++i) { + } + } + + ErrorLopper::ErrorLopper() { + Clear(); + } + ErrorLopper::~ErrorLopper() { + Clear(); + } #endif #if MOBILEGL_LOG_ACTIVE_LEVEL <= MOBILEGL_LOG_LEVEL_DEBUG From bbc7b9ca841cc994960e9f79f29e94cc3b12daa6 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:21:10 -0400 Subject: [PATCH 07/21] [Fix] (DirectGLES): report refused renderbuffer storage and collect dead backend twins on object churn --- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 20 +++++++++++++++++ MobileGL/MG_Backend/DirectGLES/Managers.h | 25 +++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index c5d3227b..efb45795 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -5655,6 +5655,14 @@ namespace MobileGL::MG_Backend::DirectGLES { GLenum glInternalFormat, glType, glFormat; TextureImpl::GenerateRenderbufferFormatInfo(internalFormat, &glInternalFormat, &glFormat, &glType); + // The allocation is deferred to here, so an ES driver that refuses it (a + // multi-gigabyte renderbuffer is refused routinely) used to leave m_isInitialized + // true over a renderbuffer with no storage and say nothing at all: the attachment + // then rendered nowhere. Drain first so the check cannot pick up an unrelated stale + // flag, and report GL_OUT_OF_MEMORY to the application. The error lands on whatever + // entry point triggered the sync rather than on glRenderbufferStorage itself, which + // is where the deferred model puts it - still far better than silence. + DebugImpl::ErrorLopper::Clear(); if (samples > 0) { // Same clamp as the multisample texture path: the frontend accepts the count it // advertised, the driver only takes the count it supports for this format, and @@ -5668,6 +5676,18 @@ namespace MobileGL::MG_Backend::DirectGLES { g_GLESFuncs.glRenderbufferStorage(GL_RENDERBUFFER, glInternalFormat, static_cast(width), static_cast(height)); } + if (g_GLESFuncs.glGetError() == GL_OUT_OF_MEMORY) { + MGLOG_E_ONCE("Renderbuffer %u storage allocation ran out of memory: %dx%d, samples=%d, format=%s", + stateRBOObject->GetExternalIndex(), width, height, samples, + MG_Util::ConvertGLEnumToString(glInternalFormat).c_str()); + if (MG_State::pGLContext) { + MG_State::pGLContext->RecordError( + ErrorCode::OutOfMemory, + MakeUnique("DirectGLES", "BackendRenderbufferObject::SyncToBackend", + "The ES driver could not allocate the renderbuffer storage.")); + } + } + DebugImpl::ErrorLopper::Clear(); m_cacheInternalFormat = internalFormat; m_cacheWidth = width; diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 34eb45e0..f60a038b 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -129,7 +129,28 @@ namespace MobileGL::MG_Backend::DirectGLES { // Twin creation is the moment a driver-owned id starts needing a guarded // destructor; cold path, so the once-guard costs nothing per draw. EnsureProcessTeardownSentinel(); + // Sweep BEFORE the entry reference below exists: the map is open-addressed and an + // erase relocates the rest of the probe cluster, so collecting once that reference + // is taken would invalidate it. The sweep is therefore owed from an earlier call + // rather than triggered by this one. + if (m_creationTick >= kCreationGCInterval) { + m_creationTick = 0; + CollectGarbage(); + } + const SizeT entryCountBeforeInsert = m_entries.size(); auto& entry = m_entries[stateObj.get()]; + if (m_entries.size() != entryCountBeforeInsert) { + // A key the registry has never held. Nothing tells the backend that a texture or + // renderbuffer was DELETED - the twin, and the driver storage it owns, lives + // until a collection - and CollectGarbageIfNeeded is ticked only from the + // per-draw sync paths, which a CTS-shaped workload runs about ten times per + // case. 1024 of those ticks then span ~100 cases, so ~100 cases' worth of dead + // (and, for this suite, gigabyte-sized) objects stay allocated at once. Object + // CHURN rather than draw count is what makes the sweep urgent, so a twin the + // registry has never seen ticks it too - and it does so on the path that is + // about to allocate, which is exactly when the memory is needed. + ++m_creationTick; + } if (entry.stateRef.expired()) { // The previous owner of this address is gone and the allocator handed it // to a new object: its twin describes ids the new state object never made. @@ -203,8 +224,12 @@ namespace MobileGL::MG_Backend::DirectGLES { private: static constexpr Uint32 kGCInterval = 1024; + // Creations are far rarer than draws, so this counts in a much smaller unit than + // kGCInterval does. + static constexpr Uint32 kCreationGCInterval = 64; BackendMap m_entries; Uint32 m_gcTick = 0; + Uint32 m_creationTick = 0; Bool m_isCollecting = false; }; From 261cfd1591a8482326e1818b118d563e0a3e4400 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:31:22 -0400 Subject: [PATCH 08/21] [Fix] (DirectGLES): retry a failed blit's colour and depth/stencil aspects independently --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 46 +++++++++++++++---- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 2259ebb1..71eef18d 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -4646,16 +4646,46 @@ namespace MobileGL::MG_Backend::DirectGLES { } return; } - if (readSamples <= 0 || drawSamples > 0 || (mask & GL_COLOR_BUFFER_BIT) == 0) { - return; - } - if (ResolveThenBlit(srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, filter) && - (mask & ~static_cast(GL_COLOR_BUFFER_BIT)) != 0) { - DrainBlitErrors(); - g_GLESFuncs.glBlitFramebuffer(srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, - mask & ~static_cast(GL_COLOR_BUFFER_BIT), filter); + // The combined call raised an error, so by GL 4.6 2.3.1 it wrote nothing at all: BOTH + // aspect groups still owe their copy, and each has to be retried on its own. Re-issuing + // the depth/stencil half only as a rider on a SUCCESSFUL colour resolve dropped it + // silently whenever the colour half could not be emulated - and on a framebuffer whose + // only attachment is depth it never can, because the colour emulation has no attachment + // to take a format from (KHR-GL33.framebuffer_blit's depth config test blits + // COLOR|DEPTH|STENCIL across depth-only framebuffers and kept reading the clear value). + const GLbitfield colourBit = mask & static_cast(GL_COLOR_BUFFER_BIT); + const GLbitfield dsBits = mask & static_cast(GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT); + // The colour group's one emulation is the multisample resolve that also converts format, + // which is the shape this names. It used to double as an early-out for the whole + // function, which is what cost a depth-only mask its single-aspect retry. + const Bool multisampleResolve = readSamples > 0 && drawSamples <= 0; + if (colourBit != 0) { DrainBlitErrors(); + g_GLESFuncs.glBlitFramebuffer(srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, colourBit, filter); + if (g_GLESFuncs.glGetError() != GL_NO_ERROR) { + const Bool emulated = + multisampleResolve && + ResolveThenBlit(srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, filter); + if (!emulated) { + MGLOG_E_ONCE("BlitFramebuffer: the colour aspect was dropped - the driver rejected it on its " + "own and no emulation applies"); + } + } } + if (dsBits != 0) { + DrainBlitErrors(); + g_GLESFuncs.glBlitFramebuffer(srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, dsBits, filter); + if (g_GLESFuncs.glGetError() != GL_NO_ERROR) { + // Nothing to fall back on yet: ResolveThenBlit is colour-only and the replicate + // pass runs in the opposite direction, so a driver that declines a multisample + // depth/stencil resolve leaves the destination holding its clear value. The log + // is the whole diagnostic - the frontend performs no validation of its own, so + // this never reaches the application as a GL error. + MGLOG_E_ONCE("BlitFramebuffer: the depth/stencil aspect was dropped - the driver rejected it on " + "its own and no emulation applies"); + } + } + DrainBlitErrors(); } void BlitFramebuffer(GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1, GLint dstX0, GLint dstY0, GLint dstX1, From 7d2c16a90e470686407b68247db18d8c68b78e9d Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:32:14 -0400 Subject: [PATCH 09/21] [Fix] (DirectGLES): bound every driver error drain so a lost context cannot spin forever --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 71eef18d..3e4086d9 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -3867,11 +3867,28 @@ namespace MobileGL::MG_Backend::DirectGLES { sizeof(DrawArraysIndirectCommand), "DrawArraysIndirect"); } - static void DrainBlitErrors() { - while (g_GLESFuncs.glGetError() != GL_NO_ERROR) { + // Empties the ES driver's error queue, BOUNDED. A driver that never answers GL_NO_ERROR - a + // lost context is the usual way, and GL_CONTEXT_LOST is allowed to keep coming back - would + // otherwise spin an unbounded drain forever inside whichever GL entry point happened to be + // cleaning up, which is how a GPU reset reads as an unkillable process whose log simply + // stops. A healthy context cannot queue anywhere near the cap, so reaching it IS the + // diagnostic. Every drain in this backend goes through here so the bound cannot drift apart + // between them. + static constexpr Int kMaxDrainedGLErrors = 32; + + static void DrainDriverErrors(const char* site) { + Int drained = 0; + while (drained < kMaxDrainedGLErrors && g_GLESFuncs.glGetError() != GL_NO_ERROR) { + ++drained; + } + if (drained == kMaxDrainedGLErrors) { + MGLOG_E_ONCE("%s: the ES driver still reported errors after %d drains - the context is most likely lost", + site, kMaxDrainedGLErrors); } } + static void DrainBlitErrors() { DrainDriverErrors("BlitFramebuffer"); } + // Sized internal format of the currently bound READ framebuffer's read colour // attachment, 0 when it cannot be determined. static GLenum QueryReadColorAttachmentInternalFormat() { @@ -5042,9 +5059,7 @@ namespace MobileGL::MG_Backend::DirectGLES { return false; } - static void ClearGLErrors() { - while (g_GLESFuncs.glGetError() != GL_NO_ERROR) {} - } + static void ClearGLErrors() { DrainDriverErrors("DirectGLES"); } // Binds a guaranteed-complete 1x1 scratch framebuffer at both targets for the // scope (GenerateMipmap must respecify texture storage while no incomplete @@ -7102,10 +7117,7 @@ namespace MobileGL::MG_Backend::DirectGLES { data = std::move(expanded); } - static void DrainESErrors() { - for (Int i = 0; i < 32 && g_GLESFuncs.glGetError() != GL_NO_ERROR; ++i) { - } - } + static void DrainESErrors() { DrainDriverErrors("ReadPixels"); } static GLenum QueryReadAttachmentComponentType() { GLint framebufferId = 0; From 56377d20257220c59704f4c50bceeef5e8a56129 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:36:01 -0400 Subject: [PATCH 10/21] [Fix, Test] (DirectGLES, MG_IntegrationTest): drop the image-binding layer for targets that have none --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 17 ++- .../Scenarios/ImageTargetKindScenario.cpp | 105 ++++++++++++++++++ 2 files changed, 119 insertions(+), 3 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 3e4086d9..72842256 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -1341,6 +1341,16 @@ namespace MobileGL::MG_Backend::DirectGLES { // is stored as an ES 2D array (MapToBackendTextureTarget), and so is layerable; asking // the state target instead answered "no" for it and pinned every 1D-array image binding // to layer 0, whatever the application passed. + // + // `layer` travels with the answer, because GL 4.6 core 8.26 (and ES 3.2 8.22, word for + // word) makes them one rule: "If the texture identified by texture does not have + // multiple layers or faces, the entire texture level is bound, regardless of the values + // of layered and layer." REGARDLESS means ignored - not clamped, and not an error - so + // the driver must not be handed a layer index the texture has no room for. Adreno takes + // such a request literally and leaves the image unit reading zero, which is what failed + // KHR-GL42.bind_image_texture.single_layer's layer:1 rows on GL_TEXTURE_2D and on the + // GL_TEXTURE_1D that is stored as one. Normalizing here and not in the frontend shadow + // is deliberate: GL_IMAGE_BINDING_LAYER must keep echoing what the application passed. static Bool SupportsLayeredImageBinding(TextureTarget target) { const TextureTarget backendTarget = TextureImpl::MapToBackendTextureTarget(target); return backendTarget == TextureTarget::Texture3D || backendTarget == TextureTarget::TextureCubeMap || @@ -1396,10 +1406,11 @@ namespace MobileGL::MG_Backend::DirectGLES { } auto& backendTexture = SyncTextureObjectToBackend(imageBinding.Texture, true); - const GLboolean layered = - SupportsLayeredImageBinding(imageBinding.Texture->GetTarget()) ? imageBinding.Layered : GL_FALSE; + const Bool layerable = SupportsLayeredImageBinding(imageBinding.Texture->GetTarget()); + const GLboolean layered = layerable ? imageBinding.Layered : GL_FALSE; + const GLint layer = layerable ? imageBinding.Layer : 0; g_GLESFuncs.glBindImageTexture(unit, backendTexture->GetBackendTextureId(), imageBinding.Level, - layered, imageBinding.Layer, imageBinding.Access, imageBinding.Format); + layered, layer, imageBinding.Access, imageBinding.Format); } // A buffer texture bound to a WRITABLE image unit is a buffer the shader is about to diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp index b35661d2..df4fdded 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp @@ -374,6 +374,86 @@ namespace MGITest { glUseProgram(0); } + // The same texture, bound four times over, varying nothing but `layered` and `layer`. + // + // GL 4.6 core 8.26 (and ES 3.2 8.22, word for word): "If the texture identified by + // texture does not have multiple layers or faces, the entire texture level is bound, + // regardless of the values of layered and layer." REGARDLESS means ignored - not + // clamped, and not an error - so every one of the four rows has to read the same texel + // out of a target that has no layers, including the two rows that name layer 1 on a + // texture whose only layer is 0. DirectGLES used to normalize `layered` and forward + // `layer` verbatim; Adreno honours the bogus layer by leaving the image unit reading + // zero, which is exactly the two rows KHR-GL42.bind_image_texture.single_layer failed. + // + // The bindings are checked back as well, because the fix depends on WHERE the + // normalization happens: the frontend shadow must keep echoing the application's own + // values (gl4cShaderImageLoadStoreTests' CheckBinding compares them exactly), and only + // the backend's driver call may drop the layer. + void RunNonLayerableLayerSweepCase(const TargetKind& kind) { + const GLuint program = MakeComputeProgram(SingleLoadSource(kind)); + if (program == 0) return; + const GLuint texture = MakeTexture(kind, true); + if (texture == 0) return; + + // A multisample texture has no TexSubImage, so MakeTexture leaves it unwritten and + // it is seeded the way the store cases do it - through a dispatch of its own. + const GLuint expected = kind.multisample ? kStoredValue : kFilledValue; + if (kind.multisample) { + const GLuint storeProgram = MakeComputeProgram(SingleStoreSource(kind)); + if (storeProgram == 0) return; + glBindImageTexture(0, texture, 0, GL_TRUE, 0, GL_READ_WRITE, GL_R32UI); + glUseProgram(storeProgram); + glUniform1i(0, 0); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + ASSERT_EQ(FirstGLError(), 0u) << kind.name << ": seeding the multisample texture errored"; + } + + const GLuint ssbo = MakeResultBuffer(); + glUseProgram(program); + glUniform1i(0, 0); + ASSERT_EQ(FirstGLError(), 0u) << kind.name << ": assigning the image unit errored"; + + // glcBindImageTextureTests' own four rows, in its own order. + struct LayerRow { + GLboolean layered; + GLint layer; + }; + static constexpr LayerRow kRows[] = {{GL_TRUE, 1}, {GL_TRUE, 0}, {GL_FALSE, 1}, {GL_FALSE, 0}}; + + for (const LayerRow& row : kRows) { + const std::string where = std::string(kind.name) + + ": layered=" + (row.layered == GL_TRUE ? "TRUE" : "FALSE") + + " layer=" + std::to_string(row.layer); + // Re-zeroed per row, so a row whose binding reads nothing cannot pass on the + // previous row's answer. + const GLuint zero = 0u; + glBindBuffer(GL_SHADER_STORAGE_BUFFER, ssbo); + glBufferSubData(GL_SHADER_STORAGE_BUFFER, 0, sizeof(GLuint), &zero); + + glBindImageTexture(0, texture, 0, row.layered, row.layer, GL_READ_ONLY, GL_R32UI); + EXPECT_EQ(FirstGLError(), 0u) << where << ": glBindImageTexture errored"; + + GLint reportedLayered = -1; + GLint reportedLayer = -1; + glGetIntegeri_v(GL_IMAGE_BINDING_LAYERED, 0, &reportedLayered); + glGetIntegeri_v(GL_IMAGE_BINDING_LAYER, 0, &reportedLayer); + EXPECT_EQ(reportedLayered, row.layered == GL_TRUE ? 1 : 0) + << where << ": GL_IMAGE_BINDING_LAYERED stopped reporting the application's value"; + EXPECT_EQ(reportedLayer, row.layer) + << where << ": GL_IMAGE_BINDING_LAYER stopped reporting the application's value"; + + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + EXPECT_EQ(FirstGLError(), 0u) << where << ": the dispatch leaked a GL error"; + EXPECT_EQ(ReadResult(ssbo), expected) + << where + << ": the texel did not come back, so the binding named a layer the texture " + "does not have instead of the whole level"; + } + glUseProgram(0); + } + std::vector m_programs; std::vector m_textures; std::vector m_buffers; @@ -435,6 +515,31 @@ namespace MGITest { #undef MGL_DEFINE_LOAD_CASE #undef MGL_DEFINE_STORE_CASE + // ---- and the same texture bound four times, varying only layered/layer --- + // + // KHR-GL42.bind_image_texture.single_layer's sweep, on the kinds whose backend target has + // neither layers nor faces. Two of its four rows name layer 1 on a single-layer texture, + // which the spec says is to be ignored outright rather than honoured or rejected - and + // which DirectGLES used to forward to the ES driver as written. + +#define MGL_DEFINE_LAYER_SWEEP_CASE(CaseName, Kind) \ + TEST_F(ImageTargetKindScenario, IgnoresLayerFor##CaseName) { \ + if (!Ready()) return; \ + if (!ImagesAreUsable()) GTEST_SKIP() << "no compute image uniforms"; \ + if ((Kind).multisample && !MultisampleImagesAreUsable()) { \ + GTEST_SKIP() << "GL_MAX_IMAGE_SAMPLES is 0, so the conformance case substitutes a plain 2D image " \ + "here and never asks for a multisample one"; \ + } \ + RunNonLayerableLayerSweepCase(Kind); \ + } + + MGL_DEFINE_LAYER_SWEEP_CASE(Texture2D, kKind2D) + MGL_DEFINE_LAYER_SWEEP_CASE(Texture1D, kKind1D) + MGL_DEFINE_LAYER_SWEEP_CASE(TextureRectangle, kKindRect) + MGL_DEFINE_LAYER_SWEEP_CASE(Texture2DMultisample, kKind2DMS) + +#undef MGL_DEFINE_LAYER_SWEEP_CASE + // ---- and all of them at once ------------------------------------------- // // The conformance case's actual shape. The single-kind cases above cannot see a defect that From 085eb5835bb6e6ede580cbc7d84fd3ff465bc263 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:58:54 -0400 Subject: [PATCH 11/21] [Fix, Test] (GLImpl, MG_State, DirectGLES, MG_Test): separate the transform feedback query counters and prefer the exact CPU count on DirectGLES --- MobileGL/MG_Backend/BackendObject.h | 8 + .../DirectGLES/BackendObject_DirectGLES.cpp | 6 + .../MG_Impl/GLImpl/Drawing/GL_Drawing.cpp | 11 + MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp | 72 +++++- MobileGL/MG_State/GLState/Core.h | 26 ++ MobileGL/MG_Test/Query/QueryTest.cpp | 223 ++++++++++++++++++ 6 files changed, 340 insertions(+), 6 deletions(-) diff --git a/MobileGL/MG_Backend/BackendObject.h b/MobileGL/MG_Backend/BackendObject.h index 616cdd06..884a59b2 100644 --- a/MobileGL/MG_Backend/BackendObject.h +++ b/MobileGL/MG_Backend/BackendObject.h @@ -236,6 +236,14 @@ namespace MobileGL { // (optional; null = frontend falls back to CPU accounting). BackendQueryHandle (*BeginXfbPrimitivesQuery)(Bool generated); void (*EndXfbPrimitivesQuery)(BackendQueryHandle query); + // Whether GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN should be answered from the + // frontend's own accounting wherever that accounting is exact - a capture with no + // geometry stage - instead of from the query above. Set by DirectGLES, whose result + // is whatever the ES driver's PRIMITIVES_WRITTEN counter says: Adreno reports twice + // the written count for a vertex-only capture that follows a large render pass, + // where the desktop-exact answer is the one the frontend already computed. Defaults + // to false, so a backend that never sets it keeps using its GPU result. + Bool PrefersCpuXfbPrimitiveAccounting = false; // Transform feedback capture spans, for backends whose own GL/ES driver // performs the capture (DirectGLES). Both optional; null means the backend // drives capture from its draw recording instead (DirectVulkan). End is diff --git a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp index b35b82cc..2a212c4e 100644 --- a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp @@ -1166,6 +1166,12 @@ namespace MobileGL::MG_Backend::DirectGLES { // geometry shader's amplification. funcsTable.GL.BeginXfbPrimitivesQuery = BeginXfbPrimitivesQuery; funcsTable.GL.EndXfbPrimitivesQuery = EndXfbPrimitivesQuery; + // ...but where it CAN see the whole capture - no geometry stage - the frontend's + // own count is the desktop-exact one and the ES driver's is only as good as the + // vendor made it (Adreno doubles PRIMITIVES_WRITTEN for a vertex-only capture that + // follows a large render pass). The query above stays installed: it is still what + // answers an amplifying span, and PRIMITIVES_GENERATED always. + funcsTable.GL.PrefersCpuXfbPrimitiveAccounting = true; funcsTable.GL.IsQueryResultAvailable = IsQueryResultAvailable; funcsTable.GL.GetQueryResult64 = GetQueryResult64; funcsTable.GL.DeleteBackendQuery = DeleteBackendQuery; diff --git a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp index 080445a0..121af025 100644 --- a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp +++ b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp @@ -108,6 +108,12 @@ namespace MobileGL::MG_Impl::GLImpl { const auto& program = MG_State::pGLContext->GetTransformFeedbackProgram(); if (program != nullptr) { + // A geometry stage writes what it emits, not what the draw assembled, and the + // amplification factor lives in the shader. Record that this span contained such + // a draw so the transform feedback queries keep their backend result for it. + if (program->GetShaderIndexByStage(ShaderStage::Geometry) >= 0) { + MG_State::pGLContext->AddTransformFeedbackGeometryCaptureDraw(); + } // Capacity in captured vertices = the tightest bound buffer. Uint64 capacityVertices = ~0ull; for (SizeT i = 0; i < program->GetTransformFeedbackBufferCount(); ++i) { @@ -127,6 +133,11 @@ namespace MobileGL::MG_Impl::GLImpl { } MG_State::pGLContext->AddTransformFeedbackPrimitives(primitives); MG_State::pGLContext->AddTransformFeedbackCapturedVertices(primitives * verticesPerPrimitive); + // Only draws that get this far are in the written counter at all. The instanced and + // indirect entry points never call this function, so a span that contains one is NOT + // fully accounted, and the queries must be able to tell: they compare this counter's + // delta against zero before standing in for the backend's own result. + MG_State::pGLContext->AddTransformFeedbackAccountedCaptureDraw(); } // Every primitive mode a draw command accepts (GL 4.6 core table 10.1, plus diff --git a/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp b/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp index 5a7d84f1..c8b95895 100644 --- a/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp +++ b/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp @@ -31,8 +31,15 @@ namespace MobileGL::MG_Impl::GLImpl { Bool ended = false; Bool resultCached = false; Uint64 cachedResult = 0; - // Transform feedback primitive counter at BeginQuery time. + // The transform feedback primitive counter matching this query's target, at + // BeginQuery time. Uint64 counterSnapshot = 0; + // Capture-draw counters at BeginQuery time: how many capture draws the CPU + // accounting had reproduced exactly, and how many of those it could not (a + // geometry stage amplifies). Their deltas decide whether the CPU result may + // stand in for the backend's. + Uint64 accountedCaptureDrawSnapshot = 0; + Uint64 geometryCaptureDrawSnapshot = 0; }; // Query calls may arrive from any thread (launchers migrate the context @@ -122,6 +129,46 @@ namespace MobileGL::MG_Impl::GLImpl { g_activeTimeElapsedQueryId = 0; } + // The CPU accounting counter a transform feedback query target reads: what the capture + // buffers took for GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN, and everything the capture + // stage assembled - a paused span included - for GL_PRIMITIVES_GENERATED. One counter + // for both targets would report the clamped written count as the generated one. + Uint64 TransformFeedbackCounterForTarget(GLenum target) { + return target == GL_PRIMITIVES_GENERATED + ? MG_State::pGLContext->GetTransformFeedbackGeneratedCounter() + : MG_State::pGLContext->GetTransformFeedbackPrimitiveCounter(); + } + + // The span's CPU accounting delta. Saturating: a snapshot left above its counter (a + // context switch between Begin and End, a counter that never moved) would otherwise + // wrap to 2^64-1, which GetQueryObjectuiv hands the app as 4294967295. + Uint64 TransformFeedbackCpuResult(const QueryObject* queryObject) { + const Uint64 counter = TransformFeedbackCounterForTarget(queryObject->target); + return counter > queryObject->counterSnapshot ? counter - queryObject->counterSnapshot : 0; + } + + // Whether this ended span's result should come from the CPU accounting rather than from + // the backend query it also ran. Three conditions, all necessary: + // * the backend asked for it (DirectGLES, whose ES driver counter is the unreliable + // one; DirectVulkan never sets the bit and so is untouched by any of this); + // * the target is PRIMITIVES_WRITTEN. GL_PRIMITIVES_GENERATED counts primitives + // whether or not a capture is active, and the accounting only ever sees capture + // draws, so the backend's counter is the more complete answer there; + // * the span was fully accounted: at least one capture draw reached the accounting + // (the instanced, indirect and multi-draw entry points do not call it at all, so a + // span made of those is invisible to it) and none of them amplified through a + // geometry stage, which the CPU cannot model. + Bool PrefersCpuTransformFeedbackResult(const QueryObject* queryObject) { + if (!MG_Backend::gBackendFunctionsTable.GL.PrefersCpuXfbPrimitiveAccounting) return false; + if (queryObject->target != GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN) return false; + if (MG_State::pGLContext->GetTransformFeedbackGeometryCaptureDraws() != + queryObject->geometryCaptureDrawSnapshot) { + return false; + } + return MG_State::pGLContext->GetTransformFeedbackAccountedCaptureDraws() != + queryObject->accountedCaptureDrawSnapshot; + } + // Shared GetQueryObject* implementation. Returns false when an error // was recorded and no value should be written back. `outValueProduced`, when given, // additionally distinguishes "succeeded with a value" from "succeeded but the result is not @@ -407,7 +454,11 @@ namespace MobileGL::MG_Impl::GLImpl { const auto beginXfbPrimitivesQuery = MG_Backend::gBackendFunctionsTable.GL.BeginXfbPrimitivesQuery; queryObject->backendHandle = beginXfbPrimitivesQuery ? beginXfbPrimitivesQuery(target == GL_PRIMITIVES_GENERATED) : nullptr; - queryObject->counterSnapshot = MG_State::pGLContext->GetTransformFeedbackPrimitiveCounter(); + queryObject->counterSnapshot = TransformFeedbackCounterForTarget(target); + queryObject->accountedCaptureDrawSnapshot = + MG_State::pGLContext->GetTransformFeedbackAccountedCaptureDraws(); + queryObject->geometryCaptureDrawSnapshot = + MG_State::pGLContext->GetTransformFeedbackGeometryCaptureDraws(); } else if (isOcclusionQuery) { queryObject->backendHandle = MG_Backend::gBackendFunctionsTable.GL.BeginOcclusionQuery(); } else { @@ -448,12 +499,21 @@ namespace MobileGL::MG_Impl::GLImpl { if (const auto endXfbPrimitivesQuery = MG_Backend::gBackendFunctionsTable.GL.EndXfbPrimitivesQuery) { endXfbPrimitivesQuery(queryObject->backendHandle); } - // Result comes from the GPU query at read time. - } else { - queryObject->cachedResult = - MG_State::pGLContext->GetTransformFeedbackPrimitiveCounter() - queryObject->counterSnapshot; + } + // A backend query that is not going to be read is released here, not left to be + // collected later: the span is over, the driver object has nothing left to say. + // Ending it first is what makes that legal. + if (!queryObject->backendHandle || PrefersCpuTransformFeedbackResult(queryObject)) { + if (queryObject->backendHandle) { + if (const auto deleteBackendQuery = MG_Backend::gBackendFunctionsTable.GL.DeleteBackendQuery) { + deleteBackendQuery(queryObject->backendHandle); + } + queryObject->backendHandle = nullptr; + } + queryObject->cachedResult = TransformFeedbackCpuResult(queryObject); queryObject->resultCached = true; } + // Otherwise the result comes from the GPU query at read time. queryObject->active = false; queryObject->ended = true; activeQueryId = 0; diff --git a/MobileGL/MG_State/GLState/Core.h b/MobileGL/MG_State/GLState/Core.h index 4dfb1078..b72c899d 100644 --- a/MobileGL/MG_State/GLState/Core.h +++ b/MobileGL/MG_State/GLState/Core.h @@ -328,6 +328,7 @@ namespace MobileGL { // transform feedback counter cannot see them - nothing was being captured. void AddTransformFeedbackPausedPrimitives(Uint64 primitives) { m_transformFeedbackPausedPrimitiveCounter += primitives; + m_transformFeedbackGeneratedPrimitiveCounter += primitives; } Uint64 GetTransformFeedbackPausedPrimitiveCounter() const { return m_transformFeedbackPausedPrimitiveCounter; @@ -342,8 +343,30 @@ namespace MobileGL { // (pre-clamp; drives the GS strip capture-order fixup at EndTF). void AddTransformFeedbackInputPrimitives(Uint64 primitives) { m_transformFeedbackInputPrimitives += primitives; + m_transformFeedbackGeneratedPrimitiveCounter += primitives; } Uint64 GetTransformFeedbackInputPrimitives() const { return m_transformFeedbackInputPrimitives; } + // What a GL_PRIMITIVES_GENERATED query counts over its span: every primitive the + // capture stage assembled, including the ones a paused span discarded (those are + // generated but never written). Kept as its own running total rather than derived + // from the input counter above, which BeginTransformFeedback resets per span while + // a query may cover several of them. + Uint64 GetTransformFeedbackGeneratedCounter() const { + return m_transformFeedbackGeneratedPrimitiveCounter; + } + // Capture draws whose written-primitive count the CPU accounting reproduced + // exactly, and the subset it could not: a program with a geometry stage amplifies + // by whatever the shader emits, which only the driver's own counter knows. The + // transform feedback queries diff both over their span to decide whether the CPU + // delta may stand in for the backend's GPU result (GL_Query.cpp). + void AddTransformFeedbackAccountedCaptureDraw() { ++m_transformFeedbackAccountedCaptureDraws; } + Uint64 GetTransformFeedbackAccountedCaptureDraws() const { + return m_transformFeedbackAccountedCaptureDraws; + } + void AddTransformFeedbackGeometryCaptureDraw() { ++m_transformFeedbackGeometryCaptureDraws; } + Uint64 GetTransformFeedbackGeometryCaptureDraws() const { + return m_transformFeedbackGeometryCaptureDraws; + } // Transform feedback objects (ARB_transform_feedback2 / GL 4.0 core). // The capture state above and the indexed GL_TRANSFORM_FEEDBACK_BUFFER @@ -439,6 +462,9 @@ namespace MobileGL { Uint64 m_transformFeedbackPausedPrimitiveCounter = 0; Uint64 m_transformFeedbackCapturedVertices = 0; Uint64 m_transformFeedbackInputPrimitives = 0; + Uint64 m_transformFeedbackGeneratedPrimitiveCounter = 0; + Uint64 m_transformFeedbackAccountedCaptureDraws = 0; + Uint64 m_transformFeedbackGeometryCaptureDraws = 0; // Everything a transform feedback object owns while it is NOT the bound one. struct TransformFeedbackObjectState { diff --git a/MobileGL/MG_Test/Query/QueryTest.cpp b/MobileGL/MG_Test/Query/QueryTest.cpp index 78c3c7d9..34430c8e 100644 --- a/MobileGL/MG_Test/Query/QueryTest.cpp +++ b/MobileGL/MG_Test/Query/QueryTest.cpp @@ -19,6 +19,7 @@ #include #include #include +#include using namespace MobileGL; @@ -119,6 +120,56 @@ namespace { g_stubResultObtainable = true; g_stubResultNs = 0; } + + // Stub backend transform feedback primitive queries. g_stubXfbQuerySupported = false + // models a backend with no GPU counter at all (null handle), which is what leaves the + // frontend's CPU accounting as the only source; g_stubResultNs is what the "driver" + // would answer when its query IS read, deliberately set to a value the CPU accounting + // never produces so the two sources are told apart. + Int g_stubXfbBeginCount = 0; + Int g_stubXfbEndCount = 0; + Bool g_stubXfbQuerySupported = true; + + MG_Backend::BackendQueryHandle StubBeginXfbPrimitivesQuery(Bool) { + if (!g_stubXfbQuerySupported) { + return nullptr; + } + ++g_stubXfbBeginCount; + return reinterpret_cast(static_cast(0x53)); + } + + void StubEndXfbPrimitivesQuery(MG_Backend::BackendQueryHandle) { ++g_stubXfbEndCount; } + + void InstallStubBackendXfbQueries() { + auto& backendGL = MG_Backend::gBackendFunctionsTable.GL; + backendGL.BeginXfbPrimitivesQuery = StubBeginXfbPrimitivesQuery; + backendGL.EndXfbPrimitivesQuery = StubEndXfbPrimitivesQuery; + backendGL.IsQueryResultAvailable = StubIsQueryResultAvailable; + backendGL.GetQueryResult64 = StubGetQueryResult64; + backendGL.DeleteBackendQuery = StubDeleteBackendQuery; + // Off by default: the tests that exercise the DirectGLES preference turn it on. + backendGL.PrefersCpuXfbPrimitiveAccounting = false; + g_stubXfbBeginCount = 0; + g_stubXfbEndCount = 0; + g_stubXfbQuerySupported = true; + g_stubDeleteCount = 0; + g_stubResultAvailable = true; + g_stubResultObtainable = true; + g_stubResultNs = 0; + } + + // What AccountTransformFeedbackPrimitives (GL_Drawing.cpp) records for one captured + // draw, without needing a draw: `assembled` primitives came out of the vertex stage + // and `written` of them fitted in the capture buffers (they differ once the buffers + // overflow, which is the whole point of PRIMITIVES_WRITTEN). + void SimulateAccountedCaptureDraw(Uint64 assembled, Uint64 written, Bool throughGeometryStage = false) { + MG_State::pGLContext->AddTransformFeedbackInputPrimitives(assembled); + if (throughGeometryStage) { + MG_State::pGLContext->AddTransformFeedbackGeometryCaptureDraw(); + } + MG_State::pGLContext->AddTransformFeedbackPrimitives(written); + MG_State::pGLContext->AddTransformFeedbackAccountedCaptureDraw(); + } } // namespace class QueryTest : public ::testing::Test { @@ -448,6 +499,178 @@ TEST_F(QueryTest, BackendResultsPropagateThroughFrontend) { EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); } +// The two transform feedback targets count different things and must therefore read +// different counters: PRIMITIVES_WRITTEN what the capture buffers took, PRIMITIVES_GENERATED +// every primitive the capture stage assembled - including the ones a paused span threw away, +// which are generated but never written. Answering both from the written counter (as the +// fallback used to) reports the clamped number as the generated one. +TEST_F(QueryTest, TransformFeedbackQueryTargetsReadTheirOwnCounter) { + const ScopedBackendFunctionsOverride backendGuard; + InstallStubBackendXfbQueries(); + g_stubXfbQuerySupported = false; // no GPU counter: the CPU accounting is the only source + + GLuint ids[2] = {0, 0}; + MG_Impl::GLImpl::GenQueries(2, ids); + ASSERT_NE(ids[0], 0u); + ASSERT_NE(ids[1], 0u); + + MG_Impl::GLImpl::BeginQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN, ids[0]); + MG_Impl::GLImpl::BeginQuery(GL_PRIMITIVES_GENERATED, ids[1]); + // Four points assembled into a buffer with room for three. + SimulateAccountedCaptureDraw(/*assembled=*/4, /*written=*/3); + // ...and two more points assembled while the span was paused: generated, never written. + MG_State::pGLContext->AddTransformFeedbackPausedPrimitives(2); + MG_Impl::GLImpl::EndQuery(GL_PRIMITIVES_GENERATED); + MG_Impl::GLImpl::EndQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN); + + GLuint written = 0; + GLuint generated = 0; + MG_Impl::GLImpl::GetQueryObjectuiv(ids[0], GL_QUERY_RESULT, &written); + MG_Impl::GLImpl::GetQueryObjectuiv(ids[1], GL_QUERY_RESULT, &generated); + EXPECT_EQ(written, 3u); + EXPECT_EQ(generated, 6u); + + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + MG_Impl::GLImpl::DeleteQueries(2, ids); +} + +// A query span that captured nothing at all reads zero from the CPU accounting rather than +// the unsigned wrap-around a bare End-minus-Begin subtraction produces the moment the +// snapshot is not below the counter (GetQueryObjectuiv would hand the app 4294967295). +TEST_F(QueryTest, AnEmptyTransformFeedbackSpanReadsZero) { + const ScopedBackendFunctionsOverride backendGuard; + InstallStubBackendXfbQueries(); + g_stubXfbQuerySupported = false; + + GLuint id = 0; + MG_Impl::GLImpl::GenQueries(1, &id); + ASSERT_NE(id, 0u); + + MG_Impl::GLImpl::BeginQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN, id); + MG_Impl::GLImpl::EndQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN); + + GLuint result = 123u; + MG_Impl::GLImpl::GetQueryObjectuiv(id, GL_QUERY_RESULT, &result); + EXPECT_EQ(result, 0u); + + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + MG_Impl::GLImpl::DeleteQueries(1, &id); +} + +// The DirectGLES preference: for a capture the frontend counted exactly - every draw +// accounted, none of them amplified by a geometry stage - the CPU number is the +// desktop-exact one and the ES driver's PRIMITIVES_WRITTEN counter is not consulted, even +// though the backend query ran. The backend query object is released at EndQuery instead of +// being left to a result read that will never come. +TEST_F(QueryTest, VertexOnlyCaptureSpansPreferTheCpuPrimitiveAccounting) { + const ScopedBackendFunctionsOverride backendGuard; + InstallStubBackendXfbQueries(); + MG_Backend::gBackendFunctionsTable.GL.PrefersCpuXfbPrimitiveAccounting = true; + g_stubResultNs = 6; // what the driver's counter would have said - twice the truth + + GLuint id = 0; + MG_Impl::GLImpl::GenQueries(1, &id); + ASSERT_NE(id, 0u); + + MG_Impl::GLImpl::BeginQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN, id); + SimulateAccountedCaptureDraw(/*assembled=*/4, /*written=*/3); + MG_Impl::GLImpl::EndQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN); + EXPECT_EQ(g_stubXfbBeginCount, 1); + EXPECT_EQ(g_stubXfbEndCount, 1); + EXPECT_EQ(g_stubDeleteCount, 1); // ended, then released - not leaked + + GLint available = -1; + MG_Impl::GLImpl::GetQueryObjectiv(id, GL_QUERY_RESULT_AVAILABLE, &available); + EXPECT_EQ(available, 1); + + GLuint result = 0; + MG_Impl::GLImpl::GetQueryObjectuiv(id, GL_QUERY_RESULT, &result); + EXPECT_EQ(result, 3u); + EXPECT_EQ(g_stubDeleteCount, 1); // the read had no handle left to release + + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + MG_Impl::GLImpl::DeleteQueries(1, &id); + EXPECT_EQ(g_stubDeleteCount, 1); +} + +// The regression gate for that preference: a capture fed by a geometry stage writes whatever +// the shader emits, which the CPU accounting cannot model, so the backend's counter stays the +// answer and its handle survives EndQuery to be read later. +TEST_F(QueryTest, AGeometryStageCaptureKeepsTheBackendPrimitiveResult) { + const ScopedBackendFunctionsOverride backendGuard; + InstallStubBackendXfbQueries(); + MG_Backend::gBackendFunctionsTable.GL.PrefersCpuXfbPrimitiveAccounting = true; + g_stubResultNs = 9; // the amplified count only the driver knows + + GLuint id = 0; + MG_Impl::GLImpl::GenQueries(1, &id); + ASSERT_NE(id, 0u); + + MG_Impl::GLImpl::BeginQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN, id); + SimulateAccountedCaptureDraw(/*assembled=*/1, /*written=*/1, /*throughGeometryStage=*/true); + MG_Impl::GLImpl::EndQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN); + EXPECT_EQ(g_stubDeleteCount, 0); // still to be read + + GLuint result = 0; + MG_Impl::GLImpl::GetQueryObjectuiv(id, GL_QUERY_RESULT, &result); + EXPECT_EQ(result, 9u); + EXPECT_EQ(g_stubDeleteCount, 1); + + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + MG_Impl::GLImpl::DeleteQueries(1, &id); +} + +// The other half of that gate: the instanced, indirect and multi-draw entry points never +// reach the CPU accounting, so a span made of those moves no counter at all. Its delta would +// be zero, which is not "nothing was written" - it is "nothing was counted" - and the +// backend's result has to stand. +TEST_F(QueryTest, ACaptureSpanTheAccountingNeverSawKeepsTheBackendResult) { + const ScopedBackendFunctionsOverride backendGuard; + InstallStubBackendXfbQueries(); + MG_Backend::gBackendFunctionsTable.GL.PrefersCpuXfbPrimitiveAccounting = true; + g_stubResultNs = 12; + + GLuint id = 0; + MG_Impl::GLImpl::GenQueries(1, &id); + ASSERT_NE(id, 0u); + + MG_Impl::GLImpl::BeginQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN, id); + MG_Impl::GLImpl::EndQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN); + + GLuint result = 0; + MG_Impl::GLImpl::GetQueryObjectuiv(id, GL_QUERY_RESULT, &result); + EXPECT_EQ(result, 12u); + + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + MG_Impl::GLImpl::DeleteQueries(1, &id); +} + +// GL_PRIMITIVES_GENERATED counts primitives whether or not a capture is active, while the +// CPU accounting only ever sees capture draws - so the preference above deliberately does +// not extend to that target, whatever the backend asked for. +TEST_F(QueryTest, PrimitivesGeneratedKeepsTheBackendResultUnderTheCpuPreference) { + const ScopedBackendFunctionsOverride backendGuard; + InstallStubBackendXfbQueries(); + MG_Backend::gBackendFunctionsTable.GL.PrefersCpuXfbPrimitiveAccounting = true; + g_stubResultNs = 7; + + GLuint id = 0; + MG_Impl::GLImpl::GenQueries(1, &id); + ASSERT_NE(id, 0u); + + MG_Impl::GLImpl::BeginQuery(GL_PRIMITIVES_GENERATED, id); + SimulateAccountedCaptureDraw(/*assembled=*/4, /*written=*/3); + MG_Impl::GLImpl::EndQuery(GL_PRIMITIVES_GENERATED); + EXPECT_EQ(g_stubDeleteCount, 0); + + GLuint result = 0; + MG_Impl::GLImpl::GetQueryObjectuiv(id, GL_QUERY_RESULT, &result); + EXPECT_EQ(result, 7u); + + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + MG_Impl::GLImpl::DeleteQueries(1, &id); +} + // Environment-agnostic property test for the env -> ConfigLoader -> Features // chain: whatever MOBILEGL_DISABLE_TIMERQUERY is set to in the environment of // this test process, MG_ConfigLoader::Init must have parsed it with the From 9b37c77ae2f3b40b673b6c7162992f87f4bf6953 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 04:59:00 -0400 Subject: [PATCH 12/21] [Test] (MG_IntegrationTest): pin an overflowing vertex-only transform feedback capture's written and generated counts --- MobileGL/MG_IntegrationTest/CMakeLists.txt | 1 + .../Scenarios/XfbPrimitiveQueryScenario.cpp | 268 ++++++++++++++++++ 2 files changed, 269 insertions(+) create mode 100644 MobileGL/MG_IntegrationTest/Scenarios/XfbPrimitiveQueryScenario.cpp diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 881d09bc..a36a0c2c 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -86,6 +86,7 @@ add_executable(MobileGLIntegrationTest Scenarios/BufferTextureScenario.cpp Scenarios/VertexAttribBindingScenario.cpp Scenarios/XfbCaptureBufferReuseScenario.cpp + Scenarios/XfbPrimitiveQueryScenario.cpp Scenarios/VertexArrayEnableDisableScenario.cpp Scenarios/CopyImageLevelRangeScenario.cpp Scenarios/CopyImageLayeredScenario.cpp diff --git a/MobileGL/MG_IntegrationTest/Scenarios/XfbPrimitiveQueryScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/XfbPrimitiveQueryScenario.cpp new file mode 100644 index 00000000..134d6419 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/XfbPrimitiveQueryScenario.cpp @@ -0,0 +1,268 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/XfbPrimitiveQueryScenario.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// What the two transform feedback queries report for a VERTEX-ONLY capture that +// OVERFLOWS its buffer - the shape of KHR-GL30.transform_feedback.query_vertex_*, +// and the one place where the two targets must disagree: +// +// * GL_PRIMITIVES_GENERATED counts what the capture stage assembled: 4 points. +// * GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN counts what the capture buffers +// took. With room for three vertices, a full buffer stops recording whole +// primitives (GL 4.6 core 13.2.2), so the answer is 3, not 4 and not 6. +// +// Both numbers came from the backend's own GPU counter until the driver underneath +// DirectGLES was caught reporting exactly twice the written count for this shape +// (Adreno 830, vertex-only capture issued right after a large render pass). The +// frontend already computes the desktop-exact number for a capture with no geometry +// stage, so that is what answers PRIMITIVES_WRITTEN there now - and this scenario is +// what pins the value, on every backend, without a device. +// +// The non-overflowing case is the negative control: with room for all four points +// the two targets must AGREE at 4, so a "written" that silently reports the +// generated count cannot pass both cases at once. + +#include +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr float kPoison = -1234.0f; + // One vec4 per captured point. + constexpr std::size_t kFloatsPerVertex = 4; + constexpr std::size_t kBytesPerVertex = kFloatsPerVertex * sizeof(float); + // The draw: four points, whichever way the capture buffer is sized. + constexpr GLsizei kDrawnPoints = 4; + + GLuint CompileShader(GLenum type, const std::string& source, std::string* log) { + const GLuint shader = glCreateShader(type); + const char* text = source.c_str(); + glShaderSource(shader, 1, &text, nullptr); + glCompileShader(shader); + GLint status = GL_FALSE; + glGetShaderiv(shader, GL_COMPILE_STATUS, &status); + if (status == GL_FALSE) { + GLint length = 0; + glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length); + std::vector buffer(static_cast(length) + 1, '\0'); + glGetShaderInfoLog(shader, length + 1, nullptr, buffer.data()); + if (log != nullptr) *log = buffer.data(); + glDeleteShader(shader); + return 0; + } + return shader; + } + + // Vertex-only capture program - no geometry stage, so nothing amplifies and the + // primitives written are the primitives drawn (up to the buffer's capacity). + GLuint BuildCaptureProgram(std::string* log) { + const std::string vertexSource = R"(#version 430 core +layout(location = 0) in vec4 vs_in_value; +out vec4 vs_out_value; +void main() { + vs_out_value = vs_in_value; +} +)"; + const GLuint vertexShader = CompileShader(GL_VERTEX_SHADER, vertexSource, log); + if (vertexShader == 0) return 0; + const GLuint program = glCreateProgram(); + glAttachShader(program, vertexShader); + const char* varying = "vs_out_value"; + glTransformFeedbackVaryings(program, 1, &varying, GL_INTERLEAVED_ATTRIBS); + glLinkProgram(program); + glDeleteShader(vertexShader); + GLint status = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &status); + if (status == GL_FALSE) { + GLint length = 0; + glGetProgramiv(program, GL_INFO_LOG_LENGTH, &length); + std::vector buffer(static_cast(length) + 1, '\0'); + glGetProgramInfoLog(program, length + 1, nullptr, buffer.data()); + if (log != nullptr) *log = buffer.data(); + glDeleteProgram(program); + return 0; + } + return program; + } + + class XfbPrimitiveQueryScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + + std::string log; + m_program = BuildCaptureProgram(&log); + ASSERT_NE(m_program, 0u) << "capture program failed to build: " << log; + + glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + glGenBuffers(1, &m_vbo); + glBindBuffer(GL_ARRAY_BUFFER, m_vbo); + // Vertex i is (i, i+1, i+2, i+3), so a record that landed in the wrong slot + // is as visible as one that never landed at all. + float vertices[kDrawnPoints * kFloatsPerVertex] = {}; + for (int point = 0; point < kDrawnPoints; ++point) { + for (std::size_t component = 0; component < kFloatsPerVertex; ++component) { + vertices[static_cast(point) * kFloatsPerVertex + component] = + static_cast(point) + static_cast(component); + } + } + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glVertexAttribPointer(0, 4, GL_FLOAT, GL_FALSE, 0, nullptr); + glEnableVertexAttribArray(0); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glGenQueries(2, m_queries); + ASSERT_NE(m_queries[0], 0u); + ASSERT_NE(m_queries[1], 0u); + } + + void TearDown() override { + if (!Ready()) return; + glDeleteQueries(2, m_queries); + glBindVertexArray(0); + if (m_vbo != 0) glDeleteBuffers(1, &m_vbo); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + if (m_program != 0) glDeleteProgram(m_program); + glUseProgram(0); + ScenarioTest::TearDown(); + } + + // A capture buffer with room for exactly `vertexCapacity` records, poisoned so + // that "captured nothing" is legible, bound to capture point 0. + GLuint MakeCaptureBuffer(std::size_t vertexCapacity) { + GLuint buffer = 0; + glGenBuffers(1, &buffer); + glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, buffer); + const std::vector poison(vertexCapacity * kFloatsPerVertex, kPoison); + glBufferData(GL_TRANSFORM_FEEDBACK_BUFFER, + static_cast(vertexCapacity * kBytesPerVertex), poison.data(), + GL_DYNAMIC_DRAW); + return buffer; + } + + // ONE capture span, four points, with both query targets open across it - the + // order KHR-GL30.transform_feedback.query_vertex_interleaved_test uses: the + // queries wrap the whole span, never the other way round. + void RunQueriedSpan(GLuint* written, GLuint* generated) { + glEnable(GL_RASTERIZER_DISCARD); + glUseProgram(m_program); + glBindVertexArray(m_vao); + + glBeginQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN, m_queries[0]); + glBeginQuery(GL_PRIMITIVES_GENERATED, m_queries[1]); + glBeginTransformFeedback(GL_POINTS); + glDrawArrays(GL_POINTS, 0, kDrawnPoints); + glEndTransformFeedback(); + glEndQuery(GL_PRIMITIVES_GENERATED); + glEndQuery(GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN); + + glDisable(GL_RASTERIZER_DISCARD); + glUseProgram(0); + + *written = 0xFFFFFFFFu; + *generated = 0xFFFFFFFFu; + glGetQueryObjectuiv(m_queries[0], GL_QUERY_RESULT, written); + glGetQueryObjectuiv(m_queries[1], GL_QUERY_RESULT, generated); + } + + // The capture record at slot `point` must be the vertex the draw fetched there. + static ::testing::AssertionResult CapturedVertexIs(const float* record, int point) { + for (std::size_t component = 0; component < kFloatsPerVertex; ++component) { + const float expected = static_cast(point) + static_cast(component); + const float got = record[component]; + // isfinite first: every ordered comparison against a NaN is false, so a + // pair of one-sided range tests REPORTS SUCCESS for uninitialised storage + // that happens to read as NaN. + if (!std::isfinite(got) || std::fabs(got - expected) > 0.01f) { + return ::testing::AssertionFailure() + << "point " << point << " component " << component << " is " << got << ", expected " + << expected << (got == kPoison ? " (the capture never reached these bytes)" : ""); + } + } + return ::testing::AssertionSuccess(); + } + + GLuint m_program = 0; + GLuint m_vao = 0; + GLuint m_vbo = 0; + GLuint m_queries[2] = {0, 0}; + }; + + // The negative control: the buffer holds every point the draw produces, so both + // targets must report the same 4. A "written" that is really the generated count + // passes this case and fails the next one; a "written" that is really zero fails + // this one. + TEST_F(XfbPrimitiveQueryScenario, ACaptureThatFitsReportsEveryPrimitiveOnBothTargets) { + if (!Ready()) GTEST_SKIP(); + + const GLuint captureBuffer = MakeCaptureBuffer(kDrawnPoints); + GLuint written = 0; + GLuint generated = 0; + RunQueriedSpan(&written, &generated); + + EXPECT_EQ(written, 4u); + EXPECT_EQ(generated, 4u); + + std::vector readback(kDrawnPoints * kFloatsPerVertex, kPoison); + glGetBufferSubData(GL_TRANSFORM_FEEDBACK_BUFFER, 0, + static_cast(kDrawnPoints * kBytesPerVertex), readback.data()); + for (int point = 0; point < kDrawnPoints; ++point) { + EXPECT_TRUE(CapturedVertexIs(readback.data() + static_cast(point) * kFloatsPerVertex, + point)); + } + + glDeleteBuffers(1, &captureBuffer); + EXPECT_EQ(glGetError(), GL_NO_ERROR); + } + + // The pin: four points into a buffer sized for three. The fourth is not written, so + // the two targets part ways at 3 and 4 - the exact pair + // KHR-GL30.transform_feedback.query_vertex_interleaved_test checks, and the pair the + // Adreno driver counter got wrong (it answered 6). + TEST_F(XfbPrimitiveQueryScenario, AnOverflowingVertexOnlyCaptureStopsWritingAtTheBufferCapacity) { + if (!Ready()) GTEST_SKIP(); + + constexpr std::size_t kCapacityVertices = 3; + const GLuint captureBuffer = MakeCaptureBuffer(kCapacityVertices); + GLuint written = 0; + GLuint generated = 0; + RunQueriedSpan(&written, &generated); + + EXPECT_EQ(written, 3u) << "the capture buffer holds " << kCapacityVertices << " points"; + EXPECT_EQ(generated, 4u) << "every point the draw assembled is generated, capacity or not"; + + // The three records that DID fit are the first three points, in order: an + // overflow truncates the capture, it does not scramble or drop what preceded it. + std::vector readback(kCapacityVertices * kFloatsPerVertex, kPoison); + glGetBufferSubData(GL_TRANSFORM_FEEDBACK_BUFFER, 0, + static_cast(kCapacityVertices * kBytesPerVertex), readback.data()); + for (int point = 0; point < static_cast(kCapacityVertices); ++point) { + EXPECT_TRUE(CapturedVertexIs(readback.data() + static_cast(point) * kFloatsPerVertex, + point)); + } + + glDeleteBuffers(1, &captureBuffer); + EXPECT_EQ(glGetError(), GL_NO_ERROR); + } + + } // namespace +} // namespace MGITest From 916702629e0ea1c04117a8e4c6f91181ed0831e1 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 05:08:11 -0400 Subject: [PATCH 13/21] [Fix, Test] (GLImpl, MG_Test): validate glFenceSync's condition/flags and glWaitSync's flags/timeout --- MobileGL/MG_Impl/GLImpl/Sync/GL_Sync.cpp | 29 ++++++++++++++++ .../MG_Test/State/NegativeApiErrorsTest.cpp | 34 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/MobileGL/MG_Impl/GLImpl/Sync/GL_Sync.cpp b/MobileGL/MG_Impl/GLImpl/Sync/GL_Sync.cpp index 951f3fe7..e18b8120 100644 --- a/MobileGL/MG_Impl/GLImpl/Sync/GL_Sync.cpp +++ b/MobileGL/MG_Impl/GLImpl/Sync/GL_Sync.cpp @@ -8,6 +8,7 @@ #include "GL_Sync.h" #include +#include namespace MobileGL::MG_Impl::GLImpl { namespace { @@ -35,6 +36,22 @@ namespace MobileGL::MG_Impl::GLImpl { } // namespace GLsync FenceSync(GLenum condition, GLbitfield flags) { + // GL 4.6 core 4.1.2: GL_SYNC_GPU_COMMANDS_COMPLETE is the only condition and the only + // legal flags value is zero; both violations return 0 rather than a handle. A caller that + // then hands the 0 back to glDeleteSync hits the glDeleteSync(0) no-op below. + if (condition != GL_SYNC_GPU_COMMANDS_COMPLETE) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidEnum, + MakeUnique("MG_Impl/GLImpl", __func__, + "condition must be GL_SYNC_GPU_COMMANDS_COMPLETE.")); + return nullptr; + } + if (flags != 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", __func__, "flags must be zero.")); + return nullptr; + } auto* syncObject = new SyncObject; syncObject->condition = condition; syncObject->flags = flags; @@ -64,6 +81,18 @@ namespace MobileGL::MG_Impl::GLImpl { } void WaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout) { + // GL 4.6 core 4.1.2: the server-side wait takes no flags and no finite timeout - both + // arguments exist only to be forward-compatible, and anything else is INVALID_VALUE. + // Neither backend ever honored a nonzero timeout (DirectGLES hard-codes + // 0/GL_TIMEOUT_IGNORED, DirectVulkan's queue ordering makes the wait implicit), so + // rejecting the call loses no wait that used to happen. + if (flags != 0 || timeout != GL_TIMEOUT_IGNORED) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", __func__, + "flags must be zero and timeout must be GL_TIMEOUT_IGNORED.")); + return; + } const auto* syncObject = FindSyncObject(sync); if (!syncObject) { return; diff --git a/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp b/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp index 58faf612..9426a7f2 100644 --- a/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp +++ b/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp @@ -18,6 +18,7 @@ // errors that guard a parameter-buffer draw. // * KHR-GL43.compute_shader.api-indirect / .api-program. // * KHR-GLxx.texture_storage.compressed_data - compressed formats on TEXTURE_3D. +// * KHR-GL32.api.coverage - glFenceSync's condition/flags and glWaitSync's flags/timeout. // Plus the indexed-getter parity RC-7b is about: glGetBooleani_v / glGetInteger64i_v / // glGetFloati_v / glGetDoublei_v must answer every pname glGetIntegeri_v answers. // @@ -37,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -470,4 +472,36 @@ void main() { g_color = vec4(1); } EXPECT_EQ(offset, 2048); EXPECT_EQ(GetError(), GL_NO_ERROR); } + + // KHR-GL32.api.coverage: glFenceSync and glWaitSync took every argument they were handed and + // reported GL_NO_ERROR for the two calls GL 4.6 core 4.1.2 requires to fail. A rejected + // glFenceSync must also hand back 0 rather than a live handle. + TEST_F(NegativeApiErrorsTest, SyncEntryPointsRejectTheirIllegalArguments) { + DrainErrors(); + + RunRows({ + {"glFenceSync with a condition other than GL_SYNC_GPU_COMMANDS_COMPLETE", + [] { EXPECT_EQ(FenceSync(GL_SYNC_FENCE, 0), nullptr); }, GL_INVALID_ENUM}, + {"glFenceSync with nonzero flags", [] { EXPECT_EQ(FenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 1), nullptr); }, + GL_INVALID_VALUE}, + }); + + // The legal fence still works, and with no backend function table it is the always-signaled + // fallback - which is all this GPU-free suite needs to reach glWaitSync's own checks. + const GLsync sync = FenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0); + ASSERT_NE(sync, nullptr); + EXPECT_EQ(GetError(), GL_NO_ERROR); + EXPECT_EQ(IsSync(sync), GL_TRUE); + + RunRows({ + {"glWaitSync with nonzero flags", [&] { WaitSync(sync, GL_SYNC_FLUSH_COMMANDS_BIT, GL_TIMEOUT_IGNORED); }, + GL_INVALID_VALUE}, + {"glWaitSync with a finite timeout", [&] { WaitSync(sync, 0, 1000000000ull); }, GL_INVALID_VALUE}, + {"glWaitSync with the only legal argument pair", [&] { WaitSync(sync, 0, GL_TIMEOUT_IGNORED); }, + GL_NO_ERROR}, + }); + + DeleteSync(sync); + EXPECT_EQ(GetError(), GL_NO_ERROR); + } } // namespace From 1e8d4661e62170e9fe93fbdc22b191a63c21f403 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 05:10:38 -0400 Subject: [PATCH 14/21] [Fix, Test] (GLImpl, MG_Test): raise a draw's mode INVALID_ENUM before the no-current-program guard --- .../MG_Impl/GLImpl/Drawing/GL_Drawing.cpp | 38 +++++++++++++++++-- .../MG_Test/State/NegativeApiErrorsTest.cpp | 35 +++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp index 121af025..408e08fb 100644 --- a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp +++ b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp @@ -162,11 +162,23 @@ namespace MobileGL::MG_Impl::GLImpl { } } + // The `mode` INVALID_ENUM in isolation, so a draw entry point can raise it BEFORE any of the + // state-dependent INVALID_OPERATIONs below. GL 4.6 core 10.4 makes a bad mode INVALID_ENUM + // unconditionally, while "no current program" is not even a spec-listed draw error - it is + // MobileGL's own null-dereference guard - so it must never shadow the enum check + // (KHR-GL31.api.coverage calls glDrawArraysInstanced/glDrawElementsInstanced with mode + // GL_POINTS-1 against a bare context and pins GL_INVALID_ENUM). + static Bool ValidatePrimitiveModeEnum(const char* functionName, GLenum mode) { + if (IsAcceptedPrimitiveMode(mode)) return true; + + MG_State::pGLContext->RecordError( + ErrorCode::InvalidEnum, + MakeUnique("MG_Impl/GLImpl", functionName, "mode is not an accepted primitive type.")); + return false; + } + static Bool ValidatePrimitiveModeForBackend(const char* functionName, GLenum mode) { - if (!IsAcceptedPrimitiveMode(mode)) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidEnum, - MakeUnique("MG_Impl/GLImpl", functionName, "mode is not an accepted primitive type.")); + if (!ValidatePrimitiveModeEnum(functionName, mode)) { return false; } @@ -607,12 +619,14 @@ namespace MobileGL::MG_Impl::GLImpl { } void MultiDrawElementsIndirect(GLenum mode, GLenum type, const void* indirect, GLsizei drawcount, GLsizei stride) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; MultiDrawElementsIndirect_Backend(mode, type, indirect, drawcount, stride); } void MultiDrawArraysIndirect(GLenum mode, const void* indirect, GLsizei drawcount, GLsizei stride) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; MultiDrawArraysIndirect_Backend(mode, indirect, drawcount, stride); @@ -726,12 +740,14 @@ namespace MobileGL::MG_Impl::GLImpl { void DrawRangeElementsBaseVertex(GLenum mode, GLuint start, GLuint end, GLsizei count, GLenum type, const void* indices, GLint basevertex) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; DrawRangeElementsBaseVertex_Backend(mode, start, end, count, type, indices, basevertex); } void DrawRangeElements(GLenum mode, GLuint start, GLuint end, GLsizei count, GLenum type, const void* indices) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; DrawRangeElements_Backend(mode, start, end, count, type, indices); @@ -739,6 +755,7 @@ namespace MobileGL::MG_Impl::GLImpl { void DrawElementsInstancedBaseVertexBaseInstance(GLenum mode, GLsizei count, GLenum type, const void* indices, GLsizei instancecount, GLint basevertex, GLuint baseinstance) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; DrawElementsInstancedBaseVertexBaseInstance_Backend(mode, count, type, indices, instancecount, basevertex, @@ -747,6 +764,7 @@ namespace MobileGL::MG_Impl::GLImpl { void DrawElementsInstancedBaseVertex(GLenum mode, GLsizei count, GLenum type, const void* indices, GLsizei instancecount, GLint basevertex) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; DrawElementsInstancedBaseVertex_Backend(mode, count, type, indices, instancecount, basevertex); @@ -754,18 +772,21 @@ namespace MobileGL::MG_Impl::GLImpl { void DrawElementsInstancedBaseInstance(GLenum mode, GLsizei count, GLenum type, const void* indices, GLsizei instancecount, GLuint baseinstance) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; DrawElementsInstancedBaseInstance_Backend(mode, count, type, indices, instancecount, baseinstance); } void DrawElementsInstanced(GLenum mode, GLsizei count, GLenum type, const void* indices, GLsizei instancecount) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; DrawElementsInstanced_Backend(mode, count, type, indices, instancecount); } void DrawElementsIndirect(GLenum mode, GLenum type, const void* indirect) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; if (!ValidateDrawElementsIndexType(__func__, type)) return; @@ -775,18 +796,21 @@ namespace MobileGL::MG_Impl::GLImpl { void DrawArraysInstancedBaseInstance(GLenum mode, GLint first, GLsizei count, GLsizei instancecount, GLuint baseinstance) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; DrawArraysInstancedBaseInstance_Backend(mode, first, count, instancecount, baseinstance); } void DrawArraysInstanced(GLenum mode, GLint first, GLsizei count, GLsizei instancecount) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; DrawArraysInstanced_Backend(mode, first, count, instancecount); } void DrawArraysIndirect(GLenum mode, const void* indirect) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; if (!ValidateIndirectDrawSource(__func__, indirect, kDrawArraysIndirectCommandBytes)) return; @@ -794,6 +818,7 @@ namespace MobileGL::MG_Impl::GLImpl { } void DrawElementsBaseVertex(GLenum mode, GLsizei count, GLenum type, const void* indices, GLint basevertex) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; AccountTransformFeedbackPrimitives(mode, count); @@ -801,6 +826,7 @@ namespace MobileGL::MG_Impl::GLImpl { } void DrawArrays(GLenum mode, GLint first, GLsizei count) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; AccountTransformFeedbackPrimitives(mode, count); @@ -808,6 +834,7 @@ namespace MobileGL::MG_Impl::GLImpl { } void MultiDrawArrays(GLenum mode, const GLint* first, const GLsizei* count, GLsizei drawcount) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; if (drawcount < 0) { @@ -821,6 +848,7 @@ namespace MobileGL::MG_Impl::GLImpl { void MultiDrawElements(GLenum mode, const GLsizei* count, GLenum type, const void* const* indices, GLsizei drawcount) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; MultiDrawElements_Backend(mode, count, type, indices, drawcount); @@ -828,6 +856,7 @@ namespace MobileGL::MG_Impl::GLImpl { void MultiDrawElementsBaseVertex(GLenum mode, const GLsizei* count, GLenum type, const void* const* indices, GLsizei drawcount, const GLint* basevertex) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; MultiDrawElementsBaseVertex_Backend(mode, count, type, indices, drawcount, basevertex); @@ -838,6 +867,7 @@ namespace MobileGL::MG_Impl::GLImpl { } void DrawElements(GLenum mode, GLsizei count, GLenum type, const void* indices) { + if (!ValidatePrimitiveModeEnum(__func__, mode)) return; if (!ValidateCurrentProgramForExecution(__func__)) return; if (!ValidatePrimitiveModeForBackend(__func__, mode)) return; AccountTransformFeedbackPrimitives(mode, count); diff --git a/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp b/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp index 9426a7f2..50ee18b2 100644 --- a/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp +++ b/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp @@ -19,6 +19,8 @@ // * KHR-GL43.compute_shader.api-indirect / .api-program. // * KHR-GLxx.texture_storage.compressed_data - compressed formats on TEXTURE_3D. // * KHR-GL32.api.coverage - glFenceSync's condition/flags and glWaitSync's flags/timeout. +// * KHR-GL31.api.coverage - a draw's mode INVALID_ENUM has to outrank MobileGL's own +// no-current-program guard. // Plus the indexed-getter parity RC-7b is about: glGetBooleani_v / glGetInteger64i_v / // glGetFloati_v / glGetDoublei_v must answer every pname glGetIntegeri_v answers. // @@ -504,4 +506,37 @@ void main() { g_color = vec4(1); } DeleteSync(sync); EXPECT_EQ(GetError(), GL_NO_ERROR); } + + // KHR-GL31.api.coverage's first two calls are glDrawArraysInstanced / glDrawElementsInstanced + // with mode GL_POINTS-1 against a context that has no program and no VAO bound, and they must + // answer GL_INVALID_ENUM. MobileGL's own "there is no current program" guard - which the spec + // does not list as a draw error at all - used to run first and shadowed the enum check with + // GL_INVALID_OPERATION. Nothing here reaches a backend: the mode is rejected before the guard. + TEST_F(NegativeApiErrorsTest, BadPrimitiveModeOutranksTheNoProgramGuard) { + DrainErrors(); + // Exactly what the coverage test passes: GL_POINTS is 0, so this is 0xFFFFFFFF. + constexpr GLenum kBadMode = static_cast(GL_POINTS - 1); + + RunRows({ + {"glDrawArraysInstanced with an unaccepted mode", [] { DrawArraysInstanced(kBadMode, 0, 3, 4); }, + GL_INVALID_ENUM}, + {"glDrawElementsInstanced with an unaccepted mode", + [] { DrawElementsInstanced(kBadMode, 3, GL_UNSIGNED_INT, nullptr, 4); }, GL_INVALID_ENUM}, + {"glDrawArrays with an unaccepted mode", [] { DrawArrays(kBadMode, 0, 3); }, GL_INVALID_ENUM}, + {"glDrawElements with an unaccepted mode", + [] { DrawElements(kBadMode, 3, GL_UNSIGNED_INT, nullptr); }, GL_INVALID_ENUM}, + {"glMultiDrawArrays with an unaccepted mode", + [] { MultiDrawArrays(kBadMode, nullptr, nullptr, 0); }, GL_INVALID_ENUM}, + {"glDrawRangeElements with an unaccepted mode", + [] { DrawRangeElements(kBadMode, 0, 2, 3, GL_UNSIGNED_INT, nullptr); }, GL_INVALID_ENUM}, + {"glDrawElementsIndirect with an unaccepted mode", + [] { DrawElementsIndirect(kBadMode, GL_UNSIGNED_INT, nullptr); }, GL_INVALID_ENUM}, + {"glDrawArraysIndirect with an unaccepted mode", [] { DrawArraysIndirect(kBadMode, nullptr); }, + GL_INVALID_ENUM}, + // A mode the enum check accepts falls through to the guard, so the INVALID_OPERATION + // that used to win is still raised for the calls it is actually about. + {"glDrawArrays with a legal mode and no program bound", [] { DrawArrays(GL_TRIANGLES, 0, 3); }, + GL_INVALID_OPERATION}, + }); + } } // namespace From 7940a094916a48d097a94f8039d5bb75d1b8171f Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 05:12:18 -0400 Subject: [PATCH 15/21] [Fix, Test] (GLImpl, MG_Test): validate glBlitFramebuffer's mask bits, filter enum and LINEAR depth rule --- .../GLImpl/Framebuffer/GL_Framebuffer.cpp | 40 ++++++++++++++++ .../MG_Test/State/NegativeApiErrorsTest.cpp | 48 +++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp index 39638faa..60292eea 100644 --- a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp +++ b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp @@ -3153,15 +3153,55 @@ namespace MobileGL::MG_Impl::GLImpl { GetNamedFramebufferAttachmentParameteriv_State(framebuffer, attachment, pname, params); } + // The three argument errors GL 4.6 core 18.3.1 asks a blit for. They have to be raised here, + // in the backend-independent frontend: DirectGLES drains the driver's error queue around the + // blit on purpose (that is how the resolve fallback probes the driver), so an ES-side + // rejection never reaches the application and glGetError() answered GL_NO_ERROR for a call + // the spec requires to fail (KHR-GL30.api.coverage's glBlitFramebuffer sub-check). DirectVulkan + // already dropped the bad-filter and LINEAR-with-depth/stencil calls on the floor with a log + // line (VulkanRenderer::BlitFramebuffer), so the only thing that changes for it is that the + // error is now visible where the spec says it should be. + static Bool ValidateBlitMaskAndFilter(const char* functionName, GLbitfield mask, GLenum filter) { + constexpr GLbitfield kBlitMaskBits = GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT; + if ((mask & ~kBlitMaskBits) != 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", functionName, + "mask contains bits other than GL_COLOR_BUFFER_BIT, " + "GL_DEPTH_BUFFER_BIT and GL_STENCIL_BUFFER_BIT.")); + return false; + } + if (filter != GL_NEAREST && filter != GL_LINEAR) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidEnum, + MakeUnique("MG_Impl/GLImpl", functionName, + "filter must be GL_NEAREST or GL_LINEAR.")); + return false; + } + // Depth and stencil have no meaningful interpolation, so GL_LINEAR is rejected outright + // rather than downgraded - even when the mask also carries the colour bit. + if (filter == GL_LINEAR && (mask & (GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT)) != 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", functionName, + "GL_LINEAR filtering is not allowed when mask includes " + "GL_DEPTH_BUFFER_BIT or GL_STENCIL_BUFFER_BIT.")); + return false; + } + return true; + } + void BlitNamedFramebuffer(GLuint readFramebuffer, GLuint drawFramebuffer, GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1, GLint dstX0, GLint dstY0, GLint dstX1, GLint dstY1, GLbitfield mask, GLenum filter) { + if (!ValidateBlitMaskAndFilter(__func__, mask, filter)) return; BlitNamedFramebuffer_State(readFramebuffer, drawFramebuffer, srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, mask, filter); } void BlitFramebuffer(GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1, GLint dstX0, GLint dstY0, GLint dstX1, GLint dstY1, GLbitfield mask, GLenum filter) { + if (!ValidateBlitMaskAndFilter(__func__, mask, filter)) return; BlitFramebuffer_Backend(srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, mask, filter); } diff --git a/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp b/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp index 50ee18b2..6459a0e7 100644 --- a/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp +++ b/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp @@ -21,6 +21,8 @@ // * KHR-GL32.api.coverage - glFenceSync's condition/flags and glWaitSync's flags/timeout. // * KHR-GL31.api.coverage - a draw's mode INVALID_ENUM has to outrank MobileGL's own // no-current-program guard. +// * KHR-GL30.api.coverage - glBlitFramebuffer's mask bits, filter enum and the LINEAR-with- +// depth/stencil rule. // Plus the indexed-getter parity RC-7b is about: glGetBooleani_v / glGetInteger64i_v / // glGetFloati_v / glGetDoublei_v must answer every pname glGetIntegeri_v answers. // @@ -36,6 +38,7 @@ #include "Init.h" #include #include +#include #include #include #include @@ -539,4 +542,49 @@ void main() { g_color = vec4(1); } GL_INVALID_OPERATION}, }); } + + // KHR-GL30.api.coverage's glBlitFramebuffer sub-check. The frontend passed mask and filter + // straight through, and DirectGLES drains the driver's error queue around the blit so the ES + // rejection never surfaced either - both illegal calls reported GL_NO_ERROR. Every row here + // is rejected before the backend function pointer is reached, which is what lets this + // GPU-free suite run them at all. + TEST_F(NegativeApiErrorsTest, BlitFramebufferRejectsBadMasksAndFilters) { + DrainErrors(); + // The bit the coverage test smuggles in: a legal glMapBufferRange flag, not a blit one. + constexpr GLbitfield kForeignBit = GL_MAP_INVALIDATE_BUFFER_BIT; + + RunRows({ + {"glBlitFramebuffer with a mask bit outside COLOR|DEPTH|STENCIL", + [] { + BlitFramebuffer(0, 0, 16, 16, 0, 0, 16, 16, GL_COLOR_BUFFER_BIT | kForeignBit, GL_NEAREST); + }, + GL_INVALID_VALUE}, + {"glBlitFramebuffer with a filter that is neither GL_NEAREST nor GL_LINEAR", + [] { BlitFramebuffer(0, 0, 16, 16, 0, 0, 16, 16, GL_COLOR_BUFFER_BIT, GL_NONE); }, GL_INVALID_ENUM}, + {"glBlitFramebuffer of colour+stencil with GL_LINEAR", + [] { + BlitFramebuffer(0, 0, 16, 16, 0, 0, 16, 16, GL_COLOR_BUFFER_BIT | GL_STENCIL_BUFFER_BIT, GL_LINEAR); + }, + GL_INVALID_OPERATION}, + {"glBlitFramebuffer of depth with GL_LINEAR", + [] { BlitFramebuffer(0, 0, 16, 16, 0, 0, 16, 16, GL_DEPTH_BUFFER_BIT, GL_LINEAR); }, + GL_INVALID_OPERATION}, + // The DSA form has to answer identically. + {"glBlitNamedFramebuffer with a mask bit outside COLOR|DEPTH|STENCIL", + [] { + BlitNamedFramebuffer(0, 0, 0, 0, 16, 16, 0, 0, 16, 16, GL_COLOR_BUFFER_BIT | kForeignBit, + GL_NEAREST); + }, + GL_INVALID_VALUE}, + {"glBlitNamedFramebuffer with a bad filter", + [] { BlitNamedFramebuffer(0, 0, 0, 0, 16, 16, 0, 0, 16, 16, GL_COLOR_BUFFER_BIT, GL_NONE); }, + GL_INVALID_ENUM}, + {"glBlitNamedFramebuffer of depth+stencil with GL_LINEAR", + [] { + BlitNamedFramebuffer(0, 0, 0, 0, 16, 16, 0, 0, 16, 16, + GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT, GL_LINEAR); + }, + GL_INVALID_OPERATION}, + }); + } } // namespace From 15e36ad1e9184c958551af41d6eacdc32f0c611d Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 05:29:08 -0400 Subject: [PATCH 16/21] [Feat, Test] (ShaderTranspiler, MG_Test): demote gl_ViewportIndex to a plain global for ESSL targets --- CMakeLists.txt | 1 + .../MG_Test/ShaderTranspiler/CMakeLists.txt | 1 + .../LowerViewportIndexTest.cpp | 237 +++++++++++++++++ .../ShaderTranspiler/ShaderCompiler.cpp | 16 ++ .../MG_Util/ShaderTranspiler/ShaderCompiler.h | 14 ++ .../SpirvPasses/LowerViewportIndexPass.cpp | 238 ++++++++++++++++++ .../SpirvPasses/LowerViewportIndexPass.h | 51 ++++ 7 files changed, 558 insertions(+) create mode 100644 MobileGL/MG_Test/ShaderTranspiler/LowerViewportIndexTest.cpp create mode 100644 MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp create mode 100644 MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 93efd897..e40a8faa 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -279,6 +279,7 @@ set(SOURCE_FILES MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/DecoratePositionInvariantPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/DemoteFloat64Pass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerDrawParametersPass.cpp + MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/PackDoubleVertexInputsPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenXfbInterfaceBlocksPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.cpp diff --git a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt index fd389fb4..6cda0d75 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt +++ b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt @@ -9,6 +9,7 @@ add_executable( EmulateSubgroupsTest.cpp DemoteFloat64Test.cpp FlattenXfbInterfaceBlocksTest.cpp + LowerViewportIndexTest.cpp ) target_include_directories(SpirvPassTest PRIVATE diff --git a/MobileGL/MG_Test/ShaderTranspiler/LowerViewportIndexTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/LowerViewportIndexTest.cpp new file mode 100644 index 00000000..3f744fe4 --- /dev/null +++ b/MobileGL/MG_Test/ShaderTranspiler/LowerViewportIndexTest.cpp @@ -0,0 +1,237 @@ +// MobileGL - MobileGL/MG_Test/ShaderTranspiler/LowerViewportIndexTest.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// LowerViewportIndexPass is the DirectGLES fallback for a driver with no GL_OES_viewport_array. +// The thing it prevents is not a wrong pixel but a missing program: ESSL has no core +// gl_ViewportIndex at any version, SPIRV-Cross prints the identifier bare, and the driver rejects +// the stage - after which DirectGLES binds program 0 and every draw renders nothing while +// GL_LINK_STATUS still answers TRUE. So what has to hold is textual and structural at once: the +// emitted ESSL must stop naming the builtin, the module must stay valid, and gl_Layer - which IS +// core in ESSL 3.20 geometry shaders - must come through untouched. +// +// Real GLSL through the same glslang path the backends use, rather than hand-assembled words, for +// the same reason MG_Test/Pipeline/ViewportIndexReflectionTest.cpp does it: what matters is what +// glslang actually emits for these shaders. + +#include + +#include +#include + +#include "Includes.h" +#include "Init.h" +#include +#include +#include + +#include + +using namespace MobileGL; +using MobileGL::MG_Util::ShaderTranspiler::SessionUsageBit; +using MobileGL::MG_Util::ShaderTranspiler::ShaderCompiler; +using MobileGL::MG_Util::ShaderTranspiler::SpvcSession; + +namespace { + Vector CompileToSpirv(GLenum stage, const String& source) { + using namespace MG_Util::ShaderTranspiler; + ShaderAttrib shaderAttrib{.shaderType = stage, .sourceStr = source}; + auto shaderResult = ShaderCompiler::CompileShader(shaderAttrib); + EXPECT_TRUE(shaderResult) << (shaderResult ? String{} : shaderResult.error().log); + if (!shaderResult) return {}; + + ProgramAttrib programAttrib{.shaders = {shaderResult.value()}}; + auto programResult = ShaderCompiler::LinkProgram(programAttrib); + EXPECT_TRUE(programResult) << (programResult ? String{} : programResult.error().log); + if (!programResult) return {}; + + ProgramBinaryAttrib binaryAttrib{.shaderTypes = {stage}, .program = *programResult.value()}; + auto binaryResult = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); + EXPECT_TRUE(binaryResult) << (binaryResult ? String{} : binaryResult.error().log); + if (!binaryResult || binaryResult->empty()) return {}; + return binaryResult->front(); + } + + String Disassemble(const Vector& spirv) { + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + String text; + tools.Disassemble(spirv, &text); + return text; + } + + // ESSL 320, i.e. exactly what the DirectGLES transpile asks SPIRV-Cross for. + String Transpile(const Vector& spirv) { + SpvcSession session(spirv, SessionUsageBit::Transpile); + auto essl = ShaderCompiler::DecompileShader(session); + EXPECT_TRUE(essl) << (essl ? String{} : essl.error().log); + return essl ? essl.value() : String{}; + } + + Bool Contains(const String& haystack, const String& needle) { + return haystack.find(needle) != String::npos; + } + + // KHR-GL4x.viewport_array.draw_to_single_layer_with_multiple_viewports' geometry stage in + // miniature: sixteen invocations, each routing its primitive to its own viewport. This is the + // shape that today loses the whole program on a driver without GL_OES_viewport_array. + const char* const kGeometryWritesViewportIndex = R"(#version 410 core +layout(points, invocations = 16) in; +layout(triangle_strip, max_vertices = 4) out; +void main() { + gl_ViewportIndex = gl_InvocationID; + gl_Position = vec4(-1.0, -1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4( 1.0, -1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4(-1.0, 1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4( 1.0, 1.0, 0.0, 1.0); EmitVertex(); + EndPrimitive(); +} +)"; + + // Layered rendering, not viewport routing. gl_Layer IS core in ESSL 3.20 geometry shaders, so + // demoting it would break a Minecraft-style cubemap pass that works today. + const char* const kGeometryWritesLayerOnly = R"(#version 410 core +layout(points, invocations = 6) in; +layout(triangle_strip, max_vertices = 4) out; +void main() { + gl_Layer = gl_InvocationID; + gl_Position = vec4(-1.0, -1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4( 1.0, -1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4(-1.0, 1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4( 1.0, 1.0, 0.0, 1.0); EmitVertex(); + EndPrimitive(); +} +)"; + + // Both at once, which is the case that separates "lowers the right builtin" from "lowers every + // builtin it can reach": KHR-GL4x.viewport_array.draw_multiple_layers writes both. + const char* const kGeometryWritesBoth = R"(#version 410 core +layout(points, invocations = 16) in; +layout(triangle_strip, max_vertices = 4) out; +void main() { + gl_ViewportIndex = gl_InvocationID; + gl_Layer = gl_InvocationID; + gl_Position = vec4(-1.0, -1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4( 1.0, -1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4(-1.0, 1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4( 1.0, 1.0, 0.0, 1.0); EmitVertex(); + EndPrimitive(); +} +)"; + + const char* const kPlainGeometry = R"(#version 410 core +layout(points, invocations = 1) in; +layout(triangle_strip, max_vertices = 4) out; +void main() { + gl_Position = vec4(-1.0, -1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4( 1.0, -1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4(-1.0, 1.0, 0.0, 1.0); EmitVertex(); + gl_Position = vec4( 1.0, 1.0, 0.0, 1.0); EmitVertex(); + EndPrimitive(); +} +)"; +} // namespace + +class LowerViewportIndexTest : public ::testing::Test { +protected: + void SetUp() override { + MobileGL::Initialize(); + m_validationFailuresAtStart = ShaderCompiler::SpirvValidationFailureCount(); + } + + void TearDown() override { + EXPECT_EQ(ShaderCompiler::SpirvValidationFailureCount(), m_validationFailuresAtStart) + << "the lowered module did not survive spirv-val"; + } + + Uint64 m_validationFailuresAtStart = 0; +}; + +// The probe is the gate that keeps every ordinary stage off an optimizer round trip, so it has to +// answer no for a shader that never routes a viewport - and yes for the one that does. +TEST_F(LowerViewportIndexTest, TheProbeAnswersOnlyForAViewportIndexWriter) { + const Vector plain = CompileToSpirv(GL_GEOMETRY_SHADER, kPlainGeometry); + ASSERT_FALSE(plain.empty()); + EXPECT_FALSE(ShaderCompiler::DeclaresViewportIndexBuiltin(plain)); + + const Vector layerOnly = CompileToSpirv(GL_GEOMETRY_SHADER, kGeometryWritesLayerOnly); + ASSERT_FALSE(layerOnly.empty()); + EXPECT_FALSE(ShaderCompiler::DeclaresViewportIndexBuiltin(layerOnly)); + + const Vector writer = CompileToSpirv(GL_GEOMETRY_SHADER, kGeometryWritesViewportIndex); + ASSERT_FALSE(writer.empty()); + EXPECT_TRUE(ShaderCompiler::DeclaresViewportIndexBuiltin(writer)); + + // Runs on every stage of every program on a driver without the extension, so it must survive a + // stage that produced no SPIR-V rather than pushing a parse diagnostic for it. + EXPECT_FALSE(ShaderCompiler::DeclaresViewportIndexBuiltin({})); +} + +// The whole point: the emitted ESSL must stop naming a builtin the language does not have. +TEST_F(LowerViewportIndexTest, DemotesTheBuiltinToAnOrdinaryGlobal) { + const Vector input = CompileToSpirv(GL_GEOMETRY_SHADER, kGeometryWritesViewportIndex); + ASSERT_FALSE(input.empty()); + + // Negative control, and the bug itself: untouched, SPIRV-Cross prints gl_ViewportIndex into + // ESSL 320 and asks for no extension to go with it. + const String before = Transpile(input); + EXPECT_TRUE(Contains(before, "gl_ViewportIndex")) << before; + + Vector output; + ASSERT_TRUE(ShaderCompiler::LowerViewportIndexForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + + const String dis = Disassemble(output); + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + ASSERT_TRUE(tools.Validate(output)) << dis; + EXPECT_FALSE(Contains(dis, "BuiltIn ViewportIndex")) << dis; + EXPECT_TRUE(Contains(dis, "mg_ViewportIndex")) << dis; + EXPECT_TRUE(Contains(dis, "Private")) << dis; + + const String after = Transpile(output); + EXPECT_TRUE(Contains(after, "mg_ViewportIndex")) << after; + EXPECT_FALSE(Contains(after, "gl_ViewportIndex")) << after; +} + +// gl_Layer is core in ESSL 3.20 geometry shaders and layered rendering works on this backend +// today. Lowering it too would trade one silent failure for another. +TEST_F(LowerViewportIndexTest, LeavesGlLayerAlone) { + const Vector input = CompileToSpirv(GL_GEOMETRY_SHADER, kGeometryWritesBoth); + ASSERT_FALSE(input.empty()); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LowerViewportIndexForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + + const String dis = Disassemble(output); + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + ASSERT_TRUE(tools.Validate(output)) << dis; + EXPECT_FALSE(Contains(dis, "BuiltIn ViewportIndex")) << dis; + EXPECT_TRUE(Contains(dis, "BuiltIn Layer")) << dis; + + const String after = Transpile(output); + EXPECT_TRUE(Contains(after, "gl_Layer")) << after; + EXPECT_FALSE(Contains(after, "gl_ViewportIndex")) << after; +} + +// Every other stage on a driver without the extension goes through this pass too (behind the +// probe), so a module it has nothing to do with must come out saying exactly what it said. +TEST_F(LowerViewportIndexTest, LeavesAModuleWithoutTheBuiltinUntouched) { + const Vector input = CompileToSpirv(GL_GEOMETRY_SHADER, kPlainGeometry); + ASSERT_FALSE(input.empty()); + + const String before = Transpile(input); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LowerViewportIndexForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + + const String dis = Disassemble(output); + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + ASSERT_TRUE(tools.Validate(output)) << dis; + EXPECT_FALSE(Contains(dis, "mg_ViewportIndex")) << dis; + EXPECT_EQ(Transpile(output), before); +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 49e10680..e3ecdd9e 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -20,6 +20,7 @@ #include "SpirvPasses/DecoratePositionInvariantPass.h" #include "SpirvPasses/DemoteFloat64Pass.h" #include "SpirvPasses/LowerDrawParametersPass.h" +#include "SpirvPasses/LowerViewportIndexPass.h" #include "SpirvPasses/PackDoubleVertexInputsPass.h" #include "SpirvPasses/FlattenXfbInterfaceBlocksPass.h" #include "SpirvPasses/SplitArrayVertexInputsPass.h" @@ -635,6 +636,21 @@ namespace MobileGL { outputBinary, true, enableSpirvValidation); } + bool ShaderCompiler::LowerViewportIndexForEssl(const Vector& inputBinary, + Vector& outputBinary, + const bool enableSpirvValidation) { + using namespace spvtools; + Optimizer optimizer(SPV_ENV_VULKAN_1_1); + optimizer.RegisterPass(LowerViewportIndexPass::CreateLowerViewportIndexPass()); + + return RunOptimizerChecked("LowerViewportIndexForEssl", optimizer, inputBinary, + outputBinary, true, enableSpirvValidation); + } + + bool ShaderCompiler::DeclaresViewportIndexBuiltin(const Vector& binary) { + return LowerViewportIndexPass::DeclaresViewportIndexBuiltin(binary); + } + bool ShaderCompiler::SplitArrayVertexInputsForEssl(const Vector& inputBinary, Vector& outputBinary, const bool enableSpirvValidation) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index 6a371603..fb52679c 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -32,6 +32,20 @@ namespace MobileGL { static bool LowerDrawParametersForEssl(const Vector& inputBinary, Vector& outputBinary, bool enableSpirvValidation = false); + // Demotes the gl_ViewportIndex OUTPUT builtin to a plain Private global named + // mg_ViewportIndex, so SPIRV-Cross emits an ordinary declaration instead of a bare + // gl_ViewportIndex that ESSL has no core spelling for. Multi-viewport routing is + // lost (everything lands in viewport 0) but the stage compiles and the program + // runs, instead of every draw made with it becoming a silent no-op. Only for the + // DirectGLES transpile path on a driver WITHOUT GL_OES_viewport_array; gl_Layer is + // deliberately left alone, being core in ESSL 3.20 geometry shaders. + static bool LowerViewportIndexForEssl(const Vector& inputBinary, + Vector& outputBinary, + bool enableSpirvValidation = false); + // Whether the module declares an output decorated BuiltIn ViewportIndex, i.e. + // whether the pass above has anything to do. The gate that keeps every other + // stage off an optimizer round trip it does not need. + static bool DeclaresViewportIndexBuiltin(const Vector& binary); // Replaces an ARRAY vertex input with one input per element at consecutive // locations, seeding a Private copy of the array so indexed reads still work. // GLSL ES has no array vertex inputs and SPIRV-Cross refuses the whole module diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp new file mode 100644 index 00000000..877ca016 --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp @@ -0,0 +1,238 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include "LowerViewportIndexPass.h" + +#include "spirv.hpp" +#include "source/opt/build_module.h" +#include "source/opt/def_use_manager.h" +#include "source/opt/instruction.h" +#include "source/opt/ir_context.h" +#include "source/opt/module.h" +#include "source/util/make_unique.h" + +#include +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + namespace { + using spvtools::opt::Instruction; + using spvtools::opt::IRContext; + using spvtools::opt::Operand; + + // The name the decompiled ESSL ends up declaring. Same mg_ prefix as the + // draw-parameter lowering, so a global that came from a demoted builtin is + // recognisable in a driver log. + constexpr const char* kLoweredName = "mg_ViewportIndex"; + + // The one decoration this pass lowers. OpDecorate only, never OpMemberDecorate: + // glslang emits gl_ViewportIndex as a standalone variable, and a member of a + // gl_PerVertex-shaped block could not be demoted on its own anyway. BuiltIn Layer + // is deliberately not matched - see the header. + Bool IsViewportIndexBuiltinDecoration(const Instruction& annotation) { + if (annotation.opcode() != spv::Op::OpDecorate || + static_cast(annotation.GetSingleWordInOperand(1)) != + spv::Decoration::BuiltIn) { + return false; + } + return static_cast(annotation.GetSingleWordInOperand(2)) == + spv::BuiltIn::ViewportIndex; + } + + // The OUTPUT variable that decoration names, or nullptr. Only an output is + // demotable: a fragment stage READS gl_ViewportIndex as an Input, and a Private + // global has no defined value to read, so lowering that one would answer the + // shader with garbage instead of the viewport it asked for. That case is left for + // the driver to reject. + Instruction* GetDecoratedViewportIndexOutput(IRContext* context, + const Instruction& annotation) { + Instruction* variable = + context->get_def_use_mgr()->GetDef(annotation.GetSingleWordInOperand(0)); + if (variable == nullptr || variable->opcode() != spv::Op::OpVariable || + static_cast(variable->GetSingleWordInOperand(0)) != + spv::StorageClass::Output) { + return nullptr; + } + return variable; + } + + // Decorations the validator accepts only on an Input/Output variable, so they have + // to go with the storage class or the demoted module stops validating. glslang + // puts none of these on gl_ViewportIndex today - the BuiltIn is all it writes - + // but a geometry `layout(stream = N)` qualifier decorates every output of the + // stage, and the pass must not be the thing that produces an invalid module. + Bool IsInterfaceOnlyDecoration(spv::Decoration decoration) { + switch (decoration) { + case spv::Decoration::Flat: + case spv::Decoration::NoPerspective: + case spv::Decoration::Centroid: + case spv::Decoration::Sample: + case spv::Decoration::Patch: + case spv::Decoration::Invariant: + case spv::Decoration::Location: + case spv::Decoration::Component: + case spv::Decoration::Stream: + case spv::Decoration::XfbBuffer: + case spv::Decoration::XfbStride: + return true; + default: + return false; + } + } + + void ReplaceName(IRContext* context, uint32_t id, const char* name) { + for (auto& debugInst : context->debugs2()) { + if (debugInst.opcode() == spv::Op::OpName && debugInst.GetSingleWordInOperand(0) == id) { + debugInst.SetInOperand( + 1, spvtools::utils::MakeVector(name)); + return; + } + } + context->AddDebug2Inst(spvtools::MakeUnique( + context, spv::Op::OpName, 0, 0, + std::initializer_list{ + {SPV_OPERAND_TYPE_ID, {id}}, + {SPV_OPERAND_TYPE_LITERAL_STRING, spvtools::utils::MakeVector(name)}})); + } + + void RemoveFromEntryPointInterfaces(IRContext* context, uint32_t id) { + for (Instruction& entryPoint : context->module()->entry_points()) { + std::vector newOperands; + Bool changed = false; + for (uint32_t i = 0; i < entryPoint.NumInOperands(); ++i) { + const Operand& operand = entryPoint.GetInOperand(i); + // Interface ids start after execution model, entry-point id and name. + if (i >= 3 && operand.type == SPV_OPERAND_TYPE_ID && + entryPoint.GetSingleWordInOperand(i) == id) { + changed = true; + continue; + } + newOperands.push_back(operand); + } + if (changed) { + entryPoint.SetInOperands(std::move(newOperands)); + } + } + } + } // namespace + + bool LowerViewportIndexPass::DeclaresViewportIndexBuiltin(const Vector& binary) { + if (binary.empty()) { + // An empty module is a stage that produced no SPIR-V, which is not a verdict + // about viewport routing; letting BuildModule reject it would push a spurious + // diagnostic through the message consumer first. + return false; + } + std::unique_ptr context = spvtools::BuildModule( + SPV_ENV_VULKAN_1_1, [](spv_message_level_t, const char*, const spv_position_t&, const char*) {}, + binary.data(), binary.size()); + if (!context) { + // Unparseable here means unusable downstream too; let the ordinary transpile + // path produce the error rather than inventing a verdict from it. + return false; + } + for (const Instruction& annotation : context->annotations()) { + if (IsViewportIndexBuiltinDecoration(annotation) && + GetDecoratedViewportIndexOutput(context.get(), annotation) != nullptr) { + return true; + } + } + return false; + } + + spvtools::opt::Pass::Status LowerViewportIndexPass::Process() { + auto* irContext = context(); + + // Collect the decorations to lower first; mutating while iterating annotations + // invalidates the range. + struct LoweredVariable { + Instruction* variable = nullptr; + Instruction* decoration = nullptr; + }; + std::vector targets; + + for (auto& annotation : irContext->annotations()) { + if (!IsViewportIndexBuiltinDecoration(annotation)) { + continue; + } + + Instruction* variable = GetDecoratedViewportIndexOutput(irContext, annotation); + if (variable == nullptr) { + continue; + } + + targets.push_back({variable, &annotation}); + } + + if (targets.empty()) { + return Status::SuccessWithoutChange; + } + + // Second collection pass, for the same reason as the first: the decorations that + // stop being legal once the variable leaves the Output storage class. + std::vector deadDecorations; + for (auto& annotation : irContext->annotations()) { + if (annotation.opcode() != spv::Op::OpDecorate || + !IsInterfaceOnlyDecoration( + static_cast(annotation.GetSingleWordInOperand(1)))) { + continue; + } + const uint32_t decoratedId = annotation.GetSingleWordInOperand(0); + for (const auto& target : targets) { + if (target.variable->result_id() == decoratedId) { + deadDecorations.push_back(&annotation); + break; + } + } + } + + auto* defUseMgr = irContext->get_def_use_mgr(); + auto* typeMgr = irContext->get_type_mgr(); + + for (auto& target : targets) { + Instruction* variable = target.variable; + const uint32_t variableId = variable->result_id(); + + // Demote the Output builtin to a plain Private global. Every store the shader + // already makes stays exactly where it is - it simply no longer reaches the + // rasterizer, which is the whole of the degradation. + Instruction* pointerType = defUseMgr->GetDef(variable->type_id()); + const uint32_t pointeeTypeId = pointerType->GetSingleWordInOperand(1); + const uint32_t privatePointerTypeId = + typeMgr->FindPointerToType(pointeeTypeId, spv::StorageClass::Private); + variable->SetResultType(privatePointerTypeId); + variable->SetInOperand(0, {static_cast(spv::StorageClass::Private)}); + + irContext->KillInst(target.decoration); + RemoveFromEntryPointInterfaces(irContext, variableId); + ReplaceName(irContext, variableId, kLoweredName); + } + + for (auto* decoration : deadDecorations) { + irContext->KillInst(decoration); + } + + // The MultiViewport / ShaderViewportIndexLayerEXT capabilities are deliberately + // left declared, unlike DrawParameters in the sibling pass. They are not exclusive + // to this builtin: ShaderViewportIndexLayerEXT also enables gl_Layer in the + // pre-geometry stages, and it DEPENDS on MultiViewport, so dropping either can + // invalidate a module that still writes Layer. A declared-but-unused capability is + // legal SPIR-V and SPIRV-Cross's GLSL backend reads neither of them, so leaving + // both costs nothing. + + return Status::SuccessWithChange; + } + + spvtools::Optimizer::PassToken LowerViewportIndexPass::CreateLowerViewportIndexPass() { + return spvtools::Optimizer::PassToken(MakeUnique()); + } + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h new file mode 100644 index 00000000..fcb207ed --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h @@ -0,0 +1,51 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#pragma once +#include "source/opt/pass.h" +#include "spirv-tools/optimizer.hpp" + +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + // ESSL has no core gl_ViewportIndex at any version - only GL_OES_viewport_array + // introduces it - and SPIRV-Cross prints the identifier bare, requesting no extension + // for it (contrast BuiltInLayer, which it backs with GL_NV_viewport_array2 on ES). On + // a driver WITHOUT that extension the stage therefore fails to compile, DirectGLES + // marks the program unusable and binds program 0 for it, and every draw silently + // renders nothing while GL_LINK_STATUS still answers TRUE - the failure signature + // KHR-GL4x.viewport_array reports as "expected N, got -1", i.e. the untouched upload. + // + // This pass demotes the ViewportIndex OUTPUT to a plain Private global named + // mg_ViewportIndex, so the decompiled ESSL declares an ordinary global the shader + // still writes and nothing reads. The program compiles and rendering degrades to + // viewport 0 - which is the single-viewport behaviour MG_IntegrationTest's + // ViewportArrayScenario already documents for this backend - instead of the whole + // program becoming a no-op. Only meant for the DirectGLES transpile path; the Vulkan + // backend keeps the native builtin and routes it for real. + // + // gl_Layer is deliberately NOT touched: BuiltIn Layer IS core in ESSL 3.20 geometry + // shaders, and demoting it would break layered rendering that works today. + class LowerViewportIndexPass : public spvtools::opt::Pass { + public: + const char* name() const override { return "lower-viewport-index"; } + Status Process() override; + + // Whether the module declares an output decorated BuiltIn ViewportIndex, i.e. + // whether running this pass could change anything. Answered from a single parse so + // the caller can skip the optimizer round trip entirely - which is every shader + // but the handful that route viewports from the shader. + static bool DeclaresViewportIndexBuiltin(const Vector& binary); + + static spvtools::Optimizer::PassToken CreateLowerViewportIndexPass(); + }; + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL From f2f6430e349ee2597eda21513b00e5ff42a6f2df Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 05:29:12 -0400 Subject: [PATCH 17/21] [Feat] (Loader): detect GL_OES_viewport_array in the GLES capability scan --- MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp | 5 +++++ MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp index 7f0c8a23..6e0128b0 100644 --- a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp +++ b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp @@ -932,6 +932,9 @@ namespace MobileGL::MG_Util::BackendLoader { if (std::strcmp(extension, "GL_EXT_clip_cull_distance") == 0) { caps.SupportsClipDistance = true; } + if (std::strcmp(extension, "GL_OES_viewport_array") == 0) { + caps.SupportsViewportArray = true; + } } } // The pointer check on top of the extension check makes each flag sufficient on its own @@ -989,6 +992,8 @@ namespace MobileGL::MG_Util::BackendLoader { MGLOG_I(" base instance (EXT_base_instance; emulated by attribute offsets when absent): %s", caps.SupportsBaseInstance ? "yes" : "no"); MGLOG_I(" clip distances (EXT_clip_cull_distance): %s", caps.SupportsClipDistance ? "yes" : "no"); + MGLOG_I(" viewport array (OES_viewport_array; gl_ViewportIndex collapses to viewport 0 when absent): %s", + caps.SupportsViewportArray ? "yes" : "no"); // LOAD-BEARING STRING, not just a banner. android-plugin/trace-replay-ci.sh's // is_angle_surface_lost() greps mobilegl.log for exactly "OpenGL ES capabilities:" to diff --git a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h index 64f6cef2..2d986489 100644 --- a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h +++ b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h @@ -1182,6 +1182,21 @@ namespace MobileGL { // compile and the per-distance enables have nowhere to go - clipping silently never // happens, which is exactly what KHR-GLxx.clip_distance.functional catches. Bool SupportsClipDistance = false; + // GL_OES_viewport_array is present: the driver knows gl_ViewportIndex in ESSL - and + // only then. ESSL has no core spelling for it at ANY version, while SPIRV-Cross prints + // the identifier bare and requests nothing for it (contrast gl_Layer, which it backs + // with GL_NV_viewport_array2 on ES), so the `#extension GL_OES_viewport_array : + // require` line has to be inserted into the emitted source - see + // RequestViewportArrayExtension. Without the extension the stage does not compile at + // all and the whole program becomes unusable, which on DirectGLES means every draw + // using it silently renders nothing; LowerViewportIndexPass is the fallback that + // demotes the builtin so the program still links and degrades to viewport 0. + // + // Extension string only, deliberately: DirectGLES does not call any of the indexed + // OES entry points yet, so there is no pointer to require. When that forwarding lands + // this must gain the pointer check as well - the rule everywhere else in this struct, + // because eglGetProcAddress can hand back a stub that silently drops every call. + Bool SupportsViewportArray = false; // GL_RENDERER contains "ANGLE". Bool IsAngleRenderer = false; // GL_RENDERER contains both "ANGLE" and "llvmpipe". From bde14cae299f6e6b18b29146bed467d03d4c1833 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 05:29:18 -0400 Subject: [PATCH 18/21] [Fix, Test] (DirectGLES, MG_Test): request GL_OES_viewport_array in the emitted ESSL, or lower the builtin away --- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 35 ++++++++++++ MobileGL/MG_Backend/DirectGLES/Utils.cpp | 37 +++++++++++++ MobileGL/MG_Backend/DirectGLES/Utils.h | 10 ++++ .../Backend/DirectGLES/EsslShaderPassTest.cpp | 53 +++++++++++++++++++ 4 files changed, 135 insertions(+) diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index efb45795..55ddadf6 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -4899,6 +4899,30 @@ namespace MobileGL::MG_Backend::DirectGLES { effectiveSpirv = &loweredSpirv; } + // ESSL cannot express gl_ViewportIndex either, but unlike the draw parameters + // there IS an extension that provides it - so this runs only when the driver does + // NOT advertise GL_OES_viewport_array. A driver that does keeps the builtin and + // gets the `#extension` request added to the decompiled source below instead. + // Demoting the builtin costs the multi-viewport routing (every invocation lands in + // viewport 0), which is the degradation ViewportArrayScenario already documents + // for this backend; NOT demoting it costs the whole program, because the stage + // fails to compile and every draw made with it silently renders nothing. + // Gated on the module actually declaring the output, so no other stage pays an + // optimizer round trip for it. + Vector loweredViewportSpirv; + if (!g_GLESCapabilities.SupportsViewportArray && + MG_Util::ShaderTranspiler::ShaderCompiler::DeclaresViewportIndexBuiltin(*effectiveSpirv) && + MG_Util::ShaderTranspiler::ShaderCompiler::LowerViewportIndexForEssl( + *effectiveSpirv, loweredViewportSpirv, enableSpirvValidation) && + !loweredViewportSpirv.empty()) { + effectiveSpirv = &loweredViewportSpirv; + MGLOG_D("Program %u stage %s writes gl_ViewportIndex, which this ES driver has " + "no GL_OES_viewport_array for. The builtin was demoted to a plain " + "global; every invocation renders into viewport 0.", + m_backendProgramId, + MG_Util::ConvertGLEnumToString(glShaderType).c_str()); + } + // GLSL ES has no ARRAY vertex inputs, and SPIRV-Cross refuses the whole module // rather than emulating them, so this has to happen before it sees the binary. Vector splitArrayInputSpirv; @@ -5080,6 +5104,17 @@ namespace MobileGL::MG_Backend::DirectGLES { source = RequestExtendedImageFormats(std::move(source), imageFormatBake.needsExtendedImageFormats && g_GLESCapabilities.SupportsExtendedImageFormats); + // The third header-level rewrite, for the builtin SPIRV-Cross prints bare: + // gl_ViewportIndex is in no version of ESSL core, so without this directive the + // stage does not compile and the whole program - not just its viewport routing - + // is lost. The token probe keeps the line off every other program and the + // capability gate keeps it off drivers that would hard-error on an unadvertised + // name; a driver without the extension took the LowerViewportIndexPass fallback + // above and its source no longer names the builtin at all, so the two are mutually + // exclusive by construction. Read `source` BEFORE it is moved from. + const Bool needsViewportArrayExtension = g_GLESCapabilities.SupportsViewportArray && + source.find("gl_ViewportIndex") != String::npos; + source = RequestViewportArrayExtension(std::move(source), needsViewportArrayExtension); source = RebindImageUniformsToFrontendUnits(std::move(source), stateProgramObject); // The completion half of the format bake, for the formats SPIRV-Cross throws on diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index 92820c34..c74014ee 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -569,6 +569,43 @@ namespace MobileGL::MG_Backend::DirectGLES { return glslCode; } + String RequestViewportArrayExtension(String glslCode, Bool needed) { +#ifdef TRACY_ENABLE + ZoneScopedC(TRACY_ZONECOLOR_BACKEND); +#endif + // gl_ViewportIndex is desktop GL 4.1 core and is in ESSL only under + // GL_OES_viewport_array. SPIRV-Cross prints the identifier as-is and requests no + // extension for it - three lines away from the BuiltInLayer case, which DOES ask for + // one on ES - so an untouched decompile reaches the driver naming a builtin its core + // language has never heard of. The stage then fails to compile, the program is marked + // unusable and every draw made with it renders nothing while raising no GL error. + // + // Same `needed` contract as RequestExtendedImageFormats, and the same hard rule: + // `#extension` on a name the driver does not advertise is itself a compile error + // (ARM's compiler is strict about it), so this must never be emitted speculatively. + // A driver without the extension does not come through here at all - its module took + // the LowerViewportIndexPass fallback and the emitted source no longer names the + // builtin. + static constexpr const char* kDirective = "#extension GL_OES_viewport_array : require\n"; + static constexpr const char* kExtName = "GL_OES_viewport_array"; + if (!needed || glslCode.find(kExtName) != String::npos) { + return glslCode; + } + // Right after the #version line, for the reason spelled out above: it is the only + // position that must stay first, and ForceSupporterOutput's scan for the LAST + // #extension directive still finds whichever one that ends up being. + const SizeT versionPos = glslCode.find("#version"); + if (versionPos == String::npos) { + return kDirective + glslCode; + } + const SizeT lineEnd = glslCode.find('\n', versionPos); + if (lineEnd == String::npos) { + return glslCode + "\n" + kDirective; + } + glslCode.insert(lineEnd + 1, kDirective); + return glslCode; + } + String BakeImageFormatQualifiers(String glslCode, const UnorderedMap& esslFormatByUniformName) { #ifdef TRACY_ENABLE diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index a679a4aa..33f04bd5 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -154,6 +154,16 @@ namespace MobileGL::MG_Backend::DirectGLES { // extension - requesting an unadvertised extension is itself a compile error, so this is // never emitted speculatively. A no-op when not needed or already present. String RequestExtendedImageFormats(String glslCode, Bool needed); + // Adds `#extension GL_OES_viewport_array : require` when the emitted ESSL names + // gl_ViewportIndex. SPIRV-Cross prints that identifier and asks for nothing (unlike + // gl_Layer, which it backs with GL_NV_viewport_array2 on ES) and ESSL has no core + // spelling for it at any version, so the request has to be made here or the stage does + // not compile - which loses the whole program, not just the multi-viewport routing. + // `needed` is the caller's answer for the same reason as above: only it knows whether the + // driver advertises the extension, and requesting an unadvertised one is itself a compile + // error, so this is never emitted speculatively. A no-op when not needed or already + // present. + String RequestViewportArrayExtension(String glslCode, Bool needed); // Writes a format layout qualifier into the image declarations named in // `esslFormatByUniformName` that still have none. The completion half of the image-format // bake, and ONLY that: the SPIR-V pass (BakeImageFormatsPass) is what normally puts the diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index 1e0bdb57..6ce2facd 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -21,6 +21,7 @@ using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ForceFlatIntegerVaryings; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_WRITE_ALIAS_PREFIX; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RemoveLayoutBinding; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RequestExtendedImageFormats; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RequestViewportArrayExtension; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::SplitReadWriteImageUniforms; namespace { @@ -550,3 +551,55 @@ void main() { imageStore(uni_image, ivec2(0), uvec4(1u)); } EXPECT_EQ(out, source); EXPECT_EQ(CountOf(out, "GL_NV_image_formats"), 1u) << out; } + +// --- GL_OES_viewport_array directive ------------------------------------------------------------- + +// SPIRV-Cross prints gl_ViewportIndex bare and requests nothing for it, and ESSL has no core +// spelling at any version - so without this directive the stage fails to compile, the program is +// marked unusable and every draw made with it silently renders nothing. +TEST(RequestViewportArrayExtensionTest, TheDirectiveGoesRightAfterTheVersionLine) { + const String source = R"(#version 320 es +layout(points) in; +layout(points, max_vertices = 1) out; +void main() { gl_ViewportIndex = gl_InvocationID; EmitVertex(); } +)"; + const String out = RequestViewportArrayExtension(source, true); + EXPECT_TRUE(Contains(out, "#version 320 es\n#extension GL_OES_viewport_array : require\n")) << out; +} + +// Never speculatively: ARM's compiler hard-errors on an `#extension` naming a string the driver +// does not advertise, so the caller's "not needed" answer has to be honoured exactly. A driver +// without the extension gets the LowerViewportIndexPass fallback instead. +TEST(RequestViewportArrayExtensionTest, NotNeededMeansNotEmitted) { + const String source = R"(#version 320 es +layout(points) in; +layout(points, max_vertices = 1) out; +void main() { gl_ViewportIndex = gl_InvocationID; EmitVertex(); } +)"; + EXPECT_EQ(RequestViewportArrayExtension(source, false), source); +} + +TEST(RequestViewportArrayExtensionTest, AnAlreadyPresentDirectiveIsNotDuplicated) { + const String source = R"(#version 320 es +#extension GL_OES_viewport_array : require +layout(points) in; +layout(points, max_vertices = 1) out; +void main() { gl_ViewportIndex = gl_InvocationID; EmitVertex(); } +)"; + const String out = RequestViewportArrayExtension(source, true); + EXPECT_EQ(out, source); + EXPECT_EQ(CountOf(out, "GL_OES_viewport_array"), 1u) << out; +} + +// The two image directives and this one share the insertion point, so a shader that needs both +// must end up with both - and with #version still first. +TEST(RequestViewportArrayExtensionTest, CoexistsWithTheImageFormatDirective) { + const String source = R"(#version 320 es +layout(r8ui, binding = 1) uniform writeonly highp uimage2D uni_image; +void main() { gl_ViewportIndex = 1; imageStore(uni_image, ivec2(0), uvec4(1u)); } +)"; + const String out = RequestViewportArrayExtension(RequestExtendedImageFormats(source, true), true); + EXPECT_EQ(out.find("#version 320 es"), 0u) << out; + EXPECT_TRUE(Contains(out, "#extension GL_NV_image_formats : require\n")) << out; + EXPECT_TRUE(Contains(out, "#extension GL_OES_viewport_array : require\n")) << out; +} From f0fd6407aee88b3f59ed961807ed9dace881289c Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 05:37:53 -0400 Subject: [PATCH 19/21] [Fix] (ShaderTranspiler): keep the demoted viewport-index variable after its private pointer type --- .../SpirvPasses/LowerViewportIndexPass.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp index 877ca016..e0074197 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp @@ -210,6 +210,14 @@ namespace MobileGL { variable->SetResultType(privatePointerTypeId); variable->SetInOperand(0, {static_cast(spv::StorageClass::Private)}); + // FindPointerToType APPENDS a newly minted pointer type to the end of the + // globals section - after this variable - and SPIR-V requires def before use. + // Re-anchor the variable directly after its new type, which is equally correct + // when the type already existed further up. + Instruction* privatePointerType = defUseMgr->GetDef(privatePointerTypeId); + variable->RemoveFromList(); + variable->InsertAfter(privatePointerType); + irContext->KillInst(target.decoration); RemoveFromEntryPointInterfaces(irContext, variableId); ReplaceName(irContext, variableId, kLoweredName); From a4fda520ed6512b00a0d9db980f6323bf745d29a Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 06:15:39 -0400 Subject: [PATCH 20/21] [Fix, Test] (ShaderTranspiler, DirectGLES): clamp multisample fetches to the backend's real sample count --- CMakeLists.txt | 1 + MobileGL/MG_Backend/DirectGLES/Managers.cpp | 38 ++ .../MG_Test/ShaderTranspiler/CMakeLists.txt | 1 + .../ClampMultisampleFetchTest.cpp | 363 +++++++++++++++++ .../ShaderTranspiler/ShaderCompiler.cpp | 21 + .../MG_Util/ShaderTranspiler/ShaderCompiler.h | 19 + .../SpirvPasses/ClampMultisampleFetchPass.cpp | 374 ++++++++++++++++++ .../SpirvPasses/ClampMultisampleFetchPass.h | 89 +++++ 8 files changed, 906 insertions(+) create mode 100644 MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp create mode 100644 MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp create mode 100644 MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h diff --git a/CMakeLists.txt b/CMakeLists.txt index e40a8faa..7bc5f92c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -292,6 +292,7 @@ set(SOURCE_FILES MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/NormalizeRectCoordinatesPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/BakeImageFormatsPass.cpp + MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/PrivateToEntryLocalPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripUniformLocationsPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripUboMemberRelaxedPrecisionPass.cpp diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 55ddadf6..2df8db54 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -4923,6 +4923,44 @@ namespace MobileGL::MG_Backend::DirectGLES { MG_Util::ConvertGLEnumToString(glShaderType).c_str()); } + // GL 4.6 core table 23.53 requires GL_MAX_SAMPLES >= 4, so every multisample + // ceiling MobileGL advertises is floored to 4 no matter what the ES driver + // reports - but the realised allocation cannot be, and + // ClampSamplesToBackendSupport quietly gives an integer or depth multisample + // texture the ONE sample Adreno and Mali actually support for it. A shader + // written against the advertised ceiling then fetches a sample that storage does + // not have and reads garbage; KHR-GL33/40/41.texture_swizzle.functional_* and + // KHR-GLxx.texture_size_promotion.functional bake exactly that literal in. Clamp + // the Sample operand to the backend-real per-category maximum so the fetch lands + // inside the allocation. Gated on some category actually being squeezed AND the + // module actually declaring a multisampled image, so no other stage pays an + // optimizer round trip for it. DirectVulkan is deliberately not given this: it + // allocates the sample count it was asked for, so its modules are already right. + Vector clampedSampleSpirv; + { + // Recomputed here rather than calling GL_Getter's GetAdvertisedMaxSamples(): + // this is backend code and must not reach into the GL frontend. 4 is that + // translation unit's kFrontendMaxSamples, which is the source of truth - + // keep the two in step. + constexpr Int kFrontendMaxSamples = 4; + const Int advertisedMaxSamples = + std::max(g_GLESCapabilities.MaxSamples, kFrontendMaxSamples); + if ((g_GLESCapabilities.MaxColorTextureSamples < advertisedMaxSamples || + g_GLESCapabilities.MaxIntegerSamples < advertisedMaxSamples || + g_GLESCapabilities.MaxDepthTextureSamples < advertisedMaxSamples) && + MG_Util::ShaderTranspiler::ShaderCompiler::DeclaresMultisampledImage( + *effectiveSpirv) && + MG_Util::ShaderTranspiler::ShaderCompiler::ClampMultisampleFetchesForEssl( + *effectiveSpirv, clampedSampleSpirv, + g_GLESCapabilities.MaxColorTextureSamples, + g_GLESCapabilities.MaxIntegerSamples, + g_GLESCapabilities.MaxDepthTextureSamples, advertisedMaxSamples, + enableSpirvValidation) && + !clampedSampleSpirv.empty()) { + effectiveSpirv = &clampedSampleSpirv; + } + } + // GLSL ES has no ARRAY vertex inputs, and SPIRV-Cross refuses the whole module // rather than emulating them, so this has to happen before it sees the binary. Vector splitArrayInputSpirv; diff --git a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt index 6cda0d75..29200a65 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt +++ b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt @@ -10,6 +10,7 @@ add_executable( DemoteFloat64Test.cpp FlattenXfbInterfaceBlocksTest.cpp LowerViewportIndexTest.cpp + ClampMultisampleFetchTest.cpp ) target_include_directories(SpirvPassTest PRIVATE diff --git a/MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp new file mode 100644 index 00000000..83f36bcf --- /dev/null +++ b/MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp @@ -0,0 +1,363 @@ +// MobileGL - MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// ClampMultisampleFetchPass exists because MobileGL advertises one multisample ceiling and the ES +// driver underneath delivers another. GL 4.6 core table 23.53 forces GL_MAX_SAMPLES and +// GL_MAX_INTEGER_SAMPLES up to 4; Adreno and Mali back an integer multisample texture with ONE +// sample, and DirectGLES quietly allocates that (ClampSamplesToBackendSupport). A CTS shader that +// bakes in `texelFetch(usampler2DMS, coord, 3)` - which is what +// KHR-GL33/40/41.texture_swizzle.functional_* and KHR-GLxx.texture_size_promotion.functional do - +// then reads a sample the storage does not have. +// +// So what has to hold is per-fetch and per-category at once: the squeezed category's Sample +// operand must come back in range, a category that is not squeezed must be untouched, a module +// with no multisampled image at all must come out byte for byte as it went in, and every result +// must still be a valid module. Real GLSL through the same glslang path the backends use, for the +// same reason LowerViewportIndexTest.cpp does it: what matters is what glslang actually emits. + +#include + +#define SPV_ENABLE_UTILITY_CODE +#include "glslang/SPIRV/spirv.hpp11" +#undef SPV_ENABLE_UTILITY_CODE + +#include "Includes.h" +#include "Init.h" +#include +#include + +#include + +#include +#include +#include + +using namespace MobileGL; +using MobileGL::MG_Util::ShaderTranspiler::ShaderCompiler; + +namespace { + // GLSL.std.450 instruction number (see 3rdparty/glslang/SPIRV/GLSL.std.450.h). The signed + // minimum, which is what a GLSL `int` sample index asks for. + constexpr Uint32 kGlslStd450SMin = 39u; + + // What MobileGL tells the application GL_MAX_SAMPLES / GL_MAX_INTEGER_SAMPLES are, i.e. + // GL_Getter's kFrontendMaxSamples floor. Each test supplies its own backend-real ceilings + // against it; Adreno and Mali's Immortalis-G925 both really answer 1 for integer formats. + constexpr Int32 kAdvertisedMaxSamples = 4; + + constexpr SizeT kSpirvHeaderWordCount = 5u; + + template + void ForEachInstruction(const Vector& spirv, Visitor&& visit) { + for (SizeT offset = kSpirvHeaderWordCount; offset < spirv.size();) { + const Uint32 wordCount = spirv[offset] >> 16u; + if (wordCount == 0u || offset + wordCount > spirv.size()) break; + visit(static_cast(spirv[offset] & 0xffffu), &spirv[offset], wordCount); + offset += wordCount; + } + } + + Vector CompileFragment(const String& source) { + using namespace MobileGL::MG_Util::ShaderTranspiler; + ShaderAttrib shaderAttrib{.shaderType = GL_FRAGMENT_SHADER, .sourceStr = source}; + auto shaderResult = ShaderCompiler::CompileShader(shaderAttrib); + EXPECT_TRUE(shaderResult) << (shaderResult ? String{} : shaderResult.error().log); + if (!shaderResult) return {}; + + ProgramAttrib programAttrib{.shaders = {shaderResult.value()}}; + auto programResult = ShaderCompiler::LinkProgram(programAttrib); + EXPECT_TRUE(programResult) << (programResult ? String{} : programResult.error().log); + if (!programResult) return {}; + + ProgramBinaryAttrib binaryAttrib{.shaderTypes = {GL_FRAGMENT_SHADER}, + .program = *programResult.value()}; + auto binaryResult = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); + EXPECT_TRUE(binaryResult) << (binaryResult ? String{} : binaryResult.error().log); + if (!binaryResult || binaryResult->empty()) return {}; + return binaryResult->front(); + } + + String Disassemble(const Vector& spirv) { + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + String text; + tools.Disassemble(spirv, &text); + return text; + } + + bool Validates(const Vector& spirv) { + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + tools.SetMessageConsumer( + [](spv_message_level_t, const char*, const spv_position_t& position, const char* message) { + ADD_FAILURE() << "spirv-val at word " << position.index << ": " << message; + }); + return tools.Validate(spirv); + } + + // OpImageFetch words: 0 opcode/count, 1 result type, 2 result id, 3 image, 4 coordinate, + // 5 the optional image-operands mask, 6.. the ids that mask asks for. + struct ImageFetch { + Uint32 resultId = 0u; + Uint32 imageId = 0u; + Uint32 mask = 0u; + Vector maskOperandIds; + }; + + Vector CollectImageFetches(const Vector& spirv) { + Vector fetches; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpImageFetch || wordCount < 5u) return; + ImageFetch fetch{}; + fetch.resultId = words[2]; + fetch.imageId = words[3]; + if (wordCount > 5u) { + fetch.mask = words[5]; + for (Uint32 word = 6u; word < wordCount; ++word) { + fetch.maskOperandIds.push_back(words[word]); + } + } + fetches.push_back(fetch); + }); + return fetches; + } + + // OpExtInst words: 0 opcode/count, 1 result type, 2 result id, 3 set, 4 instruction number, + // 5.. the operand ids. + struct ExtInst { + Uint32 resultId = 0u; + Uint32 instructionNumber = 0u; + Vector operandIds; + }; + + Vector CollectExtInsts(const Vector& spirv) { + Vector extInsts; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpExtInst || wordCount < 5u) return; + ExtInst extInst{}; + extInst.resultId = words[2]; + extInst.instructionNumber = words[4]; + for (Uint32 word = 5u; word < wordCount; ++word) { + extInst.operandIds.push_back(words[word]); + } + extInsts.push_back(extInst); + }); + return extInsts; + } + + std::map CollectScalarConstants(const Vector& spirv) { + std::map values; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode == spv::Op::OpConstant && wordCount == 4u) values[words[2]] = words[3]; + }); + return values; + } + + // The one fetch carrying an explicit Sample operand. glslang emits Sample on its own for a + // multisample texelFetch - there is no texelFetchOffset for a multisampled sampler - so the + // sample id is the mask's first and only operand. + const ImageFetch* FindSampleCarryingFetch(const Vector& fetches) { + for (const ImageFetch& fetch : fetches) { + if ((fetch.mask & static_cast(spv::ImageOperandsMask::Sample)) != 0u) { + return &fetch; + } + } + return nullptr; + } + + const ImageFetch* FindLodCarryingFetch(const Vector& fetches) { + for (const ImageFetch& fetch : fetches) { + if ((fetch.mask & static_cast(spv::ImageOperandsMask::Lod)) != 0u) { + return &fetch; + } + } + return nullptr; + } + + // KHR-GL4x.texture_swizzle.functional's integer multisample read in miniature: the sample + // index is the advertised GL_MAX_INTEGER_SAMPLES - 1, baked in as a literal, which is exactly + // the value the one-sample allocation underneath cannot answer. The plain sampler2D fetch is + // the negative control - a NON-multisampled image whose Lod operand this pass must not touch. + const char* const kIntegerMultisampleFetch = R"(#version 410 core +uniform usampler2DMS uintMs; +uniform sampler2D plain; +out vec4 fragColor; +void main() { + uvec4 texel = texelFetch(uintMs, ivec2(gl_FragCoord.xy), 3); + vec4 other = texelFetch(plain, ivec2(gl_FragCoord.xy), 0); + fragColor = vec4(texel) * 0.5 + other; +} +)"; + + // The colour class, which real devices squeeze to something above 1 rather than to 1. + const char* const kColorMultisampleFetch = R"(#version 410 core +uniform sampler2DMS colorMs; +out vec4 fragColor; +void main() { + fragColor = texelFetch(colorMs, ivec2(gl_FragCoord.xy), 3); +} +)"; + + // Every stage on a squeezed device goes through the probe, so the one that declares no + // multisampled image has to come back untouched. + const char* const kNoMultisampleFetch = R"(#version 410 core +uniform sampler2D plain; +out vec4 fragColor; +void main() { + fragColor = texelFetch(plain, ivec2(gl_FragCoord.xy), 0); +} +)"; +} // namespace + +class ClampMultisampleFetchTest : public ::testing::Test { +protected: + void SetUp() override { + MobileGL::Initialize(); + m_validationFailuresAtStart = ShaderCompiler::SpirvValidationFailureCount(); + } + + void TearDown() override { + EXPECT_EQ(ShaderCompiler::SpirvValidationFailureCount(), m_validationFailuresAtStart) + << "the clamped module did not survive spirv-val"; + } + + Uint64 m_validationFailuresAtStart = 0; +}; + +// The probe is the gate that keeps every ordinary stage off an optimizer round trip, so it has to +// answer no for a shader that never reads a multisample texture - and yes for the ones that do. +TEST_F(ClampMultisampleFetchTest, TheProbeAnswersOnlyForAMultisampledImage) { + const Vector plain = CompileFragment(kNoMultisampleFetch); + ASSERT_FALSE(plain.empty()); + EXPECT_FALSE(ShaderCompiler::DeclaresMultisampledImage(plain)); + + const Vector integerMs = CompileFragment(kIntegerMultisampleFetch); + ASSERT_FALSE(integerMs.empty()); + EXPECT_TRUE(ShaderCompiler::DeclaresMultisampledImage(integerMs)); + + const Vector colorMs = CompileFragment(kColorMultisampleFetch); + ASSERT_FALSE(colorMs.empty()); + EXPECT_TRUE(ShaderCompiler::DeclaresMultisampledImage(colorMs)); + + // Runs on every stage of every program on a squeezed device, so it must survive a stage that + // produced no SPIR-V rather than pushing a parse diagnostic for it. + EXPECT_FALSE(ShaderCompiler::DeclaresMultisampledImage({})); +} + +// The overwhelming majority of modules. Behind the probe they never reach the pass at all, but the +// pass has to be inert for them on its own, or a future caller that forgets the gate silently +// re-serialises every shader in the program. +TEST_F(ClampMultisampleFetchTest, LeavesAModuleWithoutAMultisampledImageUntouched) { + const Vector input = CompileFragment(kNoMultisampleFetch); + ASSERT_FALSE(input.empty()); + + Vector output; + ASSERT_TRUE(ShaderCompiler::ClampMultisampleFetchesForEssl( + input, output, /*maxColorSamples=*/4, /*maxIntegerSamples=*/1, /*maxDepthSamples=*/4, + kAdvertisedMaxSamples, true)); + EXPECT_EQ(output, input) << Disassemble(output); +} + +// The bug itself. GL_MAX_INTEGER_SAMPLES says 4, the texture has one sample, and the shader asks +// for sample 3. +TEST_F(ClampMultisampleFetchTest, ReplacesAnOutOfRangeIntegerSampleWithZero) { + const Vector input = CompileFragment(kIntegerMultisampleFetch); + ASSERT_FALSE(input.empty()); + + const Vector before = CollectImageFetches(input); + ASSERT_EQ(before.size(), 2u) << Disassemble(input); + const ImageFetch* sampleBefore = FindSampleCarryingFetch(before); + const ImageFetch* lodBefore = FindLodCarryingFetch(before); + ASSERT_NE(sampleBefore, nullptr) << Disassemble(input); + ASSERT_NE(lodBefore, nullptr) << Disassemble(input); + ASSERT_EQ(sampleBefore->maskOperandIds.size(), 1u); + const std::map constantsBefore = CollectScalarConstants(input); + ASSERT_EQ(constantsBefore.count(sampleBefore->maskOperandIds.front()), 1u); + EXPECT_EQ(constantsBefore.at(sampleBefore->maskOperandIds.front()), 3u); + + Vector output; + ASSERT_TRUE(ShaderCompiler::ClampMultisampleFetchesForEssl( + input, output, /*maxColorSamples=*/4, /*maxIntegerSamples=*/1, /*maxDepthSamples=*/4, + kAdvertisedMaxSamples, true)); + ASSERT_FALSE(output.empty()); + const String dis = Disassemble(output); + ASSERT_TRUE(Validates(output)) << dis; + + const Vector after = CollectImageFetches(output); + ASSERT_EQ(after.size(), 2u) << dis; + const ImageFetch* sampleAfter = FindSampleCarryingFetch(after); + ASSERT_NE(sampleAfter, nullptr) << dis; + ASSERT_EQ(sampleAfter->maskOperandIds.size(), 1u) << dis; + + // Sample 0 is the only one a one-sample allocation has - and it is a CONSTANT, not a computed + // minimum: at K == 1 there is nothing to compare against. An id that resolves in the constant + // table cannot also be some OpExtInst's result. + const std::map constantsAfter = CollectScalarConstants(output); + ASSERT_EQ(constantsAfter.count(sampleAfter->maskOperandIds.front()), 1u) << dis; + EXPECT_EQ(constantsAfter.at(sampleAfter->maskOperandIds.front()), 0u) << dis; + + // The float sampler2D in the same module is not multisampled, so its Lod fetch has to come + // through with the same image, the same mask and the same operand. + const ImageFetch* lodAfter = FindLodCarryingFetch(after); + ASSERT_NE(lodAfter, nullptr) << dis; + EXPECT_EQ(lodAfter->imageId, lodBefore->imageId) << dis; + EXPECT_EQ(lodAfter->mask, lodBefore->mask) << dis; + EXPECT_EQ(lodAfter->maskOperandIds, lodBefore->maskOperandIds) << dis; +} + +// The same shader on a device whose integer ceiling really is what MobileGL advertises. Nothing is +// out of range, so nothing may be rewritten - and the module must not even be re-serialised. +TEST_F(ClampMultisampleFetchTest, LeavesTheFetchAloneWhenTheCategoryReachesTheAdvertisedMaximum) { + const Vector input = CompileFragment(kIntegerMultisampleFetch); + ASSERT_FALSE(input.empty()); + + Vector output; + ASSERT_TRUE(ShaderCompiler::ClampMultisampleFetchesForEssl( + input, output, /*maxColorSamples=*/4, /*maxIntegerSamples=*/4, /*maxDepthSamples=*/4, + kAdvertisedMaxSamples, true)); + EXPECT_EQ(output, input) << Disassemble(output); +} + +// A category squeezed to something above 1 cannot be answered with a constant: an index the +// allocation does have must survive, so only the upper bound moves. +TEST_F(ClampMultisampleFetchTest, ClampsAColorSampleWithAMinimum) { + const Vector input = CompileFragment(kColorMultisampleFetch); + ASSERT_FALSE(input.empty()); + + const Vector before = CollectImageFetches(input); + ASSERT_EQ(before.size(), 1u) << Disassemble(input); + ASSERT_EQ(before.front().maskOperandIds.size(), 1u); + const Uint32 originalSampleId = before.front().maskOperandIds.front(); + + Vector output; + ASSERT_TRUE(ShaderCompiler::ClampMultisampleFetchesForEssl( + input, output, /*maxColorSamples=*/2, /*maxIntegerSamples=*/4, /*maxDepthSamples=*/4, + kAdvertisedMaxSamples, true)); + ASSERT_FALSE(output.empty()); + const String dis = Disassemble(output); + ASSERT_TRUE(Validates(output)) << dis; + + const Vector after = CollectImageFetches(output); + ASSERT_EQ(after.size(), 1u) << dis; + ASSERT_EQ(after.front().maskOperandIds.size(), 1u) << dis; + const Uint32 clampedSampleId = after.front().maskOperandIds.front(); + EXPECT_NE(clampedSampleId, originalSampleId) << dis; + + const Vector extInsts = CollectExtInsts(output); + const ExtInst* minimum = nullptr; + for (const ExtInst& extInst : extInsts) { + if (extInst.resultId == clampedSampleId) minimum = &extInst; + } + ASSERT_NE(minimum, nullptr) << dis; + EXPECT_EQ(minimum->instructionNumber, kGlslStd450SMin) << dis; + ASSERT_EQ(minimum->operandIds.size(), 2u) << dis; + EXPECT_EQ(minimum->operandIds[0], originalSampleId) << dis; + + // min(sample, K - 1), i.e. the last sample a two-sample allocation has. + const std::map constants = CollectScalarConstants(output); + ASSERT_EQ(constants.count(minimum->operandIds[1]), 1u) << dis; + EXPECT_EQ(constants.at(minimum->operandIds[1]), 1u) << dis; +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index e3ecdd9e..26420811 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -33,6 +33,7 @@ #include "SpirvPasses/NormalizeRectCoordinatesPass.h" #include "SpirvPasses/Lower1DArrayImagesPass.h" #include "SpirvPasses/BakeImageFormatsPass.h" +#include "SpirvPasses/ClampMultisampleFetchPass.h" #include "SpirvPasses/PrivateToEntryLocalPass.h" #include "SpirvPasses/StripUniformLocationsPass.h" #include "SpirvPasses/StripUboMemberRelaxedPrecisionPass.h" @@ -651,6 +652,26 @@ namespace MobileGL { return LowerViewportIndexPass::DeclaresViewportIndexBuiltin(binary); } + bool ShaderCompiler::ClampMultisampleFetchesForEssl(const Vector& inputBinary, + Vector& outputBinary, + const Int32 maxColorSamples, + const Int32 maxIntegerSamples, + const Int32 maxDepthSamples, + const Int32 advertisedMaxSamples, + const bool enableSpirvValidation) { + using namespace spvtools; + Optimizer optimizer(SPV_ENV_VULKAN_1_1); + optimizer.RegisterPass(ClampMultisampleFetchPass::CreateClampMultisampleFetchPass( + maxColorSamples, maxIntegerSamples, maxDepthSamples, advertisedMaxSamples)); + + return RunOptimizerChecked("ClampMultisampleFetchesForEssl", optimizer, inputBinary, + outputBinary, true, enableSpirvValidation); + } + + bool ShaderCompiler::DeclaresMultisampledImage(const Vector& binary) { + return ClampMultisampleFetchPass::DeclaresMultisampledImage(binary); + } + bool ShaderCompiler::SplitArrayVertexInputsForEssl(const Vector& inputBinary, Vector& outputBinary, const bool enableSpirvValidation) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index fb52679c..3cabe30c 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -46,6 +46,25 @@ namespace MobileGL { // whether the pass above has anything to do. The gate that keeps every other // stage off an optimizer round trip it does not need. static bool DeclaresViewportIndexBuiltin(const Vector& binary); + // Clamps the Sample image-operand of every multisample fetch to the sample count + // the BACKEND can really deliver for that image's category, which on Adreno and + // Mali is 1 for integer formats while the frontend advertises the GL-mandated + // floor of 4. Without it a `texelFetch(usampler2DMS, coord, 3)` reads past the + // end of a one-sample allocation. Pass the backend-real per-category ceilings and + // the advertised maximum (GL_Getter's GetAdvertisedMaxSamples); a category that + // already reaches the advertised value is left alone. DirectGLES transpile path + // only. See ClampMultisampleFetchPass. + static bool ClampMultisampleFetchesForEssl(const Vector& inputBinary, + Vector& outputBinary, + Int32 maxColorSamples, + Int32 maxIntegerSamples, + Int32 maxDepthSamples, + Int32 advertisedMaxSamples, + bool enableSpirvValidation = false); + // Whether the module declares any multisampled image type, i.e. whether the pass + // above has anything to do. The gate that keeps every other stage off an + // optimizer round trip it does not need. + static bool DeclaresMultisampledImage(const Vector& binary); // Replaces an ARRAY vertex input with one input per element at consecutive // locations, seeding a Private copy of the array so indexed reads still work. // GLSL ES has no array vertex inputs and SPIRV-Cross refuses the whole module diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp new file mode 100644 index 00000000..c1e2cbf7 --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp @@ -0,0 +1,374 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include "ClampMultisampleFetchPass.h" + +#include "spirv.hpp" +#include "source/opt/build_module.h" +#include "source/opt/constants.h" +#include "source/opt/def_use_manager.h" +#include "source/opt/instruction.h" +#include "source/opt/ir_builder.h" +#include "source/opt/ir_context.h" +#include "source/opt/module.h" +#include "source/opt/type_manager.h" +#include "source/opt/types.h" +#include "source/util/make_unique.h" +#include "source/util/string_utils.h" + +#include +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + namespace { + using spvtools::opt::Instruction; + using spvtools::opt::InstructionBuilder; + using spvtools::opt::IRContext; + namespace analysis = spvtools::opt::analysis; + + // GLSL.std.450 instruction numbers (see 3rdparty/glslang/SPIRV/GLSL.std.450.h). + // UMin is not interchangeable with SMin here: an unsigned operand large enough to + // read as negative would come back OUT of range from a signed minimum, which is + // the very thing this pass exists to prevent. + constexpr uint32_t kGlslUMin = 38u; + constexpr uint32_t kGlslSMin = 39u; + + // OpTypeImage in-operands: 0 sampled type, 1 Dim, 2 Depth, 3 Arrayed, 4 MS, + // 5 Sampled, 6 Format. + constexpr uint32_t kSampledTypeOperand = 0; + constexpr uint32_t kDepthOperand = 2; + constexpr uint32_t kMultisampledOperand = 4; + + // OpImageFetch / OpImageRead in-operands: 0 image, 1 coordinate, 2 the optional + // image-operands mask, 3.. the ids that mask asks for. + constexpr uint32_t kImageOperand = 0; + constexpr uint32_t kImageOperandsMaskOperand = 2; + + // The categories GL keeps a separate GL_MAX_*_SAMPLES ceiling for. + enum class SampleCategory { Color, Depth, Integer }; + + // The two operations that can carry a Sample image-operand and take their + // coordinate in in-operand 1. OpImageWrite can carry one too, but its operand + // layout differs (image, coordinate, TEXEL, mask) and writing a multisample + // texel is not expressible in the ESSL this backend emits, so it is left out + // rather than given an untested second index arithmetic. The sparse forms are + // out of scope for the same reason: ESSL has no sparse texturing at all, so a + // module containing one cannot reach a driver through this path anyway. + bool CarriesSampleImageOperand(spv::Op opcode) { + return opcode == spv::Op::OpImageFetch || opcode == spv::Op::OpImageRead; + } + + bool IsMultisampledImageType(const Instruction* imageType) { + return imageType != nullptr && imageType->opcode() == spv::Op::OpTypeImage && + imageType->NumInOperands() > kMultisampledOperand && + imageType->GetSingleWordInOperand(kMultisampledOperand) == 1u; + } + + // The OpTypeImage behind whatever an image operation was handed - a sampled + // image, a bare image, or a pointer to (or array of) either. Same unwrapping as + // Lower1DArrayImagesPass. + Instruction* ResolveImageType(IRContext* context, uint32_t objectId) { + auto* defUseMgr = context->get_def_use_mgr(); + Instruction* object = defUseMgr->GetDef(objectId); + if (object == nullptr) return nullptr; + Instruction* type = defUseMgr->GetDef(object->type_id()); + while (type != nullptr) { + switch (type->opcode()) { + case spv::Op::OpTypeImage: + return type; + case spv::Op::OpTypeSampledImage: + case spv::Op::OpTypePointer: + case spv::Op::OpTypeArray: + case spv::Op::OpTypeRuntimeArray: + // Each names its element type in its last in-operand, except arrays, + // whose element type is the FIRST. + type = defUseMgr->GetDef(type->opcode() == spv::Op::OpTypeArray || + type->opcode() == spv::Op::OpTypeRuntimeArray + ? type->GetSingleWordInOperand(0) + : type->GetSingleWordInOperand(type->NumInOperands() - 1)); + continue; + default: + return nullptr; + } + } + return nullptr; + } + + SampleCategory CategoryOf(IRContext* context, const Instruction* imageType) { + const Instruction* sampledType = + context->get_def_use_mgr()->GetDef(imageType->GetSingleWordInOperand(kSampledTypeOperand)); + if (sampledType != nullptr && sampledType->opcode() == spv::Op::OpTypeInt) { + return SampleCategory::Integer; + } + // Depth == 1 is the ONLY spelling that positively means a depth image. + // glslang writes 0 for a plain sampler and 2 ("no indication") wherever it + // cannot tell, and GLSL has no multisampled shadow sampler for it to write 1 + // for, so everything but an explicit 1 falls to the colour ceiling - which is + // also the safer of the two to guess at, being the one GL_MAX_SAMPLES itself + // describes. Guarded because Depth is only readable on a well-formed type. + if (imageType->NumInOperands() > kDepthOperand && + imageType->GetSingleWordInOperand(kDepthOperand) == 1u) { + return SampleCategory::Depth; + } + return SampleCategory::Color; + } + + // Where the Sample id sits among an image operation's in-operands, or false when + // the operation carries no Sample at all. + // + // The position is NOT fixed. The mask's ids follow it in ASCENDING BIT ORDER, so + // every lower bit that is set pushes Sample along by the number of ids that bit + // asks for: Bias/Lod/ConstOffset/Offset/ConstOffsets one each, Grad two (dx and + // dy). Bits at or above Sample cannot move it and are irrelevant here. glslang + // only ever emits Sample on its own for a GLSL texelFetch - there is no + // texelFetchOffset for a multisampled sampler - so in practice this always + // answers 3; the walk is what keeps that from being an assumption. + bool TryGetSampleOperandIndex(const Instruction& instruction, uint32_t* sampleOperandIndex) { + if (instruction.NumInOperands() <= kImageOperandsMaskOperand) { + // No image-operands mask at all, so no explicit sample: SPIR-V reads + // sample 0, which is in range of any allocation. Nothing to clamp. + return false; + } + const uint32_t mask = instruction.GetSingleWordInOperand(kImageOperandsMaskOperand); + const auto has = [mask](spv::ImageOperandsMask bit) { + return (mask & static_cast(bit)) != 0u; + }; + if (!has(spv::ImageOperandsMask::Sample)) { + return false; + } + + uint32_t index = kImageOperandsMaskOperand + 1; + if (has(spv::ImageOperandsMask::Bias)) ++index; + if (has(spv::ImageOperandsMask::Lod)) ++index; + if (has(spv::ImageOperandsMask::Grad)) index += 2; + if (has(spv::ImageOperandsMask::ConstOffset)) ++index; + if (has(spv::ImageOperandsMask::Offset)) ++index; + if (has(spv::ImageOperandsMask::ConstOffsets)) ++index; + if (instruction.NumInOperands() <= index) { + // A mask promising more operands than the instruction carries is a + // malformed module; leave it to the validator rather than indexing past + // the end of it. + return false; + } + *sampleOperandIndex = index; + return true; + } + + // The module's GLSL.std.450 import, creating it when the module has none. + // glslang emits one for all but the most trivial shaders, but a module that + // reached here without one must still be clampable. 0 means no id was available, + // and in that case NOTHING was added - the caller can still leave the module + // untouched. IRContext::AddExtInstImport rather than Module's: it is the one that + // keeps the def-use and feature managers in step with the new import. + uint32_t EnsureGlslStd450Import(IRContext* context) { + for (const Instruction& import : context->module()->ext_inst_imports()) { + if (spvtools::utils::MakeString(import.GetInOperand(0).words) == "GLSL.std.450") { + return import.result_id(); + } + } + const uint32_t importId = context->TakeNextId(); + if (importId == 0u) return 0u; + context->AddExtInstImport(spvtools::MakeUnique( + context, spv::Op::OpExtInstImport, 0, importId, + Instruction::OperandList{ + {SPV_OPERAND_TYPE_LITERAL_STRING, spvtools::utils::MakeVector("GLSL.std.450")}})); + return importId; + } + } // namespace + + bool ClampMultisampleFetchPass::DeclaresMultisampledImage(const Vector& binary) { + if (binary.empty()) { + // An empty module is a stage that produced no SPIR-V, which is not a verdict + // about multisample fetches; letting BuildModule reject it would push a + // spurious diagnostic through the message consumer first. + return false; + } + std::unique_ptr context = spvtools::BuildModule( + SPV_ENV_VULKAN_1_1, [](spv_message_level_t, const char*, const spv_position_t&, const char*) {}, + binary.data(), binary.size()); + if (!context) { + // Unparseable here means unusable downstream too; let the ordinary transpile + // path produce the error rather than inventing a verdict from it. + return false; + } + for (const Instruction& type : context->module()->types_values()) { + if (IsMultisampledImageType(&type)) { + return true; + } + } + return false; + } + + spvtools::opt::Pass::Status ClampMultisampleFetchPass::Process() { + // No category is squeezed, so no fetch can be out of range. This is the whole + // answer on a driver whose per-format ceilings all reach what MobileGL + // advertises, and it costs nothing. + if (m_maxColorSamples >= m_advertisedMaxSamples && + m_maxIntegerSamples >= m_advertisedMaxSamples && + m_maxDepthSamples >= m_advertisedMaxSamples) { + return Status::SuccessWithoutChange; + } + + auto* irContext = context(); + + // The type table settles it for almost every shader: no multisampled image + // declared, nothing any fetch in the body could be reading. + bool hasMultisampledImageType = false; + for (const Instruction& type : irContext->types_values()) { + if (IsMultisampledImageType(&type)) { + hasMultisampledImageType = true; + break; + } + } + if (!hasMultisampledImageType) { + return Status::SuccessWithoutChange; + } + + auto* defUseMgr = irContext->get_def_use_mgr(); + auto* typeMgr = irContext->get_type_mgr(); + auto* constantMgr = irContext->get_constant_mgr(); + + bool clampedAnything = false; + for (auto& function : *irContext->module()) { + for (auto& block : function) { + for (auto& instruction : block) { + if (!CarriesSampleImageOperand(instruction.opcode()) || + instruction.NumInOperands() <= kImageOperandsMaskOperand) { + continue; + } + const Instruction* imageType = + ResolveImageType(irContext, instruction.GetSingleWordInOperand(kImageOperand)); + if (!IsMultisampledImageType(imageType)) { + continue; + } + uint32_t sampleOperandIndex = 0; + if (!TryGetSampleOperandIndex(instruction, &sampleOperandIndex)) { + continue; + } + + Int32 categoryMaxSamples = m_maxColorSamples; + switch (CategoryOf(irContext, imageType)) { + case SampleCategory::Integer: + categoryMaxSamples = m_maxIntegerSamples; + break; + case SampleCategory::Depth: + categoryMaxSamples = m_maxDepthSamples; + break; + case SampleCategory::Color: + break; + } + if (categoryMaxSamples >= m_advertisedMaxSamples) { + continue; + } + + // The replacement has to carry the ORIGINAL operand's type: SPIR-V + // permits either signedness for Sample, and handing OpImageFetch an + // int where it had a uint is an invalid module rather than a wrong + // answer - the kind of defect that reaches a driver as "compiles + // here, not there". + const uint32_t sampleOperandId = instruction.GetSingleWordInOperand(sampleOperandIndex); + const Instruction* sampleOperandDef = defUseMgr->GetDef(sampleOperandId); + if (sampleOperandDef == nullptr) { + continue; + } + const uint32_t sampleTypeId = sampleOperandDef->type_id(); + const analysis::Type* sampleType = + sampleTypeId != 0u ? typeMgr->GetType(sampleTypeId) : nullptr; + const analysis::Integer* sampleInteger = + sampleType != nullptr ? sampleType->AsInteger() : nullptr; + if (sampleInteger == nullptr || sampleInteger->width() != 32u) { + // GLSL spells the sample index `int` and SPIR-V requires an + // integer scalar, so this is unreachable from any shader this + // backend compiles. Declining beats minting a constant of a + // width the operand never had. + MGLOG_D("ClampMultisampleFetchPass: sample operand %%%u of a " + "multisample fetch is not a 32-bit integer scalar; left " + "unclamped.", + sampleOperandId); + continue; + } + + if (categoryMaxSamples <= 1) { + // One sample exists, and its index is 0. + const analysis::Constant* zero = constantMgr->GetConstant(sampleType, {0u}); + const Instruction* zeroInst = + zero != nullptr ? constantMgr->GetDefiningInstruction(zero, sampleTypeId) + : nullptr; + if (zeroInst == nullptr) { + return Status::Failure; + } + instruction.SetInOperand(sampleOperandIndex, {zeroInst->result_id()}); + irContext->UpdateDefUse(&instruction); + clampedAnything = true; + continue; + } + + // min(operand, K-1). Only the upper bound: an index already inside + // the allocation comes through untouched, which is what makes this + // safe to apply to a shader that was already correct. + // + // Everything from here on either completes or fails the module. + // Anything that gives up half way - after the import or the bound + // constant has been added - would leave a MUTATED module reported as + // SuccessWithoutChange, which spvtools::Optimizer asserts against + // (it re-serialises and compares byte for byte in that case). + const uint32_t glslStd450Id = EnsureGlslStd450Import(irContext); + if (glslStd450Id == 0u) { + // Id space exhausted, and the import was NOT added. Nothing has + // changed yet, but nothing further can be built either. + return Status::Failure; + } + const uint32_t resultId = irContext->TakeNextId(); + const analysis::Constant* bound = constantMgr->GetConstant( + sampleType, {static_cast(categoryMaxSamples - 1)}); + const Instruction* boundInst = + bound != nullptr ? constantMgr->GetDefiningInstruction(bound, sampleTypeId) + : nullptr; + if (resultId == 0u || boundInst == nullptr) { + return Status::Failure; + } + + InstructionBuilder builder( + irContext, &instruction, + IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); + Instruction* clamped = builder.AddInstruction(spvtools::MakeUnique( + irContext, spv::Op::OpExtInst, sampleTypeId, resultId, + Instruction::OperandList{ + {SPV_OPERAND_TYPE_ID, {glslStd450Id}}, + {SPV_OPERAND_TYPE_EXTENSION_INSTRUCTION_NUMBER, + {sampleInteger->IsSigned() ? kGlslSMin : kGlslUMin}}, + {SPV_OPERAND_TYPE_ID, {sampleOperandId}}, + {SPV_OPERAND_TYPE_ID, {boundInst->result_id()}}})); + if (clamped == nullptr) { + return Status::Failure; + } + instruction.SetInOperand(sampleOperandIndex, {clamped->result_id()}); + irContext->UpdateDefUse(&instruction); + clampedAnything = true; + } + } + } + + if (!clampedAnything) { + return Status::SuccessWithoutChange; + } + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + return Status::SuccessWithChange; + } + + spvtools::Optimizer::PassToken ClampMultisampleFetchPass::CreateClampMultisampleFetchPass( + const Int32 maxColorSamples, const Int32 maxIntegerSamples, const Int32 maxDepthSamples, + const Int32 advertisedMaxSamples) { + return spvtools::Optimizer::PassToken(spvtools::MakeUnique( + maxColorSamples, maxIntegerSamples, maxDepthSamples, advertisedMaxSamples)); + } + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h new file mode 100644 index 00000000..4823baca --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h @@ -0,0 +1,89 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#pragma once +#include "source/opt/pass.h" +#include "spirv-tools/optimizer.hpp" + +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + // GL 4.6 core table 23.53 requires GL_MAX_SAMPLES >= 4, so MobileGL floors every + // multisample ceiling it advertises to 4 (GL_Getter's kFrontendMaxSamples) no matter + // what the ES driver reports. The realised allocation cannot be floored the same way - + // the driver would simply reject it - so DirectGLES clamps the count it passes to + // glTexStorage*Multisample down to what the format really supports + // (ClampSamplesToBackendSupport). On Adreno and on Mali's Immortalis-G925 that is ONE + // sample for every integer format, while the frontend keeps telling the application + // GL_MAX_INTEGER_SAMPLES is 4. + // + // A shader written against the advertised ceiling therefore fetches a sample the + // backing storage does not have. KHR-GL33/40/41.texture_swizzle.functional_* and + // KHR-GLxx.texture_size_promotion.functional bake `texelFetch(usampler2DMS, coord, 3)` + // in as a literal, and the fetch comes back as 0/garbage ("Found pixel with wrong + // value", "read value = 0") on a texture the backend quietly allocated with one + // sample. + // + // This pass closes that gap from the shader side: for every fetch of a multisampled + // image it clamps the Sample image-operand to the backend's REAL maximum for that + // image's category, so the lookup lands inside the allocation the backend made. + // - K >= advertisedMaxSamples: the category is not squeezed, nothing is rewritten. + // - K <= 1: the Sample operand becomes a constant 0 of its own type - the only + // sample that exists. + // - 1 < K < advertisedMaxSamples: the operand is wrapped in min(operand, K-1), + // which leaves an in-range index exactly as it was. + // Only the UPPER bound is clamped. A negative index is out of range in GL before this + // pass and after it alike, and MobileGL is not the component that should be inventing + // a value for it. + // + // Category comes from the OpTypeImage: an OpTypeInt sampled type is the integer + // class (GL_MAX_INTEGER_SAMPLES), a float one is depth when the image's Depth operand + // is exactly 1 and colour otherwise. That last clause is deliberate: glslang writes + // Depth 0 for a plain sampler and 2 ("unknown") wherever it cannot tell, and GLSL has + // no multisampled shadow sampler at all, so only an explicit 1 is treated as a depth + // image and everything else falls to the colour limit - which is the one a + // mis-classified image would want anyway. + // + // DirectGLES transpile path only. DirectVulkan allocates the sample count it was + // asked for and must see the module unchanged. + class ClampMultisampleFetchPass : public spvtools::opt::Pass { + public: + // The three backend-REAL per-category ceilings, plus the count the GL frontend + // advertises (GL_Getter's GetAdvertisedMaxSamples). A category whose real ceiling + // already reaches the advertised one is left completely alone. + ClampMultisampleFetchPass(Int32 maxColorSamples, Int32 maxIntegerSamples, + Int32 maxDepthSamples, Int32 advertisedMaxSamples) + : m_maxColorSamples(maxColorSamples), + m_maxIntegerSamples(maxIntegerSamples), + m_maxDepthSamples(maxDepthSamples), + m_advertisedMaxSamples(advertisedMaxSamples) {} + + const char* name() const override { return "clamp-multisample-fetch"; } + Status Process() override; + + // Whether the module declares any multisampled image type, i.e. whether running + // this pass could change anything. Answered from a single parse so the caller can + // skip the optimizer round trip entirely - which is every shader but the handful + // that read a multisample texture directly. + static bool DeclaresMultisampledImage(const Vector& binary); + + static spvtools::Optimizer::PassToken CreateClampMultisampleFetchPass( + Int32 maxColorSamples, Int32 maxIntegerSamples, Int32 maxDepthSamples, + Int32 advertisedMaxSamples); + + private: + Int32 m_maxColorSamples; + Int32 m_maxIntegerSamples; + Int32 m_maxDepthSamples; + Int32 m_advertisedMaxSamples; + }; + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL From 0b3101bf6bf84e098cbb1aaf020395da7d4394b9 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 07:53:07 -0400 Subject: [PATCH 21/21] [Perf, Test] (ShaderTranspiler, DirectGLES): answer both pass-gate probes from one SPIR-V parse --- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 58 +++++++++++-------- .../ClampMultisampleFetchTest.cpp | 20 +++++++ .../ShaderTranspiler/ShaderCompiler.cpp | 22 +++++++ .../MG_Util/ShaderTranspiler/ShaderCompiler.h | 10 ++++ .../SpirvPasses/ClampMultisampleFetchPass.cpp | 4 ++ .../SpirvPasses/ClampMultisampleFetchPass.h | 4 ++ .../SpirvPasses/LowerViewportIndexPass.cpp | 6 +- .../SpirvPasses/LowerViewportIndexPass.h | 4 ++ 8 files changed, 103 insertions(+), 25 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 2df8db54..15e6a30b 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -4909,9 +4909,32 @@ namespace MobileGL::MG_Backend::DirectGLES { // fails to compile and every draw made with it silently renders nothing. // Gated on the module actually declaring the output, so no other stage pays an // optimizer round trip for it. + // One parse of the module answers every armed pass gate below. The per-gate + // Declares* probes each cost a BuildModule per stage, and on a driver where both + // gates are armed (Mali: no GL_OES_viewport_array AND integer multisample + // squeezed to 1) the doubled parse made compile-heavy workloads ~10% slower. + // Probing the pre-lowering module is sound for both gates: demoting + // gl_ViewportIndex neither adds nor removes multisampled image types. + // Recomputed here rather than calling GL_Getter's GetAdvertisedMaxSamples(): + // this is backend code and must not reach into the GL frontend. 4 is that + // translation unit's kFrontendMaxSamples, which is the source of truth - + // keep the two in step. + constexpr Int kFrontendMaxSamples = 4; + const Int advertisedMaxSamples = + std::max(g_GLESCapabilities.MaxSamples, kFrontendMaxSamples); + const Bool viewportLoweringArmed = !g_GLESCapabilities.SupportsViewportArray; + const Bool sampleClampArmed = + g_GLESCapabilities.MaxColorTextureSamples < advertisedMaxSamples || + g_GLESCapabilities.MaxIntegerSamples < advertisedMaxSamples || + g_GLESCapabilities.MaxDepthTextureSamples < advertisedMaxSamples; + MG_Util::ShaderTranspiler::ShaderCompiler::SpirvGateFeatures spirvGates; + if (viewportLoweringArmed || sampleClampArmed) { + spirvGates = MG_Util::ShaderTranspiler::ShaderCompiler::ProbeSpirvGateFeatures( + *effectiveSpirv); + } + Vector loweredViewportSpirv; - if (!g_GLESCapabilities.SupportsViewportArray && - MG_Util::ShaderTranspiler::ShaderCompiler::DeclaresViewportIndexBuiltin(*effectiveSpirv) && + if (viewportLoweringArmed && spirvGates.WritesViewportIndexOutput && MG_Util::ShaderTranspiler::ShaderCompiler::LowerViewportIndexForEssl( *effectiveSpirv, loweredViewportSpirv, enableSpirvValidation) && !loweredViewportSpirv.empty()) { @@ -4937,28 +4960,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // optimizer round trip for it. DirectVulkan is deliberately not given this: it // allocates the sample count it was asked for, so its modules are already right. Vector clampedSampleSpirv; - { - // Recomputed here rather than calling GL_Getter's GetAdvertisedMaxSamples(): - // this is backend code and must not reach into the GL frontend. 4 is that - // translation unit's kFrontendMaxSamples, which is the source of truth - - // keep the two in step. - constexpr Int kFrontendMaxSamples = 4; - const Int advertisedMaxSamples = - std::max(g_GLESCapabilities.MaxSamples, kFrontendMaxSamples); - if ((g_GLESCapabilities.MaxColorTextureSamples < advertisedMaxSamples || - g_GLESCapabilities.MaxIntegerSamples < advertisedMaxSamples || - g_GLESCapabilities.MaxDepthTextureSamples < advertisedMaxSamples) && - MG_Util::ShaderTranspiler::ShaderCompiler::DeclaresMultisampledImage( - *effectiveSpirv) && - MG_Util::ShaderTranspiler::ShaderCompiler::ClampMultisampleFetchesForEssl( - *effectiveSpirv, clampedSampleSpirv, - g_GLESCapabilities.MaxColorTextureSamples, - g_GLESCapabilities.MaxIntegerSamples, - g_GLESCapabilities.MaxDepthTextureSamples, advertisedMaxSamples, - enableSpirvValidation) && - !clampedSampleSpirv.empty()) { - effectiveSpirv = &clampedSampleSpirv; - } + if (sampleClampArmed && spirvGates.DeclaresMultisampledImage && + MG_Util::ShaderTranspiler::ShaderCompiler::ClampMultisampleFetchesForEssl( + *effectiveSpirv, clampedSampleSpirv, + g_GLESCapabilities.MaxColorTextureSamples, + g_GLESCapabilities.MaxIntegerSamples, + g_GLESCapabilities.MaxDepthTextureSamples, advertisedMaxSamples, + enableSpirvValidation) && + !clampedSampleSpirv.empty()) { + effectiveSpirv = &clampedSampleSpirv; } // GLSL ES has no ARRAY vertex inputs, and SPIRV-Cross refuses the whole module diff --git a/MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp index 83f36bcf..4c538317 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp +++ b/MobileGL/MG_Test/ShaderTranspiler/ClampMultisampleFetchTest.cpp @@ -247,6 +247,26 @@ TEST_F(ClampMultisampleFetchTest, TheProbeAnswersOnlyForAMultisampledImage) { EXPECT_FALSE(ShaderCompiler::DeclaresMultisampledImage({})); } +// The combined probe answers both gate questions from one parse; it must agree with the +// per-gate probes on the same modules and stay quiet for an empty stage. +TEST_F(ClampMultisampleFetchTest, TheCombinedProbeAgreesWithThePerGateOnes) { + const Vector integerMs = CompileFragment(kIntegerMultisampleFetch); + ASSERT_FALSE(integerMs.empty()); + const auto msFeatures = ShaderCompiler::ProbeSpirvGateFeatures(integerMs); + EXPECT_TRUE(msFeatures.DeclaresMultisampledImage); + EXPECT_FALSE(msFeatures.WritesViewportIndexOutput); + + const Vector plain = CompileFragment(kNoMultisampleFetch); + ASSERT_FALSE(plain.empty()); + const auto plainFeatures = ShaderCompiler::ProbeSpirvGateFeatures(plain); + EXPECT_FALSE(plainFeatures.DeclaresMultisampledImage); + EXPECT_FALSE(plainFeatures.WritesViewportIndexOutput); + + const auto emptyFeatures = ShaderCompiler::ProbeSpirvGateFeatures({}); + EXPECT_FALSE(emptyFeatures.DeclaresMultisampledImage); + EXPECT_FALSE(emptyFeatures.WritesViewportIndexOutput); +} + // The overwhelming majority of modules. Behind the probe they never reach the pass at all, but the // pass has to be inert for them on its own, or a future caller that forgets the gate silently // re-serialises every shader in the program. diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 26420811..7cf8722a 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -672,6 +672,28 @@ namespace MobileGL { return ClampMultisampleFetchPass::DeclaresMultisampledImage(binary); } + ShaderCompiler::SpirvGateFeatures ShaderCompiler::ProbeSpirvGateFeatures( + const Vector& binary) { + SpirvGateFeatures features; + if (binary.empty()) { + return features; + } + std::unique_ptr context = spvtools::BuildModule( + SPV_ENV_VULKAN_1_1, + [](spv_message_level_t, const char*, const spv_position_t&, const char*) {}, + binary.data(), binary.size()); + if (!context) { + // Unparseable here means unusable downstream too; let the ordinary transpile + // path produce the error rather than inventing a verdict from it. + return features; + } + features.WritesViewportIndexOutput = + LowerViewportIndexPass::DeclaresViewportIndexBuiltin(context.get()); + features.DeclaresMultisampledImage = + ClampMultisampleFetchPass::DeclaresMultisampledImage(context.get()); + return features; + } + bool ShaderCompiler::SplitArrayVertexInputsForEssl(const Vector& inputBinary, Vector& outputBinary, const bool enableSpirvValidation) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index 3cabe30c..94cc8ddd 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -65,6 +65,16 @@ namespace MobileGL { // above has anything to do. The gate that keeps every other stage off an // optimizer round trip it does not need. static bool DeclaresMultisampledImage(const Vector& binary); + // Both gate questions above answered from ONE parse. Every armed gate costs a + // BuildModule per shader stage, and on a driver where both are armed (Mali: no + // GL_OES_viewport_array AND integer multisample squeezed to 1) the separate + // probes made compile-heavy workloads measurably slower - ReservedNames-class + // CTS cases paid ~10%. Callers with more than one armed gate use this instead. + struct SpirvGateFeatures { + Bool WritesViewportIndexOutput = false; + Bool DeclaresMultisampledImage = false; + }; + static SpirvGateFeatures ProbeSpirvGateFeatures(const Vector& binary); // Replaces an ARRAY vertex input with one input per element at consecutive // locations, seeding a Private copy of the array so indexed reads still work. // GLSL ES has no array vertex inputs and SPIRV-Cross refuses the whole module diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp index c1e2cbf7..21bb30bb 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.cpp @@ -198,6 +198,10 @@ namespace MobileGL { // path produce the error rather than inventing a verdict from it. return false; } + return DeclaresMultisampledImage(context.get()); + } + + bool ClampMultisampleFetchPass::DeclaresMultisampledImage(IRContext* context) { for (const Instruction& type : context->module()->types_values()) { if (IsMultisampledImageType(&type)) { return true; diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h index 4823baca..8d4fb722 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ClampMultisampleFetchPass.h @@ -74,6 +74,10 @@ namespace MobileGL { // that read a multisample texture directly. static bool DeclaresMultisampledImage(const Vector& binary); + // Same question answered from an already-built module, so one parse can feed + // several gates (ShaderCompiler::ProbeSpirvGateFeatures). + static bool DeclaresMultisampledImage(spvtools::opt::IRContext* context); + static spvtools::Optimizer::PassToken CreateClampMultisampleFetchPass( Int32 maxColorSamples, Int32 maxIntegerSamples, Int32 maxDepthSamples, Int32 advertisedMaxSamples); diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp index e0074197..93a63dba 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.cpp @@ -138,9 +138,13 @@ namespace MobileGL { // path produce the error rather than inventing a verdict from it. return false; } + return DeclaresViewportIndexBuiltin(context.get()); + } + + bool LowerViewportIndexPass::DeclaresViewportIndexBuiltin(IRContext* context) { for (const Instruction& annotation : context->annotations()) { if (IsViewportIndexBuiltinDecoration(annotation) && - GetDecoratedViewportIndexOutput(context.get(), annotation) != nullptr) { + GetDecoratedViewportIndexOutput(context, annotation) != nullptr) { return true; } } diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h index fcb207ed..9e8f9017 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerViewportIndexPass.h @@ -44,6 +44,10 @@ namespace MobileGL { // but the handful that route viewports from the shader. static bool DeclaresViewportIndexBuiltin(const Vector& binary); + // Same question answered from an already-built module, so one parse can feed + // several gates (ShaderCompiler::ProbeSpirvGateFeatures). + static bool DeclaresViewportIndexBuiltin(spvtools::opt::IRContext* context); + static spvtools::Optimizer::PassToken CreateLowerViewportIndexPass(); }; } // namespace ShaderTranspiler