From 97facf777b6d3ced455a962d1af3541f2c8c66d2 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 01:09:56 -0400 Subject: [PATCH] [Fix] (MG_Backend/DirectVulkan): act on a failed binding remap, bound the storage-block array count, and correct the decline rationale review found describing the reverted mechanism --- .../DirectVulkan/Renderer/ProgramFactory.cpp | 51 +++++++++++++++-- .../DirectVulkan/Renderer/ProgramFactory.h | 19 ++++--- .../DirectVulkan/Renderer/UniformManager.cpp | 56 +++++++++++-------- 3 files changed, 92 insertions(+), 34 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index 8e8f2e13..cec12f2b 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -2419,8 +2419,14 @@ namespace MobileGL::MG_Backend::DirectVulkan { // // Asking the reflection whether baseLocation and baseLocation + count - 1 are slots of the // SAME uniform tests exactly that precondition, without this code having to model how - // glslang chooses to lay an array of arrays out - if a future glslang reserves all six, the - // check passes and the per-element paths are right for it too. + // glslang chooses to lay an array of arrays out. + // + // That is NOT on its own enough to start supporting the shape, though, and this check must + // not be relaxed alone: the binding-qualifier unit seeding in ProgramLinkTask looks an + // opaque uniform up by its name minus a trailing "[0]", so `goku[0][0]` misses the `goku` + // key and every element of an array of arrays seeds texture unit 0. Resolving those elements + // would then paint silently-wrong pixels with no diagnostic at all - strictly worse than + // declining. The decline goes away together with the seeding fix, not before it. static Uint32 DescriptorCountForOpaqueUniformArray(const MG_State::GLState::ProgramObject& program, const String& uniformName, Uint32 binding, Int baseLocation, Uint32 reflectedCount, Uint32 maxBindings, @@ -2432,8 +2438,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { 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. + // (65536 becomes 0). Unlike the extent case below, this one CANNOT keep the layout + // consistent with the shader, so refusing the draw does not fully protect it - the + // driver still JITs a shader indexing past the declared count. Declaring as many as + // the device allows keeps vkCreateDescriptorSetLayout succeeding and the program + // inert; a device whose binding cap is smaller than a shader's array is not a + // configuration MobileGL can serve at all. Needs a >maxBindings-element array to + // reach (256 on desktop, ~16 on mobile). 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); @@ -2669,7 +2680,23 @@ namespace MobileGL::MG_Backend::DirectVulkan { // `b[1].data.length()` answered from an unconstrained buffer instead of its // own bound range (KHR-GL43.shader_storage_buffer_object.- // advanced-unsizedArrayLength-*). - entry.bindingDescriptorCounts[binding] = static_cast(std::max(1u, sampler->count)); + // + // Bounds-checked like every other array kind. The EXTENT rule differs - a + // block array's elements take consecutive GL binding points rather than + // consecutive uniform locations, so DescriptorCountForOpaqueUniformArray's + // location test does not apply here - but the size rule is identical: this + // count goes straight into a VkDescriptorSetLayoutBinding and is narrowed to + // a Uint16 on the way, where 65536 would silently become 0. + const Uint32 storageArrayCount = std::max(1u, sampler->count); + if (storageArrayCount > m_maxBindings) { + MGLOG_I("ProgramFactory::ReflectLayout: storage block array '%s' at binding %u has %u " + "elements, past the %u this device can describe - declining the program", + uniformName.c_str(), binding, storageArrayCount, m_maxBindings); + entry.declinedDescriptors = true; + entry.bindingDescriptorCounts[binding] = static_cast(m_maxBindings); + continue; + } + entry.bindingDescriptorCounts[binding] = static_cast(storageArrayCount); continue; } @@ -3089,6 +3116,20 @@ namespace MobileGL::MG_Backend::DirectVulkan { ReflectVertexInputs(shaders, moduleSpirvs, entry); ReflectFragmentOutputs(shaders, moduleSpirvs, entry); ReflectLayout(program, moduleSpirvs, entry); + // A failed remap means the modules kept glslang's per-stage auto-mapped binding numbers - + // no cross-stage unification, no set->0 normalisation - so the bindings this layout + // describes are not the bindings the shader reads. That has to stop the program from + // drawing, and until now nothing did: the MOBILEGL_ASSERT above compiles out of every + // build past DEBUG, and RemapDescriptorBindingsForVulkan's own refusal message said so at + // a level an INFO build also drops. Declining is the mechanism that already exists for + // "the layout and the shader disagree", so route it through that. Set AFTER ReflectLayout, + // which clears the flag. + if (!remapOk) { + MGLOG_I("ProgramFactory::GetOrCreateProgram: declining program %u - its descriptor bindings could not " + "be remapped, so the layout does not describe what the shader reads", + program.GetExternalIndex()); + entry.declinedDescriptors = true; + } return entry; } diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index d942a0ac..bb310781 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -104,13 +104,18 @@ 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. + // Something about this program's descriptors could not be resolved - an opaque + // uniform array whose elements have no addressable uniform locations (the + // multi-dimensional case), or a binding remap that failed outright. The binding + // STAYS DECLARED in the descriptor set layout; declining is done here, by refusing + // every draw, and BindProgramUniformBuffers returns false so the draw setup skips + // the draw exactly as it does for any other bind failure. + // + // Keeping the layout intact is the load-bearing half. Shrinking it instead - which + // is what the first cut of this did - leaves the shader reading a descriptor the + // layout never declared, and lavapipe segfaults on that inside PIPELINE CREATION, + // in a JIT worker thread, before any draw runs where a refusal could help. The + // reason was logged once at MGLOG_I when the descriptor was declined. Bool declinedDescriptors = false; Int globalUboBinding = -1; Uint32 activeVertexInputLocationMask = 0; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index d0195b3f..61590f0e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -311,23 +311,24 @@ namespace MobileGL::MG_Backend::DirectVulkan { } MOBILEGL_ASSERT(binding < programObj.samplerNameByBinding.size(), "ResolveSamplerDescriptor: sampler binding %u name lookup out of range", binding); - // Raw-pointer resolve to skip the SharedPtr atomic refcount churn: the bound texture stays - // alive through the draw via GL binding state. Only the fallback path needs a SharedPtr to - // keep the fallback texture alive for the rest of this call. - MG_State::GLState::ITextureObject* texture = ResolveSamplerTextureRaw(program, programObj, binding, element); - - // Per ELEMENT: GLSL 4.20 gives every element of `uniform sampler2D goku[4]` its own - // texture unit (consecutive from the declared binding, but glUniform1i may scatter them - // afterwards), so the unit - and with it the bound texture, the unit's sampler override - // and the fallback decision - is the element's, not the binding's. + // Per ELEMENT, and resolved BEFORE anything is looked up through it: GLSL 4.20 gives every + // element of `uniform sampler2D goku[4]` its own texture unit (consecutive from the + // declared binding, but glUniform1i may scatter them afterwards), so the unit - and with + // it the bound texture, the unit's sampler override and the fallback decision - is the + // element's, not the binding's. An element past the array's reserved extent has no unit + // at all, and must not fall back to resolving unit 0's texture. const Int location = ResolveDescriptorElementLocation(program, programObj.samplerUniformLocationByBinding[binding], element); if (location < 0 && element > 0) { - MGLOG_I("ResolveSamplerDescriptor: binding %u element %u is past the end of its sampler array", binding, + MGLOG_D("ResolveSamplerDescriptor: binding %u element %u is past the end of its sampler array", binding, element); return false; } const Int unit = ResolveSamplerUnitIndex(program, location, binding); + // Raw-pointer resolve to skip the SharedPtr atomic refcount churn: the bound texture stays + // alive through the draw via GL binding state. Only the fallback path needs a SharedPtr to + // keep the fallback texture alive for the rest of this call. + MG_State::GLState::ITextureObject* texture = ResolveSamplerTextureRaw(program, programObj, binding, element); auto& textureUnit = MG_State::pGLContext->GetTextureUnitObject(unit); const auto& samplerOverride = textureUnit.GetSamplerObject(); const auto preferredTarget = programObj.samplerTextureTargetByBinding[binding]; @@ -500,9 +501,18 @@ namespace MobileGL::MG_Backend::DirectVulkan { // Only for a binding that carries a single descriptor - an array's elements would // publish each other's descriptors here, and the next hinted draw would hand element // N-1's texture to element 0. - if (descriptorMemoUsable && binding < m_samplerResolveMemo.size()) { - m_samplerResolveMemo[binding].info = outImageInfo; - m_samplerResolveMemo[binding].infoValid = true; + if (binding < m_samplerResolveMemo.size()) { + if (descriptorMemoUsable) { + m_samplerResolveMemo[binding].info = outImageInfo; + m_samplerResolveMemo[binding].infoValid = true; + } else { + // An arrayed binding publishes nothing here, and clears what a previous program + // published at this index. Not strictly required - the hint's proof obligations + // are program-scoped and the entry is reset every frame - but leaving another + // program's descriptor sitting in a slot this one never refreshes is the kind of + // thing the next reader has to re-derive is safe. + m_samplerResolveMemo[binding].infoValid = false; + } NoteSamplerResolveMemoTouched(binding); } return true; @@ -557,6 +567,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { // different texture through its own unit. const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); for (Uint32 element = 0; element < descriptorCount; ++element) { + // The element's own location first, exactly as ResolveSamplerDescriptor resolves + // it - an element with no location would otherwise be judged on unit 0's texture. + const Int location = ResolveDescriptorElementLocation( + program, programObj.samplerUniformLocationByBinding[binding], element); + if (location < 0 && element > 0) return false; const auto* texture = ResolveSamplerTextureRaw(program, programObj, binding, element); if (texture == nullptr) return false; const auto& levelRange = texture->GetLevelRange(); @@ -565,9 +580,6 @@ namespace MobileGL::MG_Backend::DirectVulkan { // An explicit-LOD sample is a single filtered tap, so it also gives up anisotropic // filtering - which a single-level view can still have. Resolve the sampler exactly // the way ResolveSamplerDescriptor does and bail if anisotropy would apply. - const Int location = ResolveDescriptorElementLocation( - program, programObj.samplerUniformLocationByBinding[binding], element); - if (location < 0 && element > 0) return false; const Int unit = ResolveSamplerUnitIndex(program, location, binding); const auto& samplerOverride = MG_State::pGLContext->GetTextureUnitObject(unit).GetSamplerObject(); const auto* effectiveSampler = @@ -1476,12 +1488,12 @@ 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. + // This program has a descriptor MobileGL could not resolve (see + // VkProgramObject::declinedDescriptors). Refusing here is the whole of the decline: the + // binding is still declared in the layout, so the pipeline is consistent with the shader + // and creating it is safe - what must not happen is the draw, because the descriptor + // behind that binding can never be written. The draw setup skips the draw on a false + // return. 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");