When we're not using the NULL buffer data hint update with glBufferData rather than glBufferSubData.

Review URL: http://codereview.appspot.com/5253047/




git-svn-id: http://skia.googlecode.com/svn/trunk@2443 2bbb7eff-a529-9590-31e7-b0007b416f81

9 files changed, 41 insertions, 59 deletions

diff --git a/gpu/include/GrConfig.h b/gpu/include/GrConfig.h
index 982d63ec54..1d07af1919 100644
--- a/gpu/include/GrConfig.h
+++ b/gpu/include/GrConfig.h
@@ -356,9 +356,9 @@ inline void GrCrash(const char* msg) { GrPrintf(msg); GrAlwaysAssert(false); }
 
 /**
  * GR_GEOM_BUFFER_LOCK_THRESHOLD gives a threshold (in bytes) for when Gr should
- * lock a GrGeometryBuffer to update its contents. It will use Lock() if the
- * size of the udpated region is greater than the threshold. Otherwise it will
- * use updateData() or updateSubData().
+ * lock a GrGeometryBuffer to update its contents. It will use lock() if the
+ * size of the updated region is greater than the threshold. Otherwise it will
+ * use updateData().
  */
 #if !defined(GR_GEOM_BUFFER_LOCK_THRESHOLD)
     #define GR_GEOM_BUFFER_LOCK_THRESHOLD (1 << 15)
diff --git a/gpu/include/GrGLConfig.h b/gpu/include/GrGLConfig.h
index b56f2ca7cb..44a3b8647c 100644
--- a/gpu/include/GrGLConfig.h
+++ b/gpu/include/GrGLConfig.h
@@ -65,9 +65,9 @@
  *  glBufferData(GL_..._BUFFER, size, NULL, usage);       //<--hint, NULL means
  *  glBufferSubData(GL_..._BUFFER, 0, lessThanSize, data) //   old data can't be
  *                                                        //   used again.
- * However, this can cause a performance decrease on Chrome cmd buffer because
- * it will create a new allocation and memset the whole thing to zero (for
- * security reasons). Defaults to 1 (enabled).
+ * However, this can be an unoptimization on some platforms, esp. Chrome.
+ * Chrome's cmd buffer will create a new allocation and memset the whole thing
+ * to zero (for security reasons). Defaults to 1 (enabled).
  *
  * GR_GL_PER_GL_FUNC_CALLBACK: When set to 1 the GrGLInterface object provides
  * a function pointer that is called just before every gl function. The ptr must
diff --git a/gpu/include/GrUserConfig.h b/gpu/include/GrUserConfig.h
index 09eb9f4b14..9f616388cf 100644
--- a/gpu/include/GrUserConfig.h
+++ b/gpu/include/GrUserConfig.h
@@ -58,8 +58,8 @@
 
 /*
  * This gives a threshold in bytes of when to lock a GrGeometryBuffer vs using
- * updateData or updateSubData. (Note the depending on the underlying 3D API
- * the update functions may always be implemented using a lock)
+ * updateData. (Note the depending on the underlying 3D API the update functions
+ * may always be implemented using a lock)
  */
 //#define GR_GEOM_BUFFER_LOCK_THRESHOLD (1<<15)
 
diff --git a/gpu/src/GrBufferAllocPool.cpp b/gpu/src/GrBufferAllocPool.cpp
index 715d4a9bef..c01192db70 100644
--- a/gpu/src/GrBufferAllocPool.cpp
+++ b/gpu/src/GrBufferAllocPool.cpp
@@ -169,10 +169,13 @@ void* GrBufferAllocPool::makeSpace(size_t size,
         }
     }
 
-    // We could honor the space request using updateSubData on the current VB
-    // (if there is room). But we don't currently use draw calls to GL that
+    // We could honor the space request using by a partial update of the current
+    // VB (if there is room). But we don't currently use draw calls to GL that
     // allow the driver to know that previously issued draws won't read from
-    // the part of the buffer we update.
+    // the part of the buffer we update. Also, the GL buffer implementation
+    // may be cheating on the actual buffer size by shrinking the buffer on
+    // updateData() if the amount of data passed is less than the full buffer
+    // size.
     
     if (!createBlock(size)) {
         return NULL;
diff --git a/gpu/src/GrGLIndexBuffer.cpp b/gpu/src/GrGLIndexBuffer.cpp
index 084a4c61b0..b64668ede2 100644
--- a/gpu/src/GrGLIndexBuffer.cpp
+++ b/gpu/src/GrGLIndexBuffer.cpp
@@ -104,31 +104,28 @@ bool GrGLIndexBuffer::updateData(const void* src, size_t srcSizeInBytes) {
     }
     this->bind();
     GrGLenum usage = dynamic() ? GR_GL_DYNAMIC_DRAW : GR_GL_STATIC_DRAW;
+#if !GR_GL_USE_BUFFER_DATA_NULL_HINT
+    // Note that we're cheating on the size here. Currently no methods
+    // allow a partial update that preserves contents of non-updated
+    // portions of the buffer (and lock() does a glBufferData(..size, NULL..))
+    GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, srcSizeInBytes, src, usage));
+#else
     if (this->sizeInBytes() == srcSizeInBytes) {
         GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER,
                             srcSizeInBytes, src, usage));
     } else {
-#if GR_GL_USE_BUFFER_DATA_NULL_HINT
+        // Before we call glBufferSubData we give the driver a hint using
+        // glBufferData with NULL. This makes the old buffer contents
+        // inaccessible to future draws. The GPU may still be processing draws
+        // that reference the old contents. With this hint it can assign a
+        // different allocation for the new contents to avoid flushing the gpu
+        // past draws consuming the old contents.
         GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER,
                            this->sizeInBytes(), NULL, usage));
