Implementing FlushMappedBufferRange 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 85b7df82f8ca9908ac73845b9a4515c343df7495..7b3cdf483a04861674d23dc70579ef8d83a002c2 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -166,6 +166,18 @@ struct TexSubCoord3D {
   int depth;
 };
 
+// Check if all |ref| bits are set in |bits|.
+bool AllBitsSet(GLbitfield bits, GLbitfield ref) {
+  DCHECK_NE(0u, ref);
+  return ((bits & ref) == ref);
+}
+
+// Check if any of |ref| bits are set in |bits|.
+bool AnyBitsSet(GLbitfield bits, GLbitfield ref) {
+  DCHECK_NE(0u, ref);
+  return ((bits & ref) != 0);
+}
+
 }  // namespace
 
 class GLES2DecoderImpl;
@@ -1034,6 +1046,9 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   void DoScheduleCALayerInUseQueryCHROMIUM(GLsizei count,
                                            const volatile GLuint* textures);
 
+  void DoFlushMappedBufferRange(
+      GLenum target, GLintptr offset, GLsizeiptr size);
+
   // Creates a Program for the given program.
   Program* CreateProgram(GLuint client_id, GLuint service_id) {
     return program_manager()->CreateProgram(client_id, service_id);
@@ -16831,37 +16846,10 @@ error::Error GLES2DecoderImpl::HandleMapBufferRange(
     *result = 0;
     return error::kInvalidArguments;
   }
-  int8_t* mem =
-      GetSharedMemoryAs<int8_t*>(data_shm_id, data_shm_offset, size);
-  if (!mem) {
-    return error::kOutOfBounds;
-  }
-
   if (!validators_->buffer_target.IsValid(target)) {
     LOCAL_SET_GL_ERROR_INVALID_ENUM(func_name, target, "target");
     return error::kNoError;
   }
-
-  GLbitfield mask = GL_MAP_INVALIDATE_BUFFER_BIT;
-  if ((access & mask) == mask) {
-    // TODO(zmo): To be on the safe side, always map
-    // GL_MAP_INVALIDATE_BUFFER_BIT to GL_MAP_INVALIDATE_RANGE_BIT.
-    access = (access & ~GL_MAP_INVALIDATE_BUFFER_BIT);
-    access = (access | GL_MAP_INVALIDATE_RANGE_BIT);
-  }
-  // TODO(zmo): Always filter out GL_MAP_UNSYNCHRONIZED_BIT to get rid of
-  // undefined behaviors.
-  mask = GL_MAP_READ_BIT | GL_MAP_UNSYNCHRONIZED_BIT;
-  if ((access & mask) == mask) {
-    LOCAL_SET_GL_ERROR(
-        GL_INVALID_OPERATION, func_name, "incompatible access bits");
-    return error::kNoError;
-  }
-  access = (access & ~GL_MAP_UNSYNCHRONIZED_BIT);
-  if ((access & GL_MAP_WRITE_BIT) == GL_MAP_WRITE_BIT &&
-      (access & GL_MAP_INVALIDATE_RANGE_BIT) == 0) {
-    access = (access | GL_MAP_READ_BIT);
-  }
   Buffer* buffer = buffer_manager()->GetBufferInfoForTarget(&state_, target);
   if (!buffer) {
     LOCAL_SET_GL_ERROR(
@@ -16871,11 +16859,53 @@ error::Error GLES2DecoderImpl::HandleMapBufferRange(
   if (buffer->GetMappedRange()) {
     LOCAL_SET_GL_ERROR(
         GL_INVALID_OPERATION, func_name, "buffer is already mapped");
+    return error::kNoError;
+  }
+  if (size == 0) {

[vmiura, 2016/10/04 22:18:52]

I think size == 0 should not generate a GL error.  Perhaps just return kNoError,
after all other error checks are done?

[Zhenyao Mo, 2016/10/04 22:30:27]

This is explicitly required by the spec, ES 3.04, page 39, the first
INVALID_OPERATION condition.

+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name, "size is zero");
+    return error::kNoError;
   }
   if (!buffer->CheckRange(offset, size)) {
     LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, func_name, "invalid range");
     return error::kNoError;
   }
+  int8_t* mem =
+      GetSharedMemoryAs<int8_t*>(data_shm_id, data_shm_offset, size);
+  if (!mem) {
+    return error::kOutOfBounds;
+  }
+  if (!AnyBitsSet(access, GL_MAP_READ_BIT | GL_MAP_WRITE_BIT)) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name,
+        "neither MAP_READ_BIT nore MAP_WRITE_BIT is set");
+    return error::kNoError;
+  }

