Metal: fix vertex attribute's conversion lost after changing buffer binding.

src/libANGLE/renderer/metal/BufferMtl.h

@@ -36,6 +36,10 @@ struct ConversionBufferMtl
 
     // The conversion is stored in a dynamic buffer.
     mtl::BufferPool data;
+
+    // These properties are to be filled by user of this buffer conversion
+    mtl::BufferRef convertedBuffer;
+    size_t convertedOffset;
 };
 
 struct IndexConversionBufferMtl : public ConversionBufferMtl
@@ -46,10 +50,6 @@ struct IndexConversionBufferMtl : public ConversionBufferMtl
 
     const gl::DrawElementsType type;
     const size_t offset;
-
-    // These properties are to be filled by user of this buffer conversion
-    mtl::BufferRef convertedBuffer;
-    size_t convertedOffset;
 };
 
 class BufferHolderMtl
@@ -128,7 +128,7 @@ class BufferMtl : public BufferImpl, public BufferHolderMtl
                                                        gl::DrawElementsType type,
                                                        size_t offset);
 
-    size_t size() const { return mState.getSize(); }
+    size_t size() const { return static_cast<size_t>(mState.getSize()); }
 
   private:
     angle::Result setSubDataImpl(const gl::Context *context,
@@ -137,9 +137,12 @@ class BufferMtl : public BufferImpl, public BufferHolderMtl
                                  size_t offset);
 
     angle::Result commitShadowCopy(const gl::Context *context);
+    angle::Result commitShadowCopy(const gl::Context *context, size_t size);
 
     void markConversionBuffersDirty();
 
+    void clearConversionBuffers();
+
     // Client side shadow buffer
     angle::MemoryBuffer mShadowCopy;
 

src/libANGLE/renderer/metal/BufferMtl.mm

