From 06744fde7f4015b6840b67bcce3fc30c9646c8d9 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 27 Aug 2026 03:24:34 -0400 Subject: [PATCH] [Fix] (Getter): close the review findings - six-stage combined uniform blocks, bounded uniform-block bindings, honest vertex-stream count, per-format sample ceilings --- .../DirectGLES/BackendObject_DirectGLES.cpp | 21 +- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 10 + .../DirectVulkan/Renderer/ProgramFactory.cpp | 168 +++++++++++++--- .../DirectVulkan/Renderer/ProgramFactory.h | 67 +++++-- .../DirectVulkan/Renderer/VulkanRenderer.cpp | 6 +- .../MG_Impl/GLImpl/Drawing/GL_Drawing.cpp | 13 +- .../GLImpl/Framebuffer/GL_Framebuffer.cpp | 37 +++- MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp | 67 +++++-- .../MG_Impl/GLImpl/Program/GL_Program.cpp | 25 +++ MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp | 52 +---- .../MG_Impl/GLImpl/Texture/GL_Texture.cpp | 17 +- .../Scenarios/AdvertisedLimitsScenario.cpp | 50 ++++- .../GLState/ProgramState/ProgramLinkTask.cpp | 20 ++ .../Pipeline/PassthroughTessControlTest.cpp | 185 ++++++++++++------ MobileGL/MG_Test/SanityTest.cpp | 80 ++++++++ 15 files changed, 640 insertions(+), 178 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp index eeb97e2f..7270a446 100644 --- a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp @@ -307,6 +307,23 @@ namespace MobileGL::MG_Backend::DirectGLES { return capabilities.MaxColorTextureSamples; } + // The RENDERBUFFER twin, and it is a different set of pnames on purpose. + // GL_MAX_{COLOR,DEPTH}_TEXTURE_SAMPLES bound multisample TEXTURES; a renderbuffer is + // bounded by GL_MAX_SAMPLES (GL 4.6 core 9.2.4), with GL_MAX_INTEGER_SAMPLES for the + // integer formats. Using the texture ceilings here - which is what the renderbuffer probe + // did - is not merely untidy: the two texture pnames are ES 3.1 state, so on an ES 3.0 + // context the loader's rejected-probe clamp leaves them at 1 (see the multisample clamps + // in the GLES loader) and the walk below would never run past one sample, recording {1} + // for EVERY colour format while GL_MAX_SAMPLES - ES 3.0 core, so genuinely answered - + // reports 4. Once the frontend validates against this list, that would reject every + // multisample renderbuffer on such a context. + Int GetGLESRenderbufferFormatMaxSamples(const MG_External::GLESCapabilities& capabilities, + GLenum imageFormat) { + const Bool isInteger = imageFormat == GL_RED_INTEGER || imageFormat == GL_RG_INTEGER || + imageFormat == GL_RGB_INTEGER || imageFormat == GL_RGBA_INTEGER; + return isInteger ? capabilities.MaxIntegerSamples : capabilities.MaxSamples; + } + Bool ProbeFramebufferCompletenessForTexture(const MG_External::GLESFunctionsTable& gl, TextureTarget target, GLuint texture, TextureInternalFormat format) { GLuint framebuffer = 0; @@ -717,7 +734,7 @@ namespace MobileGL::MG_Backend::DirectGLES { AddFullFormatCaps(cache, renderbufferTargetIndex, formatIndex, GetRenderbufferFeatureCaps(logicalFormat)); const Int maxSamples = - GetGLESFormatMaxSamples(capabilities, logicalFormat, nativeInfo.ImageFormat); + GetGLESRenderbufferFormatMaxSamples(capabilities, nativeInfo.ImageFormat); cache.SampleCounts[renderbufferTargetIndex][formatIndex] = ProbeRenderbufferSampleCounts(gl, nativeInfo.InternalFormat, logicalFormat, maxSamples); } else { @@ -731,7 +748,7 @@ namespace MobileGL::MG_Backend::DirectGLES { LogGLESFormatCaveat(logicalFormat, renderbufferTargetIndex, renderbufferFallbackInfo); } const Int maxSamples = - GetGLESFormatMaxSamples(capabilities, logicalFormat, renderbufferFallbackInfo.ImageFormat); + GetGLESRenderbufferFormatMaxSamples(capabilities, renderbufferFallbackInfo.ImageFormat); cache.SampleCounts[renderbufferTargetIndex][formatIndex] = ProbeRenderbufferSampleCounts( gl, renderbufferFallbackInfo.InternalFormat, logicalFormat, maxSamples); } diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 0a664b04..10428043 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -2506,6 +2506,16 @@ namespace MobileGL::MG_Backend::DirectGLES { // A float compare here would never settle for a NaN level - NaN != NaN - and every // draw of that program would re-transpile, re-compile and re-link a byte-identical // shader. glPatchParameterfv accepts NaN by design. + // + // The gl_PerVertex MEMBER SET needs no clause of its own here, and that asymmetry + // with DirectVulkan is deliberate rather than an omission. It can only change with + // the evaluation stage, i.e. across a relink - which the link-version test at the + // top of this condition already catches - and this backend never invents the shape + // in the first place: AttachPassthroughTessControlStage extracts the member text + // out of the neighbouring stages' emitted ESSL on every rebuild + // (ExtractPerVertexBlockMembers, "mirrored, never invented"). DirectVulkan needs + // the mask in its key precisely because it does NOT mirror - it redeclares from a + // member set it has to be told. (twin->GetPassthroughTessControlPatchVertices() >= 0 && (twin->GetPassthroughTessControlPatchVertices() != static_cast(MG_State::pGLContext->GetPatchVertices()) || diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index c3e60c07..9ecd8d9f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -3599,24 +3599,125 @@ namespace MobileGL::MG_Backend::DirectVulkan { Uint64 ProgramFactory::ComputePassthroughTessControlKey(Uint32 patchVertices, const FloatVec4& defaultOuterLevel, - const FloatVec2& defaultInnerLevel) { - // A plain 28-byte blob of exactly what the generator reads, hashed once. Deliberately over + const FloatVec2& defaultInnerLevel, + Uint32 perVertexMembers) { + // A plain 32-byte blob of exactly what the generator reads, hashed once. Deliberately over // the RAW BITS rather than the values: two levels that compare unequal must key apart, and // a NaN level - which glPatchParameterfv accepts - compares unequal to itself. struct Blob { Uint32 patchVertices; Uint32 outerBits[4]; Uint32 innerBits[2]; + Uint32 perVertexMembers; } blob{}; blob.patchVertices = patchVertices; for (Uint32 i = 0; i < 4; ++i) blob.outerBits[i] = std::bit_cast(defaultOuterLevel[i]); for (Uint32 i = 0; i < 2; ++i) blob.innerBits[i] = std::bit_cast(defaultInnerLevel[i]); + blob.perVertexMembers = perVertexMembers; return XXH64(&blob, sizeof(blob), 0); } + // The member list a gl_PerVertex redeclaration must spell, derived from the mask. Order is + // glslang's declaration order and is load-bearing: a redeclaration whose members are the same + // set in a different order is a different block. + static String BuildPerVertexMemberDeclarations(Uint32 perVertexMembers) { + using Bit = ProgramFactory::PerVertexMemberBit; + String members; + if (perVertexMembers & static_cast(Bit::Position)) members += " vec4 gl_Position;\n"; + if (perVertexMembers & static_cast(Bit::PointSize)) members += " float gl_PointSize;\n"; + // Sized at one, not left unsized: an unsized built-in array in a redeclared block is + // implicitly sized by use, and this stage never indexes either distance array. + if (perVertexMembers & static_cast(Bit::ClipDistance)) members += " float gl_ClipDistance[1];\n"; + if (perVertexMembers & static_cast(Bit::CullDistance)) members += " float gl_CullDistance[1];\n"; + return members; + } + + Uint32 ProgramFactory::ReflectPerVertexInputMembers(const Vector& spirv) { + // Minimal, self-contained SPIR-V walk. SPIRV-Reflect is deliberately NOT used: for an + // array of interface blocks it reports built_in == -1 on the block and leaves every + // member's built_in at 0 (which is SpvBuiltInPosition), so a member walk through it reads + // "Position, Position, Position" - the same trap ReflectPassthroughTessControlNeed + // documents. The decorations below are unambiguous. + constexpr SizeT kHeaderWords = 5; + constexpr Uint32 kOpName = 5; + constexpr Uint32 kOpDecorate = 71; + constexpr Uint32 kOpMemberDecorate = 72; + constexpr Uint32 kOpTypeArray = 28; + constexpr Uint32 kOpTypePointer = 32; + constexpr Uint32 kOpVariable = 59; + constexpr Uint32 kDecorationBlock = 2; + constexpr Uint32 kDecorationBuiltIn = 11; + constexpr Uint32 kStorageClassInput = 1; + constexpr Uint32 kBuiltInPosition = 0; + constexpr Uint32 kBuiltInPointSize = 1; + constexpr Uint32 kBuiltInClipDistance = 3; + constexpr Uint32 kBuiltInCullDistance = 4; + (void)kOpName; + + if (spirv.size() <= kHeaderWords) return 0; + + UnorderedMap arrayElementType; // array id -> element type id + UnorderedMap> pointerPointee; // pointer id -> (storage class, pointee) + UnorderedMap structMembers; // struct id -> PerVertexMemberBit mask + std::set blockStructs; + Vector inputVariablePointerTypes; + + for (SizeT i = kHeaderWords; i < spirv.size();) { + const Uint32 wordCount = spirv[i] >> 16; + const Uint32 opcode = spirv[i] & 0xFFFFu; + if (wordCount == 0 || i + wordCount > spirv.size()) break; + const Uint32* words = &spirv[i]; + switch (opcode) { + case kOpTypeArray: + if (wordCount >= 4) arrayElementType[words[1]] = words[2]; + break; + case kOpTypePointer: + if (wordCount >= 4) pointerPointee[words[1]] = {words[2], words[3]}; + break; + case kOpVariable: + if (wordCount >= 4 && words[3] == kStorageClassInput) inputVariablePointerTypes.push_back(words[1]); + break; + case kOpDecorate: + if (wordCount >= 3 && words[2] == kDecorationBlock) blockStructs.insert(words[1]); + break; + case kOpMemberDecorate: + if (wordCount >= 5 && words[3] == kDecorationBuiltIn) { + Uint32 bit = 0; + switch (words[4]) { + case kBuiltInPosition: bit = static_cast(PerVertexMemberBit::Position); break; + case kBuiltInPointSize: bit = static_cast(PerVertexMemberBit::PointSize); break; + case kBuiltInClipDistance: bit = static_cast(PerVertexMemberBit::ClipDistance); break; + case kBuiltInCullDistance: bit = static_cast(PerVertexMemberBit::CullDistance); break; + default: break; + } + structMembers[words[1]] |= bit; + } + break; + default: + break; + } + i += wordCount; + } + + // The one Input variable whose type is an array of a Block-decorated struct IS gl_in; + // gl_TessCoord and friends are plain scalars/vectors and never match. + for (const Uint32 pointerType : inputVariablePointerTypes) { + const auto pointer = pointerPointee.find(pointerType); + if (pointer == pointerPointee.end()) continue; + const auto array = arrayElementType.find(pointer->second.second); + if (array == arrayElementType.end()) continue; + if (!blockStructs.contains(array->second)) continue; + const auto members = structMembers.find(array->second); + if (members == structMembers.end()) continue; + return members->second; + } + return 0; + } + String ProgramFactory::BuildPassthroughTessControlSource(Uint32 patchVertices, const FloatVec4& defaultOuterLevel, - const FloatVec2& defaultInnerLevel) { + const FloatVec2& defaultInnerLevel, + Uint32 perVertexMembers) { // The stage GL 4.6 core 11.2.2 describes when a program has an evaluation shader and no // control shader: "the input patch is passed through unmodified", the output patch has // as many vertices as the input one (PATCH_VERTICES), and the levels come from the @@ -3639,33 +3740,32 @@ namespace MobileGL::MG_Backend::DirectVulkan { // this from having to know the domain. String source = "#version 450 core\n"; source += "layout(vertices = " + std::to_string(patchVertices) + ") out;\n"; - // gl_in and gl_out are redeclared to the exact gl_PerVertex the FRONTEND's linked programs - // carry - gl_Position, gl_PointSize, gl_ClipDistance[1], in that order - because Vulkan - // matches built-in interface blocks by their whole shape, and the two obvious spellings - // are both wrong: + // gl_in and gl_out are redeclared to the exact gl_PerVertex the NEIGHBOURING EVALUATION + // STAGE carries, because Vulkan matches built-in interface blocks by their whole shape, + // and the two obvious spellings are both wrong: // * narrowing the block to gl_Position alone makes the evaluation stage read a patch of // zeroes (degenerate triangles, nothing rasterized), and - // * taking glslang's DEFAULT block for a standalone control stage yields FOUR members - - // it appends gl_CullDistance - where a linked vertex+evaluation program has three. - // PassthroughTessControlTest.MatchesTheFrontendPerVertexBlock is the latch: it links a - // vertex+evaluation program through this same compiler and fails if the two shapes ever - // stop agreeing, rather than letting the mismatch show up as a black frame. + // * taking glslang's DEFAULT block for a standalone control stage yields whatever THIS + // source's #version implies, which is unrelated to the evaluation stage's. + // + // The member set is a PARAMETER rather than a constant, and that is the whole point: it + // was hardcoded to {gl_Position, gl_PointSize, gl_ClipDistance[1]}, which is the shape a + // program carries only below #version 450. glslang appends gl_CullDistance to the block + // from 450 upward, so every 450/460 program - and every ESSL program, which the source + // processor rewrites to "#version 460 core" - carried FOUR members against this stage's + // three and got the black-frame-no-error case described above. The mask comes from + // ReflectPerVertexInputMembers, read off the evaluation stage's own SPIR-V. + // PassthroughTessControlTest.MatchesTheFrontendPerVertexBlock is the latch, and it now + // links the program at both 430 and 460. // // Only gl_Position is written. gl_PointSize is declared but left alone deliberately: // writing it from a tessellation stage requires the shaderTessellationAndGeometryPointSize // feature, which this renderer does not enable, so a program whose evaluation stage reads // gl_in[].gl_PointSize gets an undefined point size instead of the vertex stage's - a gap // this trades for not making every tessellated pipeline depend on an optional feature. - source += "in gl_PerVertex {\n" - " vec4 gl_Position;\n" - " float gl_PointSize;\n" - " float gl_ClipDistance[1];\n" - "} gl_in[gl_MaxPatchVertices];\n"; - source += "out gl_PerVertex {\n" - " vec4 gl_Position;\n" - " float gl_PointSize;\n" - " float gl_ClipDistance[1];\n" - "} gl_out[];\n"; + const String perVertexBody = BuildPerVertexMemberDeclarations(perVertexMembers); + source += "in gl_PerVertex {\n" + perVertexBody + "} gl_in[gl_MaxPatchVertices];\n"; + source += "out gl_PerVertex {\n" + perVertexBody + "} gl_out[];\n"; source += "void main() {\n"; source += " gl_out[gl_InvocationID].gl_Position = gl_in[gl_InvocationID].gl_Position;\n"; for (Uint32 i = 0; i < 4; ++i) { @@ -3681,12 +3781,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { } VkPipelineShaderStageCreateInfo ProgramFactory::GetOrCreatePassthroughTessControlStage( - Uint32 patchVertices, const FloatVec4& defaultOuterLevel, const FloatVec2& defaultInnerLevel) { + Uint32 patchVertices, const FloatVec4& defaultOuterLevel, const FloatVec2& defaultInnerLevel, + Uint32 perVertexMembers) { // Everything compiled into the stage, folded into one key. The patch size alone stopped // being enough once glPatchParameterfv could change the levels: two modules that differ // only in a baked-in level are different modules, and pipelines built from either may be - // alive at the same time. - const Uint64 key = ComputePassthroughTessControlKey(patchVertices, defaultOuterLevel, defaultInnerLevel); + // alive at the same time. The gl_PerVertex member set joins it for the same reason - two + // programs at different GLSL versions need differently-shaped blocks. + const Uint64 key = + ComputePassthroughTessControlKey(patchVertices, defaultOuterLevel, defaultInnerLevel, perVertexMembers); // A cached VK_NULL_HANDLE is a remembered failure, not a miss: returning it keeps a // generator that cannot compile from re-running glslang on every draw. const auto cached = m_passthroughTessControlStages.find(key); @@ -3720,7 +3823,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { stage.pName = "main"; using namespace MG_Util::ShaderTranspiler; - const String source = BuildPassthroughTessControlSource(patchVertices, defaultOuterLevel, defaultInnerLevel); + const String source = + BuildPassthroughTessControlSource(patchVertices, defaultOuterLevel, defaultInnerLevel, perVertexMembers); // Same compile configuration as every other stage of every other program: this runs on // the GL thread (the draw path), so the live compile env is the right one, and flags=0 // is the Vulkan-targeting form (CompileForOpenGL is what the GLES backend adds). @@ -3792,6 +3896,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { VkProgramObject& entry) const { entry.needsPassthroughTessControl = false; entry.passthroughTessControlEmulatable = false; + entry.passthroughPerVertexMembers = 0; Bool hasTessEval = false; Bool hasTessControl = false; @@ -3811,6 +3916,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (tessEvalModuleIndex >= spirv.size() || spirv[tessEvalModuleIndex].empty()) return; const auto& module = spirv[tessEvalModuleIndex]; + // The shape the synthesized control stage has to redeclare. Read here because this is the + // only place that holds the evaluation stage's module; a zero mask means the walk found + // no input per-vertex block at all, in which case the pre-450 shape is the safe stand-in + // (it is what every program carried before gl_CullDistance joined the block). + const Uint32 perVertexMembers = ReflectPerVertexInputMembers(module); + entry.passthroughPerVertexMembers = perVertexMembers != 0 ? perVertexMembers : kDefaultPerVertexMembers; + if (perVertexMembers == 0) { + MGLOG_W("ProgramFactory: could not read the evaluation stage's gl_PerVertex block shape; the " + "pass-through control stage falls back to the pre-450 three-member form"); + } + SpvReflectShaderModule reflectModule{}; const SpvReflectResult createResult = spvReflectCreateShaderModule(module.size() * sizeof(Uint), module.data(), &reflectModule); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index a8f9d0b5..62f53253 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -76,6 +76,23 @@ namespace MobileGL::MG_Backend::DirectVulkan { using CompileOptionFlags = Flags; using HashType = Uint64; + // The gl_PerVertex members a pass-through tessellation control stage may have to carry, + // in the order glslang declares them - which is the order a redeclaration must use. + // Which of them exist is a function of the neighbouring stage's GLSL VERSION + // (gl_CullDistance joins the block at #version 450), so the mask is read off that + // stage's SPIR-V rather than assumed. See ReflectPerVertexInputMembers. + enum class PerVertexMemberBit : Uint32 { + Position = 1u << 0, + PointSize = 1u << 1, + ClipDistance = 1u << 2, + CullDistance = 1u << 3, + }; + // What a program parsed below #version 450 carries, and the fallback when a module's + // block cannot be read. + static constexpr Uint32 kDefaultPerVertexMembers = + static_cast(PerVertexMemberBit::Position) | static_cast(PerVertexMemberBit::PointSize) | + static_cast(PerVertexMemberBit::ClipDistance); + struct UpdateAfterBindLimits { Bool enabled = false; Uint32 maxPerStageSamplers = 0; @@ -194,6 +211,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { // instead (PipelineFactory::CreatePipeline refuses the pipeline and the draw is // skipped). See ReflectPassthroughTessControlNeed. Bool passthroughTessControlEmulatable = false; + // Which gl_PerVertex members the evaluation stage's `in gl_PerVertex gl_in[]` block + // actually carries, as a PerVertexMemberBit mask read off its SPIR-V. The synthesized + // control stage has to redeclare the SAME shape: glslang appends gl_CullDistance to + // that block from #version 450 upward, so a 450/460 program - and every ESSL program, + // which the source processor rewrites to "#version 460 core" - carries four members + // where a 430 program carries three. A fixed three-member pass-through fed the + // evaluation stage a differently-shaped block, which is the black-frame-no-error case + // this whole family is written around. + Uint32 passthroughPerVertexMembers = 0; // Frame-boundary counter value of the last GetOrCreateProgram hit; drives // cache eviction (see OnFrameBoundary). Mutable: the draw snapshot's memoised // entry pointer re-stamps use through a const reference (StampProgramUse). @@ -249,6 +275,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { writesViewportIndexBuiltin = other.writesViewportIndexBuiltin; needsPassthroughTessControl = other.needsPassthroughTessControl; passthroughTessControlEmulatable = other.passthroughTessControlEmulatable; + passthroughPerVertexMembers = other.passthroughPerVertexMembers; lastUsedFrame = other.lastUsedFrame; other.hash = 0; other.descriptorSetLayout = VK_NULL_HANDLE; @@ -267,6 +294,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { other.writesViewportIndexBuiltin = false; other.needsPassthroughTessControl = false; other.passthroughTessControlEmulatable = false; + other.passthroughPerVertexMembers = 0; other.lastUsedFrame = 0; } VkProgramObject& operator=(VkProgramObject&& other) noexcept { @@ -311,6 +339,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { writesViewportIndexBuiltin = other.writesViewportIndexBuiltin; needsPassthroughTessControl = other.needsPassthroughTessControl; passthroughTessControlEmulatable = other.passthroughTessControlEmulatable; + passthroughPerVertexMembers = other.passthroughPerVertexMembers; lastUsedFrame = other.lastUsedFrame; other.hash = 0; other.descriptorSetLayout = VK_NULL_HANDLE; @@ -329,6 +358,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { other.writesViewportIndexBuiltin = false; other.needsPassthroughTessControl = false; other.passthroughTessControlEmulatable = false; + other.passthroughPerVertexMembers = 0; other.lastUsedFrame = 0; return *this; } @@ -485,30 +515,40 @@ namespace MobileGL::MG_Backend::DirectVulkan { // the caller then has no control stage to inject, and CreatePipeline refuses the // pipeline rather than handing the driver a half-tessellated one. // - // Keyed on the patch size AND the six default tessellation levels, because GL takes the - // output patch size from PATCH_VERTICES and the levels from PATCH_DEFAULT_OUTER_LEVEL / - // PATCH_DEFAULT_INNER_LEVEL, all of which are draw state rather than link state - the CTS - // case that motivated this links at the default 3 and draws at 4. The pipeline cache - // re-keys on the same three inputs, so the module a pipeline was built with is part of - // that pipeline's identity. Compiling is bounded by the number of distinct - // (size, levels) combinations a program draws with - one or two in practice - and only - // ever happens for the rare program that has no control stage at all. + // Keyed on the patch size, the six default tessellation levels AND the gl_PerVertex + // member set, because all three decide what the generator emits. The size comes from + // PATCH_VERTICES and the levels from PATCH_DEFAULT_OUTER_LEVEL / PATCH_DEFAULT_INNER_LEVEL + // - draw state rather than link state, and the CTS case that motivated this links at the + // default 3 and draws at 4. The member set comes from the neighbouring evaluation stage's + // own SPIR-V, so two programs at different GLSL versions need different modules. The + // pipeline cache re-keys on the same inputs, so the module a pipeline was built with is + // part of that pipeline's identity. Compiling is bounded by the number of distinct + // (size, levels, members) combinations a program draws with - one or two in practice - + // and only ever happens for the rare program that has no control stage at all. VkPipelineShaderStageCreateInfo GetOrCreatePassthroughTessControlStage(Uint32 patchVertices, const FloatVec4& defaultOuterLevel, - const FloatVec2& defaultInnerLevel); + const FloatVec2& defaultInnerLevel, + Uint32 perVertexMembers); // Source of the module above. Exposed for tests: the generated GLSL is the whole // contract with the evaluation stage, so it is worth pinning independently of a device. static String BuildPassthroughTessControlSource(Uint32 patchVertices, const FloatVec4& defaultOuterLevel, - const FloatVec2& defaultInnerLevel); + const FloatVec2& defaultInnerLevel, Uint32 perVertexMembers); // The identity of one such module: everything the generator bakes in, folded into a // 64-bit key over the raw bits (so -0.0 and +0.0 key apart, which is harmless, and NaN // keys to itself, which is what matters). Shared with PipelineFactory, which mixes the // same value into the pipeline hash so a pipeline can never be handed a module built for - // different levels. + // different levels or a different block shape. static Uint64 ComputePassthroughTessControlKey(Uint32 patchVertices, const FloatVec4& defaultOuterLevel, - const FloatVec2& defaultInnerLevel); + const FloatVec2& defaultInnerLevel, Uint32 perVertexMembers); + + // The PerVertexMemberBit mask of the INPUT per-vertex block a module declares, read + // straight out of its SPIR-V (OpMemberDecorate ... BuiltIn on the struct behind the one + // Input variable that is an array of a Block-decorated struct). Zero when the module has + // no such block. Exposed for tests, which is the only way to pin the shape agreement + // without a device. + static Uint32 ReflectPerVertexInputMembers(const Vector& spirv); private: struct ProgramLookupCache { @@ -578,7 +618,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { // nothing, so an application that recomputes a level per frame mints a new key per frame. // Reaching the cap destroys every module and starts over (see the flush in // GetOrCreatePassthroughTessControlStage); the cap is far above what any program that - // holds its levels still will ever need. + // holds its levels still will ever need. The gl_PerVertex member set is in the key too + // and adds only a handful of values, so it does not move the cap in practice. static constexpr SizeT kMaxPassthroughTessControlStages = 64; UnorderedMap m_passthroughTessControlStages; static inline XXH64_state_t* m_hashState = XXH64_createState(); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index b5123814..4b98c891 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -5216,9 +5216,11 @@ void main() { const FloatVec4& defaultOuterLevel = MG_State::pGLContext->GetPatchDefaultOuterLevel(); const FloatVec2& defaultInnerLevel = MG_State::pGLContext->GetPatchDefaultInnerLevel(); payload.passthroughTessControlKey = ProgramFactory::ComputePassthroughTessControlKey( - payload.patchControlPoints, defaultOuterLevel, defaultInnerLevel); + payload.patchControlPoints, defaultOuterLevel, defaultInnerLevel, + programObj.passthroughPerVertexMembers); payload.passthroughTessControlStage = m_programFactory->GetOrCreatePassthroughTessControlStage( - payload.patchControlPoints, defaultOuterLevel, defaultInnerLevel); + payload.patchControlPoints, defaultOuterLevel, defaultInnerLevel, + programObj.passthroughPerVertexMembers); } if (!payload.stencilTestEnable) { payload.frontStencilFailOp = VK_STENCIL_OP_KEEP; diff --git a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp index fa767571..1ec47485 100644 --- a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp +++ b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp @@ -1577,10 +1577,10 @@ namespace MobileGL::MG_Impl::GLImpl { std::to_string(id) + " is not a transform feedback object name.")); return; } - // GL 4.6 core 10.3.7 bounds `stream` by GL_MAX_VERTEX_STREAMS, which is 4. Only stream 0 - // can ever have been written - nothing in the shader pipeline supports - // layout(stream = N) - so a higher stream captured zero vertices and the draw is a legal - // no-op rather than an error. The bound is read from the getter so the two cannot drift. + // GL 4.6 core 10.3.7 bounds `stream` by GL_MAX_VERTEX_STREAMS, which this implementation + // answers as 1 - so stream 0 is the only one that exists and anything else is + // INVALID_VALUE. Read from the getter rather than written as `stream != 0` so the two can + // never drift: if vertex-stream support ever lands, this bound moves with the limit. GLint maxVertexStreams = 1; GetIntegerv(GL_MAX_VERTEX_STREAMS, &maxVertexStreams); if (stream >= static_cast(std::max(maxVertexStreams, 1))) { @@ -1601,9 +1601,8 @@ namespace MobileGL::MG_Impl::GLImpl { return; } - // Only stream 0 ever records anything (see the stream bound above), so a higher stream - // replays nothing. - const Uint64 vertices = stream == 0 ? MG_State::pGLContext->GetTransformFeedbackRecordedVertices(id) : 0; + // `stream` is provably 0 here (the bound above is 1), so this is stream 0's record. + const Uint64 vertices = MG_State::pGLContext->GetTransformFeedbackRecordedVertices(id); if (vertices == 0) return; const auto count = static_cast(vertices); AccountTransformFeedbackPrimitives(mode, count); diff --git a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp index 49a10d94..825af09d 100644 --- a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp +++ b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp @@ -693,6 +693,24 @@ namespace MobileGL::MG_Impl::GLImpl { // frontend would accept a count it had advertised globally - but on Adreno and Mali the // integer path is genuinely one sample, and accepting four only moved the failure from an // honest INVALID_OPERATION here to a silently under-allocated renderbuffer. + // The head of the per-format renderbuffer sample list the backend probed, or 0 when nothing + // was probed for it. Same shape as GetProbedMaxTextureSamples in GL_Texture.cpp, and reads + // the same cache glGetInternalformativ(GL_RENDERBUFFER, ..., GL_SAMPLES) answers from. + static Int GetProbedMaxRenderbufferSamples(TextureInternalFormat format) { + if (MG_Backend::pActiveBackendObject == nullptr) { + return 0; + } + const SizeT targetIndex = MG_Backend::GetRenderbufferFormatCapabilityTargetIndex(); + const SizeT formatIndex = static_cast(format); + 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(); + } + Int GetMaxRenderbufferSamplesForFormat_State(TextureInternalFormat format) { if (MG_Backend::pActiveBackendObject == nullptr) { return std::numeric_limits::max(); @@ -707,6 +725,19 @@ namespace MobileGL::MG_Impl::GLImpl { &normalizedType); const Bool isIntegerFormat = normalizedFormat == GL_RED_INTEGER || normalizedFormat == GL_RG_INTEGER || normalizedFormat == GL_RGB_INTEGER || normalizedFormat == GL_RGBA_INTEGER; + // The per-format probe first, for the same reason the texture path takes it first: GL 4.6 + // core 9.2.4 words the error as "samples is greater than the maximum number of samples + // supported for internalformat (see GetInternalformativ)", and + // glGetInternalformativ(GL_RENDERBUFFER, ..., GL_SAMPLES) is answered from exactly this + // list. It was never consulted here - the TODO that deferred it was written before the + // query was backed and had gone stale - so a format whose multisample probes fail inside + // a category that allows four was accepted at four, quietly allocated at one by + // ClampSamplesToBackendSupport, and then reported as four by + // glGetRenderbufferParameteriv(GL_RENDERBUFFER_SAMPLES). + const Int probedMaxSamples = GetProbedMaxRenderbufferSamples(format); + if (probedMaxSamples > 0) { + return probedMaxSamples; + } if (!isIntegerFormat) { return GetMaxRenderbufferSamples_State(); } @@ -743,8 +774,10 @@ namespace MobileGL::MG_Impl::GLImpl { return false; } - // TODO: Resolve the remaining per-internalformat renderbuffer sample limits once - // glGetInternalformativ is backed; integer formats are handled below. + // Per-internalformat, from the probe list glGetInternalformativ answers with, falling back + // to the format's category pname where nothing was probed. (This carried a TODO deferring + // the per-format resolution "once glGetInternalformativ is backed"; it has been backed for + // both renderbuffers and multisample textures since, so the deferral was collected.) const Int maxSamples = GetMaxRenderbufferSamplesForFormat_State(format); if (samples > maxSamples) { // GL 4.6 core 9.2.4 makes asking for more samples than the format supports diff --git a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp index 84a9e3f4..aa73f3df 100644 --- a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp +++ b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp @@ -119,13 +119,24 @@ namespace MobileGL::MG_Impl::GLImpl { constexpr GLint kFrontendMaxGeometryShaderInvocations = 32; constexpr GLint kFrontendMaxTessControlUniformBlocks = 14; constexpr GLint kFrontendMaxTessEvaluationUniformBlocks = 14; - // GL 4.6's minimum is 14 uniform blocks on each of the FIVE graphics stages (70), not - // three: the two tessellation stages were simply missing from this sum, so even a - // frontend with enough binding points advertised 42. + // The compute stage's share of the combined sum below. Compute's own per-stage answer is + // backend-derived (GL_MAX_COMPUTE_UNIFORM_BLOCKS reads dynamicParameters), so this is not + // what that query returns - it is the GL 4.3 core minimum, present here only so the + // combined total covers all SIX stages. + constexpr GLint kFrontendMaxComputeUniformBlocksShare = 14; + // GL 4.6 table 23.64 orders MAX_UNIFORM_BUFFER_BINDINGS >= MAX_COMBINED_UNIFORM_BLOCKS >= + // every per-stage count, and the sum has to run over SIX stages, not three and not five. + // Three (42) was the original bug. Five (70) replaced it and broke the middle term the + // other way: compute's per-stage count is backend-derived and clamps at the binding count, + // so a device reporting descriptor-indexing-scale uniform buffers (Adreno reports + // maxPerStageDescriptorUniformBuffers = 16777216) advertised 84 compute blocks against a + // combined 70. Six stages x 14 = 84, which is also exactly the binding-point count and the + // arithmetic the GL 4.5 minimum of 84 bindings is built from, so the ordering is now tight + // rather than accidental. constexpr GLint kFrontendMaxCombinedUniformBlocks = kFrontendMaxVertexUniformBlocks + kFrontendMaxTessControlUniformBlocks + kFrontendMaxTessEvaluationUniformBlocks + kFrontendMaxGeometryUniformBlocks + - kFrontendMaxFragmentUniformBlocks; + kFrontendMaxFragmentUniformBlocks + kFrontendMaxComputeUniformBlocksShare; constexpr GLint kFrontendMaxVaryingComponents = static_cast(MG_Util::ShaderTranspiler::MAX_VARYING_COMPONENTS); constexpr GLint kFrontendMaxVaryingVectors = @@ -135,9 +146,9 @@ namespace MobileGL::MG_Impl::GLImpl { constexpr GLint kFrontendMaxTransformFeedbackInterleavedComponents = 64; constexpr GLint kFrontendMaxTransformFeedbackSeparateAttribs = 4; constexpr GLint kFrontendMaxTransformFeedbackSeparateComponents = 4; - // ARB_transform_feedback3's vertex-stream count; see the GL_MAX_VERTEX_STREAMS case for - // what streams 1..3 mean in an implementation that can only emit to stream 0. - constexpr GLint kFrontendMaxVertexStreams = 4; + // ARB_transform_feedback3's vertex-stream count. One is what this implementation can + // actually emit to; see the GL_MAX_VERTEX_STREAMS case for why it is not four. + constexpr GLint kFrontendMaxVertexStreams = 1; constexpr GLint kFrontendMaxGeometryOutputVertices = 256; constexpr GLint kFrontendMaxGeometryTotalOutputComponents = 1024; // GL 4.5 core table 23.64 requires 84 indexed uniform binding points, and that is exactly @@ -2366,8 +2377,12 @@ namespace MobileGL::MG_Impl::GLImpl { *params = dynamicParameters.MaxComputeTextureImageUnits; break; case GL_MAX_COMBINED_COMPUTE_UNIFORM_COMPONENTS: + // The CLAMPED block count, i.e. exactly what GL_MAX_COMPUTE_UNIFORM_BLOCKS answers. + // GL 4.6 table 23.64 defines this as the components reachable through the blocks a + // stage may declare, so deriving it from the raw backend number described 256 blocks + // an application is only ever allowed 84 of. *params = GetMaxCombinedUniformComponents(kFrontendMaxComputeUniformComponents, - dynamicParameters.MaxComputeUniformBlocks, + ClampUniformBlockCount(dynamicParameters.MaxComputeUniformBlocks), dynamicParameters.MaxUniformBlockSize); break; case GL_MAX_COMPUTE_WORK_GROUP_INVOCATIONS: @@ -2415,12 +2430,12 @@ namespace MobileGL::MG_Impl::GLImpl { break; case GL_MAX_COMBINED_FRAGMENT_UNIFORM_COMPONENTS: *params = GetMaxCombinedUniformComponents(kFrontendMaxFragmentUniformComponents, - kFrontendMaxFragmentUniformBlocks, + ClampUniformBlockCount(kFrontendMaxFragmentUniformBlocks), dynamicParameters.MaxUniformBlockSize); break; case GL_MAX_COMBINED_GEOMETRY_UNIFORM_COMPONENTS: *params = GetMaxCombinedUniformComponents(kFrontendMaxGeometryUniformComponents, - kFrontendMaxGeometryUniformBlocks, + ClampUniformBlockCount(kFrontendMaxGeometryUniformBlocks), dynamicParameters.MaxUniformBlockSize); break; case GL_MAX_GEOMETRY_OUTPUT_VERTICES: @@ -2434,7 +2449,7 @@ namespace MobileGL::MG_Impl::GLImpl { break; case GL_MAX_COMBINED_VERTEX_UNIFORM_COMPONENTS: *params = GetMaxCombinedUniformComponents(kFrontendMaxVertexUniformComponents, - kFrontendMaxVertexUniformBlocks, + ClampUniformBlockCount(kFrontendMaxVertexUniformBlocks), dynamicParameters.MaxUniformBlockSize); break; case GL_MAX_CUBE_MAP_TEXTURE_SIZE: @@ -2511,12 +2526,12 @@ namespace MobileGL::MG_Impl::GLImpl { case GL_MAX_COMBINED_TESS_CONTROL_UNIFORM_COMPONENTS: *params = GetMaxCombinedUniformComponents( static_cast(MG_Util::ShaderTranspiler::MAX_TESS_CONTROL_UNIFORM_COMPONENTS), - kFrontendMaxTessControlUniformBlocks, dynamicParameters.MaxUniformBlockSize); + ClampUniformBlockCount(kFrontendMaxTessControlUniformBlocks), dynamicParameters.MaxUniformBlockSize); break; case GL_MAX_COMBINED_TESS_EVALUATION_UNIFORM_COMPONENTS: *params = GetMaxCombinedUniformComponents( static_cast(MG_Util::ShaderTranspiler::MAX_TESS_EVALUATION_UNIFORM_COMPONENTS), - kFrontendMaxTessEvaluationUniformBlocks, dynamicParameters.MaxUniformBlockSize); + ClampUniformBlockCount(kFrontendMaxTessEvaluationUniformBlocks), dynamicParameters.MaxUniformBlockSize); break; // ARB_cull_distance. Backend-derived exactly like GL_MAX_CLIP_DISTANCES beside it, and // for a stronger reason: a cull distance discards the whole primitive, so advertising @@ -2579,14 +2594,24 @@ namespace MobileGL::MG_Impl::GLImpl { *params = kFrontendMaxTransformFeedbackSeparateAttribs; break; case GL_MAX_VERTEX_STREAMS: - // GL 4.5 core table 23.62 requires four. MobileGL can only ever EMIT to stream 0 - - // nothing in the shader pipeline supports layout(stream = N), EmitStreamVertex or a - // per-stream capture layout - but that is a statement about what a geometry shader - // may produce, not about which stream indices exist. Streams 1..3 exist and are - // permanently empty, and the two entry points that address a stream say so: an - // indexed primitive query on one answers zero (GL_Query's emptyVertexStream) and - // glDrawTransformFeedbackStream on one draws nothing. Answering 1 instead used to - // make both of them GL_INVALID_VALUE. + // ONE, which is under the GL 4.5 core table 23.62 minimum of four and is a known, + // deliberate non-conformance. It was briefly raised to 4 on the theory that streams + // 1..3 could exist and be permanently empty; measuring that decision refuted it. + // Raising the limit un-gates two CTS cases per package across KHR-GL40..GL46 - + // transform_feedback.draw_xfb_stream_test (which stops being skipped) and + // transform_feedback3.multiple_streams (which stops reporting NotSupported) - and + // both then fail, because nothing in the shader pipeline supports layout(stream = N), + // EmitStreamVertex or EndStreamPrimitive, and because the query state machine tracks + // one active query per TARGET rather than per (target, stream). That is 14 new + // failures against 2 gained limits passes, and a 4 nothing can back is the + // advertised-caps lie with the sign flipped. + // + // The real fix is the feature, not the number: per-stream capture needs + // layout(stream = N) through the transpiler plus per-(target, stream) query slots, + // which DirectVulkan could back with VK_EXT_transform_feedback's geometryStreams and + // DirectGLES cannot back at all (ES has no vertex streams). Until that lands, one is + // the honest count and every stream-addressing entry point bounds itself by THIS + // query, so raising it later moves them all together. *params = kFrontendMaxVertexStreams; break; case GL_TRANSFORM_FEEDBACK_ACTIVE: diff --git a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp index 8a53cc0e..24fb005a 100644 --- a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp +++ b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp @@ -245,6 +245,30 @@ namespace MobileGL::MG_Impl::GLImpl { return true; } + // GL 4.6 core 7.6.3: INVALID_VALUE when uniformBlockBinding >= MAX_UNIFORM_BUFFER_BINDINGS. + // The storage-block twin below has always had this check; the uniform one never did, and the + // value it stores is used as a RAW SUBSCRIPT into the state layer's fixed indexed-binding + // array on every draw and dispatch (DirectGLES's per-program UBO rebind, DirectVulkan's + // descriptor resolve, whose only guard is a MOBILEGL_ASSERT that compiles away in release). + // An out-of-range binding therefore did not merely go unreported - it read past the array and + // dereferenced whatever SharedPtr it found there. + bool ValidateUniformBlockBinding(GLuint binding) { + // Exactly what glGetIntegerv(GL_MAX_UNIFORM_BUFFER_BINDINGS) advertises: the state + // layer's array width, which the getter clamps to as well. + const SizeT maxBindingCount = MG_State::pGLContext->GetBufferBindingPointCount(BufferTarget::Uniform); + if (binding < maxBindingCount) { + return true; + } + + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique( + "MG_Impl/GLImpl", __func__, + std::format("Uniform block binding {} is not less than GL_MAX_UNIFORM_BUFFER_BINDINGS ({}).", binding, + maxBindingCount))); + return false; + } + bool ValidateShaderStorageBlockBinding(GLuint binding) { SizeT maxBindingCount = MG_State::pGLContext->GetBufferBindingPointCount(BufferTarget::ShaderStorage); if (MG_Backend::pActiveBackendObject) { @@ -1898,6 +1922,7 @@ namespace MobileGL::MG_Impl::GLImpl { "Program object" + std::to_string(program) + " that has been linked.")); return; } + if (!ValidateUniformBlockBinding(uniformBlockBinding)) return; if (!programObject->IsActiveGlUniformBlock(uniformBlockIndex)) { MG_State::pGLContext->RecordError( ErrorCode::InvalidValue, diff --git a/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp b/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp index 1a79a0e2..96a091c0 100644 --- a/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp +++ b/MobileGL/MG_Impl/GLImpl/Query/GL_Query.cpp @@ -40,12 +40,6 @@ namespace MobileGL::MG_Impl::GLImpl { // stand in for the backend's. Uint64 accountedCaptureDrawSnapshot = 0; Uint64 geometryCaptureDrawSnapshot = 0; - // Set when glBeginQueryIndexed named a vertex stream above 0. MobileGL advertises - // GL_MAX_VERTEX_STREAMS = 4 because GL 4.5 requires it, and nothing in the shader - // pipeline can emit to a stream other than 0 - so the primitive count on any other - // stream is provably zero, and this makes the object report that instead of - // aliasing stream 0's backend counter. - Bool emptyVertexStream = false; }; // Query calls may arrive from any thread (launchers migrate the context @@ -122,7 +116,6 @@ namespace MobileGL::MG_Impl::GLImpl { queryObject->ended = false; queryObject->resultCached = false; queryObject->cachedResult = 0; - queryObject->emptyVertexStream = false; } // Callers must hold g_queryObjectsMutex. @@ -195,23 +188,6 @@ namespace MobileGL::MG_Impl::GLImpl { return false; } - // A span begun on a vertex stream this implementation can never emit to. The answer - // is zero, and it is available immediately - the backend query that ran alongside it - // counted stream 0 and must not be reported here. - if (queryObject->emptyVertexStream) { - switch (pname) { - case GL_QUERY_RESULT: - case GL_QUERY_RESULT_NO_WAIT: - outValue = 0; - return true; - case GL_QUERY_RESULT_AVAILABLE: - outValue = GL_TRUE; - return true; - default: - break; - } - } - switch (pname) { case GL_QUERY_TARGET: // The target a query was begun with (or created with, for glCreateQueries) - state @@ -771,7 +747,16 @@ namespace MobileGL::MG_Impl::GLImpl { // The indexed query entry points differ from the plain ones only in the vertex // stream they address (GL 4.6 core 4.2.1): index must be below GL_MAX_VERTEX_STREAMS - // for the two transform feedback targets and zero for every other target. + // for the two transform feedback targets and zero for every other target. MobileGL + // implements ONE vertex stream, so both bounds are 1 and a valid call is always index 0 - + // which is what makes the three forwards below equivalent to the unindexed entry points. + // + // THAT EQUIVALENCE IS THE WHOLE JUSTIFICATION, and it is read out of the getter rather + // than assumed: the moment GL_MAX_VERTEX_STREAMS answers more than one, index 1..3 starts + // reaching EndQueryIndexed and GetQueryIndexediv, which resolve the active query from + // per-TARGET globals and would end - or report - a query begun on a different stream. + // Raising that limit therefore means giving each active query a stream index and + // comparing it here, not just changing the number. Bool ValidateQueryStreamIndex(const char* function, GLenum target, GLuint index) { const Bool perStreamTarget = IsPerVertexStreamQueryTarget(target); GLint maxVertexStreams = 1; @@ -787,28 +772,11 @@ namespace MobileGL::MG_Impl::GLImpl { return false; } - // Streams 1..GL_MAX_VERTEX_STREAMS-1 exist but nothing can emit to them, so a span begun - // on one counts zero primitives. Flagging the object is what keeps that answer honest: - // BeginQuery below still opens a real backend query (it is the only way to reuse the - // whole target/object state machine), and that query counts STREAM 0. - // - // Known simplification, spelled out rather than hidden: because the backend query is - // shared, only ONE query may be active per target here, while GL allows one per - // (target, stream) pair. A program running a stream-0 and a stream-2 - // GL_PRIMITIVES_GENERATED query at the same time gets GL_INVALID_OPERATION on the - // second. Nothing can produce a non-zero stream-2 result to be worth more than that - // until the shader pipeline grows layout(stream = N). - void MarkQueryEmptyVertexStream(GLuint id) { - const std::lock_guard lock(g_queryObjectsMutex); - auto* queryObject = FindQueryObjectLocked(id); - if (queryObject && queryObject->active) queryObject->emptyVertexStream = true; - } } // namespace void BeginQueryIndexed(GLenum target, GLuint index, GLuint id) { if (!ValidateQueryStreamIndex(__FUNCTION__, target, index)) return; BeginQuery(target, id); - if (index != 0 && IsPerVertexStreamQueryTarget(target)) MarkQueryEmptyVertexStream(id); } void EndQueryIndexed(GLenum target, GLuint index) { diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 8ec63b27..74181753 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -557,11 +557,20 @@ namespace MobileGL::MG_Impl::GLImpl { : isIntegerFormat ? GetAdvertisedIntegerMaxSamples() : GetAdvertisedColorTextureMaxSamples(); - // 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. + // 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 - so when the probe has an answer it IS the ceiling, and the + // category limit only stands in where nothing was probed. + // + // This used to be max(probed, category), which made the probe dead: the walk starts + // AT the category limit (BackendObject_DirectGLES's ProbeTextureSampleCounts) so its + // head can never exceed it, and max() therefore always collapsed to the category + // value. A format whose 4- and 2-sample probes fail inside a 4-sample category - a + // float colour format under EXT_color_buffer_float is the natural instance - was + // still accepted at 4, silently squeezed to 1 by ClampSamplesToBackendSupport, and + // then reported as 4 by GL_TEXTURE_SAMPLES while glGetInternalformativ said 1. const Int probedMaxSamples = GetProbedMaxTextureSamples(textureTarget, textureInternalFormat); - return probedMaxSamples > 0 ? std::max(probedMaxSamples, categoryMaxSamples) : categoryMaxSamples; + return probedMaxSamples > 0 ? probedMaxSamples : categoryMaxSamples; } Bool ValidateTextureMultisampleStorage(TextureTarget textureTarget, GLsizei samples, GLsizei width, diff --git a/MobileGL/MG_IntegrationTest/Scenarios/AdvertisedLimitsScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/AdvertisedLimitsScenario.cpp index 0316d291..5ee41649 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/AdvertisedLimitsScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/AdvertisedLimitsScenario.cpp @@ -139,6 +139,49 @@ namespace MGITest { << relation.blocksName << " = " << blocks << " exceeds " << relation.bindingsName << " = " << bindings << "; a shader may declare more blocks than there are binding points to bind them to"; } + + // THE MIDDLE TERM, which the relation quoted above always had and this case never + // checked. It is the one that actually broke: widening the binding-point array to 84 + // raised what every PER-STAGE count clamps to, while the combined value was a + // five-stage sum of 70 - so a device reporting descriptor-indexing-scale uniform + // buffers (Adreno: maxPerStageDescriptorUniformBuffers = 16777216) advertised 84 + // compute uniform blocks inside a combined limit of 70. Per-stage <= combined is + // exactly the assertion that says so, and it costs one glGetIntegerv per row. + struct StageAgainstCombined { + GLenum stage; + const char* stageName; + GLenum combined; + const char* combinedName; + }; + const StageAgainstCombined stageRelations[] = { + {GL_MAX_COMPUTE_UNIFORM_BLOCKS, "GL_MAX_COMPUTE_UNIFORM_BLOCKS", GL_MAX_COMBINED_UNIFORM_BLOCKS, + "GL_MAX_COMBINED_UNIFORM_BLOCKS"}, + {GL_MAX_VERTEX_UNIFORM_BLOCKS, "GL_MAX_VERTEX_UNIFORM_BLOCKS", GL_MAX_COMBINED_UNIFORM_BLOCKS, + "GL_MAX_COMBINED_UNIFORM_BLOCKS"}, + {GL_MAX_TESS_CONTROL_UNIFORM_BLOCKS, "GL_MAX_TESS_CONTROL_UNIFORM_BLOCKS", + GL_MAX_COMBINED_UNIFORM_BLOCKS, "GL_MAX_COMBINED_UNIFORM_BLOCKS"}, + {GL_MAX_TESS_EVALUATION_UNIFORM_BLOCKS, "GL_MAX_TESS_EVALUATION_UNIFORM_BLOCKS", + GL_MAX_COMBINED_UNIFORM_BLOCKS, "GL_MAX_COMBINED_UNIFORM_BLOCKS"}, + {GL_MAX_GEOMETRY_UNIFORM_BLOCKS, "GL_MAX_GEOMETRY_UNIFORM_BLOCKS", GL_MAX_COMBINED_UNIFORM_BLOCKS, + "GL_MAX_COMBINED_UNIFORM_BLOCKS"}, + {GL_MAX_FRAGMENT_UNIFORM_BLOCKS, "GL_MAX_FRAGMENT_UNIFORM_BLOCKS", GL_MAX_COMBINED_UNIFORM_BLOCKS, + "GL_MAX_COMBINED_UNIFORM_BLOCKS"}, + {GL_MAX_COMPUTE_SHADER_STORAGE_BLOCKS, "GL_MAX_COMPUTE_SHADER_STORAGE_BLOCKS", + GL_MAX_COMBINED_SHADER_STORAGE_BLOCKS, "GL_MAX_COMBINED_SHADER_STORAGE_BLOCKS"}, + {GL_MAX_FRAGMENT_SHADER_STORAGE_BLOCKS, "GL_MAX_FRAGMENT_SHADER_STORAGE_BLOCKS", + GL_MAX_COMBINED_SHADER_STORAGE_BLOCKS, "GL_MAX_COMBINED_SHADER_STORAGE_BLOCKS"}, + }; + for (const StageAgainstCombined& relation : stageRelations) { + GLint stage = -1; + GLint combined = -1; + glGetIntegerv(relation.stage, &stage); + glGetIntegerv(relation.combined, &combined); + ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)) << relation.stageName; + EXPECT_LE(stage, combined) + << relation.stageName << " = " << stage << " exceeds " << relation.combinedName << " = " + << combined << "; GL 4.6 table 23.64 orders MAX_*_BUFFER_BINDINGS >= MAX_COMBINED_*_BLOCKS >= " + "every per-stage count, and a single-stage program may use its whole per-stage allowance"; + } } // KHR-GL44.multi_bind.functional_bind_buffers_range sizes each of an indexed target's @@ -217,7 +260,12 @@ namespace MGITest { {GL_MAX_VARYING_VECTORS, "GL_MAX_VARYING_VECTORS", 15, 256}, {GL_MAX_VERTEX_UNIFORM_VECTORS, "GL_MAX_VERTEX_UNIFORM_VECTORS", 256, 1 << 20}, {GL_MAX_VARYING_COMPONENTS, "GL_MAX_VARYING_COMPONENTS", 60, 1 << 20}, - {GL_MAX_VERTEX_STREAMS, "GL_MAX_VERTEX_STREAMS", 4, 64}, + // GL_MAX_VERTEX_STREAMS is deliberately absent. GL 4.5 requires 4 and MobileGL + // answers 1, which is a KNOWN non-conformance rather than an oversight: raising + // the number un-gates two transform-feedback CTS cases per package across + // KHR-GL40..GL46 that then fail, because no part of the shader pipeline supports + // layout(stream = N). See the GL_MAX_VERTEX_STREAMS case in GL_Getter.cpp. Adding + // a row here would pin a number the implementation cannot back. {GL_MAX_GEOMETRY_SHADER_INVOCATIONS, "GL_MAX_GEOMETRY_SHADER_INVOCATIONS", 32, 256}, {GL_MAX_SUBROUTINES, "GL_MAX_SUBROUTINES", 256, 1 << 20}, {GL_MAX_SUBROUTINE_UNIFORM_LOCATIONS, "GL_MAX_SUBROUTINE_UNIFORM_LOCATIONS", 1024, 1 << 20}, diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index 67aa2f1d..acad322c 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -10,6 +10,7 @@ #include +#include #include #include #include @@ -1683,6 +1684,25 @@ namespace MobileGL::MG_State::GLState { artifacts.uniformBlocksWithoutBinding.contains(blockTypeName) ? 0 : ubo.getBinding(); artifacts.uniformBlockBinding[i] = declaredBinding < 0 ? declaredBinding : declaredBinding + BlockArrayElement(ubo.name); + // The second way a binding reaches the state layer's indexed-binding array, and the + // one glUniformBlockBinding's new bound cannot see. glslang does not range-check a + // uniform block's layout(binding = N) against anything - TBuiltInResource has no + // maxUniformBufferBindings field at all, and ParseHelper bounds only samplers and + // atomic counters - so `layout(binding = 5000) uniform Blk {...}` compiled and linked + // clean and then had both backends subscript the array at 5000 on the first draw. + // Stated against the same ceiling glGetIntegerv(GL_MAX_UNIFORM_BUFFER_BINDINGS) + // advertises; an instance array whose LAST element passes it is a link error even + // though its base fits, same rule as the explicit-location check above. + if (artifacts.uniformBlockBinding[i] >= + static_cast(MG_State::GLState::BufferBindingPointCount)) { + artifacts.infoLog = + std::format("Uniform block '{}' declares binding {}, which is not less than " + "GL_MAX_UNIFORM_BUFFER_BINDINGS ({}).", + ubo.name, artifacts.uniformBlockBinding[i], + static_cast(MG_State::GLState::BufferBindingPointCount)); + ProgramObject::ResetLinkArtifacts(artifacts); + return false; + } MGLOG_D("ProgramObject %u: Reflection - UBO[%d] name='%s' size=%u binding=%d", in.externalIndex, i, ubo.name.c_str(), ubo.size, ubo.getBinding()); } diff --git a/MobileGL/MG_Test/Pipeline/PassthroughTessControlTest.cpp b/MobileGL/MG_Test/Pipeline/PassthroughTessControlTest.cpp index fea1e2ec..2f1d6a31 100644 --- a/MobileGL/MG_Test/Pipeline/PassthroughTessControlTest.cpp +++ b/MobileGL/MG_Test/Pipeline/PassthroughTessControlTest.cpp @@ -104,10 +104,11 @@ namespace { const FloatVec4 kDefaultOuter(1.0f, 1.0f, 1.0f, 1.0f); const FloatVec2 kDefaultInner(1.0f, 1.0f); - Vector CompileGeneratedSource(Uint32 patchVertices) { + Vector CompileGeneratedSource(Uint32 patchVertices, + Uint32 perVertexMembers = ProgramFactory::kDefaultPerVertexMembers) { using namespace MG_Util::ShaderTranspiler; - const String source = - ProgramFactory::BuildPassthroughTessControlSource(patchVertices, kDefaultOuter, kDefaultInner); + const String source = ProgramFactory::BuildPassthroughTessControlSource(patchVertices, kDefaultOuter, + kDefaultInner, perVertexMembers); ShaderAttrib shaderAttrib{.shaderType = GL_TESS_CONTROL_SHADER, .sourceStr = source}; auto shaderResult = ShaderCompiler::CompileShader(shaderAttrib); @@ -166,7 +167,8 @@ TEST_F(PassthroughTessControlTest, ForwardsPositionAndWritesBothLevelArrays) { // user-defined varying, ReflectPassthroughTessControlNeed's "built-ins only" refusal stops being // the right gate and both have to move together. TEST_F(PassthroughTessControlTest, InterfaceIsBuiltInsOnly) { - const String source = ProgramFactory::BuildPassthroughTessControlSource(4, kDefaultOuter, kDefaultInner); + const String source = ProgramFactory::BuildPassthroughTessControlSource( + 4, kDefaultOuter, kDefaultInner, ProgramFactory::kDefaultPerVertexMembers); EXPECT_EQ(source.find("layout(location"), String::npos) << source; EXPECT_NE(source.find("layout(vertices = 4) out;"), String::npos) << source; } @@ -176,20 +178,30 @@ TEST_F(PassthroughTessControlTest, InterfaceIsBuiltInsOnly) { // Without the second half a pipeline memoised at one set of levels would be handed back after the // application changed them, and the tessellation would silently stay at the old levels. TEST_F(PassthroughTessControlTest, BakesTheDefaultTessLevelsInAndKeysOnThem) { + constexpr Uint32 kMembers = ProgramFactory::kDefaultPerVertexMembers; const FloatVec4 outer(2.0f, 3.0f, 4.0f, 5.0f); const FloatVec2 inner(6.5f, 7.25f); - const String source = ProgramFactory::BuildPassthroughTessControlSource(4, outer, inner); + const String source = ProgramFactory::BuildPassthroughTessControlSource(4, outer, inner, + ProgramFactory::kDefaultPerVertexMembers); EXPECT_NE(source.find("gl_TessLevelOuter[0] = 2.0;"), String::npos) << source; EXPECT_NE(source.find("gl_TessLevelOuter[3] = 5.0;"), String::npos) << source; EXPECT_NE(source.find("gl_TessLevelInner[0] = 6.5;"), String::npos) << source; EXPECT_NE(source.find("gl_TessLevelInner[1] = 7.25;"), String::npos) << source; const Uint64 defaultKey = - ProgramFactory::ComputePassthroughTessControlKey(4, kDefaultOuter, kDefaultInner); - EXPECT_NE(ProgramFactory::ComputePassthroughTessControlKey(4, outer, inner), defaultKey); - EXPECT_NE(ProgramFactory::ComputePassthroughTessControlKey(4, kDefaultOuter, inner), defaultKey); - EXPECT_NE(ProgramFactory::ComputePassthroughTessControlKey(3, kDefaultOuter, kDefaultInner), defaultKey); - EXPECT_EQ(ProgramFactory::ComputePassthroughTessControlKey(4, kDefaultOuter, kDefaultInner), defaultKey); + ProgramFactory::ComputePassthroughTessControlKey(4, kDefaultOuter, kDefaultInner, kMembers); + EXPECT_NE(ProgramFactory::ComputePassthroughTessControlKey(4, outer, inner, kMembers), defaultKey); + EXPECT_NE(ProgramFactory::ComputePassthroughTessControlKey(4, kDefaultOuter, inner, kMembers), defaultKey); + EXPECT_NE(ProgramFactory::ComputePassthroughTessControlKey(3, kDefaultOuter, kDefaultInner, kMembers), defaultKey); + EXPECT_EQ(ProgramFactory::ComputePassthroughTessControlKey(4, kDefaultOuter, kDefaultInner, kMembers), defaultKey); + // ...and the gl_PerVertex member set is in the same key, for the same reason: two programs at + // different GLSL versions need differently-shaped modules, and a pipeline memoised against one + // shape must not be handed back for the other. + constexpr Uint32 kMembersWithCull = + ProgramFactory::kDefaultPerVertexMembers | + static_cast(ProgramFactory::PerVertexMemberBit::CullDistance); + EXPECT_NE(ProgramFactory::ComputePassthroughTessControlKey(4, kDefaultOuter, kDefaultInner, kMembersWithCull), + defaultKey); } // The generated stage still has to COMPILE with non-default levels: an integral level spelled @@ -198,12 +210,42 @@ TEST_F(PassthroughTessControlTest, CompilesWithNonDefaultLevels) { using namespace MG_Util::ShaderTranspiler; const String source = ProgramFactory::BuildPassthroughTessControlSource(4, FloatVec4(2.0f, 2.0f, 2.0f, 2.0f), - FloatVec2(2.0f, 2.0f)); + FloatVec2(2.0f, 2.0f), + ProgramFactory::kDefaultPerVertexMembers); ShaderAttrib shaderAttrib{.shaderType = GL_TESS_CONTROL_SHADER, .sourceStr = source}; auto shaderResult = ShaderCompiler::CompileShader(shaderAttrib); EXPECT_TRUE(shaderResult) << (shaderResult ? String{} : shaderResult.error().log) << source; } +// The generator honours the mask it is given, in both directions. This is what covers the +// pre-cutoff three-member form now that no authorable tessellation evaluation stage produces it +// (ARB_tessellation_shader is GL 4.0 and gl_CullDistance joins the block at 400), and it is also +// the fallback ReflectPassthroughTessControlNeed uses when it cannot read a module's block. +TEST_F(PassthroughTessControlTest, RedeclaresExactlyTheRequestedMembers) { + using Bit = ProgramFactory::PerVertexMemberBit; + constexpr Uint32 kWithCull = ProgramFactory::kDefaultPerVertexMembers | static_cast(Bit::CullDistance); + constexpr Uint32 kBuiltInCullDistance = 4; + constexpr Uint32 kBuiltInClipDistance = 3; + + const Vector withoutCull = CompileGeneratedSource(4, ProgramFactory::kDefaultPerVertexMembers); + ASSERT_FALSE(withoutCull.empty()); + const std::set withoutCullBuiltIns = DeclaredBuiltIns(withoutCull); + EXPECT_TRUE(withoutCullBuiltIns.contains(kBuiltInClipDistance)); + EXPECT_FALSE(withoutCullBuiltIns.contains(kBuiltInCullDistance)) + << "the three-member mask must not emit gl_CullDistance"; + for (const auto& [structId, shape] : BuiltInBlockShapes(withoutCull)) { + EXPECT_EQ(StructMemberCount(withoutCull, structId), 3u) << "structId=" << structId; + } + + const Vector withCull = CompileGeneratedSource(4, kWithCull); + ASSERT_FALSE(withCull.empty()); + EXPECT_TRUE(DeclaredBuiltIns(withCull).contains(kBuiltInCullDistance)) + << "the four-member mask must emit gl_CullDistance"; + for (const auto& [structId, shape] : BuiltInBlockShapes(withCull)) { + EXPECT_EQ(StructMemberCount(withCull, structId), 4u) << "structId=" << structId; + } +} + // THE load-bearing test. Vulkan matches built-in interface blocks by their whole shape, and this // stage is compiled ON ITS OWN - it never goes through the glslang link that gives a real program // its gl_PerVertex. So the shape it declares has to equal the shape a linked vertex+evaluation @@ -216,13 +258,33 @@ TEST_F(PassthroughTessControlTest, CompilesWithNonDefaultLevels) { TEST_F(PassthroughTessControlTest, MatchesTheFrontendPerVertexBlock) { using namespace MG_Util::ShaderTranspiler; - // Deliberately the shape of KHR-GL43.shader_storage_buffer_object.advanced-write-tessellation: - // a vertex stage feeding an evaluation stage with no control stage in between. - static const char* kVs = R"(#version 430 core + // MORE THAN ONE VERSION, because the shape is a function of the neighbour's GLSL version and + // a single-version case cannot see that. glslang gates gl_PerVertex's gl_CullDistance member + // on a version cutoff, and this case used to link #version 430 ONLY - which is exactly why a + // generator hardcoded to the pre-cutoff three-member form looked correct while every program + // above it, including every ESSL program (the source processor rewrites those to + // "#version 460 core"), was silently mismatched. + // + // The expected member COUNT is deliberately not spelled per version any more. It moved once + // already (the fork's GL_ARB_cull_distance work lowered the cutoff from 450 to 400, so 430 + // went from three members to four), and pinning it here only produced a test that failed for + // being right. What must hold - and is what this case now asserts - is that the generator + // reproduces whatever glslang produced, at every version, plus the floor that a per-vertex + // block always has at least gl_Position. A tessellation evaluation stage cannot be authored + // below #version 400 at all (ARB_tessellation_shader is GL 4.0), so 400 is the bottom of the + // reachable range; the pre-cutoff three-member form is covered through the explicit-mask case + // below instead. + for (const char* version : {"#version 400 core", "#version 430 core", "#version 460 core"}) { + SCOPED_TRACE(version); + + // Deliberately the shape of + // KHR-GL43.shader_storage_buffer_object.advanced-write-tessellation: a vertex stage + // feeding an evaluation stage with no control stage in between. + const String vs = String(version) + R"( layout(location = 0) in vec4 g_in_position; void main() { gl_Position = g_in_position; } )"; - static const char* kTes = R"(#version 430 core + const String tes = String(version) + R"( layout(quads) in; void main() { vec4 p0 = mix(gl_in[0].gl_Position, gl_in[1].gl_Position, gl_TessCoord.x); @@ -230,55 +292,62 @@ void main() { gl_Position = mix(p0, p1, gl_TessCoord.y); } )"; - static const char* kFs = R"(#version 430 core + const String fs = String(version) + R"( layout(location = 0) out vec4 g_fs_out; void main() { g_fs_out = vec4(0, 1, 0, 1); } )"; - const Vector types{GL_VERTEX_SHADER, GL_TESS_EVALUATION_SHADER, GL_FRAGMENT_SHADER}; - const Vector sources{kVs, kTes, kFs}; - Vector> shaders; - for (SizeT i = 0; i < types.size(); ++i) { - ShaderAttrib attrib{.shaderType = types[i], .sourceStr = sources[i]}; - auto compiled = ShaderCompiler::CompileShader(attrib); - ASSERT_TRUE(compiled) << compiled.error().log; - shaders.push_back(compiled.value()); - } - ProgramAttrib programAttrib{.shaders = shaders}; - auto linked = ShaderCompiler::LinkProgram(programAttrib); - ASSERT_TRUE(linked) << linked.error().log; - ProgramBinaryAttrib binaryAttrib{.shaderTypes = types, .program = *linked.value()}; - auto binary = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); - ASSERT_TRUE(binary); - ASSERT_EQ(binary->size(), types.size()); + const Vector types{GL_VERTEX_SHADER, GL_TESS_EVALUATION_SHADER, GL_FRAGMENT_SHADER}; + const Vector sources{&vs, &tes, &fs}; + Vector> shaders; + for (SizeT i = 0; i < types.size(); ++i) { + ShaderAttrib attrib{.shaderType = types[i], .sourceStr = *sources[i]}; + auto compiled = ShaderCompiler::CompileShader(attrib); + ASSERT_TRUE(compiled) << (compiled ? String{} : compiled.error().log); + shaders.push_back(compiled.value()); + } + ProgramAttrib programAttrib{.shaders = shaders}; + auto linked = ShaderCompiler::LinkProgram(programAttrib); + ASSERT_TRUE(linked) << (linked ? String{} : linked.error().log); + ProgramBinaryAttrib binaryAttrib{.shaderTypes = types, .program = *linked.value()}; + auto binary = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); + ASSERT_TRUE(binary); + ASSERT_EQ(binary->size(), types.size()); - // The evaluation stage's gl_in is the block the pass-through has to feed. It is the only - // built-in block that stage declares as an input, so the module holds exactly one such shape - // besides its own gl_PerVertex output - and both are the same shape, which is the point. - const auto tesShapes = BuiltInBlockShapes((*binary)[1]); - ASSERT_FALSE(tesShapes.empty()); - const Vector frontendShape = tesShapes.begin()->second; - const Uint32 frontendMembers = StructMemberCount((*binary)[1], tesShapes.begin()->first); - for (const auto& [structId, shape] : tesShapes) { - EXPECT_EQ(shape, frontendShape) << "the evaluation stage's own built-in blocks disagree"; - EXPECT_EQ(StructMemberCount((*binary)[1], structId), frontendMembers); - } + // The evaluation stage's gl_in is the block the pass-through has to feed. It is the only + // built-in block that stage declares as an input, so the module holds exactly one such + // shape besides its own gl_PerVertex output - and both are the same shape, which is the + // point. + const auto tesShapes = BuiltInBlockShapes((*binary)[1]); + ASSERT_FALSE(tesShapes.empty()); + const Vector frontendShape = tesShapes.begin()->second; + const Uint32 frontendMembers = StructMemberCount((*binary)[1], tesShapes.begin()->first); + EXPECT_GE(frontendMembers, 1u) << "a gl_PerVertex block always carries at least gl_Position"; + for (const auto& [structId, shape] : tesShapes) { + EXPECT_EQ(shape, frontendShape) << "the evaluation stage's own built-in blocks disagree"; + EXPECT_EQ(StructMemberCount((*binary)[1], structId), frontendMembers); + } - const Vector passthrough = CompileGeneratedSource(4); - ASSERT_FALSE(passthrough.empty()); - const auto passthroughShapes = BuiltInBlockShapes(passthrough); - ASSERT_FALSE(passthroughShapes.empty()); + // ...and the generator is driven the way PRODUCTION drives it: the mask comes from the + // evaluation stage's own module, not from a constant the test picked. + const Uint32 reflectedMembers = ProgramFactory::ReflectPerVertexInputMembers((*binary)[1]); + EXPECT_NE(reflectedMembers, 0u) << "the input per-vertex block walk found nothing to match against"; + const Vector passthrough = CompileGeneratedSource(4, reflectedMembers); + ASSERT_FALSE(passthrough.empty()); + const auto passthroughShapes = BuiltInBlockShapes(passthrough); + ASSERT_FALSE(passthroughShapes.empty()); - Uint32 perVertexBlocksChecked = 0; - for (const auto& [structId, shape] : passthroughShapes) { - // gl_TessLevelOuter/Inner are decorated on plain variables, not on a block, so every - // struct that reaches here is a gl_PerVertex - gl_in's and gl_out's. - EXPECT_EQ(shape, frontendShape) - << "the pass-through control stage's gl_PerVertex no longer matches the one the " - "frontend gives a linked vertex+evaluation program"; - EXPECT_EQ(StructMemberCount(passthrough, structId), frontendMembers) - << "the pass-through control stage's gl_PerVertex has a different member count"; - ++perVertexBlocksChecked; + Uint32 perVertexBlocksChecked = 0; + for (const auto& [structId, shape] : passthroughShapes) { + // gl_TessLevelOuter/Inner are decorated on plain variables, not on a block, so every + // struct that reaches here is a gl_PerVertex - gl_in's and gl_out's. + EXPECT_EQ(shape, frontendShape) + << "the pass-through control stage's gl_PerVertex no longer matches the one the " + "frontend gives a linked vertex+evaluation program"; + EXPECT_EQ(StructMemberCount(passthrough, structId), frontendMembers) + << "the pass-through control stage's gl_PerVertex has a different member count"; + ++perVertexBlocksChecked; + } + EXPECT_EQ(perVertexBlocksChecked, 2u) << "expected both gl_in and gl_out to be gl_PerVertex blocks"; } - EXPECT_EQ(perVertexBlocksChecked, 2u) << "expected both gl_in and gl_out to be gl_PerVertex blocks"; } diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 454f7296..8f93287b 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -1468,6 +1468,86 @@ TEST(GetterSanity, PerCategoryMultisampleCeilingsAreProbedRatherThanFlooredAtFou MG_State::pGLContext = Move(previousContext); } +// GL_ARB_cull_distance below #version 450, which is the band the conformance suite actually +// compiles in: cull_distance.coverage emits its compute shader at "#version 420 core" with +// `#extension GL_ARB_cull_distance : require` and reads gl_MaxCullDistances. Registering the +// extension name alone was not enough - `require` started succeeding while the constants stayed +// gated on 450, so the shader traded one error for another. +// +// The three cases below are the whole contract: the macro must be true exactly where the feature +// is, the constants must exist under the extension, and using the feature WITHOUT the extension +// must still fail (otherwise the gate is decorative). +TEST(ShaderCompilerSanity, ArbCullDistanceIsUsableBelow450) { + using namespace MobileGL; + + auto previousContext = Move(MG_State::pGLContext); + auto previousBackend = Move(MG_Backend::pActiveBackendObject); + MG_State::pGLContext = MakeUnique(); + MG_Backend::pActiveBackendObject = MakeUnique(MG_Backend::DynamicBackendParameters{}); + const auto env = MG_Util::ShaderTranspiler::CaptureCompileEnv(); + + const auto compileFragment = [&env](const String& source) { + return MG_Util::ShaderTranspiler::ShaderCompiler::CompileShader({ + .shaderType = GL_FRAGMENT_SHADER, + .sourceStr = source, + .env = env.get(), + }); + }; + + // The coverage shader's shape, reduced to a fragment stage: require the extension, then read + // the constant it brings. + const String withExtension = R"(#version 420 core +#extension GL_ARB_cull_distance : require +out vec4 mgColor; +void main() { mgColor = vec4(float(gl_MaxCullDistances + gl_MaxCombinedClipAndCullDistances)); } +)"; + auto extensionCompiled = compileFragment(withExtension); + EXPECT_TRUE(extensionCompiled) << (extensionCompiled ? String() : extensionCompiled.error().log); + + // The macro has to agree with that, or the standard `#ifdef` probe lies in one direction or + // the other. It is defined from 400 up, where the built-ins exist... + const String macroProbe420 = R"(#version 420 core +out vec4 mgColor; +#ifndef GL_ARB_cull_distance +#error GL_ARB_cull_distance should be defined at 420 +#endif +void main() { mgColor = vec4(0.0); } +)"; + auto macro420 = compileFragment(macroProbe420); + EXPECT_TRUE(macro420) << (macro420 ? String() : macro420.error().log); + + // ...and NOT below it, where they do not. A shader whose `#ifdef GL_ARB_cull_distance` branch + // reads gl_MaxCullDistances used to take that branch at 330 and fail to compile. + const String macroProbe330 = R"(#version 330 core +out vec4 mgColor; +#ifdef GL_ARB_cull_distance +#error GL_ARB_cull_distance must not be advertised where the built-ins do not exist +#endif +void main() { mgColor = vec4(0.0); } +)"; + auto macro330 = compileFragment(macroProbe330); + EXPECT_TRUE(macro330) << (macro330 ? String() : macro330.error().log); + + // The gate is real: below 450 the constants are reachable ONLY through the extension. + const String withoutExtension = R"(#version 420 core +out vec4 mgColor; +void main() { mgColor = vec4(float(gl_MaxCullDistances)); } +)"; + EXPECT_FALSE(compileFragment(withoutExtension)) + << "gl_MaxCullDistances must require GL_ARB_cull_distance below #version 450"; + + // ...and at 450 it is core, so no directive is needed. + const String core450 = R"(#version 450 core +out vec4 mgColor; +void main() { mgColor = vec4(float(gl_MaxCullDistances)); } +)"; + auto coreCompiled = compileFragment(core450); + EXPECT_TRUE(coreCompiled) << (coreCompiled ? String() : coreCompiled.error().log); + + MG_Backend::pActiveBackendObject = Move(previousBackend); + MG_State::pGLContext = Move(previousContext); +} + TEST(GetterSanity, ReportsKhrSubgroupDynamicParameters) { using namespace MobileGL;