Cache VertexArray::hasMappedBuffer.

src/libANGLE/Buffer.cpp

@@ -145,6 +145,9 @@ Error Buffer::map(const Context *context, GLenum access)
     mState.mAccessFlags = GL_MAP_WRITE_BIT;
     mIndexRangeCache.clear();
 
+    // Notify when state changes.
+    mImpl->onStateChange(context, angle::SubjectMessage::RESOURCE_MAPPED);
+
     return NoError();
 }
 
@@ -175,6 +178,9 @@ Error Buffer::mapRange(const Context *context,
         mIndexRangeCache.invalidateRange(static_cast<unsigned int>(offset), static_cast<unsigned int>(length));
     }
 
+    // Notify when state changes.
+    mImpl->onStateChange(context, angle::SubjectMessage::RESOURCE_MAPPED);
+
     return NoError();
 }
 
@@ -193,7 +199,7 @@ Error Buffer::unmap(const Context *context, GLboolean *result)
     mState.mAccessFlags = 0;
 
     // Notify when data changes.
-    mImpl->onStateChange(context, angle::SubjectMessage::CONTENTS_CHANGED);
+    mImpl->onStateChange(context, angle::SubjectMessage::RESOURCE_UNMAPPED);
 
     return NoError();
 }

src/libANGLE/Observer.h

@@ -32,6 +32,8 @@ enum class SubjectMessage
     STORAGE_CHANGED,
     BINDING_CHANGED,
     DEPENDENT_DIRTY_BITS,
+    RESOURCE_MAPPED,
+    RESOURCE_UNMAPPED,
 };
 
 // The observing class inherits from this interface class.

src/libANGLE/VertexArray.cpp

