Mark uniform samplers in an array unused per element

src/libANGLE/Context.cpp

@@ -9025,10 +9025,6 @@ void StateCache::updateActiveImageUnitIndices(Context *context)
     {
         for (const ImageBinding &imageBinding : executable->getImageBindings())
         {
-            if (imageBinding.unreferenced)
-            {
-                continue;
-            }
             for (GLuint binding : imageBinding.boundImageUnits)
             {
                 mCachedActiveImageUnitIndices.set(binding);

src/libANGLE/Program.cpp

@@ -926,12 +926,8 @@ VariableLocation::VariableLocation(unsigned int arrayIndex, unsigned int index)
 // SamplerBindings implementation.
 SamplerBinding::SamplerBinding(TextureType textureTypeIn,
                                SamplerFormat formatIn,
-                               size_t elementCount,
-                               bool unreferenced)
-    : textureType(textureTypeIn),
-      format(formatIn),
-      boundTextureUnits(elementCount, 0),
-      unreferenced(unreferenced)
+                               size_t elementCount)
+    : textureType(textureTypeIn), format(formatIn), boundTextureUnits(elementCount, 0)
 {}
 
 SamplerBinding::SamplerBinding(const SamplerBinding &other) = default;
@@ -1063,9 +1059,8 @@ ProgramAliasedBindings::const_iterator ProgramAliasedBindings::end() const
 }
 
 // ImageBinding implementation.
-ImageBinding::ImageBinding(size_t count) : boundImageUnits(count, 0), unreferenced(false) {}
-ImageBinding::ImageBinding(GLuint imageUnit, size_t count, bool unreferenced)
-    : unreferenced(unreferenced)
+ImageBinding::ImageBinding(size_t count) : boundImageUnits(count, 0) {}
+ImageBinding::ImageBinding(GLuint imageUnit, size_t count)
 {
     for (size_t index = 0; index < count; ++index)
     {
@@ -2907,7 +2902,9 @@ GLuint Program::getSamplerUniformBinding(const VariableLocation &uniformLocation
     GLuint samplerIndex = mState.getSamplerIndexFromUniformIndex(uniformLocation.index);
     const std::vector<GLuint> &boundTextureUnits =
         mState.mExecutable->mSamplerBindings[samplerIndex].boundTextureUnits;
-    return boundTextureUnits[uniformLocation.arrayIndex];
+    return (uniformLocation.arrayIndex < boundTextureUnits.size())
+               ? boundTextureUnits[uniformLocation.arrayIndex]
+               : 0;
 }
 
 GLuint Program::getImageUniformBinding(const VariableLocation &uniformLocation) const
@@ -3728,8 +3725,7 @@ void Program::linkSamplerAndImageBindings(GLuint *combinedImageUniforms)
         else
         {
             imageBindings.emplace_back(ImageBinding(imageUniform.binding + arrayOffset,
-                                                    imageUniform.getBasicTypeElementCount(),
-                                                    false));
+                                                    imageUniform.getBasicTypeElementCount()));
         }
 
         GLuint arraySize = imageUniform.isArray() ? imageUniform.arraySizes[0] : 1u;
@@ -3761,7 +3757,7 @@ void Program::linkSamplerAndImageBindings(GLuint *combinedImageUniforms)
         TextureType textureType    = SamplerTypeToTextureType(samplerUniform.type);
         unsigned int elementCount  = samplerUniform.getBasicTypeElementCount();
         SamplerFormat format       = samplerUniform.typeInfo->samplerFormat;
-        mState.mExecutable->mSamplerBindings.emplace_back(textureType, format, elementCount, false);
+        mState.mExecutable->mSamplerBindings.emplace_back(textureType, format, elementCount);
     }
 
     // Whatever is left constitutes the default uniforms.
