Make sure we call glBufferData on the same data we store internally.

gpu/command_buffer/service/buffer_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/buffer_manager.h"
 
 #include <stdint.h>
 
 #include <limits>
 
@@ skipping 101 matching lines @@
 void* Buffer::MappedRange::GetShmPointer() const {
   DCHECK(shm.get());
   return shm->GetDataAddress(static_cast<unsigned int>(offset),
                              static_cast<unsigned int>(size));
 }
 
 Buffer::Buffer(BufferManager* manager, GLuint service_id)
     : manager_(manager),
       size_(0),
       deleted_(false),
-      shadowed_(false),
       is_client_side_array_(false),
       service_id_(service_id),
       initial_target_(0),
       usage_(GL_STATIC_DRAW) {
   manager_->StartTracking(this);
 }
 
 Buffer::~Buffer() {
   if (manager_) {
     if (manager_->have_context_) {
       GLuint id = service_id();
       glDeleteBuffersARB(1, &id);
     }
     manager_->StopTracking(this);
     manager_ = NULL;
   }
 }
 
+const GLvoid* Buffer::StageShadow(bool use_shadow, GLsizeiptr size,
+                                  const GLvoid* data) {
+  if (use_shadow) {
+    shadow_.resize(size);

[piman, 2016/04/01 00:09:46]

Why the change to std::vector? We never need to append data to the shadow
buffer... Anyway, using a vector means that we make 2 copies here (vector.resize
copies the existing data, that we then overwrite by copying the new data or
memset).
Maybe clear() before resize to avoid the extra copy, unless the sizes match? (at
which point it's not much different than the previous logic).

[David Yen, 2016/04/01 17:00:53]

The reason I changed it to a vector was to reduce the number of memory
allocations. Essentially what I want is to have a capacity variable and size
variable. Then only reallocate memory if the size needed exceeds the capacity.
At that point it's looking more and more like std::vector. I also didn't like
how shadowed_ and shadow_ needed to be kept in lock step with one another,
although that could have been done by deleting shadow_ and checking if the
pointer is valid as well.

It's true that vector.resize() copies the existing data, but clearing and then
resizing will set each element to 0 which is effectively the same cost on new
allocations. For cases where no allocation happens this would have an extra
cost.

I agree neither case is ideal... perhaps the right thing to do is to clear and
then insert data or resize on NULL data?

+    if (data) {
+      memcpy(shadow_.data(), data, size);
+    } else {
+      memset(shadow_.data(), 0, size);
+    }
+    return shadow_.data();
+  } else {
+    shadow_.clear();
+    return data;
+  }
+}
+
 void Buffer::SetInfo(
-    GLsizeiptr size, GLenum usage, bool shadow, const GLvoid* data,
-    bool is_client_side_array) {
+    GLsizeiptr size, GLenum usage, bool shadow, bool is_client_side_array) {

[Ken Russell (switch to Gerrit), 2016/03/31 23:28:04]

It might be more clear now to rename "shadow" to "use_shadow".

[David Yen, 2016/04/01 17:00:53]

Done.

   usage_ = usage;
   is_client_side_array_ = is_client_side_array;
   ClearCache();
-  if (size != size_ || shadow != shadowed_) {
-    shadowed_ = shadow;
-    size_ = size;
-    if (shadowed_) {
-      shadow_.reset(new int8_t[size]);
-    } else {
-      shadow_.reset();
-    }
-  }
-  if (shadowed_) {
-    if (data) {
-      memcpy(shadow_.get(), data, size);
-    } else {
-      memset(shadow_.get(), 0, size);
-    }
-  }
+
+  // Shadow must have been setup already.
+  DCHECK_EQ(shadow_.size(), static_cast<size_t>(shadow ? size : 0u));
+  size_ = size;
+
   mapped_range_.reset(nullptr);
 }
 
 bool Buffer::CheckRange(
     GLintptr offset, GLsizeiptr size) const {
   int32_t end = 0;
   return offset >= 0 && size >= 0 &&
          offset <= std::numeric_limits<int32_t>::max() &&
          size <= std::numeric_limits<int32_t>::max() &&
          SafeAddInt32(offset, size, &end) && end <= size_;
 }
 
 bool Buffer::SetRange(
     GLintptr offset, GLsizeiptr size, const GLvoid * data) {
   if (!CheckRange(offset, size)) {
     return false;
   }
-  if (shadowed_) {
-    memcpy(shadow_.get() + offset, data, size);
+  if (!shadow_.empty()) {
+    DCHECK_LE(static_cast<size_t>(offset + size), shadow_.size());
+    memcpy(shadow_.data() + offset, data, size);
     ClearCache();
   }
   return true;
 }
 
 const void* Buffer::GetRange(
     GLintptr offset, GLsizeiptr size) const {
-  if (!shadowed_) {
+  if (shadow_.empty()) {
     return NULL;
   }
   if (!CheckRange(offset, size)) {
     return NULL;
   }
-  return shadow_.get() + offset;
+  DCHECK_LE(static_cast<size_t>(offset + size), shadow_.size());
+  return shadow_.data() + offset;
 }
 
 void Buffer::ClearCache() {
   range_set_.clear();
 }
 
 template <typename T>
 GLuint GetMaxValue(const void* data, GLuint offset, GLsizei count,
     GLuint primitive_restart_index) {
   GLuint max_value = 0;
@@ skipping 67 matching lines @@
   }
 
   if (!SafeAddUint32(offset, size, &size)) {
     return false;
   }
 
   if (size > static_cast<uint32_t>(size_)) {
     return false;
   }
 
-  if (!shadowed_) {
+  if (shadow_.empty()) {
     return false;
   }
 
   // Scan the range for the max value and store
   GLuint max_v = 0;
   switch (type) {
     case GL_UNSIGNED_BYTE:
-      max_v = GetMaxValue<uint8_t>(shadow_.get(), offset, count,
+      max_v = GetMaxValue<uint8_t>(shadow_.data(), offset, count,
           primitive_restart_index);
       break;
     case GL_UNSIGNED_SHORT:
       // Check we are not accessing an odd byte for a 2 byte value.
       if ((offset & 1) != 0) {
         return false;
       }
-      max_v = GetMaxValue<uint16_t>(shadow_.get(), offset, count,
+      max_v = GetMaxValue<uint16_t>(shadow_.data(), offset, count,
           primitive_restart_index);
       break;
     case GL_UNSIGNED_INT:
       // Check we are not accessing a non aligned address for a 4 byte value.
       if ((offset & 3) != 0) {
         return false;
       }
-      max_v = GetMaxValue<uint32_t>(shadow_.get(), offset, count,
+      max_v = GetMaxValue<uint32_t>(shadow_.data(), offset, count,
           primitive_restart_index);
       break;
     default:
       NOTREACHED();  // should never get here by validation.
       break;
   }
   range_set_.insert(std::make_pair(range, max_v));
   *max_value = max_v;
   return true;
 }
@@ skipping 13 matching lines @@
 bool BufferManager::IsUsageClientSideArray(GLenum usage) {
   return usage == GL_STREAM_DRAW && use_client_side_arrays_for_stream_buffers_;
 }
 
 bool BufferManager::UseNonZeroSizeForClientSideArrayBuffer() {
   return feature_info_.get() &&
          feature_info_->workarounds()
              .use_non_zero_size_for_client_side_stream_buffers;
 }
 
-void BufferManager::SetInfo(Buffer* buffer, GLenum target, GLsizeiptr size,
-                            GLenum usage, const GLvoid* data) {
-  DCHECK(buffer);
-  memory_type_tracker_->TrackMemFree(buffer->size());
+bool BufferManager::UseShadowBuffer(GLenum target, GLenum usage) {
   const bool is_client_side_array = IsUsageClientSideArray(usage);
   const bool support_fixed_attribs =
     gfx::GetGLImplementation() == gfx::kGLImplementationEGLGLES2;
+
   // TODO(zmo): Don't shadow buffer data on ES3. crbug.com/491002.
-  const bool shadow = target == GL_ELEMENT_ARRAY_BUFFER ||
-                      allow_buffers_on_multiple_targets_ ||
-                      (allow_fixed_attribs_ && !support_fixed_attribs) ||
-                      is_client_side_array;
-  buffer->SetInfo(size, usage, shadow, data, is_client_side_array);
+  return (target == GL_ELEMENT_ARRAY_BUFFER ||
+          allow_buffers_on_multiple_targets_ ||
+          (allow_fixed_attribs_ && !support_fixed_attribs) ||
+          is_client_side_array);
+}
+
+void BufferManager::SetInfo(Buffer* buffer, GLenum target, GLsizeiptr size,
+                            GLenum usage, bool use_shadow) {
+  DCHECK(buffer);
+  memory_type_tracker_->TrackMemFree(buffer->size());
+  buffer->SetInfo(size, usage, use_shadow, IsUsageClientSideArray(usage));
   memory_type_tracker_->TrackMemAlloc(buffer->size());
 }
 
 void BufferManager::ValidateAndDoBufferData(
     ContextState* context_state, GLenum target, GLsizeiptr size,
     const GLvoid* data, GLenum usage) {
   ErrorState* error_state = context_state->GetErrorState();
   if (!feature_info_->validators()->buffer_target.IsValid(target)) {
     ERRORSTATE_SET_GL_ERROR_INVALID_ENUM(
         error_state, "glBufferData", target, "target");
@@ skipping 27 matching lines @@
 }
 
 
 void BufferManager::DoBufferData(
     ErrorState* error_state,
     Buffer* buffer,
     GLenum target,
     GLsizeiptr size,
     GLenum usage,
     const GLvoid* data) {
-  // Clear the buffer to 0 if no initial data was passed in.
-  scoped_ptr<int8_t[]> zero;
-  if (!data) {
-    zero.reset(new int8_t[size]);
-    memset(zero.get(), 0, size);
-    data = zero.get();
-  }
+
+    // Stage the shadow buffer first if we are using a shadow buffer so that we

[Ken Russell (switch to Gerrit), 2016/03/31 23:28:04]

Is the indentation off here?

[David Yen, 2016/04/01 17:00:53]

Done. I just ran cl format.

+    // validate what we store internally.
+    const bool use_shadow = UseShadowBuffer(target, usage);
+    data = buffer->StageShadow(use_shadow, size, data);
 
   ERRORSTATE_COPY_REAL_GL_ERRORS_TO_WRAPPER(error_state, "glBufferData");
   if (IsUsageClientSideArray(usage)) {
     GLsizei empty_size = UseNonZeroSizeForClientSideArrayBuffer() ? 1 : 0;
     glBufferData(target, empty_size, NULL, usage);
   } else {
     glBufferData(target, size, data, usage);
   }
   GLenum error = ERRORSTATE_PEEK_GL_ERROR(error_state, "glBufferData");
-  if (error == GL_NO_ERROR) {
-    SetInfo(buffer, target, size, usage, data);
-  } else {
-    SetInfo(buffer, target, 0, usage, NULL);
+  if (error != GL_NO_ERROR) {
+    size = 0;
+    buffer->StageShadow(false, 0, nullptr); // Also clear the shadow.
   }
+
+  SetInfo(buffer, target, size, usage, use_shadow);
 }
 
 void BufferManager::ValidateAndDoBufferSubData(
   ContextState* context_state, GLenum target, GLintptr offset, GLsizeiptr size,
   const GLvoid * data) {
   ErrorState* error_state = context_state->GetErrorState();
   Buffer* buffer = GetBufferInfoForTarget(context_state, target);
   if (!buffer) {
     ERRORSTATE_SET_GL_ERROR(error_state, GL_INVALID_VALUE, "glBufferSubData",
                             "unknown buffer");
@@ skipping 192 matching lines @@
     auto guid = gfx::GetGLBufferGUIDForTracing(
         memory_tracker_->ShareGroupTracingGUID(), client_buffer_id);
     pmd->CreateSharedGlobalAllocatorDump(guid);
     pmd->AddOwnershipEdge(dump->guid(), guid);
   }
   return true;
 }
 
 }  // namespace gles2
 }  // namespace gpu