diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index 45f4c893..8e8f2e13 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -2391,8 +2391,18 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } - // How many descriptors to declare for an ARRAY of opaque uniforms (samplers, images) at - // one binding - or 0, meaning this binding cannot be described and must be declined. + // How many descriptors to declare for an ARRAY of opaque uniforms (samplers, images) at one + // binding. A returned count is always DECLARED in the descriptor set layout; `outDeclined` + // says whether the binding can also be RESOLVED at draw time, or whether the program has to + // be refused instead. + // + // Those are deliberately two different things. The layout must keep describing what the + // shader declares even for a binding MobileGL cannot resolve: a descriptor the shader reads + // and the layout omits is not a missing draw, it is an undefined descriptor access, and + // lavapipe segfaults on it inside pipeline creation - in a JIT worker thread, before any + // draw runs, which is why removing the binding produced a flaky crash rather than a clean + // refusal. Declining is done by refusing the draw (VkProgramObject::declinedDescriptors), + // not by shrinking the layout. // // Two separate things have to hold, and neither is checkable from the SPIR-V alone: // @@ -2414,16 +2424,21 @@ namespace MobileGL::MG_Backend::DirectVulkan { static Uint32 DescriptorCountForOpaqueUniformArray(const MG_State::GLState::ProgramObject& program, const String& uniformName, Uint32 binding, Int baseLocation, Uint32 reflectedCount, Uint32 maxBindings, - const char* kindLabel) { + const char* kindLabel, Bool& outDeclined) { const Uint32 count = std::max(1u, reflectedCount); if (count == 1) { return 1u; } if (count > maxBindings) { + // Nothing legal to declare: the count would not fit a VkDescriptorSetLayoutBinding + // this device accepts, and it would narrow badly into the Uint16 that carries it + // (65536 becomes 0). The layout ends up inconsistent with the shader whatever we do, + // so declare what we can and refuse the draw. MGLOG_I("ProgramFactory::ReflectLayout: %s array '%s' at binding %u has %u elements, past the %u " "this device can describe - declining the program", kindLabel, uniformName.c_str(), binding, count, maxBindings); - return 0u; + outDeclined = true; + return maxBindings; } if (baseLocation < 0 || !program.UniformLocationsAliasSameUniform(baseLocation, baseLocation + static_cast(count - 1u))) { @@ -2432,7 +2447,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { "is the usual cause, and MobileGL declines it rather than resolve elements onto a " "neighbouring uniform", kindLabel, uniformName.c_str(), binding, count, baseLocation); - return 0u; + outDeclined = true; } return count; } @@ -2454,6 +2469,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { entry.dynamicBindings.clear(); entry.bindingDescriptorCounts.assign(m_maxBindings, 1); entry.arrayedUniformBlockIndicesByBinding.clear(); + entry.declinedDescriptors = false; // Use SpvcSession (Reflection mode) to reflect all SPIR-V modules in a single pass per module for (const auto& module : spirv) { @@ -2673,6 +2689,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { "descriptor array with no frontend uniform location (a multi-dimensional array " "of samplers or images is the known cause)", uniformName.c_str(), binding, sampler->count); + entry.declinedDescriptors = true; + // Declared, not resolved - see DescriptorCountForOpaqueUniformArray for + // why the layout keeps describing a binding the draw path will refuse. + entry.bindingDescriptorCounts[binding] = + static_cast(std::min(sampler->count, m_maxBindings)); + continue; } entry.bindingKinds[binding] = DescriptorBindingKind::None; continue; @@ -2694,12 +2716,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { // DescriptorCountForOpaqueUniformArray for what "declined" costs and why // the reflection's reserved extent - not SPIRV-Reflect's flattened count - // is what the per-element resolve can actually address. - const Uint32 imageArrayCount = DescriptorCountForOpaqueUniformArray( - program, uniformName, binding, location, sampler->count, m_maxBindings, "image"); - if (imageArrayCount == 0) { - entry.bindingKinds[binding] = DescriptorBindingKind::None; - continue; - } + const Uint32 imageArrayCount = + DescriptorCountForOpaqueUniformArray(program, uniformName, binding, location, sampler->count, + m_maxBindings, "image", entry.declinedDescriptors); entry.bindingDescriptorCounts[binding] = static_cast(imageArrayCount); const VkFormat reflectedFormat = @@ -2741,12 +2760,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { // only element 0 - so elements 1..N read a descriptor nobody had written // (KHR-GL42.shading_language_420pack.binding_sampler_array; lavapipe faults // inside the JIT-ed shader rather than reporting). - const Uint32 samplerArrayCount = DescriptorCountForOpaqueUniformArray( - program, uniformName, binding, location, sampler->count, m_maxBindings, "sampler"); - if (samplerArrayCount == 0) { - entry.bindingKinds[binding] = DescriptorBindingKind::None; - continue; - } + const Uint32 samplerArrayCount = + DescriptorCountForOpaqueUniformArray(program, uniformName, binding, location, sampler->count, + m_maxBindings, "sampler", entry.declinedDescriptors); entry.bindingDescriptorCounts[binding] = static_cast(samplerArrayCount); const SamplerNumericDomain numericDomain = UniformTypeToSamplerNumericDomain(uniformType); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index c34fda0b..7f19be6d 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -86,8 +86,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { Vector activeBindings; Vector dynamicBindings; Vector uniformBlockIndexByBinding; - // Descriptor count per binding (1 except for UBO instance arrays, which occupy one - // binding with descriptorCount = N). + // Descriptor count per binding (1 except for a descriptor ARRAY - a UBO or storage + // block instance array, an image uniform array or a sampler uniform array - each of + // which occupies one binding with descriptorCount = N). Vector bindingDescriptorCounts; // Per-element GL uniform block indices for arrayed UBO bindings (count > 1); // element 0 of a non-arrayed binding stays in uniformBlockIndexByBinding. @@ -103,6 +104,14 @@ namespace MobileGL::MG_Backend::DirectVulkan { // Set once during ReflectLayout so the per-draw path can skip the whole // storage-image preparation for the overwhelming majority of programs. Bool hasStorageImages = false; + // ReflectLayout found a descriptor it cannot describe and dropped it from the + // layout. That leaves a layout the shader disagrees with, so this program must + // never reach a draw: BindProgramUniformBuffers refuses outright, and the draw + // setup skips the draw exactly as it does for any other bind failure. Dropping the + // binding WITHOUT refusing the draw is what a shader reading an undeclared + // descriptor looks like, and lavapipe segfaults inside the JIT-ed shader on it. + // The reason was logged at MGLOG_I when the binding was declined. + Bool declinedDescriptors = false; Int globalUboBinding = -1; Uint32 activeVertexInputLocationMask = 0; Array vertexInputTypes{}; @@ -147,6 +156,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { storageBlockNameByBinding = std::move(other.storageBlockNameByBinding); storageBlockIndexByBinding = std::move(other.storageBlockIndexByBinding); hasStorageImages = other.hasStorageImages; + declinedDescriptors = other.declinedDescriptors; globalUboBinding = other.globalUboBinding; activeVertexInputLocationMask = other.activeVertexInputLocationMask; vertexInputTypes = other.vertexInputTypes; @@ -161,6 +171,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { other.descriptorSetLayout = VK_NULL_HANDLE; other.pipelineLayout = VK_NULL_HANDLE; other.hasStorageImages = false; + other.declinedDescriptors = false; other.globalUboBinding = -1; other.activeVertexInputLocationMask = 0; other.activeFragmentOutputLocationMask = 0; @@ -196,6 +207,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { storageBlockNameByBinding = std::move(other.storageBlockNameByBinding); storageBlockIndexByBinding = std::move(other.storageBlockIndexByBinding); hasStorageImages = other.hasStorageImages; + declinedDescriptors = other.declinedDescriptors; globalUboBinding = other.globalUboBinding; activeVertexInputLocationMask = other.activeVertexInputLocationMask; vertexInputTypes = other.vertexInputTypes; @@ -210,6 +222,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { other.descriptorSetLayout = VK_NULL_HANDLE; other.pipelineLayout = VK_NULL_HANDLE; other.hasStorageImages = false; + other.declinedDescriptors = false; other.globalUboBinding = -1; other.activeVertexInputLocationMask = 0; other.activeFragmentOutputLocationMask = 0; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index 45596b4b..d0195b3f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -541,6 +541,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { Bool UniformManager::ProgramSamplesOnlySingleLevelTextures( const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj) { + // A declined program never draws (see VkProgramObject::declinedDescriptors), and its + // declined binding has no resolvable uniform location - so there is nothing to prove + // about the textures it would have sampled. + if (programObj.declinedDescriptors) { + return false; + } Bool sawSampler = false; for (Uint32 binding = 0; binding < programObj.bindingKinds.size(); ++binding) { if (programObj.bindingKinds[binding] != ProgramFactory::DescriptorBindingKind::CombinedImageSampler) { @@ -961,6 +967,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (outBindingRecords != nullptr) { outBindingRecords->clear(); } + // Nothing to prepare for a program the bind path is going to refuse; its declined + // binding has no uniform location to resolve a texture through either. + if (programObj.declinedDescriptors) { + return true; + } const Uint32 bindingCount = std::min(m_maxBindings, static_cast(programObj.bindingKinds.size())); @@ -998,6 +1009,10 @@ namespace MobileGL::MG_Backend::DirectVulkan { Bool UniformManager::SampledBindingsUnchanged(const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj, const Vector& previousRecords) const { + // A declined program takes the full path every time and is refused there. + if (programObj.declinedDescriptors) { + return false; + } SizeT recordIndex = 0; // Iterate only the bindings this program declares (ascending), exactly like // BindProgramUniformBuffers: this runs per draw whenever the texture bind @@ -1040,6 +1055,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { outTextures.clear(); MOBILEGL_ASSERT(MG_State::pGLContext != nullptr, "CollectStorageImageTextures: GL context is null"); + // Same as the sampled walk: a declined program is refused at bind time, and its declined + // binding has no uniform location to reach an image unit through. + if (programObj.declinedDescriptors) { + return true; + } const Uint32 bindingCount = std::min(m_maxBindings, static_cast(programObj.bindingKinds.size())); @@ -1456,6 +1476,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { VkPipelineBindPoint bindPoint, const SamplerBindingOverride* samplerBindingOverride, Bool samplerDescriptorsUnchangedHint) { + // ReflectLayout could not describe one of this program's descriptors and dropped it from + // the layout, which leaves the layout disagreeing with the shader. Refusing here is what + // makes that a DECLINE rather than a fault: the draw setup skips the draw on a false + // return, so the shader never runs against a descriptor the layout does not declare - + // which on lavapipe is a segfault inside the JIT-ed shader, and on a hardware driver is + // whatever it chooses. ReflectLayout already said why, once, at MGLOG_I. + if (programObj.declinedDescriptors) { + MGLOG_D("UniformDescriptorBinder::BindProgramUniformBuffers: refusing a program whose descriptor layout " + "was declined at reflection"); + return false; + } auto& frame = m_frames[frameIndex]; if (frame.descriptorPools.empty()) { MGLOG_E("UniformDescriptorBinder::BindProgramUniformBuffers failed: frame descriptor pools are invalid"); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp index db1b24fa..a0f721d5 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp @@ -91,6 +91,24 @@ void main() if (texture(goku[3], uv) != vec4(0.0, 1.0, 1.0, 1.0)) bad |= 8; o_color = vec4(float(bad) / 255.0, bad == 0 ? 1.0 : 0.0, 0.0, 1.0); } +)"; + + // Same declaration one dimension deeper. GLSL 4.30 arrays of arrays are legal here, and + // the elements still take consecutive units (1..4) in declaration order - but the two + // reflections disagree about how to count them, which is the whole point of this case. + constexpr const char* kSamplerArrayOfArraysFS = R"(#version 430 core +layout(binding = 1) uniform sampler2D goku[2][2]; +out vec4 o_color; +void main() +{ + const vec2 uv = vec2(0.5, 0.5); + int bad = 0; + if (texture(goku[0][0], uv) != vec4(1.0, 0.0, 0.0, 1.0)) bad |= 1; + if (texture(goku[0][1], uv) != vec4(0.0, 0.0, 1.0, 1.0)) bad |= 2; + if (texture(goku[1][0], uv) != vec4(1.0, 1.0, 0.0, 1.0)) bad |= 4; + if (texture(goku[1][1], uv) != vec4(0.0, 1.0, 1.0, 1.0)) bad |= 8; + o_color = vec4(float(bad) / 255.0, bad == 0 ? 1.0 : 0.0, 0.0, 1.0); +} )"; constexpr const char* kBlockArrayFS = R"(#version 420 core @@ -233,6 +251,17 @@ void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); } return image.At(gl.Width() / 2, gl.Height() / 2); } + // An array of ARRAYS is declined by Magma (ProgramFactory::ReflectLayout logs it and + // VkProgramObject::declinedDescriptors then refuses every draw), which is a defined + // outcome the case below can assert. Espryt has no such gate: it bakes the units the + // frontend reports into its ESSL, and since the binding-qualifier seeding does not + // walk the inner dimension every element reports unit 0 - so it samples one texture + // four times and paints a mismatch. That gap is in the FRONTEND, one level below + // either backend, and fixing it is the feature that would make this shape work + // everywhere; it is not part of wiring descriptor arrays through Magma, so the + // Espryt arm is SCOPED and the reflection half is asserted on both backends. + bool MultiDimensionalSamplerArraysAreDeclined() const { return Gl().BackendName() == "DirectVulkan"; } + // Same shape, different gap: with the compile fixed, this shader now links on // both backends but paints nothing on Magma - the atomic counter becomes a // buffer descriptor there and that half is not wired up yet (the conformance @@ -293,6 +322,71 @@ void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); } EXPECT_EQ(centre.g, 255) << "the draw did not reach the fragment stage at all"; } + // An array of ARRAYS of samplers is the shape the two reflections count differently: + // SPIRV-Reflect reports one binding of 4 flattened descriptors, while the frontend hands out + // uniform locations along the outer dimension only and keys the uniform by its full + // "goku[0][0]" spelling. Magma therefore cannot address elements 1..3 of that binding, and + // the contract this case pins is that it says so and DECLINES - the failure it must never + // return to is resolving those elements onto whatever uniform got the next locations, which + // is a silently wrong texture rather than a missing draw. + // + // Deliberately weak on the pixels for that reason: what is asserted on every backend is that + // the program builds, the draw raises no GL error, and the process survives. Where the + // descriptors do resolve, the colours are checked too. + TEST_F(Glsl420DeclarationScenario, AnArrayOfSamplerArraysIsHonouredOrDeclinedCleanly) { + if (!Ready()) return; + + static const std::uint8_t colors[kElements][4] = { + {255, 0, 0, 255}, {0, 0, 255, 255}, {255, 255, 0, 255}, {0, 255, 255, 255}}; + MakeElementTextures(colors); + + std::string error; + const GLuint program = CompileProgram(kQuadVS, kSamplerArrayOfArraysFS, &error); + if (program == 0) { + GTEST_SKIP() << "the frontend does not build an array of sampler arrays: " << error; + } + m_programs.push_back(program); + + // The reflection DOES reserve one location per flattened element, in the order + // SPIRV-Reflect flattens them - which is the whole reason baseLocation + element is the + // right addressing rule for a descriptor array, and would be right for this shape too. + // What is missing is one level up: the `layout(binding = 1)` unit seeding walks the outer + // dimension only, so all four elements report unit 0 instead of 1..4. That is why this + // shape is declined rather than supported, and it is asserted here because the day the + // seeding learns arrays of arrays, the decline should be revisited rather than kept. + glUseProgram(program); + for (int outer = 0; outer < 2; ++outer) { + for (int inner = 0; inner < 2; ++inner) { + const std::string name = "goku[" + std::to_string(outer) + "][" + std::to_string(inner) + "]"; + EXPECT_EQ(glGetUniformLocation(program, name.c_str()), outer * 2 + inner) + << name << " should hold the flattened element's own location"; + } + } + glUseProgram(0); + + const Rgba8 centre = DrawAndRead(program); + EXPECT_EQ(FirstGLError(), 0u) << "declining a descriptor array must not raise a GL error"; + + if (!MultiDimensionalSamplerArraysAreDeclined()) { + GTEST_SKIP() << "the frontend's binding-qualifier seeding does not walk an array of arrays, so " + << Gl().BackendName() << " samples unit 0 for every element; the locations " + << "asserted above are the half of this case it can answer"; + } + + // Three outcomes are possible and only two are acceptable. Green means every element + // sampled its own unit. Black - the untouched clear - means the program was declined and + // painted nothing, which is the documented Magma outcome. A non-zero red channel is the + // third: the draw DID reach the fragment stage and elements read the wrong textures, + // which is exactly the silent mismatch this decline exists to prevent. + if (centre.g == 255) { + EXPECT_EQ(centre.r, 0) << "elements of the array of arrays that read the wrong texture: " + << BadElements(centre.r); + return; + } + EXPECT_EQ(centre.r, 0) << "the array of arrays was not resolved, but the draw still painted " + "a mismatch instead of being declined: " << BadElements(centre.r); + } + // Instance k of a uniform block array sits on buffer binding point N+k - again both as // reported and as fed to the shader. TEST_F(Glsl420DeclarationScenario, UniformBlockArrayInstancesTakeConsecutiveBindings) {