From e18bac8cb2dbabb75c19f53ed969ef7a4b11afda Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 21 Aug 2026 22:02:23 -0400 Subject: [PATCH] [Fix, Test] (ShaderTranspiler, DirectGLES): never widen a buffer image - its texels are the application buffer, not storage we can reallocate --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 9 +++- .../WidenImageFormatsTest.cpp | 45 +++++++++++++++++++ .../SpirvPasses/WidenImageFormatsPass.cpp | 19 ++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index c3cc734c..245786a0 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -1482,8 +1482,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // a bind format that names a class the storage does not have is left alone: GL // already calls that undefined, and inventing a carrier for it would only make the // out-of-class read wider. + // + // A BUFFER texture is excluded on both sides: it has no storage of its own to widen + // (its texels are the application's buffer object), so WidenImageFormatsPass declines + // every buffer image and the bind must decline with it, or the driver would be handed + // a carrier the shader never addressed. See the Dim::Buffer guard there for the + // 32-byte GL_RG32F measurement that pinned it. GLenum bindFormat = imageBinding.Format; - if (TextureImpl::GetImageBindableStorageWidening(imageBinding.Texture->GetFormat())) { + if (imageBinding.Texture->GetTarget() != TextureTarget::TextureBuffer && + TextureImpl::GetImageBindableStorageWidening(imageBinding.Texture->GetFormat())) { const auto boundFormatWidening = TextureImpl::GetImageBindableStorageWidening( MG_Util::ConvertGLEnumToTextureInternalFormat(imageBinding.Format)); if (boundFormatWidening) { diff --git a/MobileGL/MG_Test/ShaderTranspiler/WidenImageFormatsTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/WidenImageFormatsTest.cpp index e73c346d..efc85822 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/WidenImageFormatsTest.cpp +++ b/MobileGL/MG_Test/ShaderTranspiler/WidenImageFormatsTest.cpp @@ -241,6 +241,18 @@ void main() { imageStore(img, ivec2(gl_FragCoord.xy), vec4(1.0, 2.0, 3.0, 4.0)); fragColor = texel; } +)"; + + // rg32f again, but as a BUFFER image. Same format, same carrier on paper - and it must be + // left alone anyway, because a buffer image's texels are the application's buffer object. + const char* const kRg32fBufferLoadStore = R"(#version 430 core +layout(rg32f, binding = 0) uniform imageBuffer img; +out vec4 fragColor; +void main() { + vec4 texel = imageLoad(img, int(gl_FragCoord.x)); + imageStore(img, int(gl_FragCoord.x), vec4(1.0, 2.0, 3.0, 4.0)); + fragColor = texel; +} )"; // rg16 is one of the EIGHT with no core carrier at all - core ESSL has no 16-bit normalized @@ -439,6 +451,39 @@ TEST(WidenImageFormats, PackedFloatImageOnlyReachesEsslThroughTheCarrier) { EXPECT_EQ(after.text.find("r11f_g11f_b10f"), String::npos) << after.text; } +// A BUFFER image is declined whatever its format, and the format alone cannot say so - rg32f is +// carried exactly when it is an image2D. What makes the difference is that widening REALLOCATES +// the texture behind the image in the carrier, and a buffer image has no texture storage to +// reallocate: its texels are the application's buffer object, usually also a vertex, index or +// storage buffer. Widening one leaves the shader striding 16 bytes through 8-byte texels - the +// measured symptom on an Adreno 830 was a 32-byte GL_RG32F buffer reading back +// [1,100] [0,1] [2,100] [0,1] instead of [1,100] [2,100] [3,100] [4,100], with the last two texels +// written past the end of the application's buffer. +TEST(WidenImageFormats, BufferImagesAreDeclinedEvenWhenTheirFormatHasACarrier) { + const Vector spirv = CompileFragment(kRg32fBufferLoadStore); + ASSERT_FALSE(spirv.empty()); + + const auto types = CollectStorageImageTypes(spirv); + ASSERT_EQ(types.size(), 1u); + EXPECT_EQ(types.front().format, static_cast(spv::ImageFormat::Rg32f)) + << "the fixture stopped declaring the format this test is about"; + + // The gate says no, so the optimizer is never even run for it... + EXPECT_FALSE(ShaderCompiler::DeclaresWidenableImageFormat(spirv)); + // ...and running it anyway changes nothing, which is what keeps the gate and the pass from + // disagreeing about a module. + Vector widened; + ShaderCompiler::WidenImageFormatsForEssl(spirv, widened, /*onlyFormatsSpirvCrossRefusesToPrint=*/false, + /*enableSpirvValidation=*/true); + EXPECT_TRUE(widened.empty() || widened == spirv) << "a buffer image was rewritten"; + + // The same format in a NON-buffer image still widens, or this test would pass for the wrong + // reason - a widening that had simply stopped working. + const Vector planar = CompileFragment(kRg32fLoadStore); + ASSERT_FALSE(planar.empty()); + EXPECT_TRUE(ShaderCompiler::DeclaresWidenableImageFormat(planar)); +} + TEST(WidenImageFormats, SingleChannelUnsignedImageBecomesRgba8uiWithBothAccessesMasked) { const Vector spirv = CompileFragment(kR8uiLoadStore); ASSERT_FALSE(spirv.empty()); diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/WidenImageFormatsPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/WidenImageFormatsPass.cpp index 174896a8..eecdcbed 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/WidenImageFormatsPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/WidenImageFormatsPass.cpp @@ -39,6 +39,7 @@ namespace MobileGL { // OpTypeImage in-operands: 0 sampled type, 1 Dim, 2 Depth, 3 Arrayed, 4 MS, // 5 Sampled, 6 Format. constexpr uint32_t kImageSampledTypeOperand = 0; + constexpr uint32_t kImageDimOperand = 1; constexpr uint32_t kImageSampledOperand = 5; constexpr uint32_t kImageFormatOperand = 6; // A storage image, i.e. one reached through imageLoad/imageStore rather than a @@ -246,6 +247,24 @@ namespace MobileGL { bool onlyFormatsSpirvCrossRefusesToPrint) { if (type == nullptr || type->opcode() != spv::Op::OpTypeImage) return false; if (type->GetSingleWordInOperand(kImageSampledOperand) != kSampledStorageImage) return false; + // A BUFFER image is never widened, whatever its format. Widening works because + // the ES texture behind the image can be REALLOCATED in the carrier, so the + // texel the shader addresses and the texel the storage holds stay the same + // size. A buffer image has no storage of its own to reallocate: its texels are + // the application's buffer object, at the size and layout the application gave + // it, and that buffer is usually also a vertex, index or storage buffer whose + // contents are not ours to relayout. + // + // Widening one anyway makes the shader stride 16 bytes through 8-byte texels. + // Measured on an Adreno 830 with a 32-byte GL_RG32F buffer and a shader storing + // (i+1, 100) at texel i: the readback came back [1,100] [0,1] [2,100] [0,1] - + // texels 0 and 1 landed on top of all four, texels 2 and 3 ran off the end of + // the application's buffer. Declining leaves the honest "no GLSL ES spelling" + // failure instead, which loses the same stage but corrupts nothing. + if (static_cast(type->GetSingleWordInOperand(kImageDimOperand)) == + spv::Dim::Buffer) { + return false; + } const auto format = static_cast(type->GetSingleWordInOperand(kImageFormatOperand)); if (!WideningOfSpirvImageFormat(format)) return false;