@@ -4921,17 +4917,23 @@ void Program::updateSamplerUniform(Context *context,
     SamplerBinding &samplerBinding = mState.mExecutable->mSamplerBindings[samplerIndex];
     std::vector<GLuint> &boundTextureUnits = samplerBinding.boundTextureUnits;
 
-    if (samplerBinding.unreferenced)
+    if (locationInfo.arrayIndex >= boundTextureUnits.size())
+    {
         return;
+    }
+    GLsizei safeUniformCount = std::min(
+        clampedCount, static_cast<GLsizei>(boundTextureUnits.size() - locationInfo.arrayIndex));
 
     // Update the sampler uniforms.
-    for (GLsizei arrayIndex = 0; arrayIndex < clampedCount; ++arrayIndex)
+    for (GLsizei arrayIndex = 0; arrayIndex < safeUniformCount; ++arrayIndex)
     {
         GLint oldTextureUnit = boundTextureUnits[arrayIndex + locationInfo.arrayIndex];
         GLint newTextureUnit = v[arrayIndex];
 
         if (oldTextureUnit == newTextureUnit)
+        {
             continue;
+        }
 
         boundTextureUnits[arrayIndex + locationInfo.arrayIndex] = newTextureUnit;
 
@@ -5277,7 +5279,6 @@ angle::Result Program::serialize(const Context *context, angle::MemoryBuffer *bi
         stream.writeEnum(samplerBinding.textureType);
         stream.writeEnum(samplerBinding.format);
         stream.writeInt(samplerBinding.boundTextureUnits.size());
-        stream.writeBool(samplerBinding.unreferenced);
     }
 
     stream.writeInt(mState.getImageUniformRange().low());
@@ -5524,9 +5525,7 @@ angle::Result Program::deserialize(const Context *context,
         TextureType textureType = stream.readEnum<TextureType>();
         SamplerFormat format    = stream.readEnum<SamplerFormat>();
         size_t bindingCount     = stream.readInt<size_t>();
-        bool unreferenced       = stream.readBool();
-        mState.mExecutable->mSamplerBindings.emplace_back(textureType, format, bindingCount,
-                                                          unreferenced);
+        mState.mExecutable->mSamplerBindings.emplace_back(textureType, format, bindingCount);
     }
 
     unsigned int imageRangeLow             = stream.readInt<unsigned int>();

src/libANGLE/ProgramExecutable.cpp

@@ -284,9 +284,6 @@ void ProgramExecutable::updateActiveSamplers(const ProgramState &programState)
     for (uint32_t samplerIndex = 0; samplerIndex < samplerBindings.size(); ++samplerIndex)
     {
         const SamplerBinding &samplerBinding = samplerBindings[samplerIndex];
-        if (samplerBinding.unreferenced)
-            continue;
-
         uint32_t uniformIndex = programState.getUniformIndexFromSamplerIndex(samplerIndex);
         const gl::LinkedUniform &samplerUniform = programState.getUniforms()[uniformIndex];
 
@@ -320,10 +317,6 @@ void ProgramExecutable::updateActiveImages(const ProgramExecutable &executable)
     for (uint32_t imageIndex = 0; imageIndex < imageBindings->size(); ++imageIndex)
     {
         const gl::ImageBinding &imageBinding = imageBindings->at(imageIndex);
-        if (imageBinding.unreferenced)
-        {
-            continue;
-        }
 
         uint32_t uniformIndex = executable.getUniformIndexFromImageIndex(imageIndex);
         const gl::LinkedUniform &imageUniform = executable.getUniforms()[uniformIndex];
@@ -353,9 +346,6 @@ void ProgramExecutable::setSamplerUniformTextureTypeAndFormat(
 
     for (const SamplerBinding &binding : samplerBindings)
     {
-        if (binding.unreferenced)
-            continue;
-
         // A conflict exists if samplers of different types are sourced by the same texture unit.
         // We need to check all bound textures to detect this error case.
         for (GLuint textureUnit : binding.boundTextureUnits)

src/libANGLE/ProgramExecutable.h

@@ -24,10 +24,7 @@ namespace gl
 // This small structure encapsulates binding sampler uniforms to active GL textures.
 struct SamplerBinding
 {
-    SamplerBinding(TextureType textureTypeIn,
-                   SamplerFormat formatIn,
-                   size_t elementCount,
-                   bool unreferenced);
+    SamplerBinding(TextureType textureTypeIn, SamplerFormat formatIn, size_t elementCount);
     SamplerBinding(const SamplerBinding &other);
     ~SamplerBinding();
 
@@ -37,23 +34,20 @@ struct SamplerBinding
     SamplerFormat format;
 
     // List of all textures bound to this sampler, of type textureType.
+    // Cropped by the amount of unused elements reported by the driver.
     std::vector<GLuint> boundTextureUnits;
-
-    // A note if this sampler is an unreferenced uniform.
-    bool unreferenced;
 };
 
 struct ImageBinding
 {
     ImageBinding(size_t count);
-    ImageBinding(GLuint imageUnit, size_t count, bool unreferenced);
+    ImageBinding(GLuint imageUnit, size_t count);
     ImageBinding(const ImageBinding &other);
     ~ImageBinding();
 
+    // List of all textures bound.
+    // Cropped by the amount of unused elements reported by the driver.
     std::vector<GLuint> boundImageUnits;
-
-    // A note if this image unit is an unreferenced uniform.
-    bool unreferenced;
 };
 
 // A varying with transform feedback enabled. If it's an array, either the whole array or one of its

src/libANGLE/renderer/gl/ProgramGL.cpp

@@ -1089,12 +1089,22 @@ void ProgramGL::markUnusedUniformLocations(std::vector<gl::VariableLocation> *un
             if (mState.isSamplerUniformIndex(locationRef.index))
             {
                 GLuint samplerIndex = mState.getSamplerIndexFromUniformIndex(locationRef.index);
-                (*samplerBindings)[samplerIndex].unreferenced = true;
+                gl::SamplerBinding &samplerBinding = (*samplerBindings)[samplerIndex];
+                if (locationRef.arrayIndex < samplerBinding.boundTextureUnits.size())
+                {
+                    // Crop unused sampler bindings in the sampler array.
+                    samplerBinding.boundTextureUnits.resize(locationRef.arrayIndex);
+                }
             }
             else if (mState.isImageUniformIndex(locationRef.index))
             {
                 GLuint imageIndex = mState.getImageIndexFromUniformIndex(locationRef.index);
-                (*imageBindings)[imageIndex].unreferenced = true;
+                gl::ImageBinding &imageBinding = (*imageBindings)[imageIndex];
+                if (locationRef.arrayIndex < imageBinding.boundImageUnits.size())
+                {
+                    // Crop unused image bindings in the image array.
+                    imageBinding.boundImageUnits.resize(locationRef.arrayIndex);
+                }
             }
             // If the location has been previously bound by a glBindUniformLocation call, it should
             // be marked as ignored. Otherwise it's unused.

src/libANGLE/renderer/metal/ProgramMtl.mm

@@ -968,9 +968,6 @@ angle::Result ProgramMtl::updateTextures(const gl::Context *glContext,
              ++textureIndex)
         {
             const gl::SamplerBinding &samplerBinding = mState.getSamplerBindings()[textureIndex];
-
-            ASSERT(!samplerBinding.unreferenced);
-
             const mtl::SamplerBinding &mslBinding = shaderInfo.actualSamplerBindings[textureIndex];
             if (mslBinding.textureBinding >= mtl::kMaxShaderSamplers)
             {

src/libANGLE/renderer/vulkan/ProgramExecutableVk.cpp

@@ -1225,8 +1225,6 @@ angle::Result ProgramExecutableVk::updateImagesDescriptorSet(
 
         GetImageNameWithoutIndices(&mappedImageName);
 
-        ASSERT(!imageBinding.unreferenced);
-
         uint32_t arrayOffset = 0;
         uint32_t arraySize   = static_cast<uint32_t>(imageBinding.boundImageUnits.size());
 
@@ -1429,9 +1427,6 @@ angle::Result ProgramExecutableVk::updateTexturesDescriptorSet(ContextVk *contex
         {
             const gl::SamplerBinding &samplerBinding =
                 programState->getSamplerBindings()[textureIndex];
-
-            ASSERT(!samplerBinding.unreferenced);
-
             uint32_t uniformIndex = programState->getUniformIndexFromSamplerIndex(textureIndex);
             const gl::LinkedUniform &samplerUniform = programState->getUniforms()[uniformIndex];
             std::string mappedSamplerName = GlslangGetMappedSamplerName(samplerUniform.name);

src/tests/gl_tests/ComputeShaderTest.cpp

@@ -808,6 +808,74 @@ void main()
     }
 }
 
+// When an image array is declared without a binding qualifier, all elements are bound to unit zero.
+// Check that the unused uniform image array element does not cause any corruption. Checks for a bug
+// where unused element could make the whole array seem as unused.
+TEST_P(ComputeShaderTest, ImageArrayUnusedElement)
+{
+    ANGLE_SKIP_TEST_IF(IsD3D11());
+
+    // TODO(xinghua.cao@intel.com): On AMD desktop OpenGL, bind two image variables to unit 0,
+    // only one variable is valid.
+    ANGLE_SKIP_TEST_IF(IsAMD() && IsDesktopOpenGL());
+
+    // Vulkan is currently unable to handle unbound image units in compute shaders.
+    // http://anglebug.com/5026
+    ANGLE_SKIP_TEST_IF(IsVulkan());
+
+    GLFramebuffer framebuffer;
+    constexpr char kCS[] = R"(#version 310 es
+layout(local_size_x=1, local_size_y=1, local_size_z=1) in;
+layout(r32ui, binding=0) writeonly uniform highp uimage2D uOut;
+layout(r32ui, binding=1) readonly uniform highp uimage2D uIn[2];
+
+void main()
+{
+    uvec4 inValue = imageLoad(uIn[0], ivec2(gl_LocalInvocationID.xy));
+    imageStore(uOut, ivec2(gl_LocalInvocationIndex, 0), inValue);
+    imageStore(uOut, ivec2(gl_LocalInvocationIndex, 1), inValue);
+})";
+
+    ANGLE_GL_COMPUTE_PROGRAM(program, kCS);
+    glUseProgram(program.get());
+    constexpr int kTextureWidth = 1, kTextureHeight = 2;
+    GLuint inputValues[] = {100, 100};
+    GLTexture in;
+    glBindTexture(GL_TEXTURE_2D, in);
+    glTexStorage2D(GL_TEXTURE_2D, 1, GL_R32UI, kTextureWidth, kTextureHeight);
+    glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kTextureWidth, kTextureHeight, GL_RED_INTEGER,
+                    GL_UNSIGNED_INT, inputValues);
+    EXPECT_GL_NO_ERROR();
+    glBindImageTexture(1, in, 0, GL_FALSE, 0, GL_READ_ONLY, GL_R32UI);
+
+    GLuint initValues[] = {111, 111};
+    GLTexture out;
+    glBindTexture(GL_TEXTURE_2D, out);
+    glTexStorage2D(GL_TEXTURE_2D, 1, GL_R32UI, kTextureWidth, kTextureHeight);
+    glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kTextureWidth, kTextureHeight, GL_RED_INTEGER,
+                    GL_UNSIGNED_INT, initValues);
+    EXPECT_GL_NO_ERROR();
+
+    glBindImageTexture(0, out, 0, GL_FALSE, 0, GL_WRITE_ONLY, GL_R32UI);
+    glDispatchCompute(1, 1, 1);
+    EXPECT_GL_NO_ERROR();
+
+    glMemoryBarrier(GL_FRAMEBUFFER_BARRIER_BIT);
+    glUseProgram(0);
+    glBindFramebuffer(GL_READ_FRAMEBUFFER, framebuffer);
+
+    glFramebufferTexture2D(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, out, 0);
+    GLuint outputValues[kTextureWidth * kTextureHeight];
+    glReadPixels(0, 0, kTextureWidth, kTextureHeight, GL_RED_INTEGER, GL_UNSIGNED_INT,
+                 outputValues);
+    EXPECT_GL_NO_ERROR();
+
+    GLuint expectedValue = 100;
+    for (int i = 0; i < kTextureWidth * kTextureHeight; i++)
+    {
+        EXPECT_EQ(expectedValue, outputValues[i]);
+    }
+}
 // imageLoad functions
 TEST_P(ComputeShaderTest, ImageLoad)
 {

src/tests/gl_tests/UniformTest.cpp

@@ -1408,6 +1408,45 @@ TEST_P(UniformTest, UniformWithReservedOpenGLName)
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::white);
 }
 
+// Test that unused sampler array elements do not corrupt used sampler array elements. Checks for a
+// bug where unused samplers in an array would mark the whole array unused.
+TEST_P(UniformTest, UnusedUniformsInSamplerArray)
+{
+    constexpr char kVS[] = R"(precision highp float;
+attribute vec4 position;
+varying vec2 texcoord;
+void main()
+{
+    gl_Position = position;
+    texcoord = (position.xy * 0.5) + 0.5;
+})";
+    constexpr char kFS[] = R"(precision highp float;
+uniform sampler2D tex[3];
+varying vec2 texcoord;
+void main()
+{
+    gl_FragColor = texture2D(tex[0], texcoord);
+})";
+
+    mProgram = CompileProgram(kVS, kFS);
+
+    ASSERT_NE(mProgram, 0u);
+    GLint texLocation = glGetUniformLocation(mProgram, "tex[0]");
+    ASSERT_NE(-1, texLocation);
+    glUseProgram(mProgram);
+    glUniform1i(texLocation, 0);
+    GLTexture tex;
+    glActiveTexture(GL_TEXTURE0);
+    glBindTexture(GL_TEXTURE_2D, tex);
+    constexpr GLsizei kTextureSize = 2;
+    std::vector<GLColor> textureData(kTextureSize * kTextureSize, GLColor::green);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, kTextureSize, kTextureSize, 0, GL_RGBA,
+                 GL_UNSIGNED_BYTE, textureData.data());
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+    drawQuad(mProgram, "position", 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these
 // tests should be run against.
 ANGLE_INSTANTIATE_TEST_ES2_AND_ES3(SimpleUniformTest);