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

[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

Browse Source
This commit is contained in:
swung0x48
2026-08-12 01:09:56 -04:00
parent 5cfbb716c0
commit 97facf777b
3 changed files with 92 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp
+46 -5
View File
@@ -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<Uint16>(std::max<Uint32>(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<Uint32>(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<Uint16>(m_maxBindings);
continue;
}
entry.bindingDescriptorCounts[binding] = static_cast<Uint16>(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;
}
MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h
+12 -7
View File
@@ -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;
MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp
+34 -22
View File
@@ -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");