From 574634adfac652bd3783a7e0f2d81f276f1f37b9 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 00:14:18 -0400 Subject: [PATCH] [Fix, Test] (MG_Backend/DirectVulkan, MG_IntegrationTest): let the binding remap accept the descriptor arrays that now have per-element paths, and reserve image-info scratch for them --- .../DirectVulkan/Renderer/ProgramFactory.cpp | 38 +++++- .../DirectVulkan/Renderer/UniformManager.cpp | 8 +- .../Scenarios/ImageLoadStoreSsoScenario.cpp | 119 ++++++++++++++++++ .../Scenarios/SsboDeclarationFormScenario.cpp | 18 +-- 4 files changed, 169 insertions(+), 14 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index 0526842f..1a59383a 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -1834,10 +1834,25 @@ namespace MobileGL::MG_Backend::DirectVulkan { for (auto* binding : bindings) { MOBILEGL_ASSERT(binding != nullptr, "ProgramFactory: null descriptor binding reflection record"); const auto kind = ReflectDescriptorTypeToBindingKind(binding->descriptor_type); - // UBO instance arrays (uniform Block {...} b[N];) occupy one binding with - // descriptorCount = N; other descriptor arrays stay unsupported and must - // fail program creation cleanly rather than continue with corrupt state. - if (binding->count != 1 && kind != ProgramFactory::DescriptorBindingKind::UniformBufferDynamic) { + // 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. + // + // Getting listed here is not cosmetic: a kind that is rejected leaves + // GetOrCreateProgram's MOBILEGL_ASSERT(remapOk) as the only complaint, and + // that assert compiles out above DEBUG - so a release build SILENTLY kept + // glslang's per-stage auto-mapped binding numbers, skipping the cross-stage + // 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. + const Bool arraySupportedForKind = + kind == ProgramFactory::DescriptorBindingKind::UniformBufferDynamic || + kind == ProgramFactory::DescriptorBindingKind::StorageBuffer || + kind == ProgramFactory::DescriptorBindingKind::StorageImage; + if (binding->count != 1 && !arraySupportedForKind) { MGLOG_E("ProgramFactory: descriptor arrays are unsupported for this descriptor " "kind (name='%s' count=%u type=%d)", binding->name ? binding->name : "", binding->count, @@ -2609,8 +2624,19 @@ 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. - entry.bindingDescriptorCounts[binding] = - static_cast(std::max(1u, sampler->count)); + // 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); + entry.bindingKinds[binding] = DescriptorBindingKind::None; + continue; + } + entry.bindingDescriptorCounts[binding] = static_cast(imageArrayCount); const VkFormat reflectedFormat = ConvertSpirvImageFormatToVkFormat(sampler->image.image_format); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index fc043eaa..c7f3168e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -1443,7 +1443,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { } writes.reserve(m_maxBindings); bufferInfos.reserve(m_maxBindings + uboArrayExtra + ssboArrayExtra); - imageInfos.reserve(m_maxBindings); + // 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); texelBufferViews.reserve(m_maxBindings); dynamicOffsets.reserve(programObj.dynamicBindings.size() + uboArrayExtra); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp index a5d1c2b1..d78426cc 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp @@ -264,6 +264,125 @@ void main() gl.EndFrame(); } + // An image ARRAY sharing a program with another descriptor, which is the shape that makes + // the SPIR-V binding remap load-bearing. + // + // The remap (ProgramFactory::RemapDescriptorBindingsForVulkan) is what unifies bindings + // across stages and normalises every descriptor onto set 0; glslang hands it per-stage + // numbering that starts at 0 in EACH stage. It used to refuse any descriptor array that was + // not a UBO, and its only complaint was an assert that compiles out above DEBUG - so a + // release build carried on with the un-remapped numbering and a program holding an image + // array plus a second descriptor could see the two alias onto one binding, while a DEBUG + // build trapped on the very same program. + // + // A case with ONE descriptor cannot see any of that: with a single resource there is nothing + // to collide with and skipping the remap is indistinguishable from running it. Hence this + // one - an image array AND a uniform block in the same fragment program, with the block + // supplying the value that gets stored, so a mis-assigned binding shows up as the wrong + // colour rather than as nothing at all. + TEST_F(ImageLoadStoreSsoScenario, AnImageArrayAlongsideAnotherDescriptorKeepsBothBindings) { + if (!Ready()) return; + if (!ImagesAreUsable()) GTEST_SKIP() << "fewer than 8 image units"; + if (!PerElementImageUnitsAreHonoured()) { + GTEST_SKIP() << "non-consecutive per-element image units cannot be baked into ESSL"; + } + HeadlessGL& gl = Gl(); + + constexpr int kWidth = 8; + constexpr int kHeight = 8; + constexpr int kLayers = 2; + + static const char* kMixedFS = R"(#version 420 core +layout(rgba32f) uniform image2D g_image[2]; +layout(std140) uniform Value { vec4 u_value; }; +void main() +{ + for (int i = 0; i < g_image.length(); ++i) { + imageStore(g_image[i], ivec2(gl_FragCoord), u_value); + } + discard; +} +)"; + const GLuint vs = MakeSeparable(GL_VERTEX_SHADER, kSsoVS); + const GLuint fs = MakeSeparable(GL_FRAGMENT_SHADER, kMixedFS); + if (vs == 0 || fs == 0) return; + + // Consecutive units here on purpose: this case is about the two descriptor KINDS + // coexisting, not about non-consecutive assignment, which the case above covers. + for (int i = 0; i < 2; ++i) { + const std::string name = "g_image[" + std::to_string(i) + "]"; + const GLint loc = glGetUniformLocation(fs, name.c_str()); + ASSERT_NE(loc, -1) << "no location for " << name; + glProgramUniform1i(fs, loc, i); + } + + const GLfloat value[4] = {7.0f, 7.0f, 7.0f, 7.0f}; + GLuint ubo = 0; + glGenBuffers(1, &ubo); + glBindBuffer(GL_UNIFORM_BUFFER, ubo); + glBufferData(GL_UNIFORM_BUFFER, sizeof(value), value, GL_STATIC_DRAW); + const GLuint blockIndex = glGetUniformBlockIndex(fs, "Value"); + ASSERT_NE(blockIndex, GL_INVALID_INDEX); + glUniformBlockBinding(fs, blockIndex, 0); + glBindBufferBase(GL_UNIFORM_BUFFER, 0, ubo); + glBindBuffer(GL_UNIFORM_BUFFER, 0); + ASSERT_EQ(FirstGLError(), 0u) << "uniform block setup errored"; + + const GLuint pipeline = MakePipeline(); + glUseProgramStages(pipeline, GL_VERTEX_SHADER_BIT, vs); + glUseProgramStages(pipeline, GL_FRAGMENT_SHADER_BIT, fs); + + GLuint texture = 0; + glGenTextures(1, &texture); + glBindTexture(GL_TEXTURE_2D_ARRAY, texture); + glTexParameteri(GL_TEXTURE_2D_ARRAY, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D_ARRAY, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + const std::vector zeros(static_cast(kWidth) * kHeight * kLayers * 4, 0.0f); + glTexImage3D(GL_TEXTURE_2D_ARRAY, 0, GL_RGBA32F, kWidth, kHeight, kLayers, 0, GL_RGBA, GL_FLOAT, zeros.data()); + glBindImageTexture(0, texture, 0, GL_FALSE, 0, GL_READ_WRITE, GL_RGBA32F); + glBindImageTexture(1, texture, 0, GL_FALSE, 1, GL_READ_WRITE, GL_RGBA32F); + ASSERT_EQ(FirstGLError(), 0u) << "image texture setup errored"; + + GLuint vao = 0; + glGenVertexArrays(1, &vao); + glBindVertexArray(vao); + BindDefaultFramebuffer(); + glViewport(0, 0, kWidth, kHeight); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + glUseProgram(0); + glBindProgramPipeline(pipeline); + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); + glMemoryBarrier(GL_TEXTURE_UPDATE_BARRIER_BIT | GL_SHADER_IMAGE_ACCESS_BARRIER_BIT); + EXPECT_EQ(FirstGLError(), 0u) << "the mixed-descriptor pipeline draw leaked a GL error"; + + std::vector readback(static_cast(kWidth) * kHeight * kLayers * 4, -1.0f); + glBindTexture(GL_TEXTURE_2D_ARRAY, texture); + glGetTexImage(GL_TEXTURE_2D_ARRAY, 0, GL_RGBA, GL_FLOAT, readback.data()); + ASSERT_EQ(FirstGLError(), 0u) << "reading the array texture back errored"; + + for (int layer = 0; layer < kLayers; ++layer) { + int offenders = 0; + float firstSeen = 0.0f; + for (size_t i = 0; i < static_cast(kWidth) * kHeight * 4; ++i) { + const size_t index = static_cast(layer) * kHeight * kWidth * 4 + i; + if (readback[index] != 7.0f) { + if (offenders == 0) firstSeen = readback[index]; + ++offenders; + } + } + EXPECT_EQ(offenders, 0) << "layer " << layer << ": " << offenders + << " components are not the uniform block's value; first was " << firstSeen + << " (an image-array binding and a uniform block did not both survive)"; + } + + glBindVertexArray(0); + glDeleteVertexArrays(1, &vao); + glDeleteTextures(1, &texture); + glDeleteBuffers(1, &ubo); + gl.EndFrame(); + } + // The same units, reassigned BETWEEN draws through the same pipeline. This is the half that // the composite cache key change put weight on: the composite object now survives a // glProgramUniform1i, so nothing rebuilds by accident and the new unit has to be carried by diff --git a/MobileGL/MG_IntegrationTest/Scenarios/SsboDeclarationFormScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/SsboDeclarationFormScenario.cpp index eccbf242..321f7514 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/SsboDeclarationFormScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/SsboDeclarationFormScenario.cpp @@ -260,17 +260,21 @@ void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); } // arrays. Nothing errors anywhere, which is why this reads as a silent drop. // // So the fix is neither of the two candidates this was opened on - it is not a descriptor - // that goes missing and not a name that fails a lookup (forms 0-5 cover the block-array and - // no-binding-qualifier shapes those hypotheses rest on, and all six pass on both backends). - // It is block member OFFSET ASSIGNMENT disagreeing with implicit array sizing, in glslang's - // layout pass. That is a shared-frontend change with the blast radius of every std140/std430 + // that goes missing and not a name that fails a lookup. Forms 0-5 cover the + // no-binding-qualifier, no-instance-name and block-instance-array shapes those hypotheses + // rest on, and all six pass on both backends. (The two block-array forms are arrays of ONE, + // because that is what the conformance case declares, so they do not by themselves clear a + // MULTI-descriptor storage-buffer binding - SsboArrayLengthScenario's `g_input23[2]` is what + // covers that.) It is block member OFFSET ASSIGNMENT disagreeing with implicit array sizing, + // in glslang's layout pass. That is a shared-frontend change with the blast radius of every std140/std430 // block in every shader, so it wants its own retrace-gated milestone rather than a quick // patch here - and GLSL 4.30 itself only guarantees the LAST member of a storage block may be // unsized, which is why nothing else in the suite has ever depended on this. // - // Kept as compiled, running, SKIPPED cases rather than deleted or commented out: the - // reproduction and the reflected layout above are the whole asset, and the diagnostic in - // RunForm prints the offsets the moment the skip is lifted. + // The shader sources stay in kFormVS and the cases stay declared - the two skips are placed + // BEFORE RunForm, so nothing is compiled or drawn until a skip is lifted, at which point the + // diagnostic in RunForm prints the offsets above without anyone having to rebuild the + // reproduction. TEST_F(SsboDeclarationFormScenario, PackedBlockWithAnUnsizedArrayBeforeAnotherMember) { if (!Ready()) return; if (!StorageBlocksInVertexStage()) GTEST_SKIP() << "no vertex-stage shader storage blocks";