Fix some bugs in buffer mapping in the wire client

Bug: dawn:445
Change-Id: Ibfc55e26c2bc1757811669d84e54e9f775bee8f6
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/25440
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

src/dawn_wire/client/Buffer.cpp

@@ -121,6 +121,8 @@ namespace dawn_wire { namespace client {
         // Successfully created staging memory. The buffer now owns the WriteHandle.
         buffer->mWriteHandle = std::move(writeHandle);
         buffer->mMappedData = result.data;
+        buffer->mMapOffset = 0;
+        buffer->mMapSize = descriptor->size;
 
         result.buffer = ToAPI(buffer);
 
@@ -280,33 +282,34 @@ namespace dawn_wire { namespace client {
         proxy->callback = callback;
         proxy->userdata = userdata;
         proxy->mapOffset = offset;
+        proxy->mapSize = size;
         proxy->self = this;
         // Note technically we should keep the buffer alive until the callback is fired but the
         // client doesn't have good facilities to do that yet.
 
         // Call MapReadAsync or MapWriteAsync and forward the callback.
-        if (mode & WGPUMapMode_Read) {
+        if (isReadMode) {
             MapReadAsync(
                 [](WGPUBufferMapAsyncStatus status, const void*, uint64_t, void* userdata) {
                     ProxyData* proxy = static_cast<ProxyData*>(userdata);
+                    proxy->self->mMapOffset = proxy->mapOffset;
+                    proxy->self->mMapSize = proxy->mapSize;
                     if (proxy->callback) {
                         proxy->callback(status, proxy->userdata);
                     }
-                    proxy->self->mMapOffset = proxy->mapOffset;
-                    proxy->self->mMapSize = proxy->mapSize;
                     delete proxy;
                 },
                 proxy);
         } else {
-            ASSERT(mode & WGPUMapMode_Write);
+            ASSERT(isWriteMode);
             MapWriteAsync(
                 [](WGPUBufferMapAsyncStatus status, void*, uint64_t, void* userdata) {
                     ProxyData* proxy = static_cast<ProxyData*>(userdata);
+                    proxy->self->mMapOffset = proxy->mapOffset;
+                    proxy->self->mMapSize = proxy->mapSize;
                     if (proxy->callback) {
                         proxy->callback(status, proxy->userdata);
                     }
-                    proxy->self->mMapOffset = proxy->mapOffset;
-                    proxy->self->mMapSize = proxy->mapSize;
                     delete proxy;
                 },
                 proxy);
@@ -450,7 +453,7 @@ namespace dawn_wire { namespace client {
 
         mMappedData = nullptr;
         mMapOffset = 0;
-        mMapSize = mSize;
+        mMapSize = 0;
         ClearMapRequests(WGPUBufferMapAsyncStatus_Unknown);
 
         BufferUnmapCmd cmd;

src/tests/DawnTest.h

@@ -34,24 +34,24 @@
     AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint16_t), \
                          new ::detail::ExpectEq<uint16_t>(expected))
 
-#define EXPECT_BUFFER_U16_RANGE_EQ(expected, buffer, offset, count)                    \
+#define EXPECT_BUFFER_U16_RANGE_EQ(expected, buffer, offset, count)                      \
-    AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint16_t) * count, \
+    AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint16_t) * (count), \
                          new ::detail::ExpectEq<uint16_t>(expected, count))
 
 #define EXPECT_BUFFER_U32_EQ(expected, buffer, offset)                         \
     AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint32_t), \
                          new ::detail::ExpectEq<uint32_t>(expected))
 
-#define EXPECT_BUFFER_U32_RANGE_EQ(expected, buffer, offset, count)                    \
+#define EXPECT_BUFFER_U32_RANGE_EQ(expected, buffer, offset, count)                      \
-    AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint32_t) * count, \
+    AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint32_t) * (count), \
                          new ::detail::ExpectEq<uint32_t>(expected, count))
 
 #define EXPECT_BUFFER_FLOAT_EQ(expected, buffer, offset)                       \
     AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint32_t), \
                          new ::detail::ExpectEq<float>(expected))
 