-#endif
         GL_CALL(BufferSubData(GR_GL_ELEMENT_ARRAY_BUFFER,
                               0, srcSizeInBytes, src));
     }
-    return true;
-}
-
-bool GrGLIndexBuffer::updateSubData(const void* src,
-                                    size_t srcSizeInBytes,
-                                    size_t offset) {
-    GrAssert(fBufferID);
-    GrAssert(!isLocked());
-    if (srcSizeInBytes + offset > this->sizeInBytes()) {
-        return false;
-    }
-    this->bind();
-    GL_CALL(BufferSubData(GR_GL_ELEMENT_ARRAY_BUFFER,
-                          offset, srcSizeInBytes, src));
+#endif
     return true;
 }
 
diff --git a/gpu/src/GrGLIndexBuffer.h b/gpu/src/GrGLIndexBuffer.h
index e4e77c9e8c..c3e2287260 100644
--- a/gpu/src/GrGLIndexBuffer.h
+++ b/gpu/src/GrGLIndexBuffer.h
@@ -30,9 +30,7 @@ public:
     virtual void unlock();
     virtual bool isLocked() const;
     virtual bool updateData(const void* src, size_t srcSizeInBytes);
-    virtual bool updateSubData(const void* src,
-                               size_t srcSizeInBytes,
-                               size_t offset);
+
 protected:
     GrGLIndexBuffer(GrGpuGL* gpu,
                     GrGLuint id,
diff --git a/gpu/src/GrGLVertexBuffer.cpp b/gpu/src/GrGLVertexBuffer.cpp
index c542602a9b..33c1e7e3e0 100644
--- a/gpu/src/GrGLVertexBuffer.cpp
+++ b/gpu/src/GrGLVertexBuffer.cpp
@@ -101,28 +101,26 @@ bool GrGLVertexBuffer::updateData(const void* src, size_t srcSizeInBytes) {
     }
     this->bind();
     GrGLenum usage = dynamic() ? GR_GL_DYNAMIC_DRAW : GR_GL_STATIC_DRAW;
+#if !GR_GL_USE_BUFFER_DATA_NULL_HINT
+    // Note that we're cheating on the size here. Currently no methods
+    // allow a partial update that preserves contents of non-updated
+    // portions of the buffer (and lock() does a glBufferData(..size, NULL..))
+    GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage));
+#else
     if (this->sizeInBytes() == srcSizeInBytes) {
         GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage));
     } else {
-#if GR_GL_USE_BUFFER_DATA_NULL_HINT
+        // Before we call glBufferSubData we give the driver a hint using
+        // glBufferData with NULL. This makes the old buffer contents
+        // inaccessible to future draws. The GPU may still be processing draws
+        // that reference the old contents. With this hint it can assign a
+        // different allocation for the new contents to avoid flushing the gpu
+        // past draws consuming the old contents.
         GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, 
                            this->sizeInBytes(), NULL, usage));
-#endif
         GL_CALL(BufferSubData(GR_GL_ARRAY_BUFFER, 0, srcSizeInBytes, src));
     }
-    return true;
-}
-
-bool GrGLVertexBuffer::updateSubData(const void* src,
-                                     size_t srcSizeInBytes,
-                                     size_t offset) {
-    GrAssert(fBufferID);
-    GrAssert(!isLocked());
-    if (srcSizeInBytes + offset > this->sizeInBytes()) {
-        return false;
-    }
-    this->bind();
-    GL_CALL(BufferSubData(GR_GL_ARRAY_BUFFER, offset, srcSizeInBytes, src));
+#endif
     return true;
 }
 
diff --git a/gpu/src/GrGLVertexBuffer.h b/gpu/src/GrGLVertexBuffer.h
index f83019d3e8..15fc54a983 100644
--- a/gpu/src/GrGLVertexBuffer.h
+++ b/gpu/src/GrGLVertexBuffer.h
@@ -26,9 +26,6 @@ public:
     virtual void unlock();
     virtual bool isLocked() const;
     virtual bool updateData(const void* src, size_t srcSizeInBytes);
-    virtual bool updateSubData(const void* src,
-                               size_t srcSizeInBytes,
-                               size_t offset);
     GrGLuint bufferID() const;
 
 protected:
diff --git a/gpu/src/GrGeometryBuffer.h b/gpu/src/GrGeometryBuffer.h
index a977cdaf74..c74b25487d 100644
--- a/gpu/src/GrGeometryBuffer.h
+++ b/gpu/src/GrGeometryBuffer.h
@@ -71,17 +71,6 @@ public:
      */
     virtual bool updateData(const void* src, size_t srcSizeInBytes) = 0;
 
-    /**
-     * Updates a portion of the buffer data.
-     *
-     * The contents of the buffer outside the update region are preserved.
-     *
-     * @return returns true if the update succeeds, false otherwise.
-     */
-    virtual bool updateSubData(const void* src,
-                               size_t srcSizeInBytes,
-                               size_t offset) = 0;
-
     // GrResource overrides
     virtual size_t sizeInBytes() const { return fSizeInBytes; }