Also reject mapAsync if the buffer is being mapped
To reflect the WebGPU spec change at
https://github.com/gpuweb/gpuweb/pull/3348
Bug: chromium:1355994
Change-Id: I0159cbd9e1977a05453e3c562a2b0649a0ff930f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/110448
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Takahiro <hogehoge@gachapin.jp>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp
index 9256829..d2bdb62 100644
--- a/src/dawn/native/Buffer.cpp
+++ b/src/dawn/native/Buffer.cpp
@@ -37,25 +37,24 @@
namespace {
struct MapRequestTask : TrackTaskCallback {
- MapRequestTask(dawn::platform::Platform* platform, Ref<BufferBase> buffer, MapRequestID id)
- : TrackTaskCallback(platform), buffer(std::move(buffer)), id(id) {}
+ MapRequestTask(dawn::platform::Platform* platform, Ref<BufferBase> buffer)
+ : TrackTaskCallback(platform), buffer(std::move(buffer)) {}
void Finish() override {
ASSERT(mSerial != kMaxExecutionSerial);
TRACE_EVENT1(mPlatform, General, "Buffer::TaskInFlight::Finished", "serial",
uint64_t(mSerial));
- buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_Success);
+ buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_Success);
}
void HandleDeviceLoss() override {
- buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DeviceLost);
+ buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DeviceLost);
}
void HandleShutDown() override {
- buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+ buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
}
~MapRequestTask() override = default;
private:
Ref<BufferBase> buffer;
- MapRequestID id;
};
class ErrorBuffer final : public BufferBase {
@@ -194,7 +193,7 @@
}
void BufferBase::DestroyImpl() {
- if (mState == BufferState::Mapped) {
+ if (mState == BufferState::Mapped || mState == BufferState::PendingMap) {
UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
} else if (mState == BufferState::MappedAtCreation) {
if (mStagingBuffer != nullptr) {
@@ -311,15 +310,17 @@
case BufferState::Mapped:
case BufferState::MappedAtCreation:
return DAWN_VALIDATION_ERROR("%s used in submit while mapped.", this);
+ case BufferState::PendingMap:
+ return DAWN_VALIDATION_ERROR("%s used in submit while pending map.", this);
case BufferState::Unmapped:
return {};
}
UNREACHABLE();
}
-void BufferBase::CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status) {
+void BufferBase::CallMapCallback(WGPUBufferMapAsyncStatus status) {
ASSERT(!IsError());
- if (mMapCallback != nullptr && mapID == mLastMapID) {
+ if (mMapCallback != nullptr) {
// Tag the callback as fired before firing it, otherwise it could fire a second time if
// for example buffer.Unmap() is called inside the application-provided callback.
WGPUBufferMapCallback callback = mMapCallback;
@@ -330,6 +331,8 @@
} else {
callback(status, mMapUserdata);
}
+
+ mMapUserdata = 0;
}
}
@@ -356,20 +359,19 @@
}
ASSERT(!IsError());
- mLastMapID++;
mMapMode = mode;
mMapOffset = offset;
mMapSize = size;
mMapCallback = callback;
mMapUserdata = userdata;
- mState = BufferState::Mapped;
+ mState = BufferState::PendingMap;
if (GetDevice()->ConsumedError(MapAsyncImpl(mode, offset, size))) {
- CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DeviceLost);
+ CallMapCallback(WGPUBufferMapAsyncStatus_DeviceLost);
return;
}
std::unique_ptr<MapRequestTask> request =
- std::make_unique<MapRequestTask>(GetDevice()->GetPlatform(), this, mLastMapID);
+ std::make_unique<MapRequestTask>(GetDevice()->GetPlatform(), this);
TRACE_EVENT1(GetDevice()->GetPlatform(), General, "Buffer::APIMapAsync", "serial",
uint64_t(GetDevice()->GetPendingCommandSerial()));
GetDevice()->GetQueue()->TrackTask(std::move(request));
@@ -438,16 +440,11 @@
}
void BufferBase::UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus) {
- if (mState == BufferState::Mapped) {
- // A map request can only be called once, so this will fire only if the request wasn't
- // completed before the Unmap.
- // Callbacks are not fired if there is no callback registered, so this is correct for
- // mappedAtCreation = true.
- CallMapCallback(mLastMapID, callbackStatus);
+ if (mState == BufferState::PendingMap) {
+ CallMapCallback(callbackStatus);
UnmapImpl();
-
- mMapCallback = nullptr;
- mMapUserdata = 0;
+ } else if (mState == BufferState::Mapped) {
+ UnmapImpl();
} else if (mState == BufferState::MappedAtCreation) {
if (mStagingBuffer != nullptr) {
GetDevice()->ConsumedError(CopyFromStagingBuffer());
@@ -483,6 +480,8 @@
case BufferState::Mapped:
case BufferState::MappedAtCreation:
return DAWN_VALIDATION_ERROR("%s is already mapped.", this);
+ case BufferState::PendingMap:
+ return DAWN_VALIDATION_ERROR("%s is pending map.", this);
case BufferState::Destroyed:
return DAWN_VALIDATION_ERROR("%s is destroyed.", this);
case BufferState::Unmapped:
@@ -542,6 +541,7 @@
ASSERT(bool{mMapMode & wgpu::MapMode::Read} ^ bool{mMapMode & wgpu::MapMode::Write});
return !writable || (mMapMode & wgpu::MapMode::Write);
+ case BufferState::PendingMap:
case BufferState::Unmapped:
case BufferState::Destroyed:
return false;
@@ -554,8 +554,11 @@
return {};
}
-void BufferBase::OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status) {
- CallMapCallback(mapID, status);
+void BufferBase::OnMapRequestCompleted(WGPUBufferMapAsyncStatus status) {
+ if (status == WGPUBufferMapAsyncStatus_Success && mState == BufferState::PendingMap) {
+ mState = BufferState::Mapped;
+ }
+ CallMapCallback(status);
}
bool BufferBase::NeedsInitialization() const {
diff --git a/src/dawn/native/Buffer.h b/src/dawn/native/Buffer.h
index 6bb1503..76d7f4c 100644
--- a/src/dawn/native/Buffer.h
+++ b/src/dawn/native/Buffer.h
@@ -44,6 +44,7 @@
public:
enum class BufferState {
Unmapped,
+ PendingMap,
Mapped,
MappedAtCreation,
Destroyed,
@@ -62,7 +63,7 @@
wgpu::BufferUsage GetUsageExternalOnly() const;
MaybeError MapAtCreation();
- void OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status);
+ void OnMapRequestCompleted(WGPUBufferMapAsyncStatus status);
MaybeError ValidateCanUseOnQueueNow() const;
@@ -109,7 +110,7 @@
virtual bool IsCPUWritableAtCreation() const = 0;
MaybeError CopyFromStagingBuffer();
- void CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status);
+ void CallMapCallback(WGPUBufferMapAsyncStatus status);
MaybeError ValidateMapAsync(wgpu::MapMode mode,
size_t offset,
@@ -128,7 +129,6 @@
WGPUBufferMapCallback mMapCallback = nullptr;
void* mMapUserdata = 0;
- MapRequestID mLastMapID = MapRequestID(0);
wgpu::MapMode mMapMode = wgpu::MapMode::None;
size_t mMapOffset = 0;
size_t mMapSize = 0;
diff --git a/src/dawn/tests/end2end/DeviceLostTests.cpp b/src/dawn/tests/end2end/DeviceLostTests.cpp
index 4c2b01ed85..a225c4d 100644
--- a/src/dawn/tests/end2end/DeviceLostTests.cpp
+++ b/src/dawn/tests/end2end/DeviceLostTests.cpp
@@ -55,6 +55,24 @@
EXPECT_EQ(WGPUBufferMapAsyncStatus_DeviceLost, status);
EXPECT_EQ(&fakeUserData, userdata);
}
+
+ void MapAsyncAndWait(const wgpu::Buffer& buffer,
+ wgpu::MapMode mode,
+ size_t offset,
+ size_t size) {
+ bool done = false;
+ buffer.MapAsync(
+ mode, offset, size,
+ [](WGPUBufferMapAsyncStatus status, void* userdata) {
+ ASSERT_EQ(WGPUBufferMapAsyncStatus_Success, status);
+ *static_cast<bool*>(userdata) = true;
+ },
+ &done);
+
+ while (!done) {
+ WaitABit();
+ }
+ }
};
// Test that DeviceLostCallback is invoked when LostForTestimg is called
@@ -338,7 +356,7 @@
desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst;
wgpu::Buffer buffer = device.CreateBuffer(&desc);
- buffer.MapAsync(wgpu::MapMode::Read, 0, 4, nullptr, nullptr);
+ MapAsyncAndWait(buffer, wgpu::MapMode::Read, 0, 4);
queue.Submit(0, nullptr);
const void* rangeBeforeLoss = buffer.GetConstMappedRange();
@@ -355,7 +373,7 @@
desc.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc;
wgpu::Buffer buffer = device.CreateBuffer(&desc);
- buffer.MapAsync(wgpu::MapMode::Write, 0, 4, nullptr, nullptr);
+ MapAsyncAndWait(buffer, wgpu::MapMode::Write, 0, 4);
queue.Submit(0, nullptr);
const void* rangeBeforeLoss = buffer.GetConstMappedRange();
diff --git a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp
index 3a0b4e5..57b1dcd 100644
--- a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp
@@ -294,6 +294,7 @@
{
wgpu::Buffer buffer = CreateMapReadBuffer(4);
buffer.MapAsync(wgpu::MapMode::Read, 0, 4, nullptr, nullptr);
+ WaitForAllOperations(device);
AssertMapAsyncError(buffer, wgpu::MapMode::Read, 0, 4);
}
{
@@ -303,6 +304,7 @@
{
wgpu::Buffer buffer = CreateMapWriteBuffer(4);
buffer.MapAsync(wgpu::MapMode::Write, 0, 4, nullptr, nullptr);
+ WaitForAllOperations(device);
AssertMapAsyncError(buffer, wgpu::MapMode::Write, 0, 4);
}
{
@@ -311,6 +313,53 @@
}
}
+// Test map async with a buffer that's pending map
+TEST_F(BufferValidationTest, MapAsync_PendingMap) {
+ // Read + overlapping range
+ {
+ wgpu::Buffer buffer = CreateMapReadBuffer(4);
+ // The first map async call should succeed while the second one should fail
+ buffer.MapAsync(wgpu::MapMode::Read, 0, 4, ToMockBufferMapAsyncCallback, nullptr);
+ AssertMapAsyncError(buffer, wgpu::MapMode::Read, 0, 4);
+ EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr))
+ .Times(1);
+ WaitForAllOperations(device);
+ }
+
+ // Read + non-overlapping range
+ {
+ wgpu::Buffer buffer = CreateMapReadBuffer(16);
+ // The first map async call should succeed while the second one should fail
+ buffer.MapAsync(wgpu::MapMode::Read, 0, 8, ToMockBufferMapAsyncCallback, nullptr);
+ AssertMapAsyncError(buffer, wgpu::MapMode::Read, 8, 8);
+ EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr))
+ .Times(1);
+ WaitForAllOperations(device);
+ }
+
+ // Write + overlapping range
+ {
+ wgpu::Buffer buffer = CreateMapWriteBuffer(4);
+ // The first map async call should succeed while the second one should fail
+ buffer.MapAsync(wgpu::MapMode::Write, 0, 4, ToMockBufferMapAsyncCallback, nullptr);
+ AssertMapAsyncError(buffer, wgpu::MapMode::Write, 0, 4);
+ EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr))
+ .Times(1);
+ WaitForAllOperations(device);
+ }
+
+ // Write + non-overlapping range
+ {
+ wgpu::Buffer buffer = CreateMapWriteBuffer(16);
+ // The first map async call should succeed while the second one should fail
+ buffer.MapAsync(wgpu::MapMode::Write, 0, 8, ToMockBufferMapAsyncCallback, nullptr);
+ AssertMapAsyncError(buffer, wgpu::MapMode::Write, 8, 8);
+ EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr))
+ .Times(1);
+ WaitForAllOperations(device);
+ }
+}
+
// Test map async with a buffer that's destroyed
TEST_F(BufferValidationTest, MapAsync_Destroy) {
{