@@ -44,7 +44,7 @@ angle::Result GetFirstLastIndices(const IndexType *indices,
 ConversionBufferMtl::ConversionBufferMtl(const gl::Context *context,
                                          size_t initialSize,
                                          size_t alignment)
-    : dirty(true)
+    : dirty(true), convertedBuffer(nullptr), convertedOffset(0)
 {
     ContextMtl *contextMtl = mtl::GetImpl(context);
     data.initialize(contextMtl, initialSize, alignment);
@@ -58,11 +58,9 @@ IndexConversionBufferMtl::IndexConversionBufferMtl(const gl::Context *context,
                                                    size_t offsetIn)
     : ConversionBufferMtl(context,
                           kConvertedElementArrayBufferInitialSize,
-                          mtl::kBufferSettingOffsetAlignment),
+                          mtl::kIndexBufferOffsetAlignment),
       type(typeIn),
-      offset(offsetIn),
-      convertedBuffer(nullptr),
-      convertedOffset(0)
+      offset(offsetIn)
 {}
 
 // BufferMtl::VertexConversionBuffer implementation.
@@ -93,32 +91,34 @@ void BufferMtl::destroy(const gl::Context *context)
     mShadowCopy.resize(0);
     mBufferPool.destroy(contextMtl);
     mBuffer = nullptr;
+
+    clearConversionBuffers();
 }
 
 angle::Result BufferMtl::setData(const gl::Context *context,
                                  gl::BufferBinding target,
                                  const void *data,
-                                 size_t size,
+                                 size_t intendedSize,
                                  gl::BufferUsage usage)
 {
     ContextMtl *contextMtl = mtl::GetImpl(context);
 
-    if (!mShadowCopy.size() || size > static_cast<size_t>(mState.getSize()) ||
-        usage != mState.getUsage())
+    // Invalidate conversion buffers
+    if (mState.getSize() != static_cast<GLint64>(intendedSize))
+    {
+        clearConversionBuffers();
+    }
+    else
     {
-        if (size == 0)
-        {
-            size = 1;
-        }
-        // Re-create the buffer
         markConversionBuffersDirty();
+    }
 
-        ANGLE_MTL_CHECK(contextMtl, mShadowCopy.resize(size), GL_OUT_OF_MEMORY);
-        if (data)
-        {
-            auto ptr = static_cast<const uint8_t *>(data);
-            std::copy(ptr, ptr + size, mShadowCopy.data());
-        }
+    size_t adjustedSize = std::max<size_t>(1, intendedSize);
+
+    if (!mShadowCopy.size() || intendedSize > mShadowCopy.size() || usage != mState.getUsage())
+    {
+        // Re-create the buffer
+        ANGLE_MTL_CHECK(contextMtl, mShadowCopy.resize(adjustedSize), GL_OUT_OF_MEMORY);
 
         size_t maxBuffers;
         switch (usage)
@@ -135,15 +135,18 @@ angle::Result BufferMtl::setData(const gl::Context *context,
                 break;
         }
 
-        mBufferPool.initialize(contextMtl, size, 1, maxBuffers);
-
-        return commitShadowCopy(context);
+        mBufferPool.initialize(contextMtl, adjustedSize, 1, maxBuffers);
     }
-    else
+
+    // Transfer data to shadow copy buffer
+    if (data)
     {
-        // update data only
-        return setSubData(context, target, data, size, 0);
+        auto ptr = static_cast<const uint8_t *>(data);
+        std::copy(ptr, ptr + intendedSize, mShadowCopy.data());
     }
+
+    // Transfer data from shadow copy buffer to GPU buffer.
+    return commitShadowCopy(context, adjustedSize);
 }
 
 angle::Result BufferMtl::setSubData(const gl::Context *context,
@@ -178,7 +181,7 @@ angle::Result BufferMtl::copySubData(const gl::Context *context,
 angle::Result BufferMtl::map(const gl::Context *context, GLenum access, void **mapPtr)
 {
     ASSERT(mShadowCopy.size());
-    return mapRange(context, 0, mState.getSize(), 0, mapPtr);
+    return mapRange(context, 0, size(), 0, mapPtr);
 }
 
 angle::Result BufferMtl::mapRange(const gl::Context *context,
@@ -309,6 +312,12 @@ void BufferMtl::markConversionBuffersDirty()
     }
 }
 
+void BufferMtl::clearConversionBuffers()
+{
+    mVertexConversionBuffers.clear();
+    mIndexConversionBuffers.clear();
+}
+
 angle::Result BufferMtl::setSubDataImpl(const gl::Context *context,
                                         const void *data,
                                         size_t size,
@@ -337,16 +346,24 @@ angle::Result BufferMtl::setSubDataImpl(const gl::Context *context,
 
 angle::Result BufferMtl::commitShadowCopy(const gl::Context *context)
 {
+    return commitShadowCopy(context, size());
+}
+
+angle::Result BufferMtl::commitShadowCopy(const gl::Context *context, size_t size)
+{
     ContextMtl *contextMtl = mtl::GetImpl(context);
 
     uint8_t *ptr = nullptr;
-    ANGLE_TRY(
-        mBufferPool.allocate(contextMtl, mShadowCopy.size(), &ptr, &mBuffer, nullptr, nullptr));
+    ANGLE_TRY(mBufferPool.allocate(contextMtl, size, &ptr, &mBuffer, nullptr, nullptr));
 
-    std::copy(mShadowCopy.data(), mShadowCopy.data() + mShadowCopy.size(), ptr);
+    std::copy(mShadowCopy.data(), mShadowCopy.data() + size, ptr);
 
     ANGLE_TRY(mBufferPool.commit(contextMtl));
 
+#ifndef NDEBUG
+    ANGLE_MTL_OBJC_SCOPE { mBuffer->get().label = [NSString stringWithFormat:@"%p", this]; }
+#endif
+
     return angle::Result::Continue;
 }
 

src/libANGLE/renderer/metal/ContextMtl.mm

@@ -108,7 +108,7 @@ angle::Result ContextMtl::initialize()
     mBlendDesc.reset();
     mDepthStencilDesc.reset();
 
-    mTriFanIndexBuffer.initialize(this, 0, mtl::kBufferSettingOffsetAlignment,
+    mTriFanIndexBuffer.initialize(this, 0, mtl::kIndexBufferOffsetAlignment,
                                   kMaxTriFanLineLoopBuffersPerFrame);
     mLineLoopIndexBuffer.initialize(this, 0, 2 * sizeof(uint32_t),
                                     kMaxTriFanLineLoopBuffersPerFrame);

src/libANGLE/renderer/metal/VertexArrayMtl.h

@@ -59,6 +59,8 @@ class VertexArrayMtl : public VertexArrayImpl
                                  gl::DrawElementsType *indexTypeOut);
 
   private:
+    void reset(ContextMtl *context);
+
     angle::Result syncDirtyAttrib(const gl::Context *glContext,
                                   const gl::VertexAttribute &attrib,
                                   const gl::VertexBinding &binding,
@@ -101,7 +103,7 @@ class VertexArrayMtl : public VertexArrayImpl
     gl::AttribArray<SimpleWeakBufferHolderMtl> mConvertedArrayBufferHolders;
     gl::AttribArray<size_t> mCurrentArrayBufferOffsets;
     gl::AttribArray<GLuint> mCurrentArrayBufferStrides;
-    gl::AttribArray<MTLVertexFormat> mCurrentArrayBufferFormats;
+    gl::AttribArray<const mtl::VertexFormat *> mCurrentArrayBufferFormats;
 
     mtl::BufferPool mDynamicVertexData;
     mtl::BufferPool mDynamicIndexData;

src/libANGLE/renderer/metal/mtl_common.h

@@ -103,13 +103,12 @@ constexpr uint32_t kMaxShaderSamplers    = 16;
 constexpr size_t kDefaultUniformsMaxSize = 4 * 1024;
 constexpr uint32_t kMaxViewports         = 1;
 
-constexpr uint32_t kVertexAttribBufferOffsetAlignment = 4;
 constexpr uint32_t kVertexAttribBufferStrideAlignment = 4;
 // Alignment requirement for offset passed to setVertex|FragmentBuffer
 #if TARGET_OS_OSX || TARGET_OS_MACCATALYST
-constexpr uint32_t kBufferSettingOffsetAlignment = 256;
+constexpr uint32_t kUniformBufferSettingOffsetMinAlignment = 256;
 #else
-constexpr uint32_t kBufferSettingOffsetAlignment = 4;
+constexpr uint32_t kUniformBufferSettingOffsetMinAlignment = 4;
 #endif
 constexpr uint32_t kIndexBufferOffsetAlignment = 4;
 

src/libANGLE/renderer/metal/mtl_render_utils.h

@@ -58,7 +58,7 @@ struct TriFanFromArrayParams
     uint32_t firstVertex;
     uint32_t vertexCount;
     BufferRef dstBuffer;
-    // Must be multiples of kBufferSettingOffsetAlignment
+    // Must be multiples of kIndexBufferOffsetAlignment
     uint32_t dstOffset;
 };
 
@@ -95,7 +95,7 @@ class RenderUtils : public Context, angle::NonCopyable
                                      const BufferRef &srcBuffer,
                                      uint32_t srcOffset,
                                      const BufferRef &dstBuffer,
-                                     // Must be multiples of kBufferSettingOffsetAlignment
+                                     // Must be multiples of kIndexBufferOffsetAlignment
                                      uint32_t dstOffset);
     angle::Result generateTriFanBufferFromArrays(const gl::Context *context,
                                                  const TriFanFromArrayParams &params);
@@ -164,7 +164,7 @@ class RenderUtils : public Context, angle::NonCopyable
         const BufferRef &srcBuffer,
         uint32_t srcOffset,
         const BufferRef &dstBuffer,
-        // Must be multiples of kBufferSettingOffsetAlignment
+        // Must be multiples of kIndexBufferOffsetAlignment
         uint32_t dstOffset);
     angle::Result generateTriFanBufferFromElementsArrayCPU(const gl::Context *context,
                                                            const IndexGenerationParams &params);

src/libANGLE/renderer/metal/mtl_render_utils.mm

@@ -716,7 +716,7 @@ angle::Result RenderUtils::convertIndexBuffer(const gl::Context *context,
 
     cmdEncoder->setComputePipelineState(pipelineState);
 
-    ASSERT((dstOffset % kBufferSettingOffsetAlignment) == 0);
+    ASSERT((dstOffset % kIndexBufferOffsetAlignment) == 0);
 
     IndexConversionUniform uniform;
     uniform.srcOffset  = srcOffset;
@@ -743,7 +743,7 @@ angle::Result RenderUtils::generateTriFanBufferFromArrays(const gl::Context *con
 
     cmdEncoder->setComputePipelineState(mTriFanFromArraysGeneratorPipeline);
 
-    ASSERT((params.dstOffset % kBufferSettingOffsetAlignment) == 0);
+    ASSERT((params.dstOffset % kIndexBufferOffsetAlignment) == 0);
 
     struct TriFanArrayParams
     {
@@ -794,7 +794,7 @@ angle::Result RenderUtils::generateTriFanBufferFromElementsArrayGPU(
     const BufferRef &srcBuffer,
     uint32_t srcOffset,
     const BufferRef &dstBuffer,
-    // Must be multiples of kBufferSettingOffsetAlignment
+    // Must be multiples of kIndexBufferOffsetAlignment
     uint32_t dstOffset)
 {
     ContextMtl *contextMtl            = GetImpl(context);
@@ -808,7 +808,7 @@ angle::Result RenderUtils::generateTriFanBufferFromElementsArrayGPU(
 
     cmdEncoder->setComputePipelineState(pipelineState);
 
-    ASSERT((dstOffset % kBufferSettingOffsetAlignment) == 0);
+    ASSERT((dstOffset % kIndexBufferOffsetAlignment) == 0);
     ASSERT(indexCount > 2);
 
     IndexConversionUniform uniform;

src/tests/gl_tests/StateChangeTest.cpp

@@ -4450,6 +4450,76 @@ TEST_P(SimpleStateChangeTest, FboLateCullFaceBackCWState)
     drawToFboWithCulling(GL_CW, false);
 }
 
+// Test that vertex attribute translation is still kept after binding it to another buffer then
+// binding back to the previous buffer.
+TEST_P(SimpleStateChangeTest, RebindTranslatedAttribute)
+{
+    constexpr char kVS[] = R"(attribute vec4 a_position;
+attribute float a_attrib;
+varying float v_attrib;
+void main()
+{
+    v_attrib = a_attrib;
+    gl_Position = a_position;
+})";
+
+    constexpr char kFS[] = R"(precision mediump float;
+varying float v_attrib;
+void main()
+{
+    gl_FragColor = vec4(v_attrib, 0, 0, 1);
+})";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+    glBindAttribLocation(program, 0, "a_position");
+    glBindAttribLocation(program, 1, "a_attrib");
+    glLinkProgram(program);
+    glUseProgram(program);
+    ASSERT_GL_NO_ERROR();
+
+    // Set up color data so red is drawn
+    std::vector<GLushort> data(1000, 0xffff);
+
+    GLBuffer redBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, redBuffer);
+    glBufferData(GL_ARRAY_BUFFER, sizeof(GLushort) * data.size(), data.data(), GL_STATIC_DRAW);
+    // Use offset not multiple of 4 GLushorts, this could force vertex translation in Metal backend.
+    glVertexAttribPointer(1, 4, GL_UNSIGNED_SHORT, GL_TRUE, 0,
+                          reinterpret_cast<const void *>(sizeof(GLushort) * 97));
+    glBindBuffer(GL_ARRAY_BUFFER, 0);
+    glEnableVertexAttribArray(1);
+
+    drawQuad(program, "a_position", 0.5f);
+    // Verify red was drawn
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::red);
+
+    glClearColor(0, 1, 0, 1);
+    glClear(GL_COLOR_BUFFER_BIT);
+    // Verify that green was drawn
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+
+    // Bind black color buffer to the same attribute with zero offset
+    std::vector<GLfloat> black(6, 0.0f);
+    GLBuffer blackBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, blackBuffer);
+    glBufferData(GL_ARRAY_BUFFER, sizeof(GLfloat) * black.size(), black.data(), GL_STATIC_DRAW);
+    glVertexAttribPointer(1, 1, GL_FLOAT, GL_FALSE, 0, 0);
+
+    drawQuad(program, "a_position", 0.5f);
+    // Verify black was drawn
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::black);
+
+    // Rebind the old buffer & offset
+    glBindBuffer(GL_ARRAY_BUFFER, redBuffer);
+    // Use offset not multiple of 4 GLushorts
+    glVertexAttribPointer(1, 4, GL_UNSIGNED_SHORT, GL_TRUE, 0,
+                          reinterpret_cast<const void *>(sizeof(GLushort) * 97));
+
+    drawQuad(program, "a_position", 0.5f);
+    // Verify red was drawn
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::red);
+}
+
 // Validates GL_RASTERIZER_DISCARD state is tracked correctly
 TEST_P(SimpleStateChangeTestES3, RasterizerDiscardState)
 {

src/tests/gl_tests/VertexAttributeTest.cpp

@@ -1381,6 +1381,95 @@ TEST_P(VertexAttributeTest, DisabledAttribArrays)
     }
 }
 
