Command Buffer: read pixels into pixel pack 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 b2f7f96963843b8f1843c07ea9003cb703ef25a9..d537e67a43edffd682f4b59f9c76fdef5df981c7 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -8896,23 +8896,46 @@ error::Error GLES2DecoderImpl::HandleReadPixels(uint32 immediate_data_size,
   }
   typedef cmds::ReadPixels::Result Result;
   uint32 pixels_size;
-  if (state_.bound_pixel_pack_buffer.get()) {
-    // TODO(zmo): Need to handle the case of reading into a PIXEL_PACK_BUFFER
-    // in ES3, including more pack parameters. For now, generate a GL error.
-    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",
-                       "ReadPixels to a pixel pack buffer isn't implemented");
-    return error::kNoError;
-  }
   if (!GLES2Util::ComputeImageDataSizes(
       width, height, 1, format, type, state_.pack_alignment, &pixels_size,
       NULL, NULL)) {
     return error::kOutOfBounds;
   }
-  void* pixels = GetSharedMemoryAs<void*>(
-      c.pixels_shm_id, c.pixels_shm_offset, pixels_size);
-  if (!pixels) {
-    return error::kOutOfBounds;
+
+  void* pixels = NULL;
+  Buffer* buffer = state_.bound_pixel_pack_buffer.get();
+  if (c.pixels_shm_id == 0) {
+    if (buffer) {
+      if (buffer->GetMappedRange()) {
+        LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",
+            "pixel pack buffer should not be mapped to client memory");
+        return error::kNoError;
+      }
+      if (buffer->size() < pixels_size) {

[Zhenyao Mo, 2015/12/02 19:36:41]

Right now the pixels_size we compute could be smaller than it actually requests,
because in ComputeImageDataSizes we don't consider ES3 unpack parameters. This
exposes a security risk that the buffer is not big enough but it passes the
check here.

I will have to ask this CL to wait until I land the CL I am working on to fix
this size issue.

[yunchao, 2015/12/03 17:39:14]

Acknowledged.

+        LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",
+            "pixel pack buffer is not large enough");
+        return error::kNoError;
+      }

[Zhenyao Mo, 2015/12/02 19:36:41]

It's worth checking the result_shm_id and result_shm_offset is both 0. Otherwise
return error::kInvalidArgs.

[yunchao, 2015/12/03 17:39:14]

Done.

+      pixels = reinterpret_cast<void *>(c.pixels_shm_offset);
+    } else {
+      LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",
+          "no pixel pack buffer bound");
+      return error::kNoError;
+    }
+  } else {
+    if (buffer) {
+      LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",
+          "pixel pack buffer should not bound");
+      return error::kNoError;
+    } else {
+      pixels = GetSharedMemoryAs<void*>(
+          c.pixels_shm_id, c.pixels_shm_offset, pixels_size);
+      if (!pixels) {
+        return error::kOutOfBounds;
+      }
+    }
   }
+
   Result* result = NULL;
   if (c.result_shm_id != 0) {
     result = GetSharedMemoryAs<Result*>(
@@ -9047,7 +9070,8 @@ error::Error GLES2DecoderImpl::HandleReadPixels(uint32 immediate_data_size,
 
   ScopedResolvedFrameBufferBinder binder(this, false, true);
 
-  if (x < 0 || y < 0 || max_x > max_size.width() || max_y > max_size.height()) {
+  if ((x < 0 || y < 0 || max_x > max_size.width() || max_y > max_size.height())
+      && c.pixels_shm_id != 0) {
     // The user requested an out of range area. Get the results 1 line
     // at a time.
     uint32 temp_size;
@@ -9128,7 +9152,9 @@ error::Error GLES2DecoderImpl::HandleReadPixels(uint32 immediate_data_size,
     if (result != NULL) {
       *result = true;
     }
-    FinishReadPixels(c, 0);
+    if (c.pixels_shm_id != 0) {

[Zhenyao Mo, 2015/12/02 19:36:41]

I think we should also avoid glGetError() if we read into a bound buffer.

[yunchao, 2015/12/03 17:39:14]

Done.

+      FinishReadPixels(c, 0);
+    }
   }
 
   return error::kNoError;