[vmiura, 2016/10/04 22:18:52]

Could you add check for "GL_INVALID_VALUE is generated if access has any bits
set other than those defined above."

if ( access & (~GL_MAP_READ_BIT | GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_RANGE_BIT
| GL_MAP_INVALIDATE_BUFFER_BIT | GL_MAP_FLUSH_EXPLICIT_BIT |
GL_MAP_UNSYNCHRONIZED_BIT)) {
  ...error
}

[Zhenyao Mo, 2016/10/04 22:30:27]

Done.

+  if (AllBitsSet(access, GL_MAP_READ_BIT) &&
+      AnyBitsSet(access, (GL_MAP_INVALIDATE_RANGE_BIT |
+                          GL_MAP_INVALIDATE_BUFFER_BIT |
+                          GL_MAP_UNSYNCHRONIZED_BIT))) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name,
+        "Incompatible access bits with MAP_READ_BIT");
+    return error::kNoError;
+  }
+  if (AllBitsSet(access, GL_MAP_FLUSH_EXPLICIT_BIT) &&
+      !AllBitsSet(access, GL_MAP_WRITE_BIT)) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name,
+        "MAP_FLUSH_EXPLICIT_BIT set without MAP_WRITE_BIT");
+    return error::kNoError;
+  }
+  if (AllBitsSet(access, GL_MAP_INVALIDATE_BUFFER_BIT)) {
+    // To be on the safe side, always map GL_MAP_INVALIDATE_BUFFER_BIT to
+    // GL_MAP_INVALIDATE_RANGE_BIT.
+    access = (access & ~GL_MAP_INVALIDATE_BUFFER_BIT);
+    access = (access | GL_MAP_INVALIDATE_RANGE_BIT);
+  }
+  // Always filter out GL_MAP_UNSYNCHRONIZED_BIT to get rid of undefined
+  // behaviors.
+  access = (access & ~GL_MAP_UNSYNCHRONIZED_BIT);
+  if (AllBitsSet(access, GL_MAP_WRITE_BIT) &&
+      !AllBitsSet(access, GL_MAP_INVALIDATE_RANGE_BIT)) {
+    access = (access | GL_MAP_READ_BIT);
+  }
   void* ptr = glMapBufferRange(target, offset, size, access);
   if (ptr == nullptr) {
     return error::kNoError;
@@ -16891,41 +16921,38 @@ error::Error GLES2DecoderImpl::HandleMapBufferRange(
 }
 
 error::Error GLES2DecoderImpl::HandleUnmapBuffer(
-    uint32_t immediate_data_size,
-    const volatile void* cmd_data) {
+    uint32_t immediate_data_size, const volatile void* cmd_data) {
   if (!unsafe_es3_apis_enabled()) {
     return error::kUnknownCommand;
   }
+  const char* func_name = "glUnmapBuffer";
+
   const volatile gles2::cmds::UnmapBuffer& c =
       *static_cast<const volatile gles2::cmds::UnmapBuffer*>(cmd_data);
   GLenum target = static_cast<GLenum>(c.target);
 
   if (!validators_->buffer_target.IsValid(target)) {
-    LOCAL_SET_GL_ERROR_INVALID_ENUM("glMapBufferRange", target, "target");
+    LOCAL_SET_GL_ERROR_INVALID_ENUM(func_name, target, "target");
     return error::kNoError;
   }
 
   Buffer* buffer = buffer_manager()->GetBufferInfoForTarget(&state_, target);
   if (!buffer) {
-    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "UnmapBuffer", "no buffer bound");
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name, "no buffer bound");
     return error::kNoError;
   }
   const Buffer::MappedRange* mapped_range = buffer->GetMappedRange();
   if (!mapped_range) {
-    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "UnmapBuffer",
-                       "buffer is unmapped");
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name, "buffer is unmapped");
     return error::kNoError;
   }
-  if ((mapped_range->access & GL_MAP_WRITE_BIT) == 0 ||
-      (mapped_range->access & GL_MAP_FLUSH_EXPLICIT_BIT) ==
-           GL_MAP_FLUSH_EXPLICIT_BIT) {
+  if (!AllBitsSet(mapped_range->access, GL_MAP_WRITE_BIT) ||
+      AllBitsSet(mapped_range->access, GL_MAP_FLUSH_EXPLICIT_BIT)) {
     // If we don't need to write back, or explict flush is required, no copying
     // back is needed.
   } else {
     void* mem = mapped_range->GetShmPointer();
-    if (!mem) {
-      return error::kOutOfBounds;
-    }
+    DCHECK(mem);
     DCHECK(mapped_range->pointer);
     memcpy(mapped_range->pointer, mem, mapped_range->size);
     if (buffer->shadowed()) {
@@ -16942,7 +16969,7 @@ error::Error GLES2DecoderImpl::HandleUnmapBuffer(
     // TODO(zmo): We could redo the map / copy data / unmap to recover, but
     // the second unmap could still return GL_FALSE. For now, we simply lose
     // the contexts in the share group.
-    LOG(ERROR) << "glUnmapBuffer unexpectedly returned GL_FALSE";
+    LOG(ERROR) << func_name << " unexpectedly returned GL_FALSE";
     // Need to lose current context before broadcasting!
     MarkContextLost(error::kGuilty);
     group_->LoseContexts(error::kInnocent);
@@ -16951,6 +16978,50 @@ error::Error GLES2DecoderImpl::HandleUnmapBuffer(
   return error::kNoError;
 }
 
+void GLES2DecoderImpl::DoFlushMappedBufferRange(
+    GLenum target, GLintptr offset, GLsizeiptr size) {
+  const char* func_name = "glFlushMappedBufferRange";
+  // |size| is validated in HandleFlushMappedBufferRange().
+  if (offset < 0) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, func_name, "offset < 0");
+    return;
+  }
+  Buffer* buffer = buffer_manager()->GetBufferInfoForTarget(&state_, target);
+  if (!buffer) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name, "no buffer bound");
+    return;
+  }
+  const Buffer::MappedRange* mapped_range = buffer->GetMappedRange();
+  if (!mapped_range) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name, "buffer is unmapped");
+    return;
+  }
+  if (!AllBitsSet(mapped_range->access, GL_MAP_FLUSH_EXPLICIT_BIT)) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name,
+        "buffer is mapped without MAP_FLUSH_EXPLICIT_BIT flag");
+    return;
+  }
+  base::CheckedNumeric<int32_t> range_size = size;
+  range_size += offset;
+  if (!range_size.IsValid() ||
+      range_size.ValueOrDefault(0) > mapped_range->size) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, func_name,
+        "offset + size out of bounds");
+    return;
+  }
+  char* client_data = reinterpret_cast<char*>(mapped_range->GetShmPointer());
+  DCHECK(client_data);
+  char* gpu_data = reinterpret_cast<char*>(mapped_range->pointer);
+  DCHECK(gpu_data);
+  memcpy(gpu_data + offset, client_data + offset, size);
+  if (buffer->shadowed()) {
+    bool success = buffer->SetRange(
+        mapped_range->offset + offset, size, client_data + offset);
+    DCHECK(success);
+  }
+  glFlushMappedBufferRange(target, offset, size);
+}
+
 // Note that GL_LOST_CONTEXT is specific to GLES.
 // For desktop GL we have to query the reset status proactively.
 void GLES2DecoderImpl::OnContextLostError() {