Distinguish Map request callback fired by device
Currently Buffer doesn't distinguish Map request callback fired
by device. For example if buffer.MapAsync(), buffer.Unmap(),
and buffer.MapAsync() are called in this order before the first
MapAsync() finishes the MapAsync callback provided by application
for the first MapAsync() is fired when Map request callback
for the first MapAsync() is fired by device although the first
MapAsync callback provided by application shouldn't be fired
because it is already unmapped.
This commit resolves this problem by assigning MapRequestId to
Map request and distinguishing the callback fired by device.
Change-Id: Ic29b02d27cffb254616dc7b48a60151c39f667e2
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/113222
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Kokoro: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp
index df7e758..f90ebec 100644
--- a/src/dawn/native/Buffer.cpp
+++ b/src/dawn/native/Buffer.cpp
@@ -37,24 +37,25 @@
namespace {
struct MapRequestTask : TrackTaskCallback {
- MapRequestTask(dawn::platform::Platform* platform, Ref<BufferBase> buffer)
- : TrackTaskCallback(platform), buffer(std::move(buffer)) {}
+ MapRequestTask(dawn::platform::Platform* platform, Ref<BufferBase> buffer, MapRequestID id)
+ : TrackTaskCallback(platform), buffer(std::move(buffer)), id(id) {}
void Finish() override {
ASSERT(mSerial != kMaxExecutionSerial);
TRACE_EVENT1(mPlatform, General, "Buffer::TaskInFlight::Finished", "serial",
uint64_t(mSerial));
- buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_Success);
+ buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_Success);
}
void HandleDeviceLoss() override {
- buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DeviceLost);
+ buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DeviceLost);
}
void HandleShutDown() override {
- buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+ buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
}
~MapRequestTask() override = default;
private:
Ref<BufferBase> buffer;
+ MapRequestID id;
};
class ErrorBuffer final : public BufferBase {
@@ -316,9 +317,9 @@
UNREACHABLE();
}
-void BufferBase::CallMapCallback(WGPUBufferMapAsyncStatus status) {
+void BufferBase::CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status) {
ASSERT(!IsError());
- if (mMapCallback != nullptr) {
+ if (mMapCallback != nullptr && mapID == mLastMapID) {
// 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;
@@ -357,6 +358,7 @@
}
ASSERT(!IsError());
+ mLastMapID++;
mMapMode = mode;
mMapOffset = offset;
mMapSize = size;
@@ -365,11 +367,11 @@
mState = BufferState::PendingMap;
if (GetDevice()->ConsumedError(MapAsyncImpl(mode, offset, size))) {
- CallMapCallback(WGPUBufferMapAsyncStatus_DeviceLost);
+ CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DeviceLost);
return;
}
std::unique_ptr<MapRequestTask> request =
- std::make_unique<MapRequestTask>(GetDevice()->GetPlatform(), this);
+ std::make_unique<MapRequestTask>(GetDevice()->GetPlatform(), this, mLastMapID);
TRACE_EVENT1(GetDevice()->GetPlatform(), General, "Buffer::APIMapAsync", "serial",
uint64_t(GetDevice()->GetPendingCommandSerial()));
GetDevice()->GetQueue()->TrackTask(std::move(request));
@@ -439,7 +441,7 @@
void BufferBase::UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus) {
if (mState == BufferState::PendingMap) {
- CallMapCallback(callbackStatus);
+ CallMapCallback(mLastMapID, callbackStatus);
UnmapImpl();
} else if (mState == BufferState::Mapped) {
UnmapImpl();
@@ -552,11 +554,12 @@
return {};
}
-void BufferBase::OnMapRequestCompleted(WGPUBufferMapAsyncStatus status) {
- if (status == WGPUBufferMapAsyncStatus_Success && mState == BufferState::PendingMap) {
+void BufferBase::OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status) {
+ if (mapID == mLastMapID && status == WGPUBufferMapAsyncStatus_Success &&
+ mState == BufferState::PendingMap) {
mState = BufferState::Mapped;
}
- CallMapCallback(status);
+ CallMapCallback(mapID, status);
}
bool BufferBase::NeedsInitialization() const {
diff --git a/src/dawn/native/Buffer.h b/src/dawn/native/Buffer.h
index 76d7f4c..5fab9b7 100644
--- a/src/dawn/native/Buffer.h
+++ b/src/dawn/native/Buffer.h
@@ -63,7 +63,7 @@
wgpu::BufferUsage GetUsageExternalOnly() const;
MaybeError MapAtCreation();
- void OnMapRequestCompleted(WGPUBufferMapAsyncStatus status);
+ void OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status);
MaybeError ValidateCanUseOnQueueNow() const;
@@ -110,7 +110,7 @@
virtual bool IsCPUWritableAtCreation() const = 0;
MaybeError CopyFromStagingBuffer();
- void CallMapCallback(WGPUBufferMapAsyncStatus status);
+ void CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status);
MaybeError ValidateMapAsync(wgpu::MapMode mode,
size_t offset,
@@ -129,6 +129,7 @@
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/unittests/validation/BufferValidationTests.cpp b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp
index 57b1dcd..1dac496 100644
--- a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp
@@ -406,32 +406,40 @@
// works as expected and we don't get the cancelled request's data.
TEST_F(BufferValidationTest, MapAsync_UnmapBeforeResultAndMapAgain) {
{
- wgpu::Buffer buf = CreateMapReadBuffer(4);
- buf.MapAsync(wgpu::MapMode::Read, 0, 4, ToMockBufferMapAsyncCallback, this + 0);
+ wgpu::Buffer buf = CreateMapReadBuffer(16);
+ buf.MapAsync(wgpu::MapMode::Read, 0, 8, ToMockBufferMapAsyncCallback, this + 0);
EXPECT_CALL(*mockBufferMapAsyncCallback,
Call(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback, this + 0))
.Times(1);
buf.Unmap();
- buf.MapAsync(wgpu::MapMode::Read, 0, 4, ToMockBufferMapAsyncCallback, this + 1);
+ buf.MapAsync(wgpu::MapMode::Read, 8, 8, ToMockBufferMapAsyncCallback, this + 1);
EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, this + 1))
.Times(1);
WaitForAllOperations(device);
+
+ // Check that only the second MapAsync had an effect
+ ASSERT_EQ(nullptr, buf.GetConstMappedRange(0));
+ ASSERT_NE(nullptr, buf.GetConstMappedRange(8));
}
{
- wgpu::Buffer buf = CreateMapWriteBuffer(4);
- buf.MapAsync(wgpu::MapMode::Write, 0, 4, ToMockBufferMapAsyncCallback, this + 0);
+ wgpu::Buffer buf = CreateMapWriteBuffer(16);
+ buf.MapAsync(wgpu::MapMode::Write, 0, 8, ToMockBufferMapAsyncCallback, this + 0);
EXPECT_CALL(*mockBufferMapAsyncCallback,
Call(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback, this + 0))
.Times(1);
buf.Unmap();
- buf.MapAsync(wgpu::MapMode::Write, 0, 4, ToMockBufferMapAsyncCallback, this + 1);
+ buf.MapAsync(wgpu::MapMode::Write, 8, 8, ToMockBufferMapAsyncCallback, this + 1);
EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, this + 1))
.Times(1);
WaitForAllOperations(device);
+
+ // Check that only the second MapAsync had an effect
+ ASSERT_EQ(nullptr, buf.GetConstMappedRange(0));
+ ASSERT_NE(nullptr, buf.GetConstMappedRange(8));
}
}
