Clarify that buffers of type GrGpuBufferType::kXferGpuToCpu are mapped for reading.
Don't allow GrGpuBuffer::updateData on kXferGpuToCpu (in GL this can resize the buffer
to a smaller size).
Don't call glBufferData with NULL prior to mapping for reading.
Don't set GL_MAP_WRITE_BIT when using glMapBufferRange for reading only.
Bug: skia:8962
Change-Id: I605d83fa40c7b170082c48a456436d97cf3b70a5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/206707
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
diff --git a/src/gpu/GrGpuBuffer.cpp b/src/gpu/GrGpuBuffer.cpp
index b679b47..03f351b 100644
--- a/src/gpu/GrGpuBuffer.cpp
+++ b/src/gpu/GrGpuBuffer.cpp
@@ -17,6 +17,36 @@
, fAccessPattern(pattern)
, fIntendedType(type) {}
+void* GrGpuBuffer::map() {
+ if (this->wasDestroyed()) {
+ return nullptr;
+ }
+ if (!fMapPtr) {
+ this->onMap();
+ }
+ return fMapPtr;
+}
+
+void GrGpuBuffer::unmap() {
+ if (this->wasDestroyed()) {
+ return;
+ }
+ SkASSERT(fMapPtr);
+ this->onUnmap();
+ fMapPtr = nullptr;
+}
+
+bool GrGpuBuffer::isMapped() const { return SkToBool(fMapPtr); }
+
+bool GrGpuBuffer::updateData(const void* src, size_t srcSizeInBytes) {
+ SkASSERT(!this->isMapped());
+ SkASSERT(srcSizeInBytes <= fSizeInBytes);
+ if (this->intendedType() == GrGpuBufferType::kXferGpuToCpu) {
+ return false;
+ }
+ return this->onUpdateData(src, srcSizeInBytes);
+}
+
void GrGpuBuffer::ComputeScratchKeyForDynamicVBO(size_t size, GrGpuBufferType intendedType,
GrScratchKey* key) {
static const GrScratchKey::ResourceType kType = GrScratchKey::GenerateResourceType();
diff --git a/src/gpu/GrGpuBuffer.h b/src/gpu/GrGpuBuffer.h
index 5bc6ed1..e8c0141 100644
--- a/src/gpu/GrGpuBuffer.h
+++ b/src/gpu/GrGpuBuffer.h
@@ -30,43 +30,35 @@
void unref() const final { GrGpuResource::unref(); }
/**
- * Maps the buffer to be written by the CPU.
+ * Maps the buffer to be read or written by the CPU.
*
- * The previous content of the buffer is invalidated. It is an error
- * to draw from the buffer while it is mapped. It may fail if the backend
- * doesn't support mapping the buffer. If the buffer is CPU backed then
- * it will always succeed and is a free operation. Once a buffer is mapped,
- * subsequent calls to map() are ignored.
+ * It is an error to draw from the buffer while it is mapped or transfer to/from the buffer. It
+ * may fail if the backend doesn't support mapping the buffer. Once a buffer is mapped,
+ * subsequent calls to map() trivially succeed. No matter how many times map() is called,
+ * umap() will unmap the buffer on the first call if it is mapped.
*
- * Note that buffer mapping does not go through GrContext and therefore is
- * not serialized with other operations.
+ * If the buffer is of type GrGpuBufferType::kXferGpuToCpu then it is mapped for reading only.
+ * Otherwise it is mapped writing only. Writing to a buffer that is mapped for reading or vice
+ * versa produces undefined results. If the buffer is mapped for writing then then the buffer's
+ * previous contents are invalidated.
*
* @return a pointer to the data or nullptr if the map fails.
*/
- void* map() {
- if (!fMapPtr) {
- this->onMap();
- }
- return fMapPtr;
- }
+ void* map();
/**
- * Unmaps the buffer.
+ * Unmaps the buffer if it is mapped.
*
* The pointer returned by the previous map call will no longer be valid.
*/
- void unmap() {
- SkASSERT(fMapPtr);
- this->onUnmap();
- fMapPtr = nullptr;
- }
+ void unmap();
/**
- Queries whether the buffer has been mapped.
-
- @return true if the buffer is mapped, false otherwise.
+ * Queries whether the buffer has been mapped.
+ *
+ * @return true if the buffer is mapped, false otherwise.
*/
- bool isMapped() const { return SkToBool(fMapPtr); }
+ bool isMapped() const;
bool isCpuBuffer() const final { return false; }
@@ -79,16 +71,14 @@
*
* The buffer must not be mapped.
*
+ * Fails for GrGpuBufferType::kXferGpuToCpu.
+ *
* Note that buffer updates do not go through GrContext and therefore are
* not serialized with other operations.
*
* @return returns true if the update succeeds, false otherwise.
*/
- bool updateData(const void* src, size_t srcSizeInBytes) {
- SkASSERT(!this->isMapped());
- SkASSERT(srcSizeInBytes <= fSizeInBytes);
- return this->onUpdateData(src, srcSizeInBytes);
- }
+ bool updateData(const void* src, size_t srcSizeInBytes);
protected:
GrGpuBuffer(GrGpu*, size_t sizeInBytes, GrGpuBufferType, GrAccessPattern);
diff --git a/src/gpu/gl/GrGLBuffer.cpp b/src/gpu/gl/GrGLBuffer.cpp
index 4335409..f7eaa6f 100644
--- a/src/gpu/gl/GrGLBuffer.cpp
+++ b/src/gpu/gl/GrGLBuffer.cpp
@@ -159,10 +159,7 @@
void GrGLBuffer::onMap() {
SkASSERT(fBufferID);
- if (this->wasDestroyed()) {
- return;
- }
-
+ SkASSERT(!this->wasDestroyed());
VALIDATE();
SkASSERT(!this->isMapped());
@@ -172,12 +169,14 @@
// Handling dirty context is done in the bindBuffer call
switch (this->glCaps().mapBufferType()) {
case GrGLCaps::kNone_MapBufferType:
- break;
+ return;
case GrGLCaps::kMapBuffer_MapBufferType: {
GrGLenum target = this->glGpu()->bindBuffer(fIntendedType, this);
- // Let driver know it can discard the old data
- if (this->glCaps().useBufferDataNullHint() || fGLSizeInBytes != this->size()) {
- GL_CALL(BufferData(target, this->size(), nullptr, fUsage));
+ if (!readOnly) {
+ // Let driver know it can discard the old data
+ if (this->glCaps().useBufferDataNullHint() || fGLSizeInBytes != this->size()) {
+ GL_CALL(BufferData(target, this->size(), nullptr, fUsage));
+ }
}
GL_CALL_RET(fMapPtr, MapBuffer(target, readOnly ? GR_GL_READ_ONLY : GR_GL_WRITE_ONLY));
break;
@@ -188,13 +187,17 @@
if (fGLSizeInBytes != this->size()) {
GL_CALL(BufferData(target, this->size(), nullptr, fUsage));
}
- GrGLbitfield writeAccess = GR_GL_MAP_WRITE_BIT;
- if (GrGpuBufferType::kXferCpuToGpu != fIntendedType) {
- // TODO: Make this a function parameter.
- writeAccess |= GR_GL_MAP_INVALIDATE_BUFFER_BIT;
+ GrGLbitfield access;
+ if (readOnly) {
+ access = GR_GL_MAP_READ_BIT;
+ } else {
+ access = GR_GL_MAP_WRITE_BIT;
+ if (GrGpuBufferType::kXferCpuToGpu != fIntendedType) {
+ // TODO: Make this a function parameter.
+ access |= GR_GL_MAP_INVALIDATE_BUFFER_BIT;
+ }
}
- GL_CALL_RET(fMapPtr, MapBufferRange(target, 0, this->size(),
- readOnly ? GR_GL_MAP_READ_BIT : writeAccess));
+ GL_CALL_RET(fMapPtr, MapBufferRange(target, 0, this->size(), access));
break;
}
case GrGLCaps::kChromium_MapBufferType: {
@@ -214,10 +217,6 @@
void GrGLBuffer::onUnmap() {
SkASSERT(fBufferID);
- if (this->wasDestroyed()) {
- return;
- }
-
VALIDATE();
SkASSERT(this->isMapped());
if (0 == fBufferID) {
diff --git a/src/gpu/vk/GrVkIndexBuffer.cpp b/src/gpu/vk/GrVkIndexBuffer.cpp
index f5a7bbc..342262d 100644
--- a/src/gpu/vk/GrVkIndexBuffer.cpp
+++ b/src/gpu/vk/GrVkIndexBuffer.cpp
@@ -48,24 +48,12 @@
INHERITED::onAbandon();
}
-void GrVkIndexBuffer::onMap() {
- if (!this->wasDestroyed()) {
- this->GrGpuBuffer::fMapPtr = this->vkMap(this->getVkGpu());
- }
-}
+void GrVkIndexBuffer::onMap() { this->GrGpuBuffer::fMapPtr = this->vkMap(this->getVkGpu()); }
-void GrVkIndexBuffer::onUnmap() {
- if (!this->wasDestroyed()) {
- this->vkUnmap(this->getVkGpu());
- }
-}
+void GrVkIndexBuffer::onUnmap() { this->vkUnmap(this->getVkGpu()); }
bool GrVkIndexBuffer::onUpdateData(const void* src, size_t srcSizeInBytes) {
- if (!this->wasDestroyed()) {
- return this->vkUpdateData(this->getVkGpu(), src, srcSizeInBytes);
- } else {
- return false;
- }
+ return this->vkUpdateData(this->getVkGpu(), src, srcSizeInBytes);
}
GrVkGpu* GrVkIndexBuffer::getVkGpu() const {
diff --git a/src/gpu/vk/GrVkTransferBuffer.h b/src/gpu/vk/GrVkTransferBuffer.h
index 22f7286..3d46a2f 100644
--- a/src/gpu/vk/GrVkTransferBuffer.h
+++ b/src/gpu/vk/GrVkTransferBuffer.h
@@ -28,17 +28,9 @@
void setMemoryBacking(SkTraceMemoryDump* traceMemoryDump,
const SkString& dumpName) const override;
- void onMap() override {
- if (!this->wasDestroyed()) {
- this->GrGpuBuffer::fMapPtr = this->vkMap(this->getVkGpu());
- }
- }
+ void onMap() override { this->GrGpuBuffer::fMapPtr = this->vkMap(this->getVkGpu()); }
- void onUnmap() override {
- if (!this->wasDestroyed()) {
- this->vkUnmap(this->getVkGpu());
- }
- }
+ void onUnmap() override { this->vkUnmap(this->getVkGpu()); }
bool onUpdateData(const void* src, size_t srcSizeInBytes) override {
SK_ABORT("Not implemented for transfer buffers.");
