[Fix, Test] (MG_Util, MG_Backend/DirectVulkan, MG_IntegrationTest): review round - one module parse for the shaders with no 1D-array image, per-kind values the combined case can name, and the invariants two shared buffers rest on

MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp

@@ -843,8 +843,12 @@ namespace MobileGL::MG_Backend::DirectVulkan {
             return false;
         }
 
-        // Sized against the VIEW's format, not the texture's attached one - they may differ by the
+        // Sized from the TEXTURE's attached format even though the view may carry a different
-        // paragraph above, and a range that is not a whole number of the view's texels is invalid.
+        // one. That is not a shortcut: GL requires the shader's format qualifier, the format
+        // passed to glBindImageTexture and the texture's own internal format to belong to the
+        // same format CLASS (GL 4.6 core, table 8.27), and every member of a class has the same
+        // texel size. So the three can disagree on interpretation and never on bytes - which is
+        // what the range below has to be a whole multiple of.
         const VkDeviceSize texelSize =
             static_cast<VkDeviceSize>(MG_Util::GetSizedInternalFormatSizeInBytes(internalFormat));
         const VkDeviceSize rangeOffset = static_cast<VkDeviceSize>(textureBuffer->GetBufferRangeOffset());
@@ -1716,6 +1720,11 @@ namespace MobileGL::MG_Backend::DirectVulkan {
         // is reachable wherever m_maxBindings is small (it clamps to ~16 on Adreno and Mali),
         // which is exactly where a 7-element CTS sampler array does not fit the slack.
         imageInfos.reserve(m_maxBindings + arrayDescriptorExtra);
+        // Exact, and safe only because it is: BOTH texel kinds (samplerBuffer and imageBuffer)
+        // refuse descriptor arrays at program creation, so each contributes at most one view and
+        // the total cannot exceed the binding count. The branches below take the address of
+        // back(), so making a texel kind array-capable without also giving this the surplus
+        // imageInfos gets would dangle every pTexelBufferView already recorded in `writes`.
         texelBufferViews.reserve(m_maxBindings);
         dynamicOffsets.reserve(programObj.dynamicBindings.size() + uboArrayExtra);
 

MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp

@@ -204,8 +204,8 @@ namespace MGITest {
 
             // Storage plus a full fill with `value`, in the spelling each target kind needs.
             // Returns 0 - having already reported - when the target could not be created.
-            GLuint MakeTexture(const TargetKind& kind, bool fill) {
+            GLuint MakeTexture(const TargetKind& kind, bool fill, GLuint value = kFilledValue) {
-                const std::vector<GLuint> texels(static_cast<std::size_t>(kExtent) * kExtent * kExtent, kFilledValue);
+                const std::vector<GLuint> texels(static_cast<std::size_t>(kExtent) * kExtent * kExtent, value);
 
                 if (kind.buffer) {
                     GLuint buffer = 0;
@@ -441,8 +441,14 @@ namespace MGITest {
     // needs several kinds in one program - a binding remap that only collides when two image
     // types share a descriptor set, a per-kind rewrite that is not idempotent across declarations
     // - and that class of defect is precisely what "each kind passes alone but the case still
-    // fails" would mean. Each unit is filled with its own INDEX rather than a constant, so the
+    // fails" would mean.
-    // sum names how many units contributed and a single mis-bound unit does not cancel out.
+    //
+    // Each unit is filled with its own DISTINCT value rather than a shared one, so a shortfall
+    // names WHICH kind is missing rather than merely how many are: with one shared value, "three
+    // kinds read zero" and "one kind read zero" differ only by a multiple, and any two kinds are
+    // interchangeable in the total. A sum still cannot see two kinds SWAPPING - addition is
+    // commutative, and the conformance case has exactly the same blind spot - but the single-kind
+    // cases above pin each kind to its own texture already, so a swap cannot hide there.
     TEST_F(ImageTargetKindScenario, AllKindsInOneProgram) {
         if (!Ready()) return;
         if (!ImagesAreUsable()) GTEST_SKIP() << "no compute image uniforms";
@@ -483,12 +489,15 @@ namespace MGITest {
         const GLuint program = MakeComputeProgram(source);
         if (program == 0) return;
 
-        // Each unit gets its own value, so the sum says how many units contributed.
+        // Powers of two, so the shortfall's bit pattern names exactly which kinds read zero -
+        // no other subset of the values can sum to the same total. Eleven kinds at most, so the
+        // largest is 1 << 10 and the sum cannot approach a uint's range.
         GLuint expected = 0;
         for (std::size_t i = 0; i < kinds.size(); ++i) {
-            const GLuint texture = MakeTexture(kinds[i], true);
+            const GLuint value = 1u << i;
+            const GLuint texture = MakeTexture(kinds[i], true, value);
             if (texture == 0) return;
-            expected += kFilledValue;
+            expected += value;
             glBindImageTexture(static_cast<GLuint>(i), texture, 0, GL_TRUE, 0, GL_READ_ONLY, GL_R32UI);
             ASSERT_EQ(FirstGLError(), 0u) << kinds[i].name << ": glBindImageTexture errored";
         }
@@ -504,10 +513,19 @@ namespace MGITest {
         glMemoryBarrier(GL_ALL_BARRIER_BITS);
         EXPECT_EQ(FirstGLError(), 0u) << "the dispatch leaked a GL error";
 
-        EXPECT_EQ(ReadResult(ssbo), expected)
+        const GLuint actual = ReadResult(ssbo);
+        std::string missing;
+        for (std::size_t i = 0; i < kinds.size(); ++i) {
+            if ((actual & (1u << i)) == 0u) {
+                if (!missing.empty()) missing += ", ";
+                missing += kinds[i].name;
+            }
+        }
+        EXPECT_EQ(actual, expected)
             << "the sum over " << kinds.size()
-            << " image target kinds is wrong; each kind contributes " << kFilledValue
+            << " image target kinds is wrong; each kind contributes its own bit, and these read "
-            << ", so the shortfall is a whole number of kinds that read zero";
+               "zero: "
+            << (missing.empty() ? "(none - so some kind read a value it was never given)" : missing);
         glUseProgram(0);
     }
 

MobileGL/MG_Test/Program/ProgramUtilTest.cpp

@@ -3632,7 +3632,8 @@ void main() { ssb.sum = uint(imageSize(i0).x) + imageLoad(i0, ivec2(0, 0)).r; }
 )",
                                                    GL_COMPUTE_SHADER);
     ASSERT_FALSE(spirv.empty());
-    ASSERT_TRUE(Lower1DArrayImagesPass::BinaryQueriesA1DArrayStorageImageSize(spirv))
+    const auto traits = Lower1DArrayImagesPass::InspectBinary(spirv);
+    ASSERT_TRUE(traits.declaresImage && traits.queriesImageSize)
         << "the fixture must contain the shape the pass declines";
 
     Vector<Uint32> lowered;

MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp

@@ -839,7 +839,16 @@ namespace MobileGL {
                 // MGLOG_I, deliberately: MGLOG_E/W are compiled out at the INFO level every CI,
                 // retrace and release build uses, and this is exactly the diagnostic that has to
                 // survive to explain the shader the driver is about to reject.
-                if (Lower1DArrayImagesPass::BinaryQueriesA1DArrayStorageImageSize(inputBinary)) {
+                const auto traits = Lower1DArrayImagesPass::InspectBinary(inputBinary);
+                // The overwhelmingly common answer, and the reason the inspection exists: no
+                // 1D-array storage image, so the module is handed back byte for byte without an
+                // Optimizer ever being built. Every ESSL shader in the process passes through
+                // here, so the cost of the case with nothing to do is the cost of this pass.
+                if (!traits.declaresImage) {
+                    outputBinary = inputBinary;
+                    return true;
+                }
+                if (traits.queriesImageSize) {
                     MGLOG_I("[spirv] Lower1DArrayImagesForEssl: the module queries the size of a 1D-array "
                             "storage image, which cannot be answered in the 2D-array shape ES stores it in; "
                             "leaving the module alone, and a strict ES driver will reject it");

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp

@@ -112,16 +112,30 @@ namespace MobileGL {
                 }
             } // namespace
 
-            bool Lower1DArrayImagesPass::BinaryQueriesA1DArrayStorageImageSize(const Vector<Uint32>& binary) {
+            Lower1DArrayImagesPass::ModuleTraits Lower1DArrayImagesPass::InspectBinary(const Vector<Uint32>& binary) {
+                ModuleTraits traits{};
                 if (binary.empty()) {
-                    return false;
+                    return traits;
                 }
                 std::unique_ptr<IRContext> context = spvtools::BuildModule(
                     SPV_ENV_VULKAN_1_1, [](spv_message_level_t, const char*, const spv_position_t&, const char*) {},
                     binary.data(), binary.size());
                 if (!context) {
-                    return false;
+                    return traits;
                 }
+
+                // The type table settles it for the cheap half, and it is the half almost every
+                // shader takes: no such type declared, nothing to inspect further.
+                for (const Instruction& type : context->module()->types_values()) {
+                    if (Is1DArrayStorageImageType(&type)) {
+                        traits.declaresImage = true;
+                        break;
+                    }
+                }
+                if (!traits.declaresImage) {
+                    return traits;
+                }
+
                 for (auto& function : *context->module()) {
                     for (auto& block : function) {
                         for (auto& instruction : block) {
@@ -130,12 +144,13 @@ namespace MobileGL {
                             }
                             if (Is1DArrayStorageImageType(
                                     ResolveImageType(context.get(), instruction.GetSingleWordInOperand(0)))) {
-                                return true;
+                                traits.queriesImageSize = true;
+                                return traits;
                             }
                         }
                     }
                 }
-                return false;
+                return traits;
             }
 
             spvtools::opt::Pass::Status Lower1DArrayImagesPass::Process() {

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h

@@ -65,11 +65,22 @@ namespace MobileGL {
                 const char* name() const override { return "mobilegl-lower-1d-array-images"; }
                 Status Process() override;
 
-                // True when the module declares a 1D-array storage image whose size is queried,
+                // What one inspection of a binary tells the caller. Both answers come from a
-                // which is the shape this pass refuses to translate. Checked by the caller before
+                // SINGLE parse on purpose: every ESSL shader in the process reaches this, and
-                // running, so a declined module is handed on untouched rather than partly
+                // almost none of them declare a 1D-array storage image, so the common path has to
-                // rewritten.
+                // cost one module parse and no optimizer run at all - not one parse to ask about
-                static bool BinaryQueriesA1DArrayStorageImageSize(const Vector<Uint32>& binary);
+                // size queries and a second inside an Optimizer that then early-outs.
+                struct ModuleTraits {
+                    // The module declares a 1D-array storage image, i.e. there is anything to do.
+                    bool declaresImage = false;
+                    // ...and queries its size, which is the shape this pass refuses to translate:
+                    // afterwards the image is a 2D array, so the query yields three components
+                    // where the shader consumes two, and there is no correct two-component answer
+                    // to substitute. The caller leaves such a module alone rather than half
+                    // rewriting it.
+                    bool queriesImageSize = false;
+                };
+                static ModuleTraits InspectBinary(const Vector<Uint32>& binary);
 
                 static spvtools::Optimizer::PassToken CreateLower1DArrayImagesPass();
             };