Fix bug in updating vertex attribs with client memory pointers

src/libANGLE/renderer/gl/VertexArrayGL.cpp

@@ -467,32 +467,30 @@ void VertexArrayGL::updateAttribPointer(size_t attribIndex)
         return;
     }
 
-    mStateManager->bindVertexArray(mVertexArrayID, getAppliedElementArrayBufferID());
+    // Skip the attribute that is disabled and uses a client memory pointer.
     const Buffer *arrayBuffer = binding.buffer.get();
-    if (arrayBuffer != nullptr)
-    {
-        const BufferGL *arrayBufferGL = GetImplAs<BufferGL>(arrayBuffer);
-        mStateManager->bindBuffer(GL_ARRAY_BUFFER, arrayBufferGL->getBufferID());
-    }
-    else
+    if (arrayBuffer == nullptr)
     {
-        mStateManager->bindBuffer(GL_ARRAY_BUFFER, 0);
+        ASSERT(!attrib.enabled);
+
+        // Mark the applied attribute as dirty by setting an invalid size so that if it doesn't
+        // use a client memory pointer later, there is no chance that the caching will skip it.
+        mAppliedAttributes[attribIndex].size = static_cast<GLuint>(-1);
+        return;
     }
 
-    mAppliedBindings[bindingIndex].buffer = binding.buffer;
+    mStateManager->bindVertexArray(mVertexArrayID, getAppliedElementArrayBufferID());
 
-    const GLvoid *inputPointer = nullptr;
-    if (arrayBuffer != nullptr)
-    {
-        inputPointer = static_cast<const GLvoid *>(
-            reinterpret_cast<const uint8_t *>(binding.offset + attrib.relativeOffset));
-    }
-    else
-    {
-        // Attributes using client memory ignore the VERTEX_ATTRIB_BINDING state.
-        // https://www.opengl.org/registry/specs/ARB/vertex_attrib_binding
-        inputPointer = attrib.pointer;
-    }
+    // Since ANGLE always uses a non-zero VAO, we cannot use a client memory pointer on it:
+    // [OpenGL ES 3.0.2] Section 2.8 page 24:
+    // An INVALID_OPERATION error is generated when a non-zero vertex array object is bound,
+    // zero is bound to the ARRAY_BUFFER buffer object binding point, and the pointer argument
+    // is not NULL.
+    ASSERT(arrayBuffer != nullptr);
+    const BufferGL *arrayBufferGL = GetImplAs<BufferGL>(arrayBuffer);
+    mStateManager->bindBuffer(GL_ARRAY_BUFFER, arrayBufferGL->getBufferID());
+    const GLvoid *inputPointer =
+        reinterpret_cast<const uint8_t *>(binding.offset + attrib.relativeOffset);
 
     if (attrib.pureInteger)
     {
@@ -514,6 +512,7 @@ void VertexArrayGL::updateAttribPointer(size_t attribIndex)
 
     mAppliedBindings[bindingIndex].stride = binding.stride;
     mAppliedBindings[bindingIndex].offset = binding.offset;
+    mAppliedBindings[bindingIndex].buffer = binding.buffer;
 }
 
 void VertexArrayGL::updateAttribDivisor(size_t attribIndex)

src/tests/gl_tests/VertexAttributeTest.cpp

@@ -5,6 +5,7 @@
 //
 
 #include "test_utils/ANGLETest.h"
+#include "test_utils/gl_raii.h"
 
 using namespace angle;
 
@@ -281,7 +282,6 @@ class VertexAttributeTest : public ANGLETest
 
 TEST_P(VertexAttributeTest, UnsignedByteUnnormalized)
 {
-
     std::array<GLubyte, kVertexCount> inputData = {
         {0, 1, 2, 3, 4, 5, 6, 7, 125, 126, 127, 128, 129, 250, 251, 252, 253, 254, 255}};
     std::array<GLfloat, kVertexCount> expectedData;
@@ -626,6 +626,84 @@ TEST_P(VertexAttributeTest, DrawArraysWithBufferOffset)
     EXPECT_GL_NO_ERROR();
 }
 
