Reland of "Store the applied element array buffer as a binding pointer."

src/libANGLE/Context.cpp

@@ -1422,9 +1422,9 @@ void Context::detachBuffer(GLuint buffer)
     mState.removeArrayBufferBinding(buffer);
 
     // mark as freed among the vertex array objects
-    for (auto vaoIt = mVertexArrayMap.begin(); vaoIt != mVertexArrayMap.end(); vaoIt++)
+    for (auto &vaoPair : mVertexArrayMap)
     {
-        vaoIt->second->detachBuffer(buffer);
+        vaoPair.second->detachBuffer(buffer);
     }
 }
 

src/libANGLE/VertexArray.cpp

@@ -54,14 +54,12 @@ void VertexArray::detachBuffer(GLuint bufferName)
         if (mData.mVertexAttributes[attribute].buffer.id() == bufferName)
         {
             mData.mVertexAttributes[attribute].buffer.set(nullptr);
-            mVertexArray->setAttribute(attribute, mData.mVertexAttributes[attribute]);
         }
     }
 
     if (mData.mElementArrayBuffer.id() == bufferName)
     {
         mData.mElementArrayBuffer.set(nullptr);
-        mVertexArray->setElementArrayBuffer(nullptr);
     }
 }
 
@@ -75,14 +73,12 @@ void VertexArray::setVertexAttribDivisor(size_t index, GLuint divisor)
 {
     ASSERT(index < getMaxAttribs());
     mData.mVertexAttributes[index].divisor = divisor;
-    mVertexArray->setAttributeDivisor(index, divisor);
 }
 
 void VertexArray::enableAttribute(size_t attributeIndex, bool enabledState)
 {
     ASSERT(attributeIndex < getMaxAttribs());
     mData.mVertexAttributes[attributeIndex].enabled = enabledState;
-    mVertexArray->enableAttribute(attributeIndex, enabledState);
 
     // Update state cache
     if (enabledState)
@@ -113,14 +109,11 @@ void VertexArray::setAttributeState(size_t attributeIndex, gl::Buffer *boundBuff
     attrib->pureInteger = pureInteger;
     attrib->stride = stride;
     attrib->pointer = pointer;
-
-    mVertexArray->setAttribute(attributeIndex, *attrib);
 }
 
 void VertexArray::setElementArrayBuffer(Buffer *buffer)
 {
     mData.mElementArrayBuffer.set(buffer);
-    mVertexArray->setElementArrayBuffer(buffer);
 }
 
 }

src/libANGLE/renderer/VertexArrayImpl.h

@@ -22,11 +22,6 @@ class VertexArrayImpl : angle::NonCopyable
     VertexArrayImpl(const gl::VertexArray::Data &data) : mData(data) { }
     virtual ~VertexArrayImpl() { }
 
-    virtual void setElementArrayBuffer(const gl::Buffer *buffer) = 0;
-    virtual void setAttribute(size_t idx, const gl::VertexAttribute &attr) = 0;
-    virtual void setAttributeDivisor(size_t idx, GLuint divisor) = 0;
-    virtual void enableAttribute(size_t idx, bool enabledState) = 0;
-
   protected:
     const gl::VertexArray::Data &mData;
 };

src/libANGLE/renderer/d3d/d3d11/VertexArray11.h

@@ -23,12 +23,7 @@ class VertexArray11 : public VertexArrayImpl
         : VertexArrayImpl(data)
     {
     }
-    virtual ~VertexArray11() { }
-
-    virtual void setElementArrayBuffer(const gl::Buffer *buffer) { }
-    virtual void setAttribute(size_t idx, const gl::VertexAttribute &attr) { }
-    virtual void setAttributeDivisor(size_t idx, GLuint divisor) { }
-    virtual void enableAttribute(size_t idx, bool enabledState) { }
+    virtual ~VertexArray11() {}
 };
 
 }

src/libANGLE/renderer/d3d/d3d9/VertexArray9.h

@@ -25,11 +25,6 @@ class VertexArray9 : public VertexArrayImpl
     }
 
     virtual ~VertexArray9() { }
-
-    virtual void setElementArrayBuffer(const gl::Buffer *buffer) { }
-    virtual void setAttribute(size_t idx, const gl::VertexAttribute &attr) { }
-    virtual void setAttributeDivisor(size_t idx, GLuint divisor) { }
-    virtual void enableAttribute(size_t idx, bool enabledState) { }
 };
 
 }

src/libANGLE/renderer/gl/VertexArrayGL.cpp

@@ -21,12 +21,14 @@
 namespace rx
 {
 
-VertexArrayGL::VertexArrayGL(const gl::VertexArray::Data &data, const FunctionsGL *functions, StateManagerGL *stateManager)
+VertexArrayGL::VertexArrayGL(const gl::VertexArray::Data &data,
+                             const FunctionsGL *functions,
+                             StateManagerGL *stateManager)
     : VertexArrayImpl(data),
       mFunctions(functions),
       mStateManager(stateManager),
       mVertexArrayID(0),
-      mAppliedElementArrayBuffer(0),
+      mAppliedElementArrayBuffer(),
       mStreamingElementArrayBufferSize(0),
       mStreamingElementArrayBuffer(0),
       mStreamingArrayBufferSize(0),
@@ -55,34 +57,13 @@ VertexArrayGL::~VertexArrayGL()
     mStreamingArrayBufferSize = 0;
     mStreamingArrayBuffer = 0;
 
+    mAppliedElementArrayBuffer.set(nullptr);
     for (size_t idx = 0; idx < mAppliedAttributes.size(); idx++)
     {
         mAppliedAttributes[idx].buffer.set(nullptr);
     }
 }
 
