[Fix] (DirectGLES, DirectVulkan): answer GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT with the storage limit, not the uniform one

MobileGL/MG_Backend/BackendObject.h

@@ -301,6 +301,12 @@ namespace MobileGL {
 
         struct DynamicBackendParameters {
             SizeT UniformBufferOffsetAlignment = 256;
+            // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, which is a SEPARATE limit from the
+            // uniform one and is routinely larger: Adreno 830 reports 32 for uniform buffers and
+            // 64 for storage buffers. Answering the storage query with the uniform value let an
+            // application bind a storage range at an offset the driver cannot address, which it
+            // accepted without error and then wrote somewhere else entirely.
+            SizeT ShaderStorageBufferOffsetAlignment = 256;
             // GL_MAX_TEXTURE_MAX_ANISOTROPY_EXT. 1.0 means the backend cannot filter anisotropically,
             // which is also why the extension is not advertised in that case.
             Float MaxTextureMaxAnisotropy = 1.0f;

MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp

@@ -1323,6 +1323,8 @@ namespace MobileGL::MG_Backend::DirectGLES {
 
     void BackendObject_DirectGLES::UpdateDynamicBackendParameters() {
         m_dynamicParameters.UniformBufferOffsetAlignment = m_GLESCapabilities.UniformBufferOffsetAlignment;
+        m_dynamicParameters.ShaderStorageBufferOffsetAlignment =
+            m_GLESCapabilities.ShaderStorageBufferOffsetAlignment;
         m_dynamicParameters.MaxTextureMaxAnisotropy = m_GLESCapabilities.MaxTextureMaxAnisotropy;
         m_dynamicParameters.AliasedLineWidthRangeMin = m_GLESCapabilities.AliasedLineWidthRangeMin;
         m_dynamicParameters.AliasedLineWidthRangeMax = m_GLESCapabilities.AliasedLineWidthRangeMax;

MobileGL/MG_Backend/DirectGLES/Managers.cpp

@@ -6341,8 +6341,9 @@ namespace MobileGL::MG_Backend::DirectGLES {
             // neither std430 nor std140") and the stage never reaches the driver. Collapse the
             // block into one uint array at offset 0 and re-index each counter to the element
             // that used to be at its byte offset; the buffer then stays bound whole, which it
-            // has to (GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT is 32 on this device, so an
-            // 8-byte bind offset is not expressible). BEFORE SetAtomicCounterBlockBindings
+            // has to (GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT is 64 on Adreno 830 and 32 or
+            // more everywhere else, so an 8-byte bind offset is not expressible on any of them).
+            // BEFORE SetAtomicCounterBlockBindings
             // below, which only moves the block's BINDING and needs the block intact.
             //
             // NO KEY MATERIAL either, for the same reason - and note where the application's

MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp

@@ -855,6 +855,7 @@ namespace MobileGL::MG_Backend::DirectVulkan {
 
         static constexpr SizeT kMaxAdvertisedShaderStorageBlockSize = 512ull * 1024ull * 1024ull;
         m_dynamicParameters.UniformBufferOffsetAlignment = m_vulkanCaps.UniformBufferOffsetAlignment;
+        m_dynamicParameters.ShaderStorageBufferOffsetAlignment = m_vulkanCaps.ShaderStorageBufferOffsetAlignment;
         m_dynamicParameters.AliasedLineWidthRangeMin = m_vulkanCaps.AliasedLineWidthRangeMin;
         m_dynamicParameters.AliasedLineWidthRangeMax = m_vulkanCaps.AliasedLineWidthRangeMax;
         // Without the samplerAnisotropy feature the limit is unusable, so report 1.0 (no anisotropy)

MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp

@@ -2406,7 +2406,12 @@ namespace MobileGL::MG_Impl::GLImpl {
             *params = static_cast<GLint>(dynamicParameters.PointSizeGranularity);
             break;
         case GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT:
-            *params = static_cast<GLint>(dynamicParameters.UniformBufferOffsetAlignment);
+            // The STORAGE alignment, which is its own limit - this used to answer with the
+            // uniform one. They differ on real hardware (Adreno 830: 32 uniform, 64 storage), and
+            // under-reporting it is silent: ValidateBindBufferRange accepts the offset, the ES
+            // driver accepts it too without raising an error, and the shader's writes then land
+            // at an address the application never bound.
+            *params = static_cast<GLint>(dynamicParameters.ShaderStorageBufferOffsetAlignment);
             break;
         case GL_SMOOTH_LINE_WIDTH_RANGE:
             params[0] = static_cast<GLint>(dynamicParameters.SmoothLineWidthRangeMin);

MobileGL/MG_Test/SanityTest.cpp

@@ -988,6 +988,49 @@ TEST(GetterSanity, ReportsFragmentInterpolationLimitsForFloatAndIntegerQueries)
     MG_State::pGLContext = Move(previousContext);
 }
 
+// GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT used to be answered with the UNIFORM buffer
+// alignment. The two are separate limits and the storage one is the larger on real hardware
+// (Adreno 830: 32 uniform, 64 storage), so the substitution under-reported it - and an
+// under-reported alignment is silent all the way down: the frontend validator accepts the
+// offset, the ES driver accepts the glBindBufferRange too without raising an error, and the
+// shader's stores land at an address the application never bound. The two values are
+// deliberately different here so a query that reads the wrong field cannot coincide with the
+// right answer.
+TEST(GetterSanity, StorageAndUniformBufferOffsetAlignmentsAreSeparateLimits) {
+    using namespace MobileGL;
+
+    auto previousContext = Move(MG_State::pGLContext);
+    auto previousBackend = Move(MG_Backend::pActiveBackendObject);
+    MG_State::pGLContext = MakeUnique<MG_State::GLState::GLContext>();
+
+    MG_Backend::DynamicBackendParameters params;
+    params.UniformBufferOffsetAlignment = 32;
+    params.ShaderStorageBufferOffsetAlignment = 64;
+    MG_Backend::pActiveBackendObject = MakeUnique<DynamicParameterBackend>(params);
+
+    GLint uniformAlignment = 0;
+    MG_Impl::GLImpl::GetIntegerv(GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT, &uniformAlignment);
+    EXPECT_EQ(uniformAlignment, 32);
+
+    GLint storageAlignment = 0;
+    MG_Impl::GLImpl::GetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, &storageAlignment);
+    EXPECT_EQ(storageAlignment, 64);
+
+    // And the other way round, so the test fails on a getter that simply swapped the two fields.
+    params.UniformBufferOffsetAlignment = 128;
+    params.ShaderStorageBufferOffsetAlignment = 16;
+    MG_Backend::pActiveBackendObject = MakeUnique<DynamicParameterBackend>(params);
+
+    MG_Impl::GLImpl::GetIntegerv(GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT, &uniformAlignment);
+    EXPECT_EQ(uniformAlignment, 128);
+    MG_Impl::GLImpl::GetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, &storageAlignment);
+    EXPECT_EQ(storageAlignment, 16);
+    EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    MG_Backend::pActiveBackendObject = Move(previousBackend);
+    MG_State::pGLContext = Move(previousContext);
+}
+
 TEST(GetterSanity, PerStageImageUniformQueriesMatchShaderCompilerLimits) {
     using namespace MobileGL;
 

MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp

@@ -1064,6 +1064,16 @@ namespace MobileGL::MG_Util::BackendLoader {
         MGLOG_I("OpenGL ES capabilities:");
         glesFuncs.glGetIntegerv(GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT, &caps.UniformBufferOffsetAlignment);
         MGLOG_I("    GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT: %d", caps.UniformBufferOffsetAlignment);
+        // ES 3.1 core, so no extension gate - but a driver that somehow leaves it at zero would
+        // make every storage-range offset legal, so an unusable answer keeps the 256 default.
+        GLint shaderStorageOffsetAlignment = 0;
+        glesFuncs.glGetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, &shaderStorageOffsetAlignment);
+        while (glesFuncs.glGetError() != GL_NO_ERROR) {
+        }
+        if (shaderStorageOffsetAlignment > 0) {
+            caps.ShaderStorageBufferOffsetAlignment = shaderStorageOffsetAlignment;
+        }
+        MGLOG_I("    GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT: %d", caps.ShaderStorageBufferOffsetAlignment);
         GLfloat aliasedLineWidthRange[2] = {1.0f, 1.0f};
         GLfloat smoothLineWidthRange[2] = {1.0f, 1.0f};
         GLfloat smoothLineWidthGranularity = 1.0f;

MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h

@@ -1231,6 +1231,10 @@ namespace MobileGL {
             // InstanceIndex, which includes firstInstance.
             Bool IndirectDrawInstanceIdIncludesBaseInstance = false;
             Int UniformBufferOffsetAlignment = 256;
+            // Its storage-buffer counterpart, queried separately because it is a separate limit:
+            // Adreno 830 answers 32 for GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT and 64 for
+            // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT.
+            Int ShaderStorageBufferOffsetAlignment = 256;
             Float AliasedLineWidthRangeMin = 1.0f;
             Float AliasedLineWidthRangeMax = 1.0f;
             Float SmoothLineWidthRangeMin = 1.0f;

MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp

@@ -154,6 +154,7 @@ namespace MobileGL::MG_Util::BackendLoader {
         caps.DriverVersionString = DecodeDriverVersion(p.driverVersion);
         caps.VendorId = p.vendorID;
         caps.UniformBufferOffsetAlignment = static_cast<int>(p.limits.minUniformBufferOffsetAlignment);
+        caps.ShaderStorageBufferOffsetAlignment = static_cast<int>(p.limits.minStorageBufferOffsetAlignment);
         caps.AliasedLineWidthRangeMin = p.limits.lineWidthRange[0];
         caps.AliasedLineWidthRangeMax = p.limits.lineWidthRange[1];
         caps.MaxSamplerAnisotropy = p.limits.maxSamplerAnisotropy;
@@ -272,6 +273,8 @@ namespace MobileGL::MG_Util::BackendLoader {
         caps.DriverVersionString = DecodeDriverVersion(properties.driverVersion);
         caps.VendorId = properties.vendorID;
         caps.UniformBufferOffsetAlignment = static_cast<int>(properties.limits.minUniformBufferOffsetAlignment);
+        caps.ShaderStorageBufferOffsetAlignment =
+            static_cast<int>(properties.limits.minStorageBufferOffsetAlignment);
         caps.AliasedLineWidthRangeMin = properties.limits.lineWidthRange[0];
         caps.AliasedLineWidthRangeMax = properties.limits.lineWidthRange[1];
         caps.MaxSamplerAnisotropy = properties.limits.maxSamplerAnisotropy;

MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.h

@@ -18,6 +18,10 @@ namespace MobileGL {
             // VkPhysicalDeviceProperties::vendorID, for device-quirk vendor gating.
             Uint32 VendorId = 0;
             Int UniformBufferOffsetAlignment = 256;
+            // VkPhysicalDeviceLimits::minStorageBufferOffsetAlignment. A separate limit from
+            // the uniform one on Vulkan too, and the one GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT
+            // has to answer with.
+            Int ShaderStorageBufferOffsetAlignment = 256;
             Float AliasedLineWidthRangeMin = 1.0f;
             Float AliasedLineWidthRangeMax = 1.0f;
             // VkPhysicalDeviceLimits::maxSamplerAnisotropy. Whether it can be used at all depends on

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h

@@ -44,9 +44,10 @@ namespace MobileGL {
             //
             // Why not simply rebase the offsets to zero and bind the buffer 8 bytes in: because
             // glBindBufferRange's offset must be a multiple of
-            // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, which the target device reports as 32.
-            // A byte offset of 8 cannot be expressed as a binding at all, so the correction has
-            // to live in the shader's indexing, where it costs nothing.
+            // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, which is 64 on Adreno 830 and no smaller
+            // than 32 on the other targets. A byte offset of 8 cannot be expressed as a binding
+            // on any of them, so the correction has to live in the shader's indexing, where it
+            // costs nothing.
             //
             // A block that is ALREADY laid out naturally - which is every shader that omits the
             // offset qualifier, and so very nearly all of them - is left byte-identical: the