-#define EXPECT_BUFFER_FLOAT_RANGE_EQ(expected, buffer, offset, count)                  \
+#define EXPECT_BUFFER_FLOAT_RANGE_EQ(expected, buffer, offset, count)                    \
-    AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint32_t) * count, \
+    AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint32_t) * (count), \
                          new ::detail::ExpectEq<float>(expected, count))
 
 // Test a pixel of the mip level 0 of a 2D texture.

src/tests/end2end/BufferTests.cpp

@@ -352,16 +352,24 @@ class BufferMappingTests : public DawnTest {
     }
 };
 
+void CheckMapping(const void* actual, const void* expected, size_t size) {
+    EXPECT_NE(actual, nullptr);
+    if (actual != nullptr) {
+        EXPECT_EQ(0, memcmp(actual, expected, size));
+    }
+}
+
 // Test that the simplest map read works
 TEST_P(BufferMappingTests, MapRead_Basic) {
     wgpu::Buffer buffer = CreateMapReadBuffer(4);
 
     uint32_t myData = 0x01020304;
-    queue.WriteBuffer(buffer, 0, &myData, sizeof(myData));
+    constexpr size_t kSize = sizeof(myData);
+    queue.WriteBuffer(buffer, 0, &myData, kSize);
 
     MapAsyncAndWait(buffer, wgpu::MapMode::Read, 0, 4);
-    ASSERT_NE(nullptr, buffer.GetConstMappedRange());
+    CheckMapping(buffer.GetConstMappedRange(), &myData, kSize);
-    ASSERT_EQ(myData, *static_cast<const uint32_t*>(buffer.GetConstMappedRange()));
+    CheckMapping(buffer.GetConstMappedRange(0, kSize), &myData, kSize);
     buffer.Unmap();
 }
 
@@ -408,17 +416,72 @@ TEST_P(BufferMappingTests, MapRead_Twice) {
 // Test map-reading a large buffer.
 TEST_P(BufferMappingTests, MapRead_Large) {
     constexpr uint32_t kDataSize = 1000 * 1000;
-    wgpu::Buffer buffer = CreateMapReadBuffer(kDataSize * sizeof(uint32_t));
+    constexpr size_t kByteSize = kDataSize * sizeof(uint32_t);
+    wgpu::Buffer buffer = CreateMapReadBuffer(kByteSize);
 
     std::vector<uint32_t> myData;
     for (uint32_t i = 0; i < kDataSize; ++i) {
         myData.push_back(i);
     }
-    queue.WriteBuffer(buffer, 0, myData.data(), kDataSize * sizeof(uint32_t));
+    queue.WriteBuffer(buffer, 0, myData.data(), kByteSize);
 
-    MapAsyncAndWait(buffer, wgpu::MapMode::Read, 0, 4);
+    MapAsyncAndWait(buffer, wgpu::MapMode::Read, 0, kByteSize);
-    ASSERT_EQ(0, memcmp(buffer.GetConstMappedRange(), myData.data(), kDataSize * sizeof(uint32_t)));
+    EXPECT_EQ(nullptr, buffer.GetConstMappedRange(0, kByteSize + 4));
+    EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(), myData.data(), kByteSize));
+    EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(8), myData.data() + 2, kByteSize - 8));
+    EXPECT_EQ(
+        0, memcmp(buffer.GetConstMappedRange(8, kByteSize - 8), myData.data() + 2, kByteSize - 8));
     buffer.Unmap();
+
+    MapAsyncAndWait(buffer, wgpu::MapMode::Read, 12, kByteSize - 12);
+    EXPECT_EQ(nullptr, buffer.GetConstMappedRange(16, kByteSize - 12));
+    EXPECT_EQ(nullptr, buffer.GetConstMappedRange());
+    EXPECT_EQ(nullptr, buffer.GetConstMappedRange(8));
+    EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(12), myData.data() + 3, kByteSize - 12));
+    EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(20), myData.data() + 5, kByteSize - 20));
+    EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(20, kByteSize - 24), myData.data() + 5,
+                        kByteSize - 44));
+    buffer.Unmap();
+}
+
+// Test that GetConstMappedRange works inside map-read callback
+TEST_P(BufferMappingTests, MapRead_InCallback) {
+    constexpr size_t kBufferSize = 8;
+    wgpu::Buffer buffer = CreateMapReadBuffer(kBufferSize);
+
+    uint32_t myData[2] = {0x01020304, 0x05060708};
+    constexpr size_t kSize = sizeof(myData);
+    queue.WriteBuffer(buffer, 0, &myData, kSize);
+
+    struct UserData {
+        bool done;
+        wgpu::Buffer buffer;
+        void* expected;
+    };
+    UserData user{false, buffer, &myData};
+
+    buffer.MapAsync(
+        wgpu::MapMode::Read, 0, kBufferSize,
+        [](WGPUBufferMapAsyncStatus status, void* userdata) {
+            UserData* user = static_cast<UserData*>(userdata);
+
+            EXPECT_EQ(WGPUBufferMapAsyncStatus_Success, status);
+            if (status == WGPUBufferMapAsyncStatus_Success) {
+                CheckMapping(user->buffer.GetConstMappedRange(), user->expected, kSize);
+                CheckMapping(user->buffer.GetConstMappedRange(0, kSize), user->expected, kSize);
+
+                CheckMapping(user->buffer.GetConstMappedRange(4, 4),
+                             static_cast<const uint32_t*>(user->expected) + 1, sizeof(uint32_t));
+
+                user->buffer.Unmap();
+            }
+            user->done = true;
+        },
+        &user);
+
+    while (!user.done) {
+        WaitABit();
+    }
 }
 
 // Test that the simplest map write works.
@@ -435,6 +498,20 @@ TEST_P(BufferMappingTests, MapWrite_Basic) {
     EXPECT_BUFFER_U32_EQ(myData, buffer, 0);
 }
 
+// Test that the simplest map write works with a range.
+TEST_P(BufferMappingTests, MapWrite_BasicRange) {
+    wgpu::Buffer buffer = CreateMapWriteBuffer(4);
+
+    uint32_t myData = 2934875;
+    MapAsyncAndWait(buffer, wgpu::MapMode::Write, 0, 4);
+    ASSERT_NE(nullptr, buffer.GetMappedRange(0, 4));
+    ASSERT_NE(nullptr, buffer.GetConstMappedRange(0, 4));
+    memcpy(buffer.GetMappedRange(), &myData, sizeof(myData));
+    buffer.Unmap();
+
+    EXPECT_BUFFER_U32_EQ(myData, buffer, 0);
+}
+
 // Test map-writing a zero-sized buffer.
 TEST_P(BufferMappingTests, MapWrite_ZeroSized) {
     wgpu::Buffer buffer = CreateMapWriteBuffer(0);
@@ -479,6 +556,7 @@ TEST_P(BufferMappingTests, MapWrite_Twice) {
 // Test mapping a large buffer.
 TEST_P(BufferMappingTests, MapWrite_Large) {
     constexpr uint32_t kDataSize = 1000 * 1000;
+    constexpr size_t kByteSize = kDataSize * sizeof(uint32_t);
     wgpu::Buffer buffer = CreateMapWriteBuffer(kDataSize * sizeof(uint32_t));
 
     std::vector<uint32_t> myData;
@@ -486,11 +564,15 @@ TEST_P(BufferMappingTests, MapWrite_Large) {
         myData.push_back(i);
     }
 
-    MapAsyncAndWait(buffer, wgpu::MapMode::Write, 0, 4);
+    MapAsyncAndWait(buffer, wgpu::MapMode::Write, 12, kByteSize - 20);
-    memcpy(buffer.GetMappedRange(), myData.data(), kDataSize * sizeof(uint32_t));
+    EXPECT_EQ(nullptr, buffer.GetMappedRange());
+    EXPECT_EQ(nullptr, buffer.GetMappedRange(0));
+    EXPECT_EQ(nullptr, buffer.GetMappedRange(8));
+    EXPECT_EQ(nullptr, buffer.GetMappedRange(16, kByteSize - 8));
+    EXPECT_EQ(nullptr, buffer.GetMappedRange(16, kByteSize - 20));
+    memcpy(buffer.GetMappedRange(12), myData.data(), kByteSize - 20);
     buffer.Unmap();
-
+    EXPECT_BUFFER_U32_RANGE_EQ(myData.data(), buffer, 12, kDataSize - 5);
-    EXPECT_BUFFER_U32_RANGE_EQ(myData.data(), buffer, 0, kDataSize);
 }
 
 // Test that the map offset isn't updated when the call is an error.
@@ -521,6 +603,88 @@ TEST_P(BufferMappingTests, OffsetNotUpdatedOnError) {
     ASSERT_EQ(0, memcmp(buffer.GetConstMappedRange(4), &data[1], sizeof(uint32_t)));
 }
 
+// Test that Get(Const)MappedRange work inside map-write callback.
+TEST_P(BufferMappingTests, MapWrite_InCallbackDefault) {
+    wgpu::Buffer buffer = CreateMapWriteBuffer(4);
+
+    constexpr uint32_t myData = 2934875;
+    constexpr size_t kSize = sizeof(myData);
+
+    struct UserData {
+        bool done;
+        wgpu::Buffer buffer;
+    };
+    UserData user{false, buffer};
+
+    buffer.MapAsync(
+        wgpu::MapMode::Write, 0, kSize,
+        [](WGPUBufferMapAsyncStatus status, void* userdata) {
+            UserData* user = static_cast<UserData*>(userdata);
+
+            EXPECT_EQ(WGPUBufferMapAsyncStatus_Success, status);
+            if (status == WGPUBufferMapAsyncStatus_Success) {
+                EXPECT_NE(nullptr, user->buffer.GetConstMappedRange());
+                void* ptr = user->buffer.GetMappedRange();
+                EXPECT_NE(nullptr, ptr);
+                if (ptr != nullptr) {
+                    uint32_t data = myData;
+                    memcpy(ptr, &data, kSize);
+                }
+
+                user->buffer.Unmap();
+            }
+            user->done = true;
+        },
+        &user);
+
+    while (!user.done) {
+        WaitABit();
+    }
+
+    EXPECT_BUFFER_U32_EQ(myData, buffer, 0);
+}
+
+// Test that Get(Const)MappedRange with range work inside map-write callback.
+TEST_P(BufferMappingTests, MapWrite_InCallbackRange) {
+    wgpu::Buffer buffer = CreateMapWriteBuffer(4);
+
+    constexpr uint32_t myData = 2934875;
+    constexpr size_t kSize = sizeof(myData);
+
+    struct UserData {
+        bool done;
+        wgpu::Buffer buffer;
+    };
+    UserData user{false, buffer};
+
+    buffer.MapAsync(
+        wgpu::MapMode::Write, 0, kSize,
+        [](WGPUBufferMapAsyncStatus status, void* userdata) {
+            UserData* user = static_cast<UserData*>(userdata);
+
+            EXPECT_EQ(WGPUBufferMapAsyncStatus_Success, status);
+            if (status == WGPUBufferMapAsyncStatus_Success) {
+                EXPECT_NE(nullptr, user->buffer.GetConstMappedRange(0, kSize));
+                void* ptr = user->buffer.GetMappedRange(0, kSize);
+                EXPECT_NE(nullptr, ptr);
+                if (ptr != nullptr) {
+                    uint32_t data = myData;
+                    memcpy(ptr, &data, kSize);
+                }
+
+                user->buffer.Unmap();
+            }
+            user->done = true;
+        },
+        &user);
+
+    while (!user.done) {
+        WaitABit();
+    }
+
+    EXPECT_BUFFER_U32_EQ(myData, buffer, 0);
+}
+
 DAWN_INSTANTIATE_TEST(BufferMappingTests,
                       D3D12Backend(),
                       MetalBackend(),