Implement client side PBOs for glReadPixel

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 0da78bcf19e1b95aa67024383ddda891e37e9eb4..c6460f180fdebcdad3172ee24f89a5a1aefabf2f 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -96,6 +96,7 @@ GLES2Implementation::GLES2Implementation(
       bound_renderbuffer_(0),
       current_program_(0),
       bound_array_buffer_id_(0),
+      bound_pixel_pack_transfer_buffer_id_(0),
       bound_pixel_unpack_transfer_buffer_id_(0),
       error_bits_(0),
       debug_(false),
@@ -589,6 +590,9 @@ bool GLES2Implementation::GetHelper(GLenum pname, GLint* params) {
         return true;
       }
       return false;
+    case GL_PIXEL_PACK_TRANSFER_BUFFER_BINDING_CHROMIUM:
+      *params = bound_pixel_pack_transfer_buffer_id_;
+      return true;
     case GL_PIXEL_UNPACK_TRANSFER_BUFFER_BINDING_CHROMIUM:
       *params = bound_pixel_unpack_transfer_buffer_id_;
       return true;
@@ -1248,12 +1252,9 @@ void GLES2Implementation::BufferDataHelper(
     return;
   }
 
-  if (target == GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM) {
-    GLuint buffer_id = bound_pixel_unpack_transfer_buffer_id_;
-    if (!buffer_id) {
-      SetGLError(GL_INVALID_VALUE, "glBufferData", "unknown buffer");
-      return;
-    }
+  GLuint buffer_id;
+  if (GetBoundPixelTransferBuffer(target, "glBufferData", &buffer_id)) {
+    if (!buffer_id) return;

[piman, 2013/03/28 20:22:27]

nit: return on its own line

[hubbe, 2013/03/28 20:52:35]

Done.

 
     BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(buffer_id);
     if (buffer) {
@@ -1328,9 +1329,10 @@ void GLES2Implementation::BufferSubDataHelper(
     return;
   }
 
-  if (target == GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM) {
-    BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(
-        bound_pixel_unpack_transfer_buffer_id_);
+  GLuint buffer_id;
+  if (GetBoundPixelTransferBuffer(target, "glBufferSubData", &buffer_id)) {
+    if (!buffer_id) return;

[piman, 2013/03/28 20:22:27]

nit: put return on its own line

[hubbe, 2013/03/28 20:52:35]

Done.

+    BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(buffer_id);
     if (!buffer) {
       SetGLError(GL_INVALID_VALUE, "glBufferSubData", "unknown buffer");
       return;
@@ -1387,12 +1389,37 @@ void GLES2Implementation::BufferSubData(
   CheckGLError();
 }
 
+bool GLES2Implementation::GetBoundPixelTransferBuffer(
+    GLenum target,
+    const char* function_name,
+    GLuint* buffer_id) {
+  *buffer_id = 0;
+
+  switch (target) {
+    case GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
+      *buffer_id = bound_pixel_pack_transfer_buffer_id_;
+      break;
+    case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
+      *buffer_id = bound_pixel_unpack_transfer_buffer_id_;
+      break;
+    default:
+      // Unknown target

[piman, 2013/03/28 20:22:27]

Set error here, rather than the callsites.

[hubbe, 2013/03/28 20:52:35]

The idea is for this function to handle certain targets but not all possible
targets. Returning false just means that this function did not handle the
specified target.

+      return false;
+  }
+  if (!*buffer_id) {
+    SetGLError(GL_INVALID_VALUE, function_name, "no buffer bound");

[piman, 2013/03/28 20:22:27]

GL_INVALID_OPERATION (according to spec).

Should you return false, saving some extra tests on the callsites?

[hubbe, 2013/03/28 20:52:35]

See explanation above.
Also, could you point me to where in the spec this is found so I know better
next time? :)

+  }
+  return true;
+}
+
 BufferTracker::Buffer*
 GLES2Implementation::GetBoundPixelUnpackTransferBufferIfValid(
-    const char* function_name, GLuint offset, GLsizei size)
+    GLuint buffer_id,
+    const char* function_name,
+    GLuint offset, GLsizei size)
 {
-  BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(
-      bound_pixel_unpack_transfer_buffer_id_);
+  DCHECK(buffer_id);
+  BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(buffer_id);
   if (!buffer) {
     SetGLError(GL_INVALID_OPERATION, function_name, "invalid buffer");
     return NULL;
@@ -1431,11 +1458,16 @@ void GLES2Implementation::CompressedTexImage2D(
   if (bound_pixel_unpack_transfer_buffer_id_) {
     GLuint offset = ToGLuint(data);
     BufferTracker::Buffer* buffer = GetBoundPixelUnpackTransferBufferIfValid(
+        bound_pixel_unpack_transfer_buffer_id_,
         "glCompressedTexImage2D", offset, image_size);
-    if (buffer)
+    if (buffer) {
       helper_->CompressedTexImage2D(
           target, level, internalformat, width, height, border, image_size,
           buffer->shm_id(), buffer->shm_offset() + offset);
+      buffer->set_transfer_ready_token(helper_->InsertToken());
+    } else {
+      SetGLError(GL_INVALID_VALUE, "glCompressedTexImage2D", "invalid buffer");

[piman, 2013/03/28 20:22:27]

GetBoundPixelUnpackTransferBufferIfValid already set the correct error

[hubbe, 2013/03/28 20:52:35]

Done.

+    }
     return;
   }
   SetBucketContents(kResultBucketId, data, image_size);
@@ -1469,11 +1501,18 @@ void GLES2Implementation::CompressedTexSubImage2D(
   if (bound_pixel_unpack_transfer_buffer_id_) {
     GLuint offset = ToGLuint(data);
     BufferTracker::Buffer* buffer = GetBoundPixelUnpackTransferBufferIfValid(
+        bound_pixel_unpack_transfer_buffer_id_,
         "glCompressedTexSubImage2D", offset, image_size);
-    if (buffer)
+    if (buffer) {
       helper_->CompressedTexSubImage2D(
           target, level, xoffset, yoffset, width, height, format, image_size,
           buffer->shm_id(), buffer->shm_offset() + offset);
+      buffer->set_transfer_ready_token(helper_->InsertToken());
+      CheckGLError();
+    } else {
+      SetGLError(GL_INVALID_VALUE,

[piman, 2013/03/28 20:22:27]

GetBoundPixelUnpackTransferBufferIfValid already set the right error

[hubbe, 2013/03/28 20:52:35]

Done.

+                 "glCompressedTexSubImage2D", "invalid buffer");
+    }
     return;
   }
   SetBucketContents(kResultBucketId, data, image_size);
@@ -1552,12 +1591,17 @@ void GLES2Implementation::TexImage2D(
   if (bound_pixel_unpack_transfer_buffer_id_) {
     GLuint offset = ToGLuint(pixels);
     BufferTracker::Buffer* buffer = GetBoundPixelUnpackTransferBufferIfValid(
+        bound_pixel_unpack_transfer_buffer_id_,
         "glTexImage2D", offset, size);
-    if (buffer)
+    if (buffer) {
       helper_->TexImage2D(
           target, level, internalformat, width, height, border, format, type,
           buffer->shm_id(), buffer->shm_offset() + offset);
-    CheckGLError();
+      buffer->set_transfer_ready_token(helper_->InsertToken());
+      CheckGLError();
+    } else {
+      SetGLError(GL_INVALID_VALUE, "glTexImage2D", "invalid buffer");

[piman, 2013/03/28 20:22:27]

GetBoundPixelUnpackTransferBufferIfValid already set the right error

[hubbe, 2013/03/28 20:52:35]

Done.

+    }
     return;
   }
 
@@ -1655,12 +1699,17 @@ void GLES2Implementation::TexSubImage2D(
   if (bound_pixel_unpack_transfer_buffer_id_) {
     GLuint offset = ToGLuint(pixels);
     BufferTracker::Buffer* buffer = GetBoundPixelUnpackTransferBufferIfValid(
+        bound_pixel_unpack_transfer_buffer_id_,
         "glTexSubImage2D", offset, temp_size);
-    if (buffer)
+    if (buffer) {
       helper_->TexSubImage2D(
           target, level, xoffset, yoffset, width, height, format, type,
           buffer->shm_id(), buffer->shm_offset() + offset, false);
-    CheckGLError();
+      buffer->set_transfer_ready_token(helper_->InsertToken());
+      CheckGLError();
+    } else {
+      SetGLError(GL_INVALID_VALUE, "glTexSubImage2D", "invalid buffer");

[piman, 2013/03/28 20:22:27]

GetBoundPixelUnpackTransferBufferIfValid already set the right error

[hubbe, 2013/03/28 20:52:35]

Done.

+    }
     return;
   }
 
@@ -2103,6 +2152,29 @@ void GLES2Implementation::ReadPixels(
     SetGLError(GL_INVALID_VALUE, "glReadPixels", "size too large.");
     return;
   }
+
+  if (bound_pixel_pack_transfer_buffer_id_) {
+    GLuint offset = ToGLuint(pixels);
+    BufferTracker::Buffer* buffer = GetBoundPixelUnpackTransferBufferIfValid(
+        bound_pixel_pack_transfer_buffer_id_,
+        "glReadPixels", offset, temp_size);
+    if (buffer) {
+      helper_->ReadPixels(xoffset, yoffset, width, height, format, type,
+                          buffer->shm_id(), buffer->shm_offset(),
+                          0, 0);
+      buffer->set_transfer_ready_token(helper_->InsertToken());
+      CheckGLError();
+    } else {
+      SetGLError(GL_INVALID_VALUE, "glReadPixels", "invalid readback buffer");

[piman, 2013/03/28 20:22:27]

GetBoundPixelUnpackTransferBufferIfValid already set the right error

[hubbe, 2013/03/28 20:52:35]

Done.

+    }
+    return;
+  }
+
+  if (!pixels) {
+    SetGLError(GL_INVALID_VALUE, "glReadPixels", "pixels = NULL");

[piman, 2013/03/28 20:22:27]

note: spec doesn't say that an error would be generated. It's almost expected
that it would crash on this user error (hint: passing any non-zero offset cast
into a pointer would be equally bad)
If anything, GL_INVALID_OPERATION would be more appropriate (since it's a matter
of state combination, the value itself being valid in other contexts).

[hubbe, 2013/03/28 20:52:35]

Changed to GL_INVALID_OPERATION.

+    return;
+  }
+
   // Transfer by rows.
   // The max rows we can transfer.
   while (height) {
@@ -2220,6 +2292,9 @@ bool GLES2Implementation::BindBufferHelper(
     case GL_ELEMENT_ARRAY_BUFFER:
       changed = vertex_array_object_manager_->BindElementArray(buffer);
       break;
+    case GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
+      bound_pixel_pack_transfer_buffer_id_ = buffer;
+      break;
     case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
       bound_pixel_unpack_transfer_buffer_id_ = buffer;
       break;
@@ -3299,17 +3374,28 @@ void* GLES2Implementation::MapBufferCHROMIUM(GLuint target, GLenum access) {
   GPU_CLIENT_SINGLE_THREAD_CHECK();
   GPU_CLIENT_LOG("[" << GetLogPrefix() << "] glMapBufferCHROMIUM("
       << target << ", " << GLES2Util::GetStringEnum(access) << ")");
-  if (target != GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM) {
-    SetGLError(
-        GL_INVALID_ENUM, "glMapBufferCHROMIUM", "invalid target");
-    return NULL;
-  }
-  if (access != GL_WRITE_ONLY) {
-    SetGLError(GL_INVALID_ENUM, "glMapBufferCHROMIUM", "bad access mode");
-    return NULL;
+  switch (target)  {
+    case GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
+      if (access != GL_READ_ONLY) {
+        SetGLError(GL_INVALID_ENUM, "glMapBufferCHROMIUM", "bad access mode");
+        return NULL;
+      }
+      break;
+    case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
+      if (access != GL_WRITE_ONLY) {
+        SetGLError(GL_INVALID_ENUM, "glMapBufferCHROMIUM", "bad access mode");
+        return NULL;
+      }
+      break;
+    default:
+      SetGLError(
+          GL_INVALID_ENUM, "glMapBufferCHROMIUM", "invalid target");
+      return NULL;
   }
-  BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(
-        bound_pixel_unpack_transfer_buffer_id_);
+  GLuint buffer_id;
+  GetBoundPixelTransferBuffer(target, "glMapBufferCHROMIUM", &buffer_id);
+  if (!buffer_id) return NULL;

[piman, 2013/03/28 20:22:27]

nit: return on its own line

[hubbe, 2013/03/28 20:52:35]

Done.

+  BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(buffer_id);
   if (!buffer) {
     SetGLError(GL_INVALID_OPERATION, "glMapBufferCHROMIUM", "invalid buffer");
     return NULL;
@@ -3318,6 +3404,15 @@ void* GLES2Implementation::MapBufferCHROMIUM(GLuint target, GLenum access) {
     SetGLError(GL_INVALID_OPERATION, "glMapBufferCHROMIUM", "already mapped");
     return NULL;
   }
+  // Here we wait for previous transfer operations to be finished.
+  // TODO(hubbe): AsyncTex(Sub)Image2dCHROMIUM does not currently work
+  // with this method of synchronization. Until this is fixed,
+  // MapBufferCHROMIUM will not block even if the transfer is not ready
+  // for these calls.
+  if (buffer->transfer_ready_token()) {
+    helper_->WaitForToken(buffer->transfer_ready_token());
+    buffer->set_transfer_ready_token(0);
+  }
   buffer->set_mapped(true);
 
   GPU_DCHECK(buffer->address());
@@ -3330,12 +3425,12 @@ GLboolean GLES2Implementation::UnmapBufferCHROMIUM(GLuint target) {
   GPU_CLIENT_SINGLE_THREAD_CHECK();
   GPU_CLIENT_LOG(
       "[" << GetLogPrefix() << "] glUnmapBufferCHROMIUM(" << target << ")");
-  if (target != GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM) {
+  GLuint buffer_id;
+  if (!GetBoundPixelTransferBuffer(target, "glMapBufferCHROMIUM", &buffer_id)) {
     SetGLError(GL_INVALID_ENUM, "glUnmapBufferCHROMIUM", "invalid target");
-    return false;
   }
-  BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(
-      bound_pixel_unpack_transfer_buffer_id_);
+  if (!buffer_id) return false;

[piman, 2013/03/28 20:22:27]

nit: return on its own line

[hubbe, 2013/03/28 20:52:35]

Done.

+  BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(buffer_id);
   if (!buffer) {
     SetGLError(GL_INVALID_OPERATION, "glMapBufferCHROMIUM", "invalid buffer");
     return false;
@@ -3385,8 +3480,12 @@ void GLES2Implementation::AsyncTexImage2DCHROMIUM(
   }
 
   // Otherwise, async uploads require a transfer buffer to be bound.
+  // TODO(hubbe): Make MapBufferCHROMIUM block if someone tries to re-use
+  // the buffer before the transfer is finished. (Currently sunch

[piman, 2013/03/28 20:22:27]

typo: sunch

[hubbe, 2013/03/28 20:52:35]

Done.

+  // synchronization has to be handled manually.)
   GLuint offset = ToGLuint(pixels);
   BufferTracker::Buffer* buffer = GetBoundPixelUnpackTransferBufferIfValid(
+      bound_pixel_unpack_transfer_buffer_id_,
       "glAsyncTexImage2DCHROMIUM", offset, size);
   if (!buffer)
     return;
@@ -3427,8 +3526,12 @@ void GLES2Implementation::AsyncTexSubImage2DCHROMIUM(
   }
 
   // Async uploads require a transfer buffer to be bound.
+  // TODO(hubbe): Make MapBufferCHROMIUM block if someone tries to re-use
+  // the buffer before the transfer is finished. (Currently sunch

[piman, 2013/03/28 20:22:27]

typo: sunch

[hubbe, 2013/03/28 20:52:35]

Done.

+  // synchronization has to be handled manually.)
   GLuint offset = ToGLuint(pixels);
   BufferTracker::Buffer* buffer = GetBoundPixelUnpackTransferBufferIfValid(
+      bound_pixel_unpack_transfer_buffer_id_,
       "glAsyncTexSubImage2DCHROMIUM", offset, size);
   if (!buffer)
     return;