From 2fced2241b8acd283b3e58a29c704d6a072d509a Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 05:36:00 -0400 Subject: [PATCH] [Fix, Test] (MG_Util, MG_State, MG_Backend): the GL 4.3 vertex binding model - array vertex inputs, zero binding strides, instance divisors, and formats ES refuses --- CMakeLists.txt | 1 + MobileGL/MG_Backend/DirectGLES/Managers.cpp | 145 +++++- .../Renderer/VertexInputStateFactory.cpp | 24 +- .../DirectVulkan/Renderer/VulkanRenderer.cpp | 23 +- .../GLImpl/VertexArray/GL_VertexArray.cpp | 11 +- MobileGL/MG_IntegrationTest/CMakeLists.txt | 1 + .../Scenarios/VertexAttribBindingScenario.cpp | 465 ++++++++++++++++++ .../VertexArrayState/VertexArrayObject.cpp | 10 +- .../VertexArrayState/VertexArrayObject.h | 14 +- .../ShaderTranspiler/ShaderCompiler.cpp | 11 + .../MG_Util/ShaderTranspiler/ShaderCompiler.h | 7 + .../SplitArrayVertexInputsPass.cpp | 394 +++++++++++++++ .../SpirvPasses/SplitArrayVertexInputsPass.h | 52 ++ 13 files changed, 1129 insertions(+), 29 deletions(-) create mode 100644 MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp create mode 100644 MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.cpp create mode 100644 MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 56cc9d12..b629c204 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -278,6 +278,7 @@ set(SOURCE_FILES MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/DecoratePositionInvariantPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LowerDrawParametersPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/PackDoubleVertexInputsPass.cpp + MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/RebaseInstanceIndexPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/ZeroBaseVertexPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/NormalizeRectCoordinatesPass.cpp diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index fb01c6c4..a50882e1 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -1481,6 +1481,50 @@ namespace MobileGL::MG_Backend::DirectGLES { return true; } + // ES 3.1 core. Queried through the loader rather than the version, because the whole + // point of using it is to express something the pointer API cannot, and falling back + // silently on a driver that lacks it is better than crashing on a null entry point. + inline Bool HasVertexBindingApi() { + return g_GLESFuncs.glBindVertexBuffer != nullptr && g_GLESFuncs.glVertexAttribFormat != nullptr && + g_GLESFuncs.glVertexAttribIFormat != nullptr && g_GLESFuncs.glVertexAttribBinding != nullptr && + g_GLESFuncs.glVertexBindingDivisor != nullptr; + } + + // Declares one attribute through the ES binding-point API, the only spelling that can + // carry a stride of zero. Returns false when the attribute has no usable buffer, in + // which case nothing was emitted. + inline Bool SyncZeroStrideAttribute(Uint attribIndex, const MG_State::GLState::VertexAttribute& attrib) { + const auto& bufferObject = attrib.Buffer; + if (!bufferObject) { + MGLOG_W("Zero-stride attribute %u has no bound buffer, skipping.", attribIndex); + return false; + } + auto* backendResource = BufferImpl::EnsureBufferResource(bufferObject); + if (!backendResource || backendResource->id == 0) { + MGLOG_E("No backend buffer for zero-stride attribute %u, cannot bind it.", attribIndex); + return false; + } + + if (!attrib.IsInteger) { + const GLint glSize = attrib.IsBgra ? static_cast(GL_BGRA) : attrib.Size; + g_GLESFuncs.glVertexAttribFormat(attribIndex, glSize, + MG_Util::ConvertDataTypeToGLEnum(attrib.Type), + attrib.Normalized ? GL_TRUE : GL_FALSE, 0); + } else { + g_GLESFuncs.glVertexAttribIFormat(attribIndex, attrib.Size, + MG_Util::ConvertDataTypeToGLEnum(attrib.Type), 0); + } + g_GLESFuncs.glVertexAttribBinding(attribIndex, attribIndex); + // The resolved offset goes on the binding point, not into a relative offset: the + // relative offset is capped by GL_MAX_VERTEX_ATTRIB_RELATIVE_OFFSET (2047 at + // minimum) while a buffer offset is not, so anything else would break on a large + // one. BindBufferId is bypassed deliberately - glBindVertexBuffer binds into the + // VAO's binding point, not the GL_ARRAY_BUFFER target that cache tracks. + g_GLESFuncs.glBindVertexBuffer(attribIndex, backendResource->id, + static_cast(attrib.Offset), 0); + return true; + } + void BackendVertexArrayObject::SyncToBackend( const SharedPtr& stateVAOObject) { #ifdef TRACY_ENABLE @@ -1537,18 +1581,66 @@ namespace MobileGL::MG_Backend::DirectGLES { // FormatVersion, not SwitchVersion, so the enable/disable block above will not run // again and an already-enabled array would stay enabled with no pointer and no // ARRAY_BUFFER binding - which ES 3.1+ makes an INVALID_OPERATION at draw. - if (attrib.IsLong) { - MGLOG_E("DirectGLES: vertex attribute %u is a 64-bit (GL_DOUBLE) array, which this " + // + // IsLong is not the only way a 64-bit array gets here: glVertexAttribFormat + // with GL_DOUBLE asks for doubles in memory CONVERTED to float, so it is not + // long, is not declined by the frontend, and still has no ES vertex format. + // Leaving that one enabled did not merely raise INVALID_ENUM - the Adreno + // driver dereferenced null inside the next draw and took the process with it + // (SIGSEGV in libGLESv2_adreno, KHR-GL43.vertex_attrib_binding.basic-input-case4), + // because the array stayed enabled with no pointer the failed call could set. + // The type test therefore covers the storage, not the spelling. + if (attrib.IsLong || attrib.Type == DataType::Float64) { + MGLOG_I("DirectGLES: vertex attribute %u is a 64-bit (GL_DOUBLE) array, which this " "backend cannot feed - disabling the array", attribIndex); g_GLESFuncs.glDisableVertexAttribArray(attribIndex); continue; } + // A resolved stride of zero is the binding model's "never advance" (see + // VertexAttribute::Stride) and glVertexAttribPointer cannot say it - a zero + // stride argument there means "tightly packed" instead, i.e. exactly the + // opposite. ES 3.1's binding-point API can, so a zero-stride attribute takes + // that spelling: its own binding point (index == attribute index, the default + // mapping) carrying the buffer, the whole resolved offset and stride 0, with + // the format at relative offset 0. Everything the pointer call would have set + // for this attribute is set here too, so the two spellings stay interchangeable + // from one sync to the next. + if (attrib.Stride == 0 && HasVertexBindingApi()) { + if (!SyncZeroStrideAttribute(attribIndex, attrib)) { + continue; + } + if (needsSyncFormat) { + g_GLESFuncs.glVertexBindingDivisor(attribIndex, attrib.Divisor); + } + continue; + } + if (!BindAttributeBuffer(attrib)) { continue; } + // GL_BGRA as a vertex SIZE is desktop-only; ES has no equivalent and rejects + // it. That rejection is not benign: it leaves the array ENABLED with no + // pointer, and the Adreno driver then dereferences null inside the next draw + // and kills the process rather than reporting an error (SIGSEGV in + // libGLESv2_adreno, KHR-GL43.vertex_attrib_binding.basic-input-case5). So the + // refusal has to be observed and the array disabled. + // + // Deliberately ONLY this format. Everything else MobileGL can reach here is ES + // core - the packed 2_10_10_10 pair included, whose size the frontend has + // already pinned to the 4 that ES requires - so nothing else can be refused, + // and the per-draw sync must not grow a glGetError round trip (a driver + // pipeline stall) for the formats real applications actually use. BGRA is also + // still ATTEMPTED rather than refused up front: some ES drivers do accept it, + // and the ones that do should keep working. + const Bool formatMayBeRefused = attrib.IsBgra; + if (formatMayBeRefused) { + while (g_GLESFuncs.glGetError() != GL_NO_ERROR) { + } // start from a clean slate so the check below is about THIS call + } + if (!attrib.IsInteger) { // GL_BGRA is passed to the driver as the size argument (the driver reorders BGRA). const GLint glSize = attrib.IsBgra ? static_cast(GL_BGRA) : attrib.Size; @@ -1561,6 +1653,16 @@ namespace MobileGL::MG_Backend::DirectGLES { (const void*)attrib.Offset); } + if (formatMayBeRefused && g_GLESFuncs.glGetError() != GL_NO_ERROR) { + MGLOG_I("DirectGLES: the driver refused the vertex format of attribute %u " + "(size=%d bgra=%d type=%s) - disabling the array so the draw cannot " + "fetch through a pointer the driver never accepted", + attribIndex, attrib.Size, attrib.IsBgra ? 1 : 0, + MG_Util::ConvertGLEnumToString(MG_Util::ConvertDataTypeToGLEnum(attrib.Type)).c_str()); + g_GLESFuncs.glDisableVertexAttribArray(attribIndex); + continue; + } + if (needsSyncFormat) { g_GLESFuncs.glVertexAttribDivisor(attribIndex, attrib.Divisor); } @@ -1609,9 +1711,10 @@ namespace MobileGL::MG_Backend::DirectGLES { continue; } - // Same reason as SyncToBackend: there is no ES vertex format for a 64-bit array, and - // this path only ever reaches glVertexAttribPointer/IPointer. - if (attrib.IsLong) { + // Same reason as SyncToBackend, including why the test is on the storage rather + // than on IsLong: there is no ES vertex format for a 64-bit array, and this path + // only ever reaches glVertexAttribPointer/IPointer. + if (attrib.IsLong || attrib.Type == DataType::Float64) { g_GLESFuncs.glDisableVertexAttribArray(attribIndex); continue; } @@ -4366,6 +4469,21 @@ namespace MobileGL::MG_Backend::DirectGLES { effectiveSpirv = &loweredSpirv; } + // GLSL ES has no ARRAY vertex inputs, and SPIRV-Cross refuses the whole module + // rather than emulating them, so this has to happen before it sees the binary. + Vector splitArrayInputSpirv; + if (glShaderType == GL_VERTEX_SHADER && + MG_Util::ShaderTranspiler::ShaderCompiler::SplitArrayVertexInputsForEssl( + *effectiveSpirv, splitArrayInputSpirv) && + !splitArrayInputSpirv.empty() && splitArrayInputSpirv != *effectiveSpirv) { + // Only when the pass ACTUALLY split something. The optimizer hands back a + // re-serialised copy either way, and adopting that copy for every vertex + // shader would put every one of them through a round trip they do not need + // - which is not free: it cost the create-indirect retrace 0.15 SSIM the + // first time this gate was missing. + effectiveSpirv = &splitArrayInputSpirv; + } + // ESSL stage-matches uniform blocks by member precision, but SPIRV-Cross prints // a RelaxedPrecision member as explicit "mediump" in the vertex stage and as // UNQUALIFIED (mediump-by-default) in the fragment stage; after @@ -4447,11 +4565,18 @@ namespace MobileGL::MG_Backend::DirectGLES { spvcSession.Compile(&result); if (!result) { - MG_Util::ShaderTranspiler::ResultInfo r; - r.log += "Failed to compile the shader to GLSL: \n"; - r.log += spvcSession.GetLastErrorString(); - r.errc = -5; - MGLOG_E("%s", r.log.c_str()); + // MGLOG_I, for the same reason as the compile- and link-failure diagnostics + // below: every CI, retrace and release build compiles at + // MOBILEGL_LOG_LEVEL_INFO, where MGLOG_E expands to nothing. A stage that + // never reaches the driver leaves the program short of that stage, so the + // link fails with an EMPTY driver info log - the least debuggable failure + // MobileGL can produce, and what hid the whole + // KHR-GL43.vertex_attrib_binding family behind "the draw captured zeros". + MGLOG_I("Shader transpilation to ESSL failed. State program ID: %u, stage: %s, " + "SPIRV-Cross error: %s", + stateProgramObject->GetExternalIndex(), + MG_Util::ConvertGLEnumToString(glShaderType).c_str(), + spvcSession.GetLastErrorString()); m_backendProgramUsable = false; continue; } diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp index 968ac1a2..b55b06a0 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp @@ -153,8 +153,14 @@ namespace MobileGL::MG_Backend::DirectVulkan { continue; } - const Uint32 sourceStride = - attr.Stride > 0 ? static_cast(attr.Stride) : static_cast(attribByteSize); + // Verbatim, zero included. The frontend already resolved a pointer call's + // "tightly packed" stride 0 into the element size (see VertexAttribute::Stride), + // so a zero here is the binding model's stride 0 - every vertex reads the same + // element - which is exactly what a zero VkVertexInputBindingDescription::stride + // means. Substituting the element size fetched a fresh element per vertex and ran + // off the end of the buffer (KHR-GL43.vertex_attrib_binding.basic-input-case7/8). + // Client-memory arrays cannot reach zero: they only exist on the pointer path. + const Uint32 sourceStride = static_cast(attr.Stride); const Bool packedAttribute = attr.Type == DataType::Int2101010Rev || attr.Type == DataType::Uint2101010Rev; const SizeT requiredAlignment = packedAttribute ? attribByteSize : GetComponentSize(attr.Type); @@ -175,10 +181,16 @@ namespace MobileGL::MG_Backend::DirectVulkan { } Uint32 stride = sourceStride; - if (conversion == VertexStreamConversion::Repack) { - stride = static_cast(attribByteSize); - } else if (conversion == VertexStreamConversion::ScaledIntegerToFloat32) { - stride = static_cast(attr.Size * static_cast(sizeof(Float))); + // A converted stream is tightly packed, so its stride is the converted element + // size - unless the source stride is zero, which does not describe a packing at + // all but "never advance". That survives the conversion unchanged: the draw path + // converts exactly one element and every vertex reads it. + if (sourceStride != 0) { + if (conversion == VertexStreamConversion::Repack) { + stride = static_cast(attribByteSize); + } else if (conversion == VertexStreamConversion::ScaledIntegerToFloat32) { + stride = static_cast(attr.Size * static_cast(sizeof(Float))); + } } const VkVertexInputRate inputRate = (attr.Divisor == 0) ? VK_VERTEX_INPUT_RATE_VERTEX : VK_VERTEX_INPUT_RATE_INSTANCE; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 25363fa6..00394629 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -3546,9 +3546,11 @@ void main() { const auto& attr = vao.GetAttribute(bindingLocation); const SizeT elementSize = VertexInputStateFactory::GetAttributeByteSize(attr.Type, attr.Size, attr.IsBgra); - const SizeT sourceStride = - attr.Stride > 0 ? static_cast(attr.Stride) : elementSize; - if (sourceBufferShared->MappedData() == nullptr || elementSize == 0 || sourceStride == 0 || + // Zero is a legal binding stride and means "never advance" (see + // VertexAttribute::Stride), so it is NOT folded into the element size here - + // it selects the single-element conversion below instead. + const SizeT sourceStride = static_cast(attr.Stride); + if (sourceBufferShared->MappedData() == nullptr || elementSize == 0 || baseOffset > sourceSize || elementSize > sourceSize - baseOffset) { MGLOG_E("UploadAndBindVertexStreams skipped: invalid converted source binding=%zu " "location=%u base=%zu size=%zu element=%zu stride=%zu", @@ -3557,7 +3559,8 @@ void main() { } sourceBufferShared->SyncPersistentMappedRange(); - const SizeT availableElementCount = 1 + (sourceSize - baseOffset - elementSize) / sourceStride; + const SizeT availableElementCount = + sourceStride == 0 ? 1 : 1 + (sourceSize - baseOffset - elementSize) / sourceStride; const Bool cacheable = !sourceBufferShared->IsBackendPersistentMapped(); // Convert only what this draw can fetch instead of the whole buffer tail. // Instance-rate bindings index by instance, not the vertex range, so they @@ -4737,6 +4740,7 @@ void main() { hasPatchedVertexAttributes = true; } VertexInputStateBuilder syntheticVertexInputBuilder; + VkPipelineVertexInputStateCreateInfo syntheticVertexInputState{}; const VkPipelineVertexInputStateCreateInfo* pipelineVertexInputState = &vis.state; if (missingAttribMask != 0 || hasPatchedVertexAttributes) { for (const auto& binding : vis.bindings) { @@ -4764,7 +4768,16 @@ void main() { syntheticVertexInputBuilder.AddAttribute(location, syntheticBinding, format, 0); ++syntheticBinding; } - pipelineVertexInputState = &syntheticVertexInputBuilder.Build(); + syntheticVertexInputState = syntheticVertexInputBuilder.Build(); + // Carry the divisor chain over. The synthetic rebuild copies bindings and + // attributes only, and it keeps every real binding's INDEX, so the divisor + // descriptions built for them stay valid - but dropping the pNext silently + // demoted every instanced binding to divisor 1. This path runs whenever the + // program declares an input the VAO does not feed (which is most capture + // shaders: KHR-GL43.vertex_attrib_binding declares 16 inputs and enables three), + // so the loss was near-total rather than a corner case. + syntheticVertexInputState.pNext = vis.state.pNext; + pipelineVertexInputState = &syntheticVertexInputState; } auto cullFaceEnabled = MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::CullFace); auto depthTestEnabled = MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::DepthTest); diff --git a/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp b/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp index 92a938fc..f9b616e9 100644 --- a/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp +++ b/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp @@ -315,9 +315,10 @@ namespace MobileGL::MG_Impl::GLImpl { auto offset = reinterpret_cast(pointer); - vao->SetAttributeFormat(index, size, dataType, false, stride, offset, true, false); + const int effectiveStride = EffectiveVertexStride(stride, size, type); + vao->SetAttributeFormat(index, size, dataType, false, stride, offset, true, false, effectiveStride); vao->BindAttributeBuffer(index, vbo); - vao->MirrorPointerIntoBinding(index, vbo, offset, EffectiveVertexStride(stride, size, type)); + vao->MirrorPointerIntoBinding(index, vbo, offset, effectiveStride); } void VertexAttribPointer_State(GLuint index, GLint size, GLenum type, GLboolean normalized, GLsizei stride, @@ -345,9 +346,11 @@ namespace MobileGL::MG_Impl::GLImpl { // backend can pick the reversed VkFormat / pass GL_BGRA through to a GLES driver. const bool isBgra = (size == static_cast(GL_BGRA)); const int effectiveSize = isBgra ? 4 : size; - vao->SetAttributeFormat(index, effectiveSize, dataType, normalized, stride, offset, false, isBgra); + const int effectiveStride = EffectiveVertexStride(stride, effectiveSize, type); + vao->SetAttributeFormat(index, effectiveSize, dataType, normalized, stride, offset, false, isBgra, + effectiveStride); vao->BindAttributeBuffer(index, vbo); - vao->MirrorPointerIntoBinding(index, vbo, offset, EffectiveVertexStride(stride, effectiveSize, type)); + vao->MirrorPointerIntoBinding(index, vbo, offset, effectiveStride); } void BindVertexArray_State(GLuint array) { diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 47a18a73..bc17ff79 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -69,6 +69,7 @@ add_executable(MobileGLIntegrationTest Scenarios/Glsl420DeclarationScenario.cpp Scenarios/FragmentOutputArrayIndexScenario.cpp Scenarios/BufferTextureScenario.cpp + Scenarios/VertexAttribBindingScenario.cpp ) target_include_directories(MobileGLIntegrationTest PRIVATE diff --git a/MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp new file mode 100644 index 00000000..cd2ae3c3 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp @@ -0,0 +1,465 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// ARB_vertex_attrib_binding: the separate format/binding state the GL 4.3 vertex +// input model is made of, read back out of the draw that consumed it. +// +// Every scenario here captures the vertex shader's inputs with transform feedback +// under GL_RASTERIZER_DISCARD, which is what the KHR-GL43.vertex_attrib_binding +// cases do: the captured record IS the fetched vertex, so "the binding state did +// not reach the draw" and "the draw fetched the wrong bytes" are distinguishable +// from each other and from "the capture did not run" (the buffer is pre-filled +// with a poison value). + +#include +#include +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr float kPoison = -1234.0f; + + GLuint CompileShader(GLenum type, const std::string& source, std::string* log) { + const GLuint shader = glCreateShader(type); + const char* text = source.c_str(); + glShaderSource(shader, 1, &text, nullptr); + glCompileShader(shader); + GLint status = GL_FALSE; + glGetShaderiv(shader, GL_COMPILE_STATUS, &status); + if (status == GL_FALSE) { + GLint length = 0; + glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length); + std::vector buffer(static_cast(length) + 1, '\0'); + glGetShaderInfoLog(shader, length + 1, nullptr, buffer.data()); + if (log != nullptr) *log = buffer.data(); + glDeleteShader(shader); + return 0; + } + return shader; + } + + // A vertex-only capture program, exactly how the CTS builds one: the varying + // names are declared before the link and the fragment stage is absent because + // the draw runs under GL_RASTERIZER_DISCARD. + GLuint BuildCaptureProgram(const std::string& vertexSource, const std::vector& xfbVaryings, + std::string* log) { + const GLuint vertexShader = CompileShader(GL_VERTEX_SHADER, vertexSource, log); + if (vertexShader == 0) return 0; + const GLuint program = glCreateProgram(); + glAttachShader(program, vertexShader); + if (!xfbVaryings.empty()) { + glTransformFeedbackVaryings(program, static_cast(xfbVaryings.size()), xfbVaryings.data(), + GL_INTERLEAVED_ATTRIBS); + } + glLinkProgram(program); + glDeleteShader(vertexShader); + GLint status = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &status); + if (status == GL_FALSE) { + GLint length = 0; + glGetProgramiv(program, GL_INFO_LOG_LENGTH, &length); + std::vector buffer(static_cast(length) + 1, '\0'); + glGetProgramInfoLog(program, length + 1, nullptr, buffer.data()); + if (log != nullptr) *log = buffer.data(); + glDeleteProgram(program); + return 0; + } + return program; + } + + // Four float inputs at locations 0..3, captured as four vec4s per vertex. + // Locations the test does not feed keep their current-attribute value, which + // every scenario sets to a known constant first. + std::string CaptureVertexSource() { + return R"(#version 430 core +layout(location = 0) in vec4 vs_in_attrib0; +layout(location = 1) in vec4 vs_in_attrib1; +layout(location = 2) in vec4 vs_in_attrib2; +layout(location = 3) in vec4 vs_in_attrib3; +out StageData { + vec4 attrib0; + vec4 attrib1; + vec4 attrib2; + vec4 attrib3; +} vs_out; +void main() { + vs_out.attrib0 = vs_in_attrib0; + vs_out.attrib1 = vs_in_attrib1; + vs_out.attrib2 = vs_in_attrib2; + vs_out.attrib3 = vs_in_attrib3; +} +)"; + } + + std::vector CaptureVaryingNames() { + return {"StageData.attrib0", "StageData.attrib1", "StageData.attrib2", "StageData.attrib3"}; + } + + // Runs `vertexCount` x `instanceCount` points through the capture program and + // returns the interleaved floats (16 per point: four vec4s). + std::vector CapturePoints(GLuint program, GLuint xfbBuffer, int vertexCount, int instanceCount) { + const std::size_t floats = static_cast(vertexCount) * instanceCount * 16; + std::vector poison(floats, kPoison); + glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, xfbBuffer); + glBufferData(GL_TRANSFORM_FEEDBACK_BUFFER, static_cast(floats * sizeof(float)), poison.data(), + GL_DYNAMIC_DRAW); + + glEnable(GL_RASTERIZER_DISCARD); + glUseProgram(program); + glBeginTransformFeedback(GL_POINTS); + glDrawArraysInstanced(GL_POINTS, 0, vertexCount, instanceCount); + glEndTransformFeedback(); + glDisable(GL_RASTERIZER_DISCARD); + + std::vector data(floats, kPoison); + glGetBufferSubData(GL_TRANSFORM_FEEDBACK_BUFFER, 0, static_cast(floats * sizeof(float)), + data.data()); + glUseProgram(0); + return data; + } + + // point p, attribute a, component c + float At(const std::vector& data, int point, int attrib, int component) { + const std::size_t index = static_cast(point) * 16 + attrib * 4 + component; + return index < data.size() ? data[index] : kPoison; + } + + void ResetCurrentAttribs() { + for (GLuint i = 0; i < 4; ++i) { + glVertexAttrib4f(i, 0.0f, 0.0f, 0.0f, 0.0f); + } + } + + ::testing::AssertionResult Vec4Is(const std::vector& data, int point, int attrib, float x, float y, + float z, float w) { + const float gx = At(data, point, attrib, 0); + const float gy = At(data, point, attrib, 1); + const float gz = At(data, point, attrib, 2); + const float gw = At(data, point, attrib, 3); + const float tolerance = 0.01f; + auto close = [tolerance](float a, float b) { return (a - b) < tolerance && (b - a) < tolerance; }; + if (close(gx, x) && close(gy, y) && close(gz, z) && close(gw, w)) { + return ::testing::AssertionSuccess(); + } + return ::testing::AssertionFailure() + << "point " << point << " attribute " << attrib << " is (" << gx << ", " << gy << ", " << gz << ", " + << gw << "), expected (" << x << ", " << y << ", " << z << ", " << w << ")"; + } + + } // namespace + + class VertexAttribBindingScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + m_program = BuildCaptureProgram(CaptureVertexSource(), CaptureVaryingNames(), &m_log); + ASSERT_NE(m_program, 0u) << "capture program did not link: " << m_log; + glGenVertexArrays(1, &m_vao); + glGenBuffers(1, &m_xfbo); + glBindVertexArray(m_vao); + } + + void TearDown() override { + if (!Ready()) return; + glBindVertexArray(0); + glDeleteVertexArrays(1, &m_vao); + glDeleteBuffers(1, &m_xfbo); + glDeleteProgram(m_program); + } + + GLuint m_program = 0; + GLuint m_vao = 0; + GLuint m_xfbo = 0; + std::string m_log; + }; + + // glVertexAttribFormat + glBindVertexBuffer + glVertexAttribBinding, in the order + // the CTS uses (buffer first, then format, then binding), must feed the draw. + TEST_F(VertexAttribBindingScenario, FormatAndBindingFeedTheDraw) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const float vertices[] = {1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glBindVertexBuffer(0, vbo, 0, 12); + glVertexAttribFormat(1, 3, GL_FLOAT, GL_FALSE, 0); + glVertexAttribBinding(1, 0); + glEnableVertexAttribArray(1); + + const std::vector data = CapturePoints(m_program, m_xfbo, 2, 1); + EXPECT_TRUE(Vec4Is(data, 0, 1, 1.0f, 2.0f, 3.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 1, 4.0f, 5.0f, 6.0f, 1.0f)); + // An attribute nothing configured still reports its current value. + EXPECT_TRUE(Vec4Is(data, 0, 0, 0.0f, 0.0f, 0.0f, 0.0f)); + + glDisableVertexAttribArray(1); + glDeleteBuffers(1, &vbo); + } + + // The reverse order - format and binding declared before any buffer exists on the + // binding point - has to resolve to the same thing once glBindVertexBuffer lands. + TEST_F(VertexAttribBindingScenario, FormatBeforeBufferStillResolves) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const float vertices[] = {1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glVertexAttribBinding(2, 3); + glVertexAttribFormat(2, 2, GL_FLOAT, GL_FALSE, 4); + glEnableVertexAttribArray(2); + glBindVertexBuffer(3, vbo, 0, 12); + + const std::vector data = CapturePoints(m_program, m_xfbo, 2, 1); + EXPECT_TRUE(Vec4Is(data, 0, 2, 2.0f, 3.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 2, 5.0f, 6.0f, 0.0f, 1.0f)); + + glDisableVertexAttribArray(2); + glDeleteBuffers(1, &vbo); + } + + // GL 4.6 core 10.3.1: a binding point's stride is the byte distance between + // consecutive elements, and zero means every vertex reads the SAME element. That + // is the opposite of glVertexAttribPointer's stride 0, which means "tightly + // packed" - the two spellings must not be collapsed into one another. + TEST_F(VertexAttribBindingScenario, BindingStrideZeroRepeatsOneElement) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const float vertices[] = {1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f, 7.0f, 8.0f}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glVertexAttribFormat(0, 4, GL_FLOAT, GL_FALSE, 0); + glVertexAttribBinding(0, 5); + glBindVertexBuffer(5, vbo, 16, 0); + glEnableVertexAttribArray(0); + + const std::vector data = CapturePoints(m_program, m_xfbo, 2, 1); + EXPECT_TRUE(Vec4Is(data, 0, 0, 5.0f, 6.0f, 7.0f, 8.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 0, 5.0f, 6.0f, 7.0f, 8.0f)); + + glDisableVertexAttribArray(0); + glDeleteBuffers(1, &vbo); + } + + // The pointer API keeps its own meaning of stride 0 (tightly packed) even though + // it is defined in terms of the binding model - the negative control for the + // scenario above. + TEST_F(VertexAttribBindingScenario, PointerStrideZeroStaysTightlyPacked) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const float vertices[] = {1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f, 7.0f, 8.0f}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + + glVertexAttribPointer(0, 4, GL_FLOAT, GL_FALSE, 0, nullptr); + glEnableVertexAttribArray(0); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + const std::vector data = CapturePoints(m_program, m_xfbo, 2, 1); + EXPECT_TRUE(Vec4Is(data, 0, 0, 1.0f, 2.0f, 3.0f, 4.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 0, 5.0f, 6.0f, 7.0f, 8.0f)); + + glDisableVertexAttribArray(0); + glDeleteBuffers(1, &vbo); + } + + // glVertexBindingDivisor is per BINDING POINT: it has to reach every attribute + // pointed at that binding, and the instance step must honour the divisor rather + // than advancing once per instance. + TEST_F(VertexAttribBindingScenario, BindingDivisorAppliesToEveryAttributeOnThePoint) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const float vertices[] = {10.0f, 20.0f, 30.0f, 40.0f}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glVertexAttribFormat(0, 1, GL_FLOAT, GL_FALSE, 0); + glVertexAttribFormat(1, 1, GL_FLOAT, GL_FALSE, 4); + glVertexAttribBinding(0, 4); + glVertexAttribBinding(1, 4); + glBindVertexBuffer(4, vbo, 0, 8); + glVertexBindingDivisor(4, 2); + glEnableVertexAttribArray(0); + glEnableVertexAttribArray(1); + + // The divisor is per binding point, so it has to be visible on BOTH attributes + // pointed at it - and this query is what separates "the frontend never resolved + // it" from "the backend did not apply it". + GLint divisor = -1; + glGetVertexAttribiv(0, GL_VERTEX_ATTRIB_ARRAY_DIVISOR, &divisor); + EXPECT_EQ(divisor, 2); + divisor = -1; + glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_ARRAY_DIVISOR, &divisor); + EXPECT_EQ(divisor, 2); + + // 1 vertex x 4 instances, divisor 2: instances 0,1 read element 0 and + // instances 2,3 read element 1. + const std::vector data = CapturePoints(m_program, m_xfbo, 1, 4); + EXPECT_TRUE(Vec4Is(data, 0, 0, 10.0f, 0.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 0, 10.0f, 0.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 2, 0, 30.0f, 0.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 3, 0, 30.0f, 0.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 0, 1, 20.0f, 0.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 2, 1, 40.0f, 0.0f, 0.0f, 1.0f)); + + glDisableVertexAttribArray(0); + glDisableVertexAttribArray(1); + glDeleteBuffers(1, &vbo); + } + + // Two attributes on one binding point at different relative offsets, plus a + // binding offset: the fetch address is binding offset + relative offset, and the + // relative offset must not leak into the binding's own offset. + TEST_F(VertexAttribBindingScenario, RelativeOffsetComposesWithBindingOffset) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const float vertices[] = {0.0f, 0.0f, 1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glVertexAttribFormat(0, 2, GL_FLOAT, GL_FALSE, 0); + glVertexAttribFormat(1, 1, GL_FLOAT, GL_FALSE, 8); + glVertexAttribBinding(0, 1); + glVertexAttribBinding(1, 1); + glBindVertexBuffer(1, vbo, 8, 12); + glEnableVertexAttribArray(0); + glEnableVertexAttribArray(1); + + const std::vector data = CapturePoints(m_program, m_xfbo, 2, 1); + EXPECT_TRUE(Vec4Is(data, 0, 0, 1.0f, 2.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 0, 1, 3.0f, 0.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 0, 4.0f, 5.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 1, 6.0f, 0.0f, 0.0f, 1.0f)); + + glDisableVertexAttribArray(0); + glDisableVertexAttribArray(1); + glDeleteBuffers(1, &vbo); + } + + // The KHR-GL43.vertex_attrib_binding.basic-input* capture program verbatim: a + // 16-element vec4 input ARRAY at location 0, copied element by element into a + // 16-element array inside an output interface block, all 16 members captured. + // Every one of the 17 basic-input* cases is built on it, so a backend that cannot + // produce this program fails all of them with "the draw captured zeros" and no + // other symptom. + TEST_F(VertexAttribBindingScenario, InputArrayCaptureProgramFeedsTheDraw) { + if (!Ready()) GTEST_SKIP(); + + const std::string vs = R"(#version 430 core +layout(location = 0) in vec4 vs_in_attrib[16]; +out StageData { + vec4 attrib[16]; +} vs_out; +void main() { + for (int i = 0; i < vs_in_attrib.length(); ++i) { + vs_out.attrib[i] = vs_in_attrib[i]; + } +} +)"; + std::vector names; + for (int i = 0; i < 16; ++i) names.push_back("StageData.attrib[" + std::to_string(i) + "]"); + std::vector varyings; + for (const auto& n : names) varyings.push_back(n.c_str()); + + std::string log; + const GLuint program = BuildCaptureProgram(vs, varyings, &log); + ASSERT_NE(program, 0u) << "capture program did not link: " << log; + + for (GLuint i = 0; i < 16; ++i) glVertexAttrib4f(i, 0.0f, 0.0f, 0.0f, 0.0f); + + const float vertices[] = {1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glBindVertexBuffer(0, vbo, 0, 12); + glVertexAttribFormat(1, 3, GL_FLOAT, GL_FALSE, 0); + glVertexAttribBinding(1, 0); + glEnableVertexAttribArray(1); + + // 16 vec4s per point rather than the 4 the shared helper assumes. + constexpr std::size_t kFloatsPerPoint = 64; + std::vector poison(kFloatsPerPoint * 2, kPoison); + glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, m_xfbo); + glBufferData(GL_TRANSFORM_FEEDBACK_BUFFER, static_cast(poison.size() * sizeof(float)), + poison.data(), GL_DYNAMIC_DRAW); + glEnable(GL_RASTERIZER_DISCARD); + glUseProgram(program); + glBeginTransformFeedback(GL_POINTS); + glDrawArrays(GL_POINTS, 0, 2); + glEndTransformFeedback(); + glDisable(GL_RASTERIZER_DISCARD); + + std::vector data(poison.size(), kPoison); + glGetBufferSubData(GL_TRANSFORM_FEEDBACK_BUFFER, 0, + static_cast(data.size() * sizeof(float)), data.data()); + glUseProgram(0); + + // Element 0 of the array has no enabled array behind it, so it must deliver the + // current generic attribute value set above - including its w, which is 0 here and + // NOT the 1 an unwritten vec4 input defaults to. + EXPECT_FLOAT_EQ(data[0], 0.0f); + EXPECT_FLOAT_EQ(data[3], 0.0f); + + // attribute 1 of point 0 and of point 1. + EXPECT_FLOAT_EQ(data[4], 1.0f); + EXPECT_FLOAT_EQ(data[5], 2.0f); + EXPECT_FLOAT_EQ(data[6], 3.0f); + EXPECT_FLOAT_EQ(data[7], 1.0f); + EXPECT_FLOAT_EQ(data[kFloatsPerPoint + 4], 4.0f); + EXPECT_FLOAT_EQ(data[kFloatsPerPoint + 5], 5.0f); + EXPECT_FLOAT_EQ(data[kFloatsPerPoint + 6], 6.0f); + EXPECT_FLOAT_EQ(data[kFloatsPerPoint + 7], 1.0f); + + glDisableVertexAttribArray(1); + glDeleteBuffers(1, &vbo); + glDeleteProgram(program); + } + +} // namespace MGITest diff --git a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp index e61fb6d5..db784caf 100644 --- a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp +++ b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp @@ -61,12 +61,16 @@ namespace MobileGL::MG_State::GLState { } void VertexArrayObject::SetAttributeFormat(Uint index, int size, DataType type, Bool normalized, int stride, - SizeT offset, Bool isInteger, Bool isBgra) { + SizeT offset, Bool isInteger, Bool isBgra, int effectiveStride) { if (index >= MAX_VERTEX_ATTRIBS) return; if (size < 1 || size > 4) { return; } + // See VertexAttribute::Stride: the resolved field carries the effective stride so that + // a zero in it can only ever mean the binding model's "do not advance". + const int resolvedStride = effectiveStride >= 0 ? effectiveStride : stride; + // The classic pointer-style API takes back full ownership of the resolved fields. m_attributeUsesBindingModel[index] = false; @@ -77,7 +81,7 @@ namespace MobileGL::MG_State::GLState { m_attributes[index].LegacyPointer = offset; if (m_attributes[index].Size == size && m_attributes[index].Type == type && - m_attributes[index].Normalized == normalized && m_attributes[index].Stride == stride && + m_attributes[index].Normalized == normalized && m_attributes[index].Stride == resolvedStride && m_attributes[index].Offset == offset && m_attributes[index].IsInteger == isInteger && m_attributes[index].IsBgra == isBgra && !m_attributes[index].IsLong) { return; @@ -87,7 +91,7 @@ namespace MobileGL::MG_State::GLState { attr.Size = size; attr.Type = type; attr.Normalized = normalized; - attr.Stride = stride; + attr.Stride = resolvedStride; attr.Offset = offset; attr.IsInteger = isInteger; attr.IsBgra = isBgra; diff --git a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h index 34e6e75e..2435c375 100644 --- a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h +++ b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h @@ -19,6 +19,14 @@ namespace MobileGL { int Size = 4; DataType Type = DataType::Float32; Bool Normalized = false; + // The RESOLVED byte distance between consecutive elements, never the raw + // glVertexAttrib*Pointer argument: a pointer call's stride 0 means "tightly + // packed" and is resolved to the element size here, so a zero that survives + // into this field can only have come from the binding model, where a zero + // VERTEX_BINDING_STRIDE means the opposite - every vertex reads the SAME + // element and the fetch address never advances (GL 4.6 core 10.3.1). Backends + // consume this verbatim; collapsing 0 back into the element size is what made + // KHR-GL43.vertex_attrib_binding.basic-input-case7/8 read past the buffer. int Stride = 0; SizeT Offset = 0; Bool IsInteger = false; @@ -76,8 +84,12 @@ namespace MobileGL { void DisableAttribute(Uint index); Bool IsAttributeEnabled(Uint index) const; + // `stride` is the raw glVertexAttrib*Pointer argument, reported verbatim by + // GL_VERTEX_ATTRIB_ARRAY_STRIDE. `effectiveStride` is what the fetch actually + // advances by - the same value when the argument is non-zero, the tightly + // packed element size when it is zero. Pass -1 to say the two are the same. void SetAttributeFormat(Uint index, int size, DataType type, Bool normalized, int stride, SizeT offset, - Bool isInteger, Bool isBgra = false); + Bool isInteger, Bool isBgra = false, int effectiveStride = -1); void BindAttributeBuffer(Uint index, const SharedPtr& buffer); diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 1c9d284b..8244b0b2 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -20,6 +20,7 @@ #include "SpirvPasses/DecoratePositionInvariantPass.h" #include "SpirvPasses/LowerDrawParametersPass.h" #include "SpirvPasses/PackDoubleVertexInputsPass.h" +#include "SpirvPasses/SplitArrayVertexInputsPass.h" #include "SpirvPasses/RebaseInstanceIndexPass.h" #include "SpirvPasses/ZeroBaseVertexPass.h" #include "SpirvPasses/NormalizeRectCoordinatesPass.h" @@ -630,6 +631,16 @@ namespace MobileGL { outputBinary); } + bool ShaderCompiler::SplitArrayVertexInputsForEssl(const Vector& inputBinary, + Vector& outputBinary) { + using namespace spvtools; + Optimizer optimizer(SPV_ENV_VULKAN_1_1); + optimizer.RegisterPass(SplitArrayVertexInputsPass::CreateSplitArrayVertexInputsPass()); + + return RunOptimizerChecked("SplitArrayVertexInputsForEssl", optimizer, inputBinary, + outputBinary); + } + bool ShaderCompiler::PackDoubleVertexInputsForVulkan(const Vector& inputBinary, Vector& outputBinary) { using namespace spvtools; diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index 21ab7cfc..41ff3461 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -27,6 +27,13 @@ namespace MobileGL { // Only for backends without native draw-parameter support (DirectGLES). static bool LowerDrawParametersForEssl(const Vector& inputBinary, Vector& outputBinary); + // Replaces an ARRAY vertex input with one input per element at consecutive + // locations, seeding a Private copy of the array so indexed reads still work. + // GLSL ES has no array vertex inputs and SPIRV-Cross refuses the whole module + // rather than emulating them, so without this the stage never reaches the + // driver. Only for the DirectGLES transpile path. + static bool SplitArrayVertexInputsForEssl(const Vector& inputBinary, + Vector& outputBinary); // Drops RelaxedPrecision member decorations from uniform-block structs so // SPIRV-Cross prints the same (highp) member precision in every stage; ES // drivers reject cross-stage uniform blocks whose member precisions differ. diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.cpp new file mode 100644 index 00000000..c97753b6 --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.cpp @@ -0,0 +1,394 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include "SplitArrayVertexInputsPass.h" + +#include "spirv.hpp" +#include "source/opt/constants.h" +#include "source/opt/def_use_manager.h" +#include "source/opt/instruction.h" +#include "source/opt/ir_context.h" +#include "source/opt/module.h" +#include "source/opt/types.h" +#include "source/util/make_unique.h" + +#include +#include +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + namespace { + using spvtools::opt::IRContext; + using spvtools::opt::Instruction; + using spvtools::opt::Operand; + namespace analysis = spvtools::opt::analysis; + + // How many attribute locations one element of the array consumes. GL 4.6 core + // 11.1.1: a scalar or vector of up to four 32-bit components takes one; a + // double-precision vector wider than two takes two; a matrix takes one per + // column. Zero means "this pass will not touch it" - the module keeps its array + // input and SPIRV-Cross will say so, which is a better outcome than a silently + // mis-located split. + Uint32 LocationsPerElement(const analysis::Type* type) { + if (type == nullptr) return 0; + // Any scalar takes one location, a 64-bit one included. + if (type->AsFloat() != nullptr || type->AsInteger() != nullptr || + type->AsBool() != nullptr) { + return 1u; + } + if (const auto* vector = type->AsVector()) { + const auto* element = vector->element_type(); + const auto* elementFloat = element->AsFloat(); + const Bool is64Bit = elementFloat != nullptr && elementFloat->width() == 64; + if (element->AsFloat() == nullptr && element->AsInteger() == nullptr && + element->AsBool() == nullptr) { + return 0; + } + return (is64Bit && vector->element_count() > 2) ? 2u : 1u; + } + // Matrices, structs, images and nested arrays are left alone deliberately: + // an array of them is vanishingly rare as a vertex input and each carries its + // own location-assignment rule, so getting one wrong would corrupt every + // attribute after it rather than fail loudly. + return 0; + } + + // The Location a variable is decorated with, or `false` when it carries none. + // A vertex input without an explicit location cannot be split: the split has to + // name base+i, and inventing a base would collide with whatever the linker + // assigned. + Bool FindLocationDecoration(IRContext& irContext, Uint32 variableId, Uint32& outLocation) { + for (auto& annotation : irContext.annotations()) { + if (annotation.opcode() != spv::Op::OpDecorate) continue; + if (annotation.GetSingleWordInOperand(0) != variableId) continue; + if (static_cast(annotation.GetSingleWordInOperand(1)) != + spv::Decoration::Location) { + continue; + } + outLocation = annotation.GetSingleWordInOperand(2); + return true; + } + return false; + } + } // namespace + + spvtools::opt::Pass::Status SplitArrayVertexInputsPass::Process() { + auto* irContext = context(); + auto entryPoints = irContext->module()->entry_points(); + if (entryPoints.begin() == entryPoints.end()) return Status::SuccessWithoutChange; + + Instruction* entryPoint = &*entryPoints.begin(); + if (static_cast(entryPoint->GetSingleWordInOperand(0)) != + spv::ExecutionModel::Vertex) { + return Status::SuccessWithoutChange; + } + + auto* defUseMgr = irContext->get_def_use_mgr(); + auto* typeMgr = irContext->get_type_mgr(); + auto* constMgr = irContext->get_constant_mgr(); + + struct Target { + Instruction* variable = nullptr; + Uint32 arrayTypeId = 0; + Uint32 elementTypeId = 0; + Uint32 elementCount = 0; + Uint32 baseLocation = 0; + Uint32 locationsPerElement = 1; + Uint32 elementInputPointerTypeId = 0; + Uint32 elementPrivatePointerTypeId = 0; + Uint32 arrayPrivatePointerTypeId = 0; + }; + std::vector targets; + + for (Instruction& inst : irContext->types_values()) { + if (inst.opcode() != spv::Op::OpVariable) continue; + if (static_cast(inst.GetSingleWordInOperand(0)) != + spv::StorageClass::Input) { + continue; + } + Instruction* pointerType = defUseMgr->GetDef(inst.type_id()); + if (pointerType == nullptr) continue; + const Uint32 pointeeTypeId = pointerType->GetSingleWordInOperand(1); + const analysis::Type* pointeeType = typeMgr->GetType(pointeeTypeId); + const auto* arrayType = pointeeType != nullptr ? pointeeType->AsArray() : nullptr; + if (arrayType == nullptr) continue; + + // A builtin input array (gl_ClipDistance and friends) is not an attribute and + // has no location; SPIRV-Cross emits those itself. + Uint32 baseLocation = 0; + if (!FindLocationDecoration(*irContext, inst.result_id(), baseLocation)) continue; + + const Uint32 elementTypeId = typeMgr->GetId(arrayType->element_type()); + const Uint32 locationsPerElement = LocationsPerElement(arrayType->element_type()); + if (elementTypeId == 0 || locationsPerElement == 0) { + MGLOG_I("SplitArrayVertexInputsPass: vertex input %%%u is an array whose element " + "type has no single-location mapping; leaving it declared as an array", + inst.result_id()); + continue; + } + + // OpTypeArray's length is an id of an integer constant. + Instruction* arrayTypeInst = defUseMgr->GetDef(pointeeTypeId); + if (arrayTypeInst == nullptr || arrayTypeInst->NumInOperands() < 2) continue; + const analysis::Constant* lengthConstant = + constMgr->FindDeclaredConstant(arrayTypeInst->GetSingleWordInOperand(1)); + if (lengthConstant == nullptr || lengthConstant->AsIntConstant() == nullptr) continue; + const Uint32 elementCount = lengthConstant->AsIntConstant()->GetU32(); + if (elementCount == 0) continue; + + Target target; + target.variable = &inst; + target.arrayTypeId = pointeeTypeId; + target.elementTypeId = elementTypeId; + target.elementCount = elementCount; + target.baseLocation = baseLocation; + target.locationsPerElement = locationsPerElement; + targets.push_back(target); + } + + if (targets.empty()) return Status::SuccessWithoutChange; + + // Everything the demoted array's pointer flows into, in SSA order. Demoting the + // variable changes the STORAGE CLASS of every pointer derived from it, and a + // derived pointer's own result type still says Input - which is an invalid + // module ("the result pointer storage class and base pointer storage class in + // OpAccessChain do not match"), so each one has to be retyped too. Collected + // before anything is mutated so an unsupported use can still decline the whole + // rewrite rather than leave a half-converted module behind. + std::unordered_set derivedPointers; + for (const auto& target : targets) { + derivedPointers.insert(target.variable->result_id()); + } + std::vector pointersToRetype; + for (auto& function : *irContext->module()) { + for (auto& block : function) { + for (auto& inst : block) { + const spv::Op opcode = inst.opcode(); + // A pointer this pass moved may only be loaded from or indexed into. + // Anything else (handing it to a function, storing THROUGH it, casting + // it) either cannot happen for a vertex input or would need the callee + // rewritten as well, and silently getting that wrong is worse than + // leaving the module for SPIRV-Cross to reject out loud. + const Bool indexes = + opcode == spv::Op::OpAccessChain || + opcode == spv::Op::OpInBoundsAccessChain || + opcode == spv::Op::OpCopyObject; + if (indexes) { + if (inst.NumInOperands() > 0 && + derivedPointers.count(inst.GetSingleWordInOperand(0)) != 0) { + derivedPointers.insert(inst.result_id()); + pointersToRetype.push_back(&inst); + } + continue; + } + if (opcode == spv::Op::OpLoad) continue; // reads are always fine + for (Uint32 i = 0; i < inst.NumInOperands(); ++i) { + const Operand& operand = inst.GetInOperand(i); + if (operand.type != SPV_OPERAND_TYPE_ID || operand.words.size() != 1) { + continue; + } + if (derivedPointers.count(operand.words[0]) == 0) continue; + MGLOG_I("SplitArrayVertexInputsPass: array vertex input %%%u reaches a " + "SPIR-V opcode %u that this pass cannot follow; leaving it " + "declared as an " + "array", + operand.words[0], static_cast(opcode)); + return Status::SuccessWithoutChange; + } + } + } + } + + // Entry block insertion point: after the block's leading OpVariable run, which + // SPIR-V requires to stay at the top of a function's first block. + const Uint32 entryFunctionId = entryPoint->GetSingleWordInOperand(1); + spvtools::opt::Function* entryFunction = nullptr; + for (auto& function : *irContext->module()) { + if (function.result_id() == entryFunctionId) { + entryFunction = &function; + break; + } + } + if (entryFunction == nullptr || entryFunction->begin() == entryFunction->end()) { + return Status::SuccessWithoutChange; + } + auto& entryBlock = *entryFunction->begin(); + auto insertPoint = entryBlock.begin(); + while (insertPoint != entryBlock.end() && insertPoint->opcode() == spv::Op::OpVariable) { + ++insertPoint; + } + if (insertPoint == entryBlock.end()) return Status::SuccessWithoutChange; + + // Every type this pass names has to exist before the variables that name it: the + // types-and-variables section is emitted in order and a forward reference to a + // type is invalid SPIR-V. Same staging as PackDoubleVertexInputsPass. + for (auto& target : targets) { + target.elementInputPointerTypeId = + typeMgr->FindPointerToType(target.elementTypeId, spv::StorageClass::Input); + target.elementPrivatePointerTypeId = + typeMgr->FindPointerToType(target.elementTypeId, spv::StorageClass::Private); + target.arrayPrivatePointerTypeId = + typeMgr->FindPointerToType(target.arrayTypeId, spv::StorageClass::Private); + } + + // Index constants for the seeding access chains, all of them before any variable. + Uint32 maxElementCount = 0; + for (const auto& target : targets) { + maxElementCount = std::max(maxElementCount, target.elementCount); + } + std::vector indexConstantIds(maxElementCount, 0); + for (Uint32 index = 0; index < maxElementCount; ++index) { + indexConstantIds[index] = constMgr->GetUIntConstId(index); + } + + std::vector interfaceOperands; + for (Uint32 i = 0; i < entryPoint->NumInOperands(); ++i) { + interfaceOperands.push_back(entryPoint->GetInOperand(i)); + } + + for (const auto& target : targets) { + Instruction* variable = target.variable; + const Uint32 oldVariableId = variable->result_id(); + + // Collected, never added inline: AddAnnotationInst mutates the annotation + // list this function is still walking below. + std::vector> newDecorations; + std::vector elementVariableIds(target.elementCount, 0); + for (Uint32 element = 0; element < target.elementCount; ++element) { + const Uint32 elementVariableId = irContext->TakeNextId(); + elementVariableIds[element] = elementVariableId; + irContext->AddGlobalValue(spvtools::MakeUnique( + irContext, spv::Op::OpVariable, target.elementInputPointerTypeId, + elementVariableId, + std::initializer_list{ + {SPV_OPERAND_TYPE_STORAGE_CLASS, + {static_cast(spv::StorageClass::Input)}}})); + newDecorations.push_back(spvtools::MakeUnique( + irContext, spv::Op::OpDecorate, 0, 0, + std::initializer_list{ + {SPV_OPERAND_TYPE_ID, {elementVariableId}}, + {SPV_OPERAND_TYPE_DECORATION, + {static_cast(spv::Decoration::Location)}}, + {SPV_OPERAND_TYPE_LITERAL_INTEGER, + {target.baseLocation + element * target.locationsPerElement}}})); + } + + // The array's own decorations move to the elements, except Location (each + // element got its own above) and anything that only described the aggregate. + // RelaxedPrecision is the one that MUST travel: dropping it changes the + // declared precision of the input in the emitted ESSL. + std::vector deadDecorations; + for (auto& annotation : irContext->annotations()) { + if (annotation.opcode() != spv::Op::OpDecorate) continue; + if (annotation.GetSingleWordInOperand(0) != oldVariableId) continue; + const auto decoration = + static_cast(annotation.GetSingleWordInOperand(1)); + if (decoration == spv::Decoration::RelaxedPrecision || + decoration == spv::Decoration::Flat || + decoration == spv::Decoration::NoPerspective || + decoration == spv::Decoration::Centroid || + decoration == spv::Decoration::Sample) { + for (const Uint32 elementVariableId : elementVariableIds) { + std::vector operands; + operands.push_back({SPV_OPERAND_TYPE_ID, {elementVariableId}}); + for (Uint32 i = 1; i < annotation.NumInOperands(); ++i) { + operands.push_back(annotation.GetInOperand(i)); + } + newDecorations.push_back(spvtools::MakeUnique( + irContext, spv::Op::OpDecorate, 0, 0, operands)); + } + } + deadDecorations.push_back(&annotation); + } + for (auto* annotation : deadDecorations) { + irContext->KillInst(annotation); + } + for (auto& decoration : newDecorations) { + irContext->AddAnnotationInst(std::move(decoration)); + } + + // Demote the original to a Private global: every existing OpLoad and + // OpAccessChain on it stays valid and keeps its array type, dynamic indices + // included. Moved to the end of the section for the same reason as in + // PackDoubleVertexInputsPass - a variable may not forward-reference its type. + variable->SetResultType(target.arrayPrivatePointerTypeId); + variable->SetInOperand(0, {static_cast(spv::StorageClass::Private)}); + variable->RemoveFromList(); + irContext->AddGlobalValue(std::unique_ptr(variable)); + + // SPIR-V 1.3 lists only Input/Output in the entry-point interface, and the + // array is no longer an Input: replace it with the elements in place, so the + // interface keeps one entry per live interface variable. + std::vector rebuilt; + for (Uint32 i = 0; i < interfaceOperands.size(); ++i) { + const Operand& operand = interfaceOperands[i]; + if (i >= 3 && operand.type == SPV_OPERAND_TYPE_ID && + operand.words.size() == 1 && operand.words[0] == oldVariableId) { + for (const Uint32 elementVariableId : elementVariableIds) { + rebuilt.push_back({SPV_OPERAND_TYPE_ID, {elementVariableId}}); + } + continue; + } + rebuilt.push_back(operand); + } + interfaceOperands = std::move(rebuilt); + + // Seed the Private array once, at the top of the entry point, before any of + // the code that reads it. + for (Uint32 element = 0; element < target.elementCount; ++element) { + const Uint32 loadedId = irContext->TakeNextId(); + const Uint32 elementPointerId = irContext->TakeNextId(); + insertPoint = insertPoint.InsertBefore(spvtools::MakeUnique( + irContext, spv::Op::OpLoad, target.elementTypeId, loadedId, + std::initializer_list{ + {SPV_OPERAND_TYPE_ID, {elementVariableIds[element]}}})); + ++insertPoint; + insertPoint = insertPoint.InsertBefore(spvtools::MakeUnique( + irContext, spv::Op::OpAccessChain, target.elementPrivatePointerTypeId, + elementPointerId, + std::initializer_list{ + {SPV_OPERAND_TYPE_ID, {oldVariableId}}, + {SPV_OPERAND_TYPE_ID, {indexConstantIds[element]}}})); + ++insertPoint; + insertPoint = insertPoint.InsertBefore(spvtools::MakeUnique( + irContext, spv::Op::OpStore, 0, 0, + std::initializer_list{{SPV_OPERAND_TYPE_ID, {elementPointerId}}, + {SPV_OPERAND_TYPE_ID, {loadedId}}})); + ++insertPoint; + } + } + + entryPoint->SetInOperands(std::move(interfaceOperands)); + + // Retype the derived pointers collected above. Done last, so the pointer types + // it appends land after the variables (nothing in the types-and-variables + // section names them - they are only ever the result type of an instruction in + // a function body). + for (Instruction* pointer : pointersToRetype) { + Instruction* resultType = defUseMgr->GetDef(pointer->type_id()); + if (resultType == nullptr || resultType->opcode() != spv::Op::OpTypePointer) continue; + if (static_cast(resultType->GetSingleWordInOperand(0)) == + spv::StorageClass::Private) { + continue; + } + pointer->SetResultType(typeMgr->FindPointerToType(resultType->GetSingleWordInOperand(1), + spv::StorageClass::Private)); + } + + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + return Status::SuccessWithChange; + } + + spvtools::Optimizer::PassToken SplitArrayVertexInputsPass::CreateSplitArrayVertexInputsPass() { + return spvtools::Optimizer::PassToken(MakeUnique()); + } + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.h new file mode 100644 index 00000000..0b62d207 --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.h @@ -0,0 +1,52 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/SplitArrayVertexInputsPass.h +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#pragma once +#include "source/opt/pass.h" +#include "spirv-tools/optimizer.hpp" + +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + // Replaces an ARRAY vertex input with one input variable per element, at + // consecutive locations, and demotes the original array to a Private global + // seeded once at the top of the entry point. + // + // GLSL ES has no array vertex inputs at all (GLSL ES 3.20 4.3.4: a vertex shader + // input "cannot be ... arrays"), and SPIRV-Cross does not emulate the difference: + // it refuses the whole module with "OpenGL ES doesn't support array input + // variables in vertex shader". The stage then never reaches the driver, the + // program links without a vertex shader, and every draw using it is a silent + // no-op - which is how the entire KHR-GL43.vertex_attrib_binding.basic-input* + // family (its capture program declares `in vec4 vs_in_attrib[16]`) failed on + // DirectGLES with no symptom other than "the draw captured zeros". + // + // Desktop GL DOES allow the declaration, and it means exactly what the split + // produces: element i of an input array consumes location base+i (GL 4.6 core + // 11.1.1). So the split is a spelling change, not a semantic one - the same + // vertex attributes feed the same components, and the frontend's reflection + // (which the backends bind attributes from) is not involved. + // + // Downstream code is untouched on purpose: the original variable keeps its id and + // its array type, so every OpAccessChain into it - including the DYNAMICALLY + // indexed ones a `for` loop produces, which is precisely what an input array is + // usually written for - stays valid against the Private copy. + // + // DirectGLES only. Vulkan takes array vertex inputs as they are. + class SplitArrayVertexInputsPass : public spvtools::opt::Pass { + public: + const char* name() const override { return "mobilegl-split-array-vertex-inputs"; } + Status Process() override; + + static spvtools::Optimizer::PassToken CreateSplitArrayVertexInputsPass(); + }; + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL