GPU client side changes for GpuMemoryBuffers

gpu/command_buffer/client/gles2_implementation.cc

Index: gpu/command_buffer/client/gles2_implementation.cc
diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
index aa5792a04498f6c172502a32bb72864802d1dd15..c2ec618d1a8753a1ee3ed62895d05304a5ab200f 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -16,6 +16,8 @@
 #include <GLES2/gl2ext.h>
 #include <GLES2/gl2extchromium.h>
 #include "../client/buffer_tracker.h"
+#include "../client/gpu_memory_buffer.h"
+#include "../client/gpu_memory_buffer_factory.h"
 #include "../client/mapped_memory.h"
 #include "../client/program_info_manager.h"
 #include "../client/query_tracker.h"
@@ -98,6 +100,7 @@ GLES2Implementation::GLES2Implementation(
       bound_array_buffer_id_(0),
       bound_pixel_pack_transfer_buffer_id_(0),
       bound_pixel_unpack_transfer_buffer_id_(0),
+      bound_gpu_memory_buffer_id_(0),
       error_bits_(0),
       debug_(false),
       use_count_(0),
@@ -173,6 +176,7 @@ bool GLES2Implementation::Initialize(
 
   query_tracker_.reset(new QueryTracker(mapped_memory_.get()));
   buffer_tracker_.reset(new BufferTracker(mapped_memory_.get()));
+  gpu_memory_buffer_tracker_.reset(new GpuMemoryBufferTracker());
 
 #if defined(GLES2_SUPPORT_CLIENT_SIDE_ARRAYS)
   GetIdHandler(id_namespaces::kBuffers)->MakeIds(
@@ -630,6 +634,9 @@ bool GLES2Implementation::GetHelper(GLenum pname, GLint* params) {
         return true;
       }
       return false;
+    case GL_BOUND_GPU_MEMORY_BUFFER_CHROMIUM:

[greggman, 2013/04/25 23:45:34]

Seems like following the other ENUMs this should be

  GL_IMAGE_BUFFER_BINDING_CHROMIUM

To match the target GL_IMAGE_BUFFER_CHROMIUM

[kaanb1, 2013/04/26 03:01:21]

Done.

+      *params = bound_gpu_memory_buffer_id_;
+      return true;
     default:
       return false;
   }
@@ -1245,6 +1252,17 @@ void GLES2Implementation::ShaderSource(
   CheckGLError();
 }
 
+bool GLES2Implementation::GetBoundGpuMemoryBuffer(

[greggman, 2013/04/25 23:45:34]

This function always returns true. Did you mean it to return false if it
generates an error?

[kaanb1, 2013/04/26 03:01:21]

GetBoundPixelTransferBuffer only returns false when called with a target it
doesn't know about. Since we don't take target as a parameter here it isn't
applicable. Are there any other error situations?

+    const char* function_name, GLuint* buffer_id) {
+  *buffer_id = bound_gpu_memory_buffer_id_;
+
+  if (*buffer_id == 0) {
+    SetGLError(GL_INVALID_OPERATION, function_name, "no buffer bound");
+  }
+
+  return true;
+}
+
 void GLES2Implementation::BufferDataHelper(
     GLenum target, GLsizeiptr size, const void* data, GLenum usage) {
   if (size < 0) {
@@ -2017,6 +2035,11 @@ void GLES2Implementation::GetShaderPrecisionFormat(
 const GLubyte* GLES2Implementation::GetStringHelper(GLenum name) {
   const char* result = NULL;
   // Clears the bucket so if the command fails nothing will be in it.
+  if (name == GL_BOUND_GPU_MEMORY_BUFFER_CHROMIUM_POINTER) {

[greggman, 2013/04/25 23:45:34]

Doesn't this seem like a kind of abuse of this function?

How about adding some other function? Maybe either
glGetMemoryBufferPointerCHROMIUM

Or

void glGetPointervCHROMIUM(GLenum pname, GLvoid** pointer);

Which you can call like this

  GLvoid* buffer = NULL;
  glGetPointervCHROMIUM(
      GL_IMAGE_BUFFER_POINTER_CHROMIUM, &buffer);

I'd prefer the second as it can be used for other things later. What do you
think?

[kaanb1, 2013/04/26 03:01:21]

Done.

+    return const_cast<const GLubyte*>(static_cast<GLubyte*>(
+        GetNativeHandleForBoundGpuMemoryBuffer()));

[jamesr, 2013/04/25 23:24:15]

this really seems like voodoo. are there no better entry points?

[apatrick_chromium, 2013/04/25 23:39:03]

Agreed. I had a "wat?" moment.

[kaanb1, 2013/04/25 23:42:19]

Would you be okay if I added a new public API that returns a GpuMemoryBuffer
pointer? 

[no sievers, 2013/04/26 00:17:43]

You don't need a full-fledged GL API. This is only used from inside WGC3DImpl
and your test. Both can get to the GLES2Implementation (the test can get it
through GLManager). So just expose it from GLES2Implementation only.

[kaanb1, 2013/04/26 03:01:21]

Looks like the test doesn't build when I just expose it from
GLES2Implementation, is there anything I'm missing? Or should I just add a new
command like I did for imageBufferDataCHROMIUM

+  }
+
   helper_->SetBucketSize(kResultBucketId, 0);
   helper_->GetString(name, kResultBucketId);
   std::string str;
@@ -2301,6 +2324,9 @@ bool GLES2Implementation::BindBufferHelper(
     case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
       bound_pixel_unpack_transfer_buffer_id_ = buffer;
       break;
+    case GL_IMAGE_BUFFER_CHROMIUM:
+      bound_gpu_memory_buffer_id_ = buffer;
+      break;
     default:
       changed = true;
       break;
@@ -2445,6 +2471,14 @@ void GLES2Implementation::DeleteBuffersHelper(
     if (buffers[ii] == bound_pixel_unpack_transfer_buffer_id_) {
       bound_pixel_unpack_transfer_buffer_id_ = 0;
     }
+    if (buffers[ii] == bound_gpu_memory_buffer_id_) {
+      bound_gpu_memory_buffer_id_ = 0;
+    }
+    GpuMemoryBuffer* gpu_buffer =
+        gpu_memory_buffer_tracker_->GetBuffer(buffers[ii]);
+    if (gpu_buffer) {
+      gpu_memory_buffer_tracker_->RemoveBuffer(buffers[ii]);
+    }
   }
 }
 
@@ -3390,12 +3424,50 @@ void* GLES2Implementation::MapBufferCHROMIUM(GLuint target, GLenum access) {
         return NULL;
       }
       break;
+    case GL_IMAGE_BUFFER_CHROMIUM:
+      break;
     default:
       SetGLError(
           GL_INVALID_ENUM, "glMapBufferCHROMIUM", "invalid target");
       return NULL;
   }
+
   GLuint buffer_id;
+  if (target == GL_IMAGE_BUFFER_CHROMIUM &&
+      GetBoundGpuMemoryBuffer("glMapBufferCHROMIUM", &buffer_id)) {

[greggman, 2013/04/25 23:45:34]

If GetBoundGPUMemoryBuffer returns false (assuming you meant it to return falls
on error), then it just falls through to GetBoundPixelBuffer. Is that correct?

[kaanb1, 2013/04/26 03:01:21]

Converted this function to use a switch statement.

+    if (buffer_id == 0) {
+      return NULL;
+    }
+
+    GpuMemoryBuffer* gpu_buffer =
+        gpu_memory_buffer_tracker_->GetBuffer(buffer_id);
+    if (!gpu_buffer) {
+      SetGLError(GL_INVALID_OPERATION,
+                 "glMapBufferCHROMIUM",
+                 "invalid GPU memory buffer");
+      return NULL;
+    }
+    GpuMemoryBuffer::AccessMode mode = GpuMemoryBuffer::READ_ONLY;
+    switch (access) {
+      case GL_WRITE_ONLY:
+        mode = GpuMemoryBuffer::WRITE_ONLY;
+        break;
+      case GL_READ_ONLY:
+        mode = GpuMemoryBuffer::READ_ONLY;
+        break;
+      // TODO(kaanb): should we add GL_READ_WRITE to gl2ext.h?
+      default:
+        SetGLError(
+            GL_INVALID_ENUM, "glMapBufferCHROMIUM", "invalid GPU access mode");
+        return NULL;
+    }
+
+    void* mapped_buffer = NULL;
+    gpu_buffer->Map(mode, &mapped_buffer);

[greggman, 2013/04/25 23:45:34]

The semantics of map buffer require that you can only map once, otherwise
generate GL_INVALID_OPERATION

In other words calling glMapBuffer twice for the same buffer should fail. The
buffer must be unmapped before mapping it again.

[kaanb1, 2013/04/26 03:01:21]

Done.

+    CheckGLError();
+    return mapped_buffer;
+  }
+
   GetBoundPixelTransferBuffer(target, "glMapBufferCHROMIUM", &buffer_id);
   if (!buffer_id) {
     return NULL;
@@ -3431,6 +3503,19 @@ GLboolean GLES2Implementation::UnmapBufferCHROMIUM(GLuint target) {
   GPU_CLIENT_LOG(
       "[" << GetLogPrefix() << "] glUnmapBufferCHROMIUM(" << target << ")");
   GLuint buffer_id;
+  if (target == GL_IMAGE_BUFFER_CHROMIUM &&
+      GetBoundGpuMemoryBuffer("glUnmapBufferCHROMIUM", &buffer_id)) {

[greggman, 2013/04/25 23:45:34]

Same here. If GetBoundGpuMemroyBuffer returns false (assuming it's supposed to
return false) then it doesn't seem like you want it to fall through?

Actually, looking at the fall through works but it seems fragile to me. Maybe
I'm being dumb.

I'd think it might be clearer if the code here at in MapBuffer was more like
either

   switch (target) {
     case GL_IMAGE_BUFFER_CHROMIUM:
       ...
       break;
     case GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
     case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
       ...
       break;
     default:
       SetGLError(..., INVALID_ENUM, ...)

Or at least

   if (target == GL_IMAGE_BUFFER_CHROMIUM) {
     ...

     return;  // don't let the code follow through since it can't
              // possibly be interested in anything since we 
              // know the target is incorrect for any code below.
   }

What do you think? I think I'd prefer the switch as far as readability goes.

[kaanb1, 2013/04/26 03:01:21]

Done.

+    if (buffer_id == 0) {
+      return false;
+    }
+
+    GpuMemoryBuffer* gpu_buffer =
+        gpu_memory_buffer_tracker_->GetBuffer(buffer_id);
+
+    gpu_buffer->Unmap();

[greggman, 2013/04/25 23:45:34]

The semantics of map buffer require if the buffer is not mapped this should
generate GL_INVALID_OPERATION.

[kaanb1, 2013/04/26 03:01:21]

Done.

+    return true;
+  }
+
   if (!GetBoundPixelTransferBuffer(target, "glMapBufferCHROMIUM", &buffer_id)) {
     SetGLError(GL_INVALID_ENUM, "glUnmapBufferCHROMIUM", "invalid target");
   }
@@ -3563,6 +3648,70 @@ GLuint GLES2Implementation::InsertSyncPointCHROMIUM() {
   return helper_->InsertSyncPointCHROMIUM();
 }
 
+void* GLES2Implementation::GetNativeHandleForBoundGpuMemoryBuffer() {
+  if (bound_gpu_memory_buffer_id_ == 0) {
+    return NULL;
+  }
+
+  GpuMemoryBuffer* buffer =
+      gpu_memory_buffer_tracker_->GetBuffer(bound_gpu_memory_buffer_id_);
+
+  return (buffer != NULL) ? buffer->GetNativeBuffer() : NULL;
+}
+
+void GLES2Implementation::ImageBufferDataHelperCHROMIUM(
+    GLenum target, GLsizei width, GLsizei height) {
+  if (width < 0) {
+    SetGLError(GL_INVALID_VALUE, "glImageBufferDataCHROMIUM", "width < 0");
+    return;
+  }
+
+  if (height < 0) {
+    SetGLError(GL_INVALID_VALUE, "glImageBufferDataCHROMIUM", "height < 0");
+    return;
+  }
+
+  if (target != GL_IMAGE_BUFFER_CHROMIUM) {
+    SetGLErrorInvalidEnum("glImageBufferDataCHROMIUM", target, "target");
+    return;
+  }
+
+  GLuint buffer_id;
+  bool found_buffer = GetBoundGpuMemoryBuffer(
+      "glImageBufferDataCHROMIUM", &buffer_id);
+
+  if (!found_buffer || buffer_id == 0) {
+    return;
+  }
+
+  GpuMemoryBuffer* buffer =
+      gpu_memory_buffer_tracker_->GetBuffer(buffer_id);
+  if (buffer) {
+    // Remove old buffer, deletes the buffer pointer.
+    gpu_memory_buffer_tracker_->RemoveBuffer(buffer_id);
+  }
+
+  if (width != 0 && height != 0) {
+    // Create new buffer.
+    buffer = gpu_memory_buffer_tracker_->CreateBuffer(buffer_id, width, height);
+    GPU_DCHECK(buffer);
+  }
+
+  return;
+}
+
+void GLES2Implementation::ImageBufferDataCHROMIUM(
+    GLenum target, GLsizei width, GLsizei height) {
+  GPU_CLIENT_SINGLE_THREAD_CHECK();
+  GPU_CLIENT_LOG("[" << GetLogPrefix() << "] glImageBufferDataCHROMIUM("
+      << GLES2Util::GetStringBufferTarget(target) << ", "
+      << width << ", "
+      << height << ")");
+  ImageBufferDataHelperCHROMIUM(target, width, height);
+  CheckGLError();
+}
+
+
 // Include the auto-generated part of this file. We split this because it means
 // we can easily edit the non-auto generated parts right here in this file
 // instead of having to edit some template or the code generator.