From d2a36d65a3baf7c264afecc898b724be9f83150e Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 00:31:37 -0400 Subject: [PATCH] [Fix, Test] (MG_Backend/DirectVulkan, MG_IntegrationTest): a sampler uniform array is one binding with many descriptors too - Magma wrote only element zero, and declines the multi-dimensional shape loudly --- .../DirectVulkan/Renderer/ProgramFactory.cpp | 109 +++++- .../DirectVulkan/Renderer/UniformManager.cpp | 348 ++++++++++++------ .../DirectVulkan/Renderer/UniformManager.h | 33 +- .../Scenarios/Glsl420DeclarationScenario.cpp | 18 - 4 files changed, 346 insertions(+), 162 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index 1a59383a..45f4c893 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -1837,9 +1837,10 @@ namespace MobileGL::MG_Backend::DirectVulkan { // A descriptor ARRAY occupies one binding with descriptorCount = N, and is // supported for exactly the kinds that have a per-element resolve path in // UniformManager::BindProgramUniformBuffers: UBO instance arrays - // (uniform Block {...} b[N];), storage-block instance arrays, and image - // uniform arrays. Anything else must fail program creation cleanly rather - // than continue with corrupt state. + // (uniform Block {...} b[N];), storage-block instance arrays, image uniform + // arrays, and combined-image-sampler arrays (uniform sampler2D s[N];). + // Anything else - a uniform TEXEL buffer array is the one remaining kind - + // must fail program creation cleanly rather than continue with corrupt state. // // Getting listed here is not cosmetic: a kind that is rejected leaves // GetOrCreateProgram's MOBILEGL_ASSERT(remapOk) as the only complaint, and @@ -1848,12 +1849,16 @@ namespace MobileGL::MG_Backend::DirectVulkan { // unification and the set->0 normalisation this function exists to do. A // program with an image array plus any second descriptor got aliased // bindings out of that, and a DEBUG build trapped on the same program. + // Which is also why the message below is MGLOG_I: MGLOG_E is compiled out + // of an INFO build, so a refusal that only said MGLOG_E said nothing at all + // in the builds that ship. const Bool arraySupportedForKind = kind == ProgramFactory::DescriptorBindingKind::UniformBufferDynamic || kind == ProgramFactory::DescriptorBindingKind::StorageBuffer || - kind == ProgramFactory::DescriptorBindingKind::StorageImage; + kind == ProgramFactory::DescriptorBindingKind::StorageImage || + kind == ProgramFactory::DescriptorBindingKind::CombinedImageSampler; if (binding->count != 1 && !arraySupportedForKind) { - MGLOG_E("ProgramFactory: descriptor arrays are unsupported for this descriptor " + MGLOG_I("ProgramFactory: descriptor arrays are unsupported for this descriptor " "kind (name='%s' count=%u type=%d)", binding->name ? binding->name : "", binding->count, static_cast(binding->descriptor_type)); @@ -2386,6 +2391,52 @@ 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. + // + // Two separate things have to hold, and neither is checkable from the SPIR-V alone: + // + // * the count has to fit a VkDescriptorSetLayoutBinding this device will accept, and fit + // the Uint16 it is stored in (65536 would narrow to 0) and the scratch the bind path + // reserves from it; + // * the frontend reflection has to have RESERVED that many consecutive uniform locations + // for this uniform, because the per-element resolve paths address element k as + // baseLocation + k. SPIRV-Reflect's `count` is the FLATTENED element count, while GL + // locations follow the OUTER dimension only (ProgramObject::GetUniformArraySizeByTIndex + // answers TType::getOuterArraySize()). For a one-dimensional array the two agree; for + // `uniform sampler2D g[2][3]` SPIR-V says 6 where the reflection reserved 2, and + // elements 2..5 would silently resolve onto whichever uniform got the next locations. + // + // 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. + static Uint32 DescriptorCountForOpaqueUniformArray(const MG_State::GLState::ProgramObject& program, + const String& uniformName, Uint32 binding, Int baseLocation, + Uint32 reflectedCount, Uint32 maxBindings, + const char* kindLabel) { + const Uint32 count = std::max(1u, reflectedCount); + if (count == 1) { + return 1u; + } + if (count > maxBindings) { + 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; + } + if (baseLocation < 0 || + !program.UniformLocationsAliasSameUniform(baseLocation, baseLocation + static_cast(count - 1u))) { + MGLOG_I("ProgramFactory::ReflectLayout: %s array '%s' at binding %u spans %u descriptors but the " + "reflection reserved fewer uniform locations for it (base=%d) - a multi-dimensional array " + "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; + } + return count; + } + void ProgramFactory::ReflectLayout(const MG_State::GLState::ProgramObject& program, const Vector>& spirv, VkProgramObject& entry) const { // Initialize layout vectors @@ -2608,6 +2659,21 @@ namespace MobileGL::MG_Backend::DirectVulkan { const Int location = program.GetUniformLocation(uniformName); if (location < 0) { + // A uniform with no location is ordinarily one GL never made active, and + // dropping it is routine. An ARRAY reaching here is not routine: it is the + // multi-dimensional case. `uniform sampler2D g[2][3]` arrives from + // SPIRV-Reflect as one binding of 6 descriptors named "g", while the frontend + // reflection keys an array of arrays by its full "[0]"-terminated spelling + // ("g[0][0]"), so no base location resolves and the per-element paths have + // nothing to count from. Declining is the honest answer - but it has to SAY + // so at a level that survives a release build, because dropping the binding + // leaves the shader reading a descriptor the layout never declared. + if (sampler->count > 1) { + MGLOG_I("ProgramFactory::ReflectLayout: declining '%s' at binding %u - a %u-element " + "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.bindingKinds[binding] = DescriptorBindingKind::None; continue; } @@ -2624,15 +2690,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { // a storage BLOCK array, whose elements take consecutive GL binding points // from the declared one, each element of an image array carries its own // independently assigned image unit - see ResolveStorageImageDescriptor. - // Bounds-checked like the UBO array path above: descriptorCount goes - // straight into a VkDescriptorSetLayoutBinding, and the bind path reserves - // scratch from it, so an absurd array size has to be refused here rather - // than narrowed into a Uint16 (where 65536 would become 0). - const Uint32 imageArrayCount = std::max(1u, sampler->count); - if (imageArrayCount > m_maxBindings) { - MGLOG_E("ProgramFactory::ReflectLayout: image array '%s' at binding %u has %u elements, " - "past the %u this device can describe", - uniformName.c_str(), binding, imageArrayCount, m_maxBindings); + // Bounds- and extent-checked like the UBO array path above; see + // 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; } @@ -2668,6 +2732,23 @@ namespace MobileGL::MG_Backend::DirectVulkan { "ProgramFactory::ReflectLayout: failed to resolve texture target for '%s'", uniformName.c_str()); if (descriptorKind == DescriptorBindingKind::CombinedImageSampler) { + // An ARRAY of sampler uniforms is ONE binding carrying `count` descriptors, + // exactly like the image array above, and for the same reason: GLSL 4.20 + // gives `layout(binding = 1) uniform sampler2D goku[4]` one declaration + // spanning texture units 1..4, each element with its own glUniform1i-assigned + // unit. Leaving descriptorCount at 1 declared a single-descriptor binding + // while the shader indexed descriptors 1..3 of it, and the bind path wrote + // 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; + } + entry.bindingDescriptorCounts[binding] = static_cast(samplerArrayCount); + const SamplerNumericDomain numericDomain = UniformTypeToSamplerNumericDomain(uniformType); MOBILEGL_ASSERT(numericDomain != SamplerNumericDomain::Unknown, "ProgramFactory::ReflectLayout: failed to resolve sampler numeric domain " diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index c7f3168e..45596b4b 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -68,6 +68,29 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } + // Uniform location of ELEMENT `element` of the opaque-uniform array at `baseLocation`, or + // -1 when the reflection did not reserve that element. DoReflection hands out one location + // per array element, so the element's location is the base plus its index - bounded by the + // array's real extent so a descriptorCount that outran the reflection cannot walk onto the + // next uniform. Element 0 is the ordinary non-array case and costs nothing extra. + static Int ResolveDescriptorElementLocation(const MG_State::GLState::ProgramObject& program, Int baseLocation, + Uint32 element) { + if (baseLocation < 0 || element == 0) { + return baseLocation; + } + const Int location = baseLocation + static_cast(element); + return program.UniformLocationsAliasSameUniform(baseLocation, location) ? location : -1; + } + + // descriptorCount this binding declares in the descriptor set layout (1 for everything that + // is not an array). Kept in one place because the layout, the scratch reservation and the + // per-element write loops must all agree on it. + static Uint32 BindingDescriptorCount(const ProgramFactory::VkProgramObject& programObj, Uint32 binding) { + return binding < programObj.bindingDescriptorCounts.size() + ? std::max(1u, programObj.bindingDescriptorCounts[binding]) + : 1u; + } + static Int ResolveSamplerUnitIndex(const MG_State::GLState::ProgramObject& program, Int location, Uint32 binding) { MOBILEGL_ASSERT(location >= -1, "ResolveSamplerUnitIndex: invalid sampler location for binding %u", binding); if (location < 0) { @@ -268,15 +291,20 @@ namespace MobileGL::MG_Backend::DirectVulkan { Bool UniformManager::ResolveSamplerDescriptor(VkCommandBuffer commandBuffer, const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj, - Uint32 binding, VkDescriptorImageInfo& outImageInfo, + Uint32 binding, Uint32 element, + VkDescriptorImageInfo& outImageInfo, Bool trustUnchangedHint) const { MOBILEGL_ASSERT(m_textureManager != nullptr, "ResolveSamplerDescriptor: texture manager is null"); MOBILEGL_ASSERT(m_samplerManager != nullptr, "ResolveSamplerDescriptor: sampler manager is null"); + // The whole-descriptor memo below is keyed by binding alone, so it describes a binding + // that carries exactly one descriptor. An arrayed binding's elements would overwrite + // each other in it (see SamplerResolveMemo::info); they re-resolve instead. + const Bool descriptorMemoUsable = BindingDescriptorCount(programObj, binding) == 1u; // The caller proved every input of this binding's resolution unchanged since the // last full resolve (which also filled the cache), so the whole chain below - // texture/sampler resolution, completeness probe, sync, layout handling, sampler // and view lookups - would recompute the identical descriptor. - if (trustUnchangedHint && binding < m_samplerResolveMemo.size() && + if (trustUnchangedHint && descriptorMemoUsable && binding < m_samplerResolveMemo.size() && m_samplerResolveMemo[binding].infoValid) { outImageInfo = m_samplerResolveMemo[binding].info; return true; @@ -286,9 +314,19 @@ namespace MobileGL::MG_Backend::DirectVulkan { // 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); + MG_State::GLState::ITextureObject* texture = ResolveSamplerTextureRaw(program, programObj, binding, element); - const Int location = programObj.samplerUniformLocationByBinding[binding]; + // 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. + 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, + element); + return false; + } const Int unit = ResolveSamplerUnitIndex(program, location, binding); auto& textureUnit = MG_State::pGLContext->GetTextureUnitObject(unit); const auto& samplerOverride = textureUnit.GetSamplerObject(); @@ -459,7 +497,10 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (outImageInfo.sampler == VK_NULL_HANDLE) { return false; } - if (binding < m_samplerResolveMemo.size()) { + // 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; NoteSamplerResolveMemoTouched(binding); @@ -505,37 +546,45 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (programObj.bindingKinds[binding] != ProgramFactory::DescriptorBindingKind::CombinedImageSampler) { continue; } - const auto* texture = ResolveSamplerTextureRaw(program, programObj, binding); - if (texture == nullptr) return false; - const auto& levelRange = texture->GetLevelRange(); - if (levelRange.x() != levelRange.y()) return false; + // The rewrite this gates is program-wide, so EVERY sampler the program can read has + // to qualify - including every element of a sampler array, each of which reaches a + // different texture through its own unit. + const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); + for (Uint32 element = 0; element < descriptorCount; ++element) { + const auto* texture = ResolveSamplerTextureRaw(program, programObj, binding, element); + if (texture == nullptr) return false; + const auto& levelRange = texture->GetLevelRange(); + if (levelRange.x() != levelRange.y()) return false; - // 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 = programObj.samplerUniformLocationByBinding[binding]; - const Int unit = ResolveSamplerUnitIndex(program, location, binding); - const auto& samplerOverride = MG_State::pGLContext->GetTextureUnitObject(unit).GetSamplerObject(); - const auto* effectiveSampler = - samplerOverride ? samplerOverride.get() : texture->GetSamplerObject().get(); - if (effectiveSampler == nullptr) return false; - if (effectiveSampler->GetMaxAnisotropy() > 1.0f && - effectiveSampler->GetMinFilter() == SamplerFilterMode::Linear && - effectiveSampler->GetMagFilter() == SamplerFilterMode::Linear) { - return false; - } + // 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 = + samplerOverride ? samplerOverride.get() : texture->GetSamplerObject().get(); + if (effectiveSampler == nullptr) return false; + if (effectiveSampler->GetMaxAnisotropy() > 1.0f && + effectiveSampler->GetMinFilter() == SamplerFilterMode::Linear && + effectiveSampler->GetMagFilter() == SamplerFilterMode::Linear) { + return false; + } - // An explicit LOD 0 makes lambda exactly 0, which is the magnification side of the - // min/mag decision. That only matches the implicit form when lambda could not have been - // positive anyway (the LOD clamp already pins it at or below 0), or when the two - // filters are the same and the choice cannot be observed. - const Float effectiveMaxLod = effectiveSampler->GetMipmapMode() == SamplerMipmapMode::None - ? 0.0f - : effectiveSampler->GetMaxLod(); - if (effectiveMaxLod > 0.0f && effectiveSampler->GetMinFilter() != effectiveSampler->GetMagFilter()) { - return false; + // An explicit LOD 0 makes lambda exactly 0, which is the magnification side of the + // min/mag decision. That only matches the implicit form when lambda could not have been + // positive anyway (the LOD clamp already pins it at or below 0), or when the two + // filters are the same and the choice cannot be observed. + const Float effectiveMaxLod = effectiveSampler->GetMipmapMode() == SamplerMipmapMode::None + ? 0.0f + : effectiveSampler->GetMaxLod(); + if (effectiveMaxLod > 0.0f && effectiveSampler->GetMinFilter() != effectiveSampler->GetMagFilter()) { + return false; + } + sawSampler = true; } - sawSampler = true; } return sawSampler; } @@ -568,14 +617,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { MG_State::GLState::ITextureObject* UniformManager::ResolveSamplerTextureRaw( const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj, - Uint32 binding) { + Uint32 binding, Uint32 element) { MOBILEGL_ASSERT(MG_State::pGLContext != nullptr, "ResolveSamplerTextureRaw: GL context is null"); MOBILEGL_ASSERT(binding < programObj.samplerUniformLocationByBinding.size(), "ResolveSamplerTextureRaw: sampler location binding %u out of range", binding); MOBILEGL_ASSERT(binding < programObj.samplerTextureTargetByBinding.size(), "ResolveSamplerTextureRaw: sampler target binding %u out of range", binding); - const Int location = programObj.samplerUniformLocationByBinding[binding]; + const Int location = + ResolveDescriptorElementLocation(program, programObj.samplerUniformLocationByBinding[binding], element); const Int unit = ResolveSamplerUnitIndex(program, location, binding); auto& textureUnit = MG_State::pGLContext->GetTextureUnitObject(unit); @@ -860,7 +910,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { Bool UniformManager::ResolveSampledBinding(const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj, - Uint32 binding, + Uint32 binding, Uint32 element, MG_State::GLState::ITextureObject*& outTexture, const MG_State::GLState::SamplerObject*& outSampler) const { // Open-coded ResolveSamplerTextureRaw so the unit is resolved once for both the @@ -871,7 +921,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { "ResolveSampledBinding: sampler location binding %u out of range", binding); MOBILEGL_ASSERT(binding < programObj.samplerTextureTargetByBinding.size(), "ResolveSampledBinding: sampler target binding %u out of range", binding); - const Int location = programObj.samplerUniformLocationByBinding[binding]; + const Int location = + ResolveDescriptorElementLocation(program, programObj.samplerUniformLocationByBinding[binding], element); + if (location < 0 && element > 0) { + return false; + } const Int unit = ResolveSamplerUnitIndex(program, location, binding); auto& textureUnit = MG_State::pGLContext->GetTextureUnitObject(unit); const TextureTarget preferredTarget = programObj.samplerTextureTargetByBinding[binding]; @@ -915,19 +969,27 @@ namespace MobileGL::MG_Backend::DirectVulkan { continue; } - MG_State::GLState::ITextureObject* texture = nullptr; - const MG_State::GLState::SamplerObject* sampler = nullptr; - if (!ResolveSampledBinding(program, programObj, binding, texture, sampler)) { - continue; - } - if (outBindingRecords != nullptr) { - outBindingRecords->push_back({texture != nullptr ? texture->GetLifetimeId() : 0, - sampler != nullptr ? sampler->GetLifetimeId() : 0}); - } + // Every ELEMENT of a sampler array reaches its own texture through its own unit, + // so every element has to be in the sampled set: this walk is what gets those + // textures synced and transitioned to a sampled layout BEFORE the render pass + // opens, and a missed element would first be touched by the descriptor resolve + // inside an active pass. + const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); + for (Uint32 element = 0; element < descriptorCount; ++element) { + MG_State::GLState::ITextureObject* texture = nullptr; + const MG_State::GLState::SamplerObject* sampler = nullptr; + if (!ResolveSampledBinding(program, programObj, binding, element, texture, sampler)) { + continue; + } + if (outBindingRecords != nullptr) { + outBindingRecords->push_back({texture != nullptr ? texture->GetLifetimeId() : 0, + sampler != nullptr ? sampler->GetLifetimeId() : 0}); + } - auto found = std::find(outTextures.begin(), outTextures.end(), texture); - if (found == outTextures.end()) { - outTextures.push_back(texture); + auto found = std::find(outTextures.begin(), outTextures.end(), texture); + if (found == outTextures.end()) { + outTextures.push_back(texture); + } } } return true; @@ -948,18 +1010,24 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (programObj.bindingKinds[binding] != ProgramFactory::DescriptorBindingKind::CombinedImageSampler) { continue; } - MG_State::GLState::ITextureObject* texture = nullptr; - const MG_State::GLState::SamplerObject* sampler = nullptr; - if (!ResolveSampledBinding(program, programObj, binding, texture, sampler)) { - continue; - } - if (recordIndex >= previousRecords.size()) { - return false; - } - const SampledBindingRecord& record = previousRecords[recordIndex++]; - if (record.textureLifetimeId != (texture != nullptr ? texture->GetLifetimeId() : 0) || - record.samplerLifetimeId != (sampler != nullptr ? sampler->GetLifetimeId() : 0)) { - return false; + // Element-for-element, in the same order CollectSampledTextures recorded them - + // the two walks have to visit the identical descriptor sequence or the positional + // comparison below drifts. + const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); + for (Uint32 element = 0; element < descriptorCount; ++element) { + MG_State::GLState::ITextureObject* texture = nullptr; + const MG_State::GLState::SamplerObject* sampler = nullptr; + if (!ResolveSampledBinding(program, programObj, binding, element, texture, sampler)) { + continue; + } + if (recordIndex >= previousRecords.size()) { + return false; + } + const SampledBindingRecord& record = previousRecords[recordIndex++]; + if (record.textureLifetimeId != (texture != nullptr ? texture->GetLifetimeId() : 0) || + record.samplerLifetimeId != (sampler != nullptr ? sampler->GetLifetimeId() : 0)) { + return false; + } } } return recordIndex == previousRecords.size(); @@ -984,26 +1052,40 @@ namespace MobileGL::MG_Backend::DirectVulkan { return false; } - const Int location = programObj.samplerUniformLocationByBinding[binding]; - if (location < 0) { + const Int baseLocation = programObj.samplerUniformLocationByBinding[binding]; + if (baseLocation < 0) { MGLOG_E("CollectStorageImageTextures: binding %u has no image uniform location", binding); return false; } - const Int imageUnit = program.GetUniformSamplerOrImageUnitIndex(static_cast(location)); - if (imageUnit < 0 || imageUnit >= MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS) { - MGLOG_E("CollectStorageImageTextures: image unit %d is invalid for binding %u", - imageUnit, binding); - return false; - } + // Per ELEMENT, for the same reason the sampled walk above is: an image ARRAY is one + // binding whose elements each carry their own image unit, so each reaches its own + // texture. This walk is what puts those textures into the pre-pass sync and layout + // transition; collecting only element 0 left elements 1..N to be first touched by + // the descriptor resolve, which happens with a render pass already open. + const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); + for (Uint32 element = 0; element < descriptorCount; ++element) { + const Int location = ResolveDescriptorElementLocation(program, baseLocation, element); + if (location < 0) { + MGLOG_E("CollectStorageImageTextures: binding %u element %u is past the end of its image array", + binding, element); + return false; + } + const Int imageUnit = program.GetUniformSamplerOrImageUnitIndex(static_cast(location)); + if (imageUnit < 0 || imageUnit >= MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS) { + MGLOG_E("CollectStorageImageTextures: image unit %d is invalid for binding %u element %u", + imageUnit, binding, element); + return false; + } - auto* texture = MG_State::pGLContext->GetImageTextureBinding(imageUnit).Texture.get(); - if (texture == nullptr) { - MGLOG_E("CollectStorageImageTextures: image unit %d is unbound for binding %u", - imageUnit, binding); - return false; - } - if (std::find(outTextures.begin(), outTextures.end(), texture) == outTextures.end()) { - outTextures.push_back(texture); + auto* texture = MG_State::pGLContext->GetImageTextureBinding(imageUnit).Texture.get(); + if (texture == nullptr) { + MGLOG_E("CollectStorageImageTextures: image unit %d is unbound for binding %u element %u", + imageUnit, binding, element); + return false; + } + if (std::find(outTextures.begin(), outTextures.end(), texture) == outTextures.end()) { + outTextures.push_back(texture); + } } } return true; @@ -1437,19 +1519,22 @@ namespace MobileGL::MG_Backend::DirectVulkan { for (const auto& arrayEntry : programObj.arrayedUniformBlockIndicesByBinding) { uboArrayExtra += static_cast(arrayEntry.second.size()) - 1u; } - Uint32 ssboArrayExtra = 0; + // Surplus descriptors over "one per binding", summed across EVERY arrayed binding + // whatever its kind - storage blocks, image arrays and sampler arrays all land here. + // One number for all of them because each container below is bounded by the same total. + Uint32 arrayDescriptorExtra = 0; for (const Uint16 count : programObj.bindingDescriptorCounts) { - if (count > 1) ssboArrayExtra += static_cast(count) - 1u; + if (count > 1) arrayDescriptorExtra += static_cast(count) - 1u; } writes.reserve(m_maxBindings); - bufferInfos.reserve(m_maxBindings + uboArrayExtra + ssboArrayExtra); - // ssboArrayExtra sums EVERY arrayed binding's surplus, image arrays included, so it is - // the right worst case for this container too now that a storage-image binding pushes - // one info per element. Reserving only m_maxBindings here was exact while every binding - // pushed exactly one - and would have let the vector reallocate under an image array, - // dangling every pImageInfo already recorded in `writes` (including the sampler - // branch's &imageInfos.back()) before vkUpdateDescriptorSets reads them. - imageInfos.reserve(m_maxBindings + ssboArrayExtra); + bufferInfos.reserve(m_maxBindings + uboArrayExtra + arrayDescriptorExtra); + // Every binding pushes at most descriptorCount image infos, so bindings + surplus is the + // worst case. Reserving only m_maxBindings here was exact while every binding pushed + // exactly one - and reallocates under an image or sampler array, dangling every + // pImageInfo already recorded in `writes` before vkUpdateDescriptorSets reads them. That + // is reachable wherever m_maxBindings is small (it clamps to ~16 on Adreno and Mali), + // which is exactly where a 7-element CTS sampler array does not fit the slack. + imageInfos.reserve(m_maxBindings + arrayDescriptorExtra); texelBufferViews.reserve(m_maxBindings); dynamicOffsets.reserve(programObj.dynamicBindings.size() + uboArrayExtra); @@ -1477,10 +1562,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { write.descriptorCount = 1; if (kind == ProgramFactory::DescriptorBindingKind::UniformBufferDynamic) { - const Uint32 descriptorCount = - binding < programObj.bindingDescriptorCounts.size() - ? std::max(1, programObj.bindingDescriptorCounts[binding]) - : 1u; + const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); dynamicUboDescriptorCount += descriptorCount; fastRebindUboBinding = binding; const SizeT firstBufferInfoIndex = bufferInfos.size(); @@ -1523,10 +1605,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // One write per binding, but `descriptorCount` buffer infos: a GLSL block // instance array occupies a single binding whose elements each come from their // own GL binding point. - const Uint32 descriptorCount = - binding < programObj.bindingDescriptorCounts.size() - ? std::max(1, programObj.bindingDescriptorCounts[binding]) - : 1u; + const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); const SizeT firstBufferInfoIndex = bufferInfos.size(); for (Uint32 element = 0; element < descriptorCount; ++element) { VkDescriptorBufferInfo bufferInfo{}; @@ -1552,10 +1631,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // never written at all, and a shader that indexes them reads an undefined // descriptor (lavapipe faults inside the shader; a real driver is free to do // anything). - const Uint32 descriptorCount = - binding < programObj.bindingDescriptorCounts.size() - ? std::max(1, programObj.bindingDescriptorCounts[binding]) - : 1u; + const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); const SizeT firstImageInfoIndex = imageInfos.size(); for (Uint32 element = 0; element < descriptorCount; ++element) { VkDescriptorImageInfo imageInfo{}; @@ -1575,32 +1651,60 @@ namespace MobileGL::MG_Backend::DirectVulkan { write.pImageInfo = &imageInfos[firstImageInfoIndex]; writes.push_back(write); } else { - VkDescriptorImageInfo imageInfo{}; - Bool hasImage = false; - if (samplerBindingOverride != nullptr && - samplerBindingOverride->binding == binding && - samplerBindingOverride->texture != nullptr && - samplerBindingOverride->sampler != nullptr) { - hasImage = ResolveSamplerDescriptorOverride(*samplerBindingOverride, imageInfo); - } else { - hasImage = ResolveSamplerDescriptor(commandBuffer, program, programObj, binding, imageInfo, - samplerDescriptorsUnchangedHint); + // One write per binding, but `descriptorCount` image infos: a sampler ARRAY is a + // single binding whose elements each carry their own texture unit. Writing only + // element 0 - which is all this used to do - left elements 1..N never written, + // so a shader indexing them sampled a descriptor nobody had filled in + // (KHR-GL42.shading_language_420pack.binding_sampler_array). + const Uint32 descriptorCount = BindingDescriptorCount(programObj, binding); + // Overrides come only from MobileGL's own blit and depth-mipmap programs, whose + // samplers are scalars; the override replaces THE descriptor at its binding, so + // there is no element for it to mean on an arrayed one. + const Bool overrideThisBinding = samplerBindingOverride != nullptr && + samplerBindingOverride->binding == binding && + samplerBindingOverride->texture != nullptr && + samplerBindingOverride->sampler != nullptr; + MOBILEGL_ASSERT( + !overrideThisBinding || descriptorCount == 1, + "BindProgramUniformBuffers: sampler override targets arrayed binding %u (%u descriptors)", + binding, descriptorCount); + const SizeT firstImageInfoIndex = imageInfos.size(); + for (Uint32 element = 0; element < descriptorCount; ++element) { + VkDescriptorImageInfo imageInfo{}; + Bool hasImage = false; + if (overrideThisBinding && element == 0) { + hasImage = ResolveSamplerDescriptorOverride(*samplerBindingOverride, imageInfo); + } else { + hasImage = ResolveSamplerDescriptor(commandBuffer, program, programObj, binding, element, + imageInfo, samplerDescriptorsUnchangedHint); + } + if (!hasImage) { + MGLOG_E( + "UniformDescriptorBinder::BindProgramUniformBuffers failed: sampler binding %u element %u " + "has no valid texture descriptor", + binding, element); + return false; + } + if (imageInfo.sampler == VK_NULL_HANDLE || imageInfo.imageView == VK_NULL_HANDLE) { + MGLOG_E( + "UniformDescriptorBinder::BindProgramUniformBuffers failed: sampler binding %u element %u " + "has null sampler or imageView", + binding, element); + return false; + } + imageInfos.push_back(imageInfo); } - if (!hasImage) { - MGLOG_E( - "UniformDescriptorBinder::BindProgramUniformBuffers failed: sampler binding %u has no valid texture descriptor", - binding); - return false; + if (descriptorCount > 1) { + // The dynamic-offset-only rebind replays a whole descriptor set on the + // strength of the sampler hint alone, and its eligibility probe was written + // for bindings that carry one descriptor each. An arrayed sampler binding + // also bypasses the per-binding descriptor memo, so there is nothing for it + // to win here either. + fastRebindKindsEligible = false; } - if (imageInfo.sampler == VK_NULL_HANDLE || imageInfo.imageView == VK_NULL_HANDLE) { - MGLOG_E( - "UniformDescriptorBinder::BindProgramUniformBuffers failed: sampler binding %u has null sampler or imageView", - binding); - return false; - } - imageInfos.push_back(imageInfo); write.descriptorType = VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER; - write.pImageInfo = &imageInfos.back(); + write.descriptorCount = descriptorCount; + write.pImageInfo = &imageInfos[firstImageInfoIndex]; writes.push_back(write); } } diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h index 8bbbf633..21233772 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h @@ -53,10 +53,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { // caches - a live layout's entry must never be purged (its sets would be // unreachable pool slots), so there is deliberately no age-based sweep here. void OnDescriptorSetLayoutDestroyed(VkDescriptorSetLayout descriptorSetLayout); - // One record per visited CombinedImageSampler binding (post fallback substitution, - // in binding order): the resolved texture and effective sampler, as never-reused - // lifetime ids so a freed-and-reallocated object at the same heap address can only - // MISS a comparison, never false-hit it (same ABA rule as SamplerResolveMemo). + // One record per visited CombinedImageSampler DESCRIPTOR (post fallback substitution, + // in binding order, and within a binding in array-element order): the resolved texture + // and effective sampler, as never-reused lifetime ids so a freed-and-reallocated object + // at the same heap address can only MISS a comparison, never false-hit it (same ABA + // rule as SamplerResolveMemo). An arrayed binding contributes one record per element - + // element granularity is required, or swapping the textures of two elements of the same + // array would leave the record list identical and the fast path would keep a stale set. struct SampledBindingRecord { Uint64 textureLifetimeId = 0; Uint64 samplerLifetimeId = 0; @@ -143,8 +146,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { // texture after the fallback substitution (may still be null when no fallback // exists), effective sampler = unit override else the texture's own sampler. // False = the binding is skipped (unbound with a non-2D fallback target). + // `element` indexes a sampler array inside the binding; see ResolveSamplerDescriptor. Bool ResolveSampledBinding(const MG_State::GLState::ProgramObject& program, - const ProgramFactory::VkProgramObject& programObj, Uint32 binding, + const ProgramFactory::VkProgramObject& programObj, Uint32 binding, Uint32 element, MG_State::GLState::ITextureObject*& outTexture, const MG_State::GLState::SamplerObject*& outSampler) const; // Raw-pointer variant for the per-draw sampled-texture walk (CollectSampledTextures): @@ -152,14 +156,19 @@ namespace MobileGL::MG_Backend::DirectVulkan { // only need the pointer skip the SharedPtr copy's atomic refcount churn. static MG_State::GLState::ITextureObject* ResolveSamplerTextureRaw( const MG_State::GLState::ProgramObject& program, - const ProgramFactory::VkProgramObject& programObj, Uint32 binding); + const ProgramFactory::VkProgramObject& programObj, Uint32 binding, Uint32 element); SharedPtr GetFallbackTexture(TextureTarget target) const; + // `element` indexes a sampler ARRAY inside one binding; each element carries its own + // independently assigned GL texture unit, so it selects the texture, the sampler + // override and the fallback separately from its neighbours. + // // trustUnchangedHint: reuse this binding's cached VkDescriptorImageInfo outright // (see BindProgramUniformBuffers' samplerDescriptorsUnchangedHint for the proof - // obligations the caller carries). + // obligations the caller carries). The cache is keyed by binding alone, so it is + // used ONLY for single-descriptor bindings - see m_samplerResolveMemo. Bool ResolveSamplerDescriptor(VkCommandBuffer commandBuffer, const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj, Uint32 binding, - VkDescriptorImageInfo& outImageInfo, + Uint32 element, VkDescriptorImageInfo& outImageInfo, Bool trustUnchangedHint = false) const; Bool ResolveSamplerDescriptorOverride(const SamplerBindingOverride& samplerBindingOverride, VkDescriptorImageInfo& outImageInfo) const; @@ -346,6 +355,14 @@ namespace MobileGL::MG_Backend::DirectVulkan { // proves every resolve input unchanged; cleared with the per-frame reset // (the cached VkSampler outlives a frame only via a fresh resolve, which // also re-stamps it against VkSamplerManager's frame-boundary sweep). + // + // This one field is keyed by binding but describes ONE descriptor, so it is + // written and read only for single-descriptor bindings. A sampler ARRAY's + // elements share the binding and would overwrite each other here - the last + // element resolved would then be handed to element 0 on the next hinted draw. + // Every other field above is self-validating (each compares its full key + // before reuse, and the view-format entry is a pure function of format and + // numeric domain), so an arrayed binding may keep using those. VkDescriptorImageInfo info{}; Bool infoValid = false; }; diff --git a/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp index 3412396e..db1b24fa 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp @@ -233,19 +233,6 @@ void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); } return image.At(gl.Width() / 2, gl.Height() / 2); } - // Magma turns a sampler array into ONE descriptor with descriptorCount = N, and - // ProgramFactory::ReflectLayout refuses any descriptor array that is not a - // dynamic UBO (MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp - "descriptor - // arrays are unsupported for this descriptor kind"), so program creation fails - // and the draw samples descriptors that were never written. On a hardware driver - // that reads back as wrong pixels; under lavapipe it is a segfault in the - // rasterizer thread. Supporting it means carrying an element dimension through - // UniformManager's per-binding location tables, which is a feature, not a fix - - // so this case is SCOPED rather than disabled, because the frontend half it also - // covers (the seeded units) is real on both backends and is asserted below - // before the draw. - bool SamplerArrayDescriptorsAreSupported() 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 @@ -300,11 +287,6 @@ void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); } } glUseProgram(0); - if (!SamplerArrayDescriptorsAreSupported()) { - GTEST_SKIP() << "sampler descriptor arrays are unimplemented on " << Gl().BackendName() - << "; the seeded units above are the half of this case it can answer"; - } - const Rgba8 centre = DrawAndRead(program); EXPECT_EQ(FirstGLError(), 0u); EXPECT_EQ(centre.r, 0) << "sampler array elements that read the wrong texture: " << BadElements(centre.r);