Improve BindBufferBase/BindBufferRange in GPU command buffer.

gpu/command_buffer/service/gles2_cmd_decoder.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index 27114203b5b27b931c87b463842f3c3669d6bbf7..71df2b40b809301a5622df72513a41e1a2891ec5 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -729,6 +729,11 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
     kFramebufferInvalidateSub
   };
 
+  enum BindIndexedBufferFunctionType {
+    kBindBufferBase,
+    kBindBufferRange
+  };
+
   // Initialize or re-initialize the shader translator.
   bool InitializeShaderTranslator();
 
@@ -1398,6 +1403,12 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   void DoBindBufferRange(GLenum target, GLuint index, GLuint buffer,
       GLintptr offset, GLsizeiptr size);
 
+  // Helper for DoBindBufferBase and DoBindBufferRange.
+  void BindIndexedBufferImpl(GLenum target, GLuint index, GLuint buffer,
+                             GLintptr offset, GLsizeiptr size,
+                             BindIndexedBufferFunctionType function_type,
+                             const char* function_name);
+
   // Wrapper for glBindFramebuffer since we need to track the current targets.
   void DoBindFramebuffer(GLenum target, GLuint framebuffer);
 
@@ -4652,76 +4663,74 @@ void GLES2DecoderImpl::DoBindBuffer(GLenum target, GLuint client_id) {
   glBindBuffer(target, service_id);
 }
 
-void GLES2DecoderImpl::DoBindBufferBase(GLenum target, GLuint index,
-                                        GLuint client_id) {
-  Buffer* buffer = NULL;
-  GLuint service_id = 0;
-  if (client_id != 0) {
-    buffer = GetBuffer(client_id);
-    if (!buffer) {
-      if (!group_->bind_generates_resource()) {
-        LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION,
-                           "glBindBufferBase",
-                           "id not generated by glGenBuffers");
-        return;
-      }
-
-      // It's a new id so make a buffer for it.
-      glGenBuffersARB(1, &service_id);
-      CreateBuffer(client_id, service_id);
-      buffer = GetBuffer(client_id);
-    }
-  }
-  LogClientServiceForInfo(buffer, client_id, "glBindBufferBase");
-  if (buffer) {
-    // TODO(kbr): track indexed bound buffers.
-    service_id = buffer->service_id();
-  }
+void GLES2DecoderImpl::BindIndexedBufferImpl(
+    GLenum target, GLuint index, GLuint client_id,
+    GLintptr offset, GLsizeiptr size,
+    BindIndexedBufferFunctionType function_type, const char* function_name) {
   switch (target) {
     case GL_TRANSFORM_FEEDBACK_BUFFER: {
-      GLint max_transform_feedback_separate_attribs = 0;
-      DoGetIntegerv(GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS,
-                    &max_transform_feedback_separate_attribs);
-      if (index >=
-          static_cast<GLuint>(max_transform_feedback_separate_attribs)) {
-        LOCAL_SET_GL_ERROR(GL_INVALID_VALUE,
-                           "glBindBufferBase", "index out of range");
+      if (index >= group_->max_transform_feedback_separate_attribs()) {
+        LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, function_name,
+                           "index out of range");
         return;
       }
+      // TODO(zmo): Check transform feedback isn't currently active.
       break;
     }
     case GL_UNIFORM_BUFFER: {
-      GLint max_uniform_buffer_bindings = 0;
-      DoGetIntegerv(GL_MAX_UNIFORM_BUFFER_BINDINGS,
-                    &max_uniform_buffer_bindings);
-      if (index >= static_cast<GLuint>(max_uniform_buffer_bindings)) {
-        LOCAL_SET_GL_ERROR(GL_INVALID_VALUE,
-                           "glBindBufferBase", "index out of range");
+      if (index >= group_->max_uniform_buffer_bindings()) {
+        LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, function_name,
+                           "index out of range");
         return;
       }
       break;
     }
     default:
-      LOCAL_SET_GL_ERROR_INVALID_ENUM(
-          "glBindBufferBase", target, "invalid target");
+      LOCAL_SET_GL_ERROR_INVALID_ENUM(function_name, target, "invalid target");
       return;
   }
-  state_.SetBoundBuffer(target, buffer);
-  glBindBufferBase(target, index, service_id);
-}
 
