[WebGL 2] primitive restart should be always enabled

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..c7b586151691bb50c4f582a53d2144ddd47133a0 100644
--- a/gpu/command_buffer/service/buffer_manager.cc
+++ b/gpu/command_buffer/service/buffer_manager.cc
@@ -34,6 +34,7 @@ BufferManager::BufferManager(MemoryTracker* memory_tracker,
       allow_buffers_on_multiple_targets_(false),
       allow_fixed_attribs_(false),
       buffer_count_(0),
+      primitive_restart_enabled_(false),
       have_context_(true),
       use_client_side_arrays_for_stream_buffers_(
           feature_info
@@ -199,13 +200,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;
     }
   }
@@ -214,11 +219,36 @@ GLuint GetMaxValue(const void* data, GLuint offset, GLsizei count) {
 
 bool Buffer::GetMaxValueForRange(
     GLuint offset, GLsizei count, GLenum type, GLuint* max_value) {
+  bool index_buffer =
+      this->initial_target() == GL_ELEMENT_ARRAY_BUFFER ? true : false;
+  GLuint primitive_restart_index = 0;
+  if (index_buffer && manager_->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);

[Ken Russell (switch to Gerrit), 2016/03/16 16:18:07]

The suggestion is to add the "primitive restart enabled" bit to the Range class,
and use that as part of the key during lookup in the RangeToMaxValueMap. Then
the map can cache the results for the same range both for having primitive
restart enabled as well as disabled. The lookup code can optimistically try to
populate the version with primitive restart enabled from the version with
primitive restart disabled, if the value isn't the primitive restart index.
Otherwise it needs to compute it from scratch.

[yunchao, 2016/03/17 00:19:02]

Ken, I tried this method at first. But the first element in std::map should be
constant. Range is the first element in RangeToMaxValueMap. So I follow the idea
you suggested and get this one: use primitive_restart_flipped (I also tried
primitive_restart_enabled, it is OK too), and use a struct to cache the status
together with max_value in the second element in RangeToMaxValue. It has the
same optimization as bajones@ suggested, it can reuse the max_value when it
flips from primitive restart disabled to primitive restart enabled if possible.
Moreover, if it flips with a even number, say 2, 4 and so forth. The untouched
ranges those are not called by GexMaxValueForRange() during flipping will keep
the same. they don't need to recalculate from scratch!

Could you have a look of this one, I think it is correct, and it may be better.

[Zhenyao Mo, 2016/03/18 20:28:44]

Yunchao, I think you misunderstood.  The suggestion is to make the key range +
primitive_restart_enabled, and the value is max_value.

So for each range, you might have two entries, (range, false) -> max_value, and
(range, true) -> max_value.

   RangeToMaxValueMap::iterator it = range_set_.find(range);
   if (it != range_set_.end()) {
-    *max_value = it->second;
-    return true;
+    if (!index_buffer || !it->second.primitive_restart_flipped_ ||
+        (manager_->primitive_restart_enabled_ &&
+        it->second.max_value_ < primitive_restart_index)) {
+      *max_value = it->second.max_value_;
+      it->second.primitive_restart_flipped_ = false;
+      return true;
+    }
   }
 
   uint32_t size;
@@ -243,27 +273,36 @@ 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.
       break;
   }
-  range_set_.insert(std::make_pair(range, max_v));
+  if (it != range_set_.end()) {
+    it->second.max_value_ = max_v;
+    it->second.primitive_restart_flipped_ = false;
+  } else {
+    struct MaxValue value(max_v);
+    range_set_.insert(std::make_pair(range, value));
+  }
   *max_value = max_v;
   return true;
 }
@@ -536,6 +575,24 @@ Buffer* BufferManager::GetBufferInfoForTarget(
   }
 }
 
+void BufferManager::SetPrimitiveRestartState(bool enabled) {
+  if (primitive_restart_enabled_ != enabled) {
+    primitive_restart_enabled_ = enabled;
+
+    for (BufferMap::const_iterator it = buffers_.begin();
+        it != buffers_.end(); ++it) {
+      if (it->second->initial_target() == GL_ELEMENT_ARRAY_BUFFER) {
+        Buffer::RangeToMaxValueMap::iterator range_set_it;
+        for (range_set_it = it->second->range_set_.begin();
+            range_set_it != it->second->range_set_.end(); ++range_set_it) {
+          range_set_it->second.primitive_restart_flipped_ =
+              !range_set_it->second.primitive_restart_flipped_;
+        }
+      }
+    }
+  }
+}
+
 bool BufferManager::OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
                                  base::trace_event::ProcessMemoryDump* pmd) {
   const int client_id = memory_tracker_->ClientId();