-void VertexArrayGL::setElementArrayBuffer(const gl::Buffer *buffer)
-{
-    // If the buffer is being unbound/deleted, reset the currently applied buffer ID
-    // so that even if a new buffer is generated with the same ID, it will be re-bound.
-    if (buffer == nullptr && mAppliedElementArrayBuffer != mStreamingElementArrayBuffer)
-    {
-        mAppliedElementArrayBuffer = 0;
-    }
-}
-
-void VertexArrayGL::setAttribute(size_t idx, const gl::VertexAttribute &attr)
-{
-}
-
-void VertexArrayGL::setAttributeDivisor(size_t idx, GLuint divisor)
-{
-}
-
-void VertexArrayGL::enableAttribute(size_t idx, bool enabledState)
-{
-}
-
 gl::Error VertexArrayGL::syncDrawArraysState(const std::vector<GLuint> &activeAttribLocations, GLint first, GLsizei count) const
 {
     return syncDrawState(activeAttribLocations, first, count, GL_NONE, nullptr, nullptr);
@@ -96,7 +77,7 @@ gl::Error VertexArrayGL::syncDrawElementsState(const std::vector<GLuint> &active
 
 gl::Error VertexArrayGL::syncDrawState(const std::vector<GLuint> &activeAttribLocations, GLint first, GLsizei count, GLenum type, const GLvoid *indices, const GLvoid **outIndices) const
 {
-    mStateManager->bindVertexArray(mVertexArrayID, mAppliedElementArrayBuffer);
+    mStateManager->bindVertexArray(mVertexArrayID, getAppliedElementArrayBufferID());
 
     // Check if any attributes need to be streamed, determines if the index range needs to be computed
     bool attributesNeedStreaming = doAttributesNeedStreaming(activeAttribLocations);
@@ -250,12 +231,11 @@ gl::Error VertexArrayGL::syncIndexData(GLsizei count, GLenum type, const GLvoid
     // Need to check the range of indices if attributes need to be streamed
     if (elementArrayBuffer != nullptr)
     {
-        const BufferGL *bufferGL = GetImplAs<BufferGL>(elementArrayBuffer);
-        GLuint elementArrayBufferID = bufferGL->getBufferID();
-        if (elementArrayBufferID != mAppliedElementArrayBuffer)
+        if (elementArrayBuffer != mAppliedElementArrayBuffer.get())
         {
-            mStateManager->bindBuffer(GL_ELEMENT_ARRAY_BUFFER, elementArrayBufferID);
-            mAppliedElementArrayBuffer = elementArrayBufferID;
+            const BufferGL *bufferGL = GetImplAs<BufferGL>(elementArrayBuffer);
+            mStateManager->bindBuffer(GL_ELEMENT_ARRAY_BUFFER, bufferGL->getBufferID());
+            mAppliedElementArrayBuffer.set(elementArrayBuffer);
         }
 
         // Only compute the index range if the attributes also need to be streamed
@@ -291,7 +271,7 @@ gl::Error VertexArrayGL::syncIndexData(GLsizei count, GLenum type, const GLvoid
         }
 
         mStateManager->bindBuffer(GL_ELEMENT_ARRAY_BUFFER, mStreamingElementArrayBuffer);
-        mAppliedElementArrayBuffer = mStreamingElementArrayBuffer;
+        mAppliedElementArrayBuffer.set(nullptr);
 
         // Make sure the element array buffer is large enough
         const gl::Type &indexTypeInfo = gl::GetTypeInfo(type);
@@ -415,7 +395,12 @@ GLuint VertexArrayGL::getVertexArrayID() const
 
 GLuint VertexArrayGL::getAppliedElementArrayBufferID() const
 {
-    return mAppliedElementArrayBuffer;
+    if (mAppliedElementArrayBuffer.get() == nullptr)
+    {
+        return mStreamingElementArrayBuffer;
+    }
+
+    return GetImplAs<BufferGL>(mAppliedElementArrayBuffer.get())->getBufferID();
 }
 
 }

src/libANGLE/renderer/gl/VertexArrayGL.h

@@ -23,11 +23,6 @@ class VertexArrayGL : public VertexArrayImpl
     VertexArrayGL(const gl::VertexArray::Data &data, const FunctionsGL *functions, StateManagerGL *stateManager);
     ~VertexArrayGL() override;
 
-    void setElementArrayBuffer(const gl::Buffer *buffer) override;
-    void setAttribute(size_t idx, const gl::VertexAttribute &attr) override;
-    void setAttributeDivisor(size_t idx, GLuint divisor) override;
-    void enableAttribute(size_t idx, bool enabledState) override;
-
     gl::Error syncDrawArraysState(const std::vector<GLuint> &activeAttribLoations, GLint first, GLsizei count) const;
     gl::Error syncDrawElementsState(const std::vector<GLuint> &activeAttribLoations, GLsizei count, GLenum type,
                                     const GLvoid *indices, const GLvoid **outIndices) const;
@@ -61,7 +56,7 @@ class VertexArrayGL : public VertexArrayImpl
 
     GLuint mVertexArrayID;
 
-    mutable GLuint mAppliedElementArrayBuffer;
+    mutable BindingPointer<gl::Buffer> mAppliedElementArrayBuffer;
     mutable std::vector<gl::VertexAttribute> mAppliedAttributes;
 
     mutable size_t mStreamingElementArrayBufferSize;

src/tests/angle_end2end_tests.gypi

@@ -24,6 +24,7 @@
             '<(angle_path)/src/tests/gl_tests/DepthStencilFormatsTest.cpp',
             '<(angle_path)/src/tests/gl_tests/DiscardFramebufferEXTTest.cpp',
             '<(angle_path)/src/tests/gl_tests/DrawBuffersTest.cpp',
+            '<(angle_path)/src/tests/gl_tests/DrawElementsTest.cpp',
             '<(angle_path)/src/tests/gl_tests/FenceSyncTests.cpp',
             '<(angle_path)/src/tests/gl_tests/FramebufferFormatsTest.cpp',
             '<(angle_path)/src/tests/gl_tests/FramebufferRenderMipmapTest.cpp',

src/tests/gl_tests/DrawElementsTest.cpp

+//
+// Copyright 2015 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// DrawElementsTest:
+//   Tests for indexed draws.
+//
+
+#include "test_utils/ANGLETest.h"
+
+using namespace angle;
+
+namespace
+{
+
+class DrawElementsTest : public ANGLETest
+{
+  protected:
+    DrawElementsTest() : mProgram(0u)
+    {
+        setWindowWidth(64);
+        setWindowHeight(64);
+        setConfigRedBits(8);
+        setConfigGreenBits(8);
+    }
+
+    ~DrawElementsTest()
+    {
+        for (GLuint indexBuffer : mIndexBuffers)
+        {
+            if (indexBuffer != 0)
+            {
+                glDeleteBuffers(1, &indexBuffer);
+            }
+        }
+
+        for (GLuint vertexArray : mVertexArrays)
+        {
+            if (vertexArray != 0)
+            {
+                glDeleteVertexArrays(1, &vertexArray);
+            }
+        }
+
+        for (GLuint vertexBuffer : mVertexBuffers)
+        {
+            if (vertexBuffer != 0)
+            {
+                glDeleteBuffers(1, &vertexBuffer);
+            }
+        }
+
+        if (mProgram != 0u)
+        {
+            glDeleteProgram(mProgram);
+        }
+    }
+
+    std::vector<GLuint> mIndexBuffers;
+    std::vector<GLuint> mVertexArrays;
+    std::vector<GLuint> mVertexBuffers;
+    GLuint mProgram;
+};
+
+// Test a state desync that can occur when using a streaming index buffer in GL in concert with
+// deleting the applied index buffer.
+TEST_P(DrawElementsTest, DeletingAfterStreamingIndexes)
+{
+    // Init program
+    const std::string &vertexShader =
+        "attribute vec2 position;\n"
+        "attribute vec2 testFlag;\n"
+        "varying vec2 v_data;\n"
+        "void main() {\n"
+        "  gl_Position = vec4(position, 0, 1);\n"
+        "  v_data = testFlag;\n"
+        "}";
+
+    const std::string &fragmentShader =
+        "varying highp vec2 v_data;\n"
+        "void main() {\n"
+        "  gl_FragColor = vec4(v_data, 0, 1);\n"
+        "}";
+
+    mProgram = CompileProgram(vertexShader, fragmentShader);
+    ASSERT_NE(0u, mProgram);
+    glUseProgram(mProgram);
+
+    GLint positionLocation = glGetAttribLocation(mProgram, "position");
+    ASSERT_NE(-1, positionLocation);
+
+    GLint testFlagLocation = glGetAttribLocation(mProgram, "testFlag");
+    ASSERT_NE(-1, testFlagLocation);
+
+    mIndexBuffers.resize(3u);
+    glGenBuffers(3, &mIndexBuffers[0]);
+
+    mVertexArrays.resize(2);
+    glGenVertexArrays(2, &mVertexArrays[0]);
+
+    mVertexBuffers.resize(2);
+    glGenBuffers(2, &mVertexBuffers[0]);
+
+    std::vector<GLuint> indexData[2];
+    indexData[0].push_back(0);
+    indexData[0].push_back(1);
+    indexData[0].push_back(2);
+    indexData[0].push_back(2);
+    indexData[0].push_back(3);
+    indexData[0].push_back(0);
+
+    indexData[1] = indexData[0];
+    for (GLuint &item : indexData[1])
+    {
+        item += 4u;
+    }
+
+    std::vector<GLfloat> positionData;
+    // quad verts
+    positionData.push_back(-1.0f);
+    positionData.push_back(1.0f);
+    positionData.push_back(-1.0f);
+    positionData.push_back(-1.0f);
+    positionData.push_back(1.0f);
+    positionData.push_back(-1.0f);
+    positionData.push_back(1.0f);
+    positionData.push_back(1.0f);
+
+    // Repeat position data
+    positionData.push_back(-1.0f);
+    positionData.push_back(1.0f);
+    positionData.push_back(-1.0f);
+    positionData.push_back(-1.0f);
+    positionData.push_back(1.0f);
+    positionData.push_back(-1.0f);
+    positionData.push_back(1.0f);
+    positionData.push_back(1.0f);
+
+    std::vector<GLfloat> testFlagData;
+    // red
+    testFlagData.push_back(1.0f);
+    testFlagData.push_back(0.0f);
+    testFlagData.push_back(1.0f);
+    testFlagData.push_back(0.0f);
+    testFlagData.push_back(1.0f);
+    testFlagData.push_back(0.0f);
+    testFlagData.push_back(1.0f);
+    testFlagData.push_back(0.0f);
+
+    // green
+    testFlagData.push_back(0.0f);
+    testFlagData.push_back(1.0f);
+    testFlagData.push_back(0.0f);
+    testFlagData.push_back(1.0f);
+    testFlagData.push_back(0.0f);
+    testFlagData.push_back(1.0f);
+    testFlagData.push_back(0.0f);
+    testFlagData.push_back(1.0f);
+
+    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, mIndexBuffers[0]);
+    glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(GLuint) * indexData[0].size(), &indexData[0][0],
+                 GL_STATIC_DRAW);
+
+    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, mIndexBuffers[2]);
+    glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(GLuint) * indexData[0].size(), &indexData[0][0],
+                 GL_STATIC_DRAW);
+
+    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, mIndexBuffers[1]);
+    glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(GLuint) * indexData[1].size(), &indexData[1][0],
+                 GL_STATIC_DRAW);
+
+    // Initialize first vertex array with second index buffer
+    glBindVertexArray(mVertexArrays[0]);
+
+    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, mIndexBuffers[1]);
+    glBindBuffer(GL_ARRAY_BUFFER, mVertexBuffers[0]);
+    glBufferData(GL_ARRAY_BUFFER, sizeof(GLfloat) * positionData.size(), &positionData[0],
+                 GL_STATIC_DRAW);
+    glVertexAttribPointer(positionLocation, 2, GL_FLOAT, GL_FALSE, sizeof(GLfloat) * 2, nullptr);
+    glEnableVertexAttribArray(positionLocation);
+
+    glBindBuffer(GL_ARRAY_BUFFER, mVertexBuffers[1]);
+    glBufferData(GL_ARRAY_BUFFER, sizeof(GLfloat) * testFlagData.size(), &testFlagData[0],
+                 GL_STATIC_DRAW);
+    glVertexAttribPointer(testFlagLocation, 2, GL_FLOAT, GL_FALSE, sizeof(GLfloat) * 2, nullptr);
+    glEnableVertexAttribArray(testFlagLocation);
+
+    // Initialize second vertex array with first index buffer
+    glBindVertexArray(mVertexArrays[1]);
+
+    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, mIndexBuffers[0]);
+
+    glBindBuffer(GL_ARRAY_BUFFER, mVertexBuffers[0]);
+    glVertexAttribPointer(positionLocation, 2, GL_FLOAT, GL_FALSE, sizeof(GLfloat) * 2, nullptr);
+    glEnableVertexAttribArray(positionLocation);
+
+    glBindBuffer(GL_ARRAY_BUFFER, mVertexBuffers[1]);
+    glVertexAttribPointer(testFlagLocation, 2, GL_FLOAT, GL_FALSE, sizeof(GLfloat) * 2, nullptr);
+    glEnableVertexAttribArray(testFlagLocation);
+
+    ASSERT_GL_NO_ERROR();
+
+    glBindVertexArray(mVertexArrays[0]);
+    glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT, nullptr);
+    EXPECT_PIXEL_EQ(0, 0, 0, 255, 0, 255);
+
+    glBindVertexArray(mVertexArrays[1]);
+    glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT, nullptr);
+    EXPECT_PIXEL_EQ(0, 0, 255, 0, 0, 255);
+
+    glBindVertexArray(mVertexArrays[0]);
+    glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT, nullptr);
+    EXPECT_PIXEL_EQ(0, 0, 0, 255, 0, 255);
+
+    // Trigger the bug here.
+    glDeleteBuffers(1, &mIndexBuffers[2]);
+
+    glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT, nullptr);
+    EXPECT_PIXEL_EQ(0, 0, 0, 255, 0, 255);
+
+    ASSERT_GL_NO_ERROR();
+}
+
+ANGLE_INSTANTIATE_TEST(DrawElementsTest, ES3_OPENGL());
+}