@@ -15,6 +15,14 @@
 
 namespace gl
 {
+namespace
+{
+bool IsElementArrayBufferSubjectIndex(angle::SubjectIndex subjectIndex)
+{
+    return (subjectIndex == MAX_VERTEX_ATTRIBS);
+}
+}  // anonymous namespce
+
 // VertexArrayState implementation.
 VertexArrayState::VertexArrayState(size_t maxAttribs, size_t maxAttribBindings)
     : mLabel(), mVertexBindings()
@@ -57,15 +65,22 @@ void VertexArrayState::setAttribBinding(size_t attribIndex, GLuint newBindingInd
     const GLuint oldBindingIndex = attrib.bindingIndex;
     ASSERT(oldBindingIndex != newBindingIndex);
 
-    ASSERT(mVertexBindings[oldBindingIndex].getBoundAttributesMask().test(attribIndex) &&
-           !mVertexBindings[newBindingIndex].getBoundAttributesMask().test(attribIndex));
+    VertexBinding &oldBinding = mVertexBindings[oldBindingIndex];
+    VertexBinding &newBinding = mVertexBindings[newBindingIndex];
+
+    ASSERT(oldBinding.getBoundAttributesMask().test(attribIndex) &&
+           !newBinding.getBoundAttributesMask().test(attribIndex));
 
-    mVertexBindings[oldBindingIndex].resetBoundAttribute(attribIndex);
-    mVertexBindings[newBindingIndex].setBoundAttribute(attribIndex);
+    oldBinding.resetBoundAttribute(attribIndex);
+    newBinding.setBoundAttribute(attribIndex);
 
     // Set the attribute using the new binding.
     attrib.bindingIndex = newBindingIndex;
-    attrib.updateCachedElementLimit(mVertexBindings[newBindingIndex]);
+    attrib.updateCachedElementLimit(newBinding);
+
+    bool isMapped = newBinding.getBuffer().get() && newBinding.getBuffer()->isMapped();
+    mCachedMappedArrayBuffers.set(attribIndex, isMapped);
+    mCachedEnabledMappedArrayBuffers.set(attribIndex, isMapped && attrib.enabled);
 }
 
 // VertexArray implementation.
@@ -76,7 +91,7 @@ VertexArray::VertexArray(rx::GLImplFactory *factory,
     : mId(id),
       mState(maxAttribs, maxAttribBindings),
       mVertexArray(factory->createVertexArray(mState)),
-      mElementArrayBufferObserverBinding(this, maxAttribBindings)
+      mElementArrayBufferObserverBinding(this, MAX_VERTEX_ATTRIBS)
 {
     for (size_t attribIndex = 0; attribIndex < maxAttribBindings; ++attribIndex)
     {
@@ -190,6 +205,7 @@ void VertexArray::bindVertexBufferImpl(const Context *context,
     updateObserverBinding(bindingIndex);
     updateCachedBufferBindingSize(binding);
     updateCachedTransformFeedbackBindingValidation(bindingIndex, boundBuffer);
+    updateCachedMappedArrayBuffers(binding);
 
     // Update client memory attribute pointers. Affects all bound attributes.
     if (boundBuffer)
@@ -303,6 +319,8 @@ void VertexArray::enableAttribute(size_t attribIndex, bool enabledState)
 
     // Update state cache
     mState.mEnabledAttributesMask.set(attribIndex, enabledState);
+    mState.mCachedEnabledMappedArrayBuffers =
+        mState.mCachedMappedArrayBuffers & mState.mEnabledAttributesMask;
 }
 
 void VertexArray::setVertexAttribPointer(const Context *context,
@@ -385,7 +403,7 @@ void VertexArray::onBindingChanged(const Context *context, bool bound)
 VertexArray::DirtyBitType VertexArray::getDirtyBitFromIndex(bool contentsChanged,
                                                             angle::SubjectIndex index) const
 {
-    if (index == mArrayBufferObserverBindings.size())
+    if (IsElementArrayBufferSubjectIndex(index))
     {
         return contentsChanged ? DIRTY_BIT_ELEMENT_ARRAY_BUFFER_DATA
                                : DIRTY_BIT_ELEMENT_ARRAY_BUFFER;
@@ -410,7 +428,7 @@ void VertexArray::onSubjectStateChange(const gl::Context *context,
             break;
 
         case angle::SubjectMessage::STORAGE_CHANGED:
-            if (index < mArrayBufferObserverBindings.size())
+            if (!IsElementArrayBufferSubjectIndex(index))
             {
                 updateCachedBufferBindingSize(&mState.mVertexBindings[index]);
             }
@@ -418,13 +436,29 @@ void VertexArray::onSubjectStateChange(const gl::Context *context,
             break;
 
         case angle::SubjectMessage::BINDING_CHANGED:
-            if (index < mArrayBufferObserverBindings.size())
+            if (!IsElementArrayBufferSubjectIndex(index))
             {
                 const Buffer *buffer = mState.mVertexBindings[index].getBuffer().get();
                 updateCachedTransformFeedbackBindingValidation(index, buffer);
             }
             break;
 
+        case angle::SubjectMessage::RESOURCE_MAPPED:
+            if (!IsElementArrayBufferSubjectIndex(index))
+            {
+                updateCachedMappedArrayBuffers(&mState.mVertexBindings[index]);
+            }
+            break;
+
+        case angle::SubjectMessage::RESOURCE_UNMAPPED:
+            setDependentDirtyBit(context, true, index);
+
+            if (!IsElementArrayBufferSubjectIndex(index))
+            {
+                updateCachedMappedArrayBuffers(&mState.mVertexBindings[index]);
+            }
+            break;
+
         default:
             UNREACHABLE();
             break;
@@ -486,21 +520,19 @@ bool VertexArray::hasTransformFeedbackBindingConflict(const gl::Context *context
     return false;
 }
 
-bool VertexArray::hasMappedEnabledArrayBuffer() const
+void VertexArray::updateCachedMappedArrayBuffers(VertexBinding *binding)
 {
-    // TODO(jmadill): Cache this. http://anglebug.com/2746
-    for (size_t attribIndex : mState.mEnabledAttributesMask)
+    Buffer *buffer = binding->getBuffer().get();
+    if (buffer && buffer->isMapped())
     {
-        const VertexAttribute &vertexAttrib = mState.mVertexAttributes[attribIndex];
-        ASSERT(vertexAttrib.enabled);
-        const VertexBinding &vertexBinding = mState.mVertexBindings[vertexAttrib.bindingIndex];
-        gl::Buffer *boundBuffer            = vertexBinding.getBuffer().get();
-        if (boundBuffer && boundBuffer->isMapped())
-        {
-            return true;
-        }
+        mState.mCachedMappedArrayBuffers |= binding->getBoundAttributesMask();
+    }
+    else
+    {
+        mState.mCachedMappedArrayBuffers &= ~binding->getBoundAttributesMask();
     }
 
-    return false;
+    mState.mCachedEnabledMappedArrayBuffers =
+        mState.mCachedMappedArrayBuffers & mState.mEnabledAttributesMask;
 }
 }  // namespace gl

src/libANGLE/VertexArray.h

@@ -86,6 +86,10 @@ class VertexArrayState final : angle::NonCopyable
     // attribs.
     gl::AttributesMask mClientMemoryAttribsMask;
     gl::AttributesMask mNullPointerClientMemoryAttribsMask;
+
+    // Used for validation cache. Indexed by attribute.
+    AttributesMask mCachedMappedArrayBuffers;
+    AttributesMask mCachedEnabledMappedArrayBuffers;
 };
 
 class VertexArray final : public angle::ObserverInterface,
@@ -178,7 +182,10 @@ class VertexArray final : public angle::ObserverInterface,
         return mState.hasEnabledNullPointerClientArray();
     }
 
-    bool hasMappedEnabledArrayBuffer() const;
+    bool hasMappedEnabledArrayBuffer() const
+    {
+        return mState.mCachedEnabledMappedArrayBuffers.any();
+    }
 
     // Observer implementation
     void onSubjectStateChange(const gl::Context *context,
@@ -265,6 +272,7 @@ class VertexArray final : public angle::ObserverInterface,
     // These are used to optimize draw call validation.
     void updateCachedBufferBindingSize(VertexBinding *binding);
     void updateCachedTransformFeedbackBindingValidation(size_t bindingIndex, const Buffer *buffer);
+    void updateCachedMappedArrayBuffers(VertexBinding *binding);
 
     GLuint mId;
 

src/tests/gl_tests/StateChangeTest.cpp

@@ -2062,6 +2062,254 @@ TEST_P(SimpleStateChangeTest, ReleaseShaderInUseThatReadsFromUniforms)
     EXPECT_PIXEL_COLOR_EQ(getWindowWidth() / 2, 0, GLColor::red);
 }
 
+static constexpr char kColorVS[] = R"(attribute vec2 position;
+attribute vec4 color;
+varying vec4 vColor;
+void main()
+{
+    gl_Position = vec4(position, 0, 1);
+    vColor = color;
+})";
+
+static constexpr char kColorFS[] = R"(precision mediump float;
+varying vec4 vColor;
+void main()
+{
+    gl_FragColor = vColor;
+})";
+
+class ValidationStateChangeTest : public ANGLETest
+{
+  protected:
+    ValidationStateChangeTest()
+    {
+        setWindowWidth(64);
+        setWindowHeight(64);
+        setConfigRedBits(8);
+        setConfigGreenBits(8);
+        setConfigBlueBits(8);
+        setConfigAlphaBits(8);
+    }
+};
+
+class ValidationStateChangeTestES31 : public ANGLETest
+{
+};
+
+// Tests that mapping and unmapping an array buffer in various ways causes rendering to fail.
+// This isn't guaranteed to produce an error by GL. But we assume ANGLE always errors.
+TEST_P(ValidationStateChangeTest, MapBufferAndDraw)
+{
+    // Initialize program and set up state.
+    ANGLE_GL_PROGRAM(program, kColorVS, kColorFS);
+
+    glUseProgram(program);
+    GLint positionLoc = glGetAttribLocation(program, "position");
+    ASSERT_NE(-1, positionLoc);
+    GLint colorLoc = glGetAttribLocation(program, "color");
+    ASSERT_NE(-1, colorLoc);
+
+    const std::array<Vector3, 6> &quadVertices = GetQuadVertices();
+    const size_t posBufferSize                 = quadVertices.size() * sizeof(Vector3);
+
+    GLBuffer posBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, posBuffer);
+    glBufferData(GL_ARRAY_BUFFER, posBufferSize, quadVertices.data(), GL_STATIC_DRAW);
+
+    // Start with position enabled.
+    glVertexAttribPointer(positionLoc, 3, GL_FLOAT, GL_FALSE, 0, nullptr);
+    glEnableVertexAttribArray(positionLoc);
+
+    std::vector<GLColor> colorVertices(6, GLColor::blue);
+    const size_t colorBufferSize = sizeof(GLColor) * 6;
+
+    GLBuffer colorBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, colorBuffer);
+    glBufferData(GL_ARRAY_BUFFER, colorBufferSize, colorVertices.data(), GL_STATIC_DRAW);
+
+    // Start with color disabled.
+    glVertexAttribPointer(colorLoc, 4, GL_UNSIGNED_BYTE, GL_TRUE, 0, nullptr);
+    glDisableVertexAttribArray(colorLoc);
+
+    ASSERT_GL_NO_ERROR();
+
+    // Draw without a mapped buffer. Should succeed.
+    glVertexAttrib4f(colorLoc, 0, 1, 0, 1);
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+
+    // Map position buffer and draw. Should fail.
+    glBindBuffer(GL_ARRAY_BUFFER, posBuffer);
+    glMapBufferRange(GL_ARRAY_BUFFER, 0, posBufferSize, GL_MAP_READ_BIT);
+    ASSERT_GL_NO_ERROR();
+
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    EXPECT_GL_ERROR(GL_INVALID_OPERATION) << "Map position buffer and draw should fail.";
+    glUnmapBuffer(GL_ARRAY_BUFFER);
+
+    // Map then enable color buffer. Should fail.
+    glBindBuffer(GL_ARRAY_BUFFER, colorBuffer);
+    glMapBufferRange(GL_ARRAY_BUFFER, 0, colorBufferSize, GL_MAP_READ_BIT);
+    glEnableVertexAttribArray(colorLoc);
+    ASSERT_GL_NO_ERROR();
+
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    EXPECT_GL_ERROR(GL_INVALID_OPERATION) << "Map then enable color buffer should fail.";
+
+    // Unmap then draw. Should succeed.
+    glUnmapBuffer(GL_ARRAY_BUFFER);
+    ASSERT_GL_NO_ERROR();
+
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::blue);
+}
+
+// Tests that changing a vertex binding with glVertexAttribDivisor updates the mapped buffer check.
+TEST_P(ValidationStateChangeTestES31, MapBufferAndDrawWithDivisor)
+{
+    // Seems to trigger a GL error in some edge cases. http://anglebug.com/2755
+    ANGLE_SKIP_TEST_IF(IsOpenGL() && IsNVIDIA());
+
+    // Initialize program and set up state.
+    ANGLE_GL_PROGRAM(program, kColorVS, kColorFS);
+
+    glUseProgram(program);
+    GLint positionLoc = glGetAttribLocation(program, "position");
+    ASSERT_NE(-1, positionLoc);
+    GLint colorLoc = glGetAttribLocation(program, "color");
+    ASSERT_NE(-1, colorLoc);
+
+    // Create a user vertex array.
+    GLVertexArray vao;
+    glBindVertexArray(vao);
+
+    const std::array<Vector3, 6> &quadVertices = GetQuadVertices();
+    const size_t posBufferSize                 = quadVertices.size() * sizeof(Vector3);
+
+    GLBuffer posBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, posBuffer);
+    glBufferData(GL_ARRAY_BUFFER, posBufferSize, quadVertices.data(), GL_STATIC_DRAW);
+
+    // Start with position enabled.
+    glVertexAttribPointer(positionLoc, 3, GL_FLOAT, GL_FALSE, 0, nullptr);
+    glEnableVertexAttribArray(positionLoc);
+
+    std::vector<GLColor> blueVertices(6, GLColor::blue);
+    const size_t blueBufferSize = sizeof(GLColor) * 6;
+
+    GLBuffer blueBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, blueBuffer);
+    glBufferData(GL_ARRAY_BUFFER, blueBufferSize, blueVertices.data(), GL_STATIC_DRAW);
+
+    // Start with color enabled at an unused binding.
+    constexpr GLint kUnusedBinding = 3;
+    ASSERT_NE(colorLoc, kUnusedBinding);
+    ASSERT_NE(positionLoc, kUnusedBinding);
+    glVertexAttribFormat(colorLoc, 4, GL_UNSIGNED_BYTE, GL_TRUE, 0);
+    glVertexAttribBinding(colorLoc, kUnusedBinding);
+    glBindVertexBuffer(kUnusedBinding, blueBuffer, 0, sizeof(GLColor));
+    glEnableVertexAttribArray(colorLoc);
+
+    // Make binding 'colorLoc' use a mapped buffer.
+    std::vector<GLColor> greenVertices(6, GLColor::green);
+    const size_t greenBufferSize = sizeof(GLColor) * 6;
+    GLBuffer greenBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, greenBuffer);
+    glBufferData(GL_ARRAY_BUFFER, greenBufferSize, greenVertices.data(), GL_STATIC_DRAW);
+    glMapBufferRange(GL_ARRAY_BUFFER, 0, greenBufferSize, GL_MAP_READ_BIT);
+    glBindVertexBuffer(colorLoc, greenBuffer, 0, sizeof(GLColor));
+
+    ASSERT_GL_NO_ERROR();
+
+    // Draw without a mapped buffer. Should succeed.
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::blue);
+
+    // Change divisor with VertexAttribDivisor. Should fail.
+    glVertexAttribDivisor(colorLoc, 0);
+    ASSERT_GL_NO_ERROR();
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    EXPECT_GL_ERROR(GL_INVALID_OPERATION) << "draw with mapped buffer should fail.";
+
+    // Unmap the buffer. Should succeed.
+    glUnmapBuffer(GL_ARRAY_BUFFER);
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
+// Tests that changing a vertex binding with glVertexAttribDivisor updates the buffer size check.
+TEST_P(ValidationStateChangeTestES31, DrawPastEndOfBufferWithDivisor)
+{
+    // Initialize program and set up state.
+    ANGLE_GL_PROGRAM(program, kColorVS, kColorFS);
+
+    glUseProgram(program);
+    GLint positionLoc = glGetAttribLocation(program, "position");
+    ASSERT_NE(-1, positionLoc);
+    GLint colorLoc = glGetAttribLocation(program, "color");
+    ASSERT_NE(-1, colorLoc);
+
+    // Create a user vertex array.
+    GLVertexArray vao;
+    glBindVertexArray(vao);
+
+    const std::array<Vector3, 6> &quadVertices = GetQuadVertices();
+    const size_t posBufferSize                 = quadVertices.size() * sizeof(Vector3);
+
+    GLBuffer posBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, posBuffer);
+    glBufferData(GL_ARRAY_BUFFER, posBufferSize, quadVertices.data(), GL_STATIC_DRAW);
+
+    // Start with position enabled.
+    glVertexAttribPointer(positionLoc, 3, GL_FLOAT, GL_FALSE, 0, nullptr);
+    glEnableVertexAttribArray(positionLoc);
+
+    std::vector<GLColor> blueVertices(6, GLColor::blue);
+    const size_t blueBufferSize = sizeof(GLColor) * 6;
+
+    GLBuffer blueBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, blueBuffer);
+    glBufferData(GL_ARRAY_BUFFER, blueBufferSize, blueVertices.data(), GL_STATIC_DRAW);
+
+    // Start with color enabled at an unused binding.
+    constexpr GLint kUnusedBinding = 3;
+    ASSERT_NE(colorLoc, kUnusedBinding);
+    ASSERT_NE(positionLoc, kUnusedBinding);
+    glVertexAttribFormat(colorLoc, 4, GL_UNSIGNED_BYTE, GL_TRUE, 0);
+    glVertexAttribBinding(colorLoc, kUnusedBinding);
+    glBindVertexBuffer(kUnusedBinding, blueBuffer, 0, sizeof(GLColor));
+    glEnableVertexAttribArray(colorLoc);
+
+    // Make binding 'colorLoc' use a small buffer.
+    std::vector<GLColor> greenVertices(6, GLColor::green);
+    const size_t greenBufferSize = sizeof(GLColor) * 3;
+    GLBuffer greenBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, greenBuffer);
+    glBufferData(GL_ARRAY_BUFFER, greenBufferSize, greenVertices.data(), GL_STATIC_DRAW);
+    glBindVertexBuffer(colorLoc, greenBuffer, 0, sizeof(GLColor));
+
+    ASSERT_GL_NO_ERROR();
+
+    // Draw without a mapped buffer. Should succeed.
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::blue);
+
+    // Change divisor with VertexAttribDivisor. Should fail.
+    glVertexAttribDivisor(colorLoc, 0);
+    ASSERT_GL_NO_ERROR();
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    EXPECT_GL_ERROR(GL_INVALID_OPERATION) << "draw with small buffer should fail.";
+
+    // Do a small draw. Should succeed.
+    glDrawArrays(GL_TRIANGLES, 0, 3);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
 }  // anonymous namespace
 
 ANGLE_INSTANTIATE_TEST(StateChangeTest, ES2_D3D9(), ES2_D3D11(), ES2_OPENGL(), ES2_VULKAN());
@@ -2078,3 +2326,5 @@ ANGLE_INSTANTIATE_TEST(StateChangeRenderTest,
                        ES2_VULKAN());
 ANGLE_INSTANTIATE_TEST(StateChangeTestES3, ES3_D3D11(), ES3_OPENGL());
 ANGLE_INSTANTIATE_TEST(SimpleStateChangeTest, ES2_VULKAN(), ES2_OPENGL());
+ANGLE_INSTANTIATE_TEST(ValidationStateChangeTest, ES3_D3D11(), ES3_OPENGL());
+ANGLE_INSTANTIATE_TEST(ValidationStateChangeTestES31, ES31_OPENGL());
\ No newline at end of file