+// Verify that when we pass a client memory pointer to a disabled attribute the draw is still
+// correct.
+TEST_P(VertexAttributeTest, DrawArraysWithDisabledAttribute)
+{
+    initBasicProgram();
+
+    std::array<GLfloat, kVertexCount> inputData;
+    std::array<GLfloat, kVertexCount> expectedData;
+    InitTestData(inputData, expectedData);
+
+    auto quadVertices        = GetQuadVertices();
+    GLsizei quadVerticesSize = static_cast<GLsizei>(quadVertices.size() * sizeof(quadVertices[0]));
+
+    glGenBuffers(1, &mQuadBuffer);
+    glBindBuffer(GL_ARRAY_BUFFER, mQuadBuffer);
+    glBufferData(GL_ARRAY_BUFFER, quadVerticesSize, quadVertices.data(), GL_STATIC_DRAW);
+
+    GLint positionLocation = glGetAttribLocation(mProgram, "position");
+    ASSERT_NE(-1, positionLocation);
+    glVertexAttribPointer(positionLocation, 3, GL_FLOAT, GL_FALSE, 0, nullptr);
+    glEnableVertexAttribArray(positionLocation);
+
+    glBindBuffer(GL_ARRAY_BUFFER, mBuffer);
+    glBufferData(GL_ARRAY_BUFFER, sizeof(inputData), inputData.data(), GL_STATIC_DRAW);
+    glVertexAttribPointer(mTestAttrib, 1, GL_FLOAT, GL_FALSE, 0, nullptr);
+    glEnableVertexAttribArray(mTestAttrib);
+
+    glBindBuffer(GL_ARRAY_BUFFER, 0);
+    glVertexAttribPointer(mExpectedAttrib, 1, GL_FLOAT, GL_FALSE, 0, expectedData.data());
+    glEnableVertexAttribArray(mExpectedAttrib);
+
+    // mProgram2 adds an attribute 'disabled' on the basis of mProgram.
+    const std::string testVertexShaderSource2 =
+        "attribute mediump vec4 position;\n"
+        "attribute mediump vec4 test;\n"
+        "attribute mediump vec4 expected;\n"
+        "attribute mediump vec4 disabled;\n"
+        "varying mediump vec4 color;\n"
+        "void main(void)\n"
+        "{\n"
+        "    gl_Position = position;\n"
+        "    vec4 threshold = max(abs(expected + disabled) * 0.005, 1.0 / 64.0);\n"
+        "    color = vec4(lessThanEqual(abs(test - expected), threshold));\n"
+        "}\n";
+
+    const std::string testFragmentShaderSource =
+        "varying mediump vec4 color;\n"
+        "void main(void)\n"
+        "{\n"
+        "    gl_FragColor = color;\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, testVertexShaderSource2, testFragmentShaderSource);
+    GLuint mProgram2 = program.get();
+
+    ASSERT_EQ(positionLocation, glGetAttribLocation(mProgram2, "position"));
+    ASSERT_EQ(mTestAttrib, glGetAttribLocation(mProgram2, "test"));
+    ASSERT_EQ(mExpectedAttrib, glGetAttribLocation(mProgram2, "expected"));
+
+    // Pass a client memory pointer to disabledAttribute and disable it.
+    GLint disabledAttribute = glGetAttribLocation(mProgram2, "disabled");
+    ASSERT_EQ(-1, glGetAttribLocation(mProgram, "disabled"));
+    glVertexAttribPointer(disabledAttribute, 1, GL_FLOAT, GL_FALSE, 0, expectedData.data());
+    glDisableVertexAttribArray(disabledAttribute);
+
+    glUseProgram(mProgram);
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    checkPixels();
+
+    // Now enable disabledAttribute which should be used in mProgram2.
+    glEnableVertexAttribArray(disabledAttribute);
+    glUseProgram(mProgram2);
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    checkPixels();
+
+    EXPECT_GL_NO_ERROR();
+}
+
 class VertexAttributeTestES31 : public VertexAttributeTestES3
 {
   protected:
@@ -874,7 +952,7 @@ void VertexAttributeCachingTest::SetUp()
 
     mExpectedData[GL_BYTE]          = GetExpectedData<GLbyte>(srcData, GL_BYTE, GL_FALSE);
     mExpectedData[GL_UNSIGNED_BYTE] = GetExpectedData<GLubyte>(srcData, GL_UNSIGNED_BYTE, GL_FALSE);
-    mExpectedData[GL_SHORT] = GetExpectedData<GLshort>(srcData, GL_SHORT, GL_FALSE);
+    mExpectedData[GL_SHORT]         = GetExpectedData<GLshort>(srcData, GL_SHORT, GL_FALSE);
     mExpectedData[GL_UNSIGNED_SHORT] =
         GetExpectedData<GLushort>(srcData, GL_UNSIGNED_SHORT, GL_FALSE);
     mExpectedData[GL_INT]          = GetExpectedData<GLint>(srcData, GL_INT, GL_FALSE);
@@ -981,8 +1059,10 @@ TEST_P(VertexAttributeCachingTest, BufferMulticachingWithOneUnchangedAttrib)
     }
 }
 
-// Use this to select which configurations (e.g. which renderer, which GLES major version) these tests should be run against.
-// D3D11 Feature Level 9_3 uses different D3D formats for vertex attribs compared to Feature Levels 10_0+, so we should test them separately.
+// Use this to select which configurations (e.g. which renderer, which GLES major version) these
+// tests should be run against.
+// D3D11 Feature Level 9_3 uses different D3D formats for vertex attribs compared to Feature Levels
+// 10_0+, so we should test them separately.
 ANGLE_INSTANTIATE_TEST(VertexAttributeTest,
                        ES2_D3D9(),
                        ES2_D3D11(),