-void GLES2DecoderImpl::DoBindBufferRange(GLenum target, GLuint index,
-                                         GLuint client_id,
-                                         GLintptr offset,
-                                         GLsizeiptr size) {
-  Buffer* buffer = NULL;
+  if (function_type == kBindBufferRange) {
+    switch (target) {
+      case GL_TRANSFORM_FEEDBACK_BUFFER:
+        if ((size % 4 != 0) || (offset % 4 != 0)) {
+          LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, function_name,
+                             "size or offset are not multiples of 4");
+          return;
+        }
+        break;
+      case GL_UNIFORM_BUFFER: {
+        if (offset % group_->uniform_buffer_offset_alignment() != 0) {
+          LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, function_name,
+              "offset is not a multiple of UNIFORM_BUFFER_OFFSET_ALIGNMENT");
+          return;
+        }
+        break;
+      }
+      default:
+        NOTREACHED();
+        break;
+    }
+
+    if (client_id != 0) {
+      if (size <= 0) {
+        LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, function_name, "size <= 0");
+        return;
+      }
+      if (offset < 0) {
+        LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, function_name, "offset < 0");
+        return;
+      }
+    }
+  }
+
+  Buffer* buffer = nullptr;
   GLuint service_id = 0;
   if (client_id != 0) {
     buffer = GetBuffer(client_id);
     if (!buffer) {
       if (!group_->bind_generates_resource()) {
-        LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION,
-                           "glBindBufferRange",
+        LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, function_name,
                            "id not generated by glGenBuffers");
         return;
       }
@@ -4730,14 +4739,37 @@ void GLES2DecoderImpl::DoBindBufferRange(GLenum target, GLuint index,
       glGenBuffersARB(1, &service_id);
       CreateBuffer(client_id, service_id);
       buffer = GetBuffer(client_id);
+      DCHECK(buffer);
     }
-  }
-  LogClientServiceForInfo(buffer, client_id, "glBindBufferRange");
-  if (buffer) {
-    // TODO(kbr): track indexed bound buffers.
     service_id = buffer->service_id();
   }
-  glBindBufferRange(target, index, service_id, offset, size);
+  LogClientServiceForInfo(buffer, client_id, function_name);
+
+  switch (function_type) {
+    case kBindBufferBase:
+      glBindBufferBase(target, index, service_id);
+      break;
+    case kBindBufferRange:
+      // On Desktop GL 4.1 or lower, clamp the offset/size not to exceed the

[piman, 2016/04/20 21:14:35]

nit: is that a TODO? Mark it so (this reads as if we *are* doing it, but we
aren't).

[Zhenyao Mo, 2016/04/20 22:08:50]

Done.

+      // size of the buffer. crbug.com/604436.
+      glBindBufferRange(target, index, service_id, offset, size);
+      break;
+  }
+  // TODO(kbr): track indexed bound buffers.
+}
+
+void GLES2DecoderImpl::DoBindBufferBase(GLenum target, GLuint index,
+                                        GLuint client_id) {
+  BindIndexedBufferImpl(target, index, client_id, 0, 0,
+                        kBindBufferBase, "glBindBufferBase");
+}
+
+void GLES2DecoderImpl::DoBindBufferRange(GLenum target, GLuint index,
+                                         GLuint client_id,
+                                         GLintptr offset,
+                                         GLsizeiptr size) {
+  BindIndexedBufferImpl(target, index, client_id, offset, size,
+                        kBindBufferRange, "glBindBufferRange");
 }
 
 bool GLES2DecoderImpl::BoundFramebufferHasColorAttachmentWithAlpha() {