[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

@@ -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) {

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<Uint32> spirv = CompileFragment(kRg32fBufferLoadStore);
+    ASSERT_FALSE(spirv.empty());
+
+    const auto types = CollectStorageImageTypes(spirv);
+    ASSERT_EQ(types.size(), 1u);
+    EXPECT_EQ(types.front().format, static_cast<Uint32>(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<Uint32> 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<Uint32> planar = CompileFragment(kRg32fLoadStore);
+    ASSERT_FALSE(planar.empty());
+    EXPECT_TRUE(ShaderCompiler::DeclaresWidenableImageFormat(planar));
+}
+
 TEST(WidenImageFormats, SingleChannelUnsignedImageBecomesRgba8uiWithBothAccessesMasked) {
     const Vector<Uint32> spirv = CompileFragment(kR8uiLoadStore);
     ASSERT_FALSE(spirv.empty());

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<spv::Dim>(type->GetSingleWordInOperand(kImageDimOperand)) ==
+                        spv::Dim::Buffer) {
+                        return false;
+                    }
                     const auto format =
                         static_cast<spv::ImageFormat>(type->GetSingleWordInOperand(kImageFormatOperand));
                     if (!WideningOfSpirvImageFormat(format)) return false;