swung0x48/MobileGL
1
0
Fork 0
mirror of https://github.com/MobileGL-Dev/MobileGL synced 2026-09-08 20:28:32 +09:00
Code Issues Packages Projects Releases Wiki Activity

[Fix, Test] (MG_Backend/DirectVulkan, MG_IntegrationTest): a declined descriptor must refuse the draw, not just leave the layout - a multi-dimensional sampler array otherwise faulted in the shader

Browse Source
This commit is contained in:
swung0x48
2026-08-12 00:45:36 -04:00
parent d2a36d65a3
commit 068786e812
4 changed files with 173 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp
+33 -17
View File
@@ -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<Uint32>(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<Int>(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<Uint16>(std::min<Uint32>(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<Uint16>(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<Uint16>(samplerArrayCount);
const SamplerNumericDomain numericDomain = UniformTypeToSamplerNumericDomain(uniformType);
MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h
+15 -2
View File
@@ -86,8 +86,9 @@ namespace MobileGL::MG_Backend::DirectVulkan {
Vector<Uint32> activeBindings;
Vector<Uint32> dynamicBindings;
Vector<Int> 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<Uint16> 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<GLenum, kMaxVertexInputLocations> 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;
MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp
+31
View File
@@ -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<Uint32>(m_maxBindings, static_cast<Uint32>(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<SampledBindingRecord>& 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<Uint32>(m_maxBindings, static_cast<Uint32>(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");
MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp
+94
View File
@@ -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) {