Fix PRIMITIVE_RESTART_FIXED_INDEX handling in command buffer and WebGL 2.0.

gpu/command_buffer/service/buffer_manager.cc

Index: gpu/command_buffer/service/buffer_manager.cc
diff --git a/gpu/command_buffer/service/buffer_manager.cc b/gpu/command_buffer/service/buffer_manager.cc
index 914d9b09da06e8fb3a71c2162e9a8e6291dea962..e2c24e6eb8c2dd8211de3d35421c7004640dcf5c 100644
--- a/gpu/command_buffer/service/buffer_manager.cc
+++ b/gpu/command_buffer/service/buffer_manager.cc
@@ -199,13 +199,17 @@ void Buffer::ClearCache() {
 }
 
 template <typename T>
-GLuint GetMaxValue(const void* data, GLuint offset, GLsizei count) {
+GLuint GetMaxValue(const void* data, GLuint offset, GLsizei count,
+    GLuint primitive_restart_index) {
   GLuint max_value = 0;
   const T* element =
       reinterpret_cast<const T*>(static_cast<const int8_t*>(data) + offset);
   const T* end = element + count;
   for (; element < end; ++element) {
     if (*element > max_value) {
+      if (*element == primitive_restart_index) {
+        continue;
+      }
       max_value = *element;
     }
   }
@@ -213,13 +217,55 @@ GLuint GetMaxValue(const void* data, GLuint offset, GLsizei count) {
 }
 
 bool Buffer::GetMaxValueForRange(
-    GLuint offset, GLsizei count, GLenum type, GLuint* max_value) {
-  Range range(offset, count, type);
+    GLuint offset, GLsizei count, GLenum type, bool primitive_restart_enabled,
+    GLuint* max_value) {
+  bool index_buffer =
+      this->initial_target() == GL_ELEMENT_ARRAY_BUFFER ? true : false;

[piman, 2016/03/19 00:56:11]

nit: "? true : false" is redundant

[piman, 2016/03/19 00:56:11]

What is this check for? This is only used for things that will be used as
indices, it doesn't really matter which target this was originally created for -
we still would want to avoid false-negatives if primitive restart is enabled.

[Ken Russell (switch to Gerrit), 2016/03/19 04:06:52]

Good point -- I copied this code from the original CL without thinking it
through. It seems unnecessary. Removed and re-tested; all tests in
deqp/functional/gles3/primitiverestart.html still pass.

[yunchao, 2016/03/21 15:11:35]

I am not familiar with GetMaxValueInBufferCHROMIUM. 
We expose GetMaxValueInBufferCHROMIUM in command buffer, which calls into
GetMaxValueForRange. Does this API only access to index buffer? Is it valid to
access to other buffers? Please considering this situation, we have 0xFF in
color buffer, and try to get the max value in that buffer in WebGL 2 (primitive
restart is always enabled in WebGL 2). 

[Ken Russell (switch to Gerrit), 2016/03/21 17:41:46]

GetMaxValueInBufferCHROMIUM is used to emulate client-side vertex arrays. It
returns the largest index accessed by the index buffer for a given draw call so
the client side can upload just those vertices into a buffer object behind the
scenes. It won't affect the fetching of values from non-element-array buffer
objects.

+  GLuint primitive_restart_index = 0;
+  if (index_buffer && primitive_restart_enabled) {
+    switch (type) {
+      case GL_UNSIGNED_BYTE:
+        primitive_restart_index = 0xFF;
+        break;
+      case GL_UNSIGNED_SHORT:
+        primitive_restart_index = 0xFFFF;
+        break;
+      case GL_UNSIGNED_INT:
+        primitive_restart_index = 0xFFFFFFFF;
+        break;
+      default:
+        NOTREACHED();  // should never get here by validation.
+        break;
+    }
+  }
+
+  Range range(offset, count, type, primitive_restart_enabled);
   RangeToMaxValueMap::iterator it = range_set_.find(range);
   if (it != range_set_.end()) {
     *max_value = it->second;
     return true;
   }
+  // Optimization. If:
+  //  - primitive restart is enabled
+  //  - we don't have an entry in the range set for these parameters
+  //    for the situation when primitive restart is enabled
+  //  - we do have an entry in the range set for these parameters for
+  //    the situation when primitive restart is disabled
+  //  - this entry is less than the primitive restart index
+  // Then we can repurpose this entry for the situation when primitive
+  // restart is enabled. Otherwise, we need to compute the max index
+  // from scratch.
+  if (primitive_restart_enabled) {
+    Range disabled_range(offset, count, type, false);
+    RangeToMaxValueMap::iterator it = range_set_.find(disabled_range);
+    if (it != range_set_.end() && it->second < primitive_restart_index) {
+      // This reuses the max value for the case where primitive
+      // restart is enabled.
+      range_set_.insert(std::make_pair(range, it->second));
+      *max_value = it->second;
+      return true;
+    }
+  }
 
   uint32_t size;
   if (!SafeMultiplyUint32(
@@ -243,21 +289,24 @@ bool Buffer::GetMaxValueForRange(
   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_.get(), 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_.get(), 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_.get(), offset, count,
+          primitive_restart_index);
       break;
     default:
       NOTREACHED();  // should never get here by validation.