+// Test that draw with offset larger than vertex attribute's stride can work
+TEST_P(VertexAttributeTest, DrawWithLargeBufferOffset)
+{
+    constexpr size_t kBufferOffset    = 10000;
+    constexpr size_t kQuadVertexCount = 4;
+
+    std::array<GLbyte, kQuadVertexCount> validInputData = {{0, 1, 2, 3}};
+
+    // 4 components
+    std::array<GLbyte, 4 *kQuadVertexCount + kBufferOffset> inputData = {};
+
+    std::array<GLfloat, 4 * kQuadVertexCount> expectedData;
+    for (size_t i = 0; i < kQuadVertexCount; i++)
+    {
+        for (int j = 0; j < 4; ++j)
+        {
+            inputData[kBufferOffset + 4 * i + j] = validInputData[i];
+            expectedData[4 * i + j]              = validInputData[i];
+        }
+    }
+
+    initBasicProgram();
+
+    glBindBuffer(GL_ARRAY_BUFFER, mBuffer);
+    glBufferData(GL_ARRAY_BUFFER, inputData.size(), inputData.data(), GL_STATIC_DRAW);
+    glVertexAttribPointer(mTestAttrib, 4, GL_BYTE, GL_FALSE, 0,
+                          reinterpret_cast<const void *>(kBufferOffset));
+    glEnableVertexAttribArray(mTestAttrib);
+    glBindBuffer(GL_ARRAY_BUFFER, 0);
+
+    glVertexAttribPointer(mExpectedAttrib, 4, GL_FLOAT, GL_FALSE, 0, expectedData.data());
+    glEnableVertexAttribArray(mExpectedAttrib);
+
+    drawIndexedQuad(mProgram, "position", 0.5f);
+
+    checkPixels();
+}
+
+// Test that drawing with large vertex attribute pointer offset and less components than
+// shader expects is OK
+TEST_P(VertexAttributeTest, DrawWithLargeBufferOffsetAndLessComponents)
+{
+    // Shader expects vec4 but glVertexAttribPointer only provides 2 components
+    constexpr char kVS[] = R"(attribute vec4 a_position;
+attribute vec4 a_attrib;
+varying vec4 v_attrib;
+void main()
+{
+    v_attrib = a_attrib;
+    gl_Position = a_position;
+})";
+
+    constexpr char kFS[] = R"(precision mediump float;
+varying vec4 v_attrib;
+void main()
+{
+    gl_FragColor = v_attrib;
+})";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+    glBindAttribLocation(program, 0, "a_position");
+    glBindAttribLocation(program, 1, "a_attrib");
+    glLinkProgram(program);
+    glUseProgram(program);
+    ASSERT_GL_NO_ERROR();
+
+    constexpr size_t kBufferOffset = 4998;
+
+    // Set up color data so yellow is drawn (only R, G components are provided)
+    std::vector<GLushort> data(kBufferOffset + 12);
+    for (int i = 0; i < 12; ++i)
+    {
+        data[kBufferOffset + i] = 0xffff;
+    }
+
+    GLBuffer buffer;
+    glBindBuffer(GL_ARRAY_BUFFER, buffer);
+    glBufferData(GL_ARRAY_BUFFER, sizeof(GLushort) * data.size(), data.data(), GL_STATIC_DRAW);
+    // Provide only 2 components for the vec4 in the shader
+    glVertexAttribPointer(1, 2, GL_UNSIGNED_SHORT, GL_TRUE, 0,
+                          reinterpret_cast<const void *>(sizeof(GLushort) * kBufferOffset));
+    glBindBuffer(GL_ARRAY_BUFFER, 0);
+    glEnableVertexAttribArray(1);
+
+    drawQuad(program, "a_position", 0.5f);
+    // Verify yellow was drawn
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::yellow);
+}
+
 class VertexAttributeTestES31 : public VertexAttributeTestES3
 {
   protected: