Command Buffer: read pixels into pixel pack buffer

gpu/command_buffer/service/gles2_cmd_decoder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
 
 #include <stdio.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 9157 matching lines @@
   GLsizei height = c.height;
   GLenum format = c.format;
   GLenum type = c.type;
   GLboolean async = static_cast<GLboolean>(c.async);
   if (width < 0 || height < 0) {
     LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, "glReadPixels", "dimensions < 0");
     return error::kNoError;
   }
   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)) {
+  PixelStoreParams params = state_.GetPackParams();
+  if (!GLES2Util::ComputeImageDataSizesES3(width, height, 1, format, type,

[Zhenyao Mo, 2015/12/16 23:09:21]

Sorry just realized this is incorrect.  You should only compute with all ES3
pack parameters if it's reading into bound pixel_pack_buffer.  If it's reading
into client buffer, we actually set everything to 0 (except for alignment)
before making the glReadPixels calls. This makes sure we only send back
meaningful pixel data to the command buffer client side, and the client side
will take the responsibility to take the pixels and write to the client buffer
according to the full ES3 pack parameters.

So you should only call ComputeImageDataSizeES3 to get the pixels_size if a
pixel_pack_buffer is bound.  Otherwise, you should use the original
ComputeImageDatSizes to compute the pixels_size.

[yunchao, 2015/12/17 08:12:59]

Done.

+      params, &pixels_size, nullptr, nullptr, nullptr)) {
     return error::kOutOfBounds;
   }
-  void* pixels = GetSharedMemoryAs<void*>(
-      c.pixels_shm_id, c.pixels_shm_offset, pixels_size);
-  if (!pixels) {
-    return error::kOutOfBounds;
+
+  void* pixels = nullptr;
+  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;
+      }
+      uint32 size = 0;
+      if (!SafeAddUint32(pixels_size, c.pixels_shm_offset, &size)) {
+        LOCAL_SET_GL_ERROR(
+            GL_INVALID_VALUE, "glReadPixels", "size + offset overflow");
+        return error::kNoError;
+      }
+      if (buffer->size() < static_cast<int>(size)) {

[Zhenyao Mo, 2015/12/16 18:12:34]

You should cast int to uint32 for comparison here.

[yunchao, 2015/12/17 08:12:59]

Done.

+        LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",
+            "pixel pack buffer is not large enough");
+        return error::kNoError;
+      }
+      pixels = reinterpret_cast<void *>(c.pixels_shm_offset);
+    } else {
+      return error::kInvalidArguments;
+    }
+  } else {
+    if (buffer) {
+      return error::kInvalidArguments;
+    } 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*>(
         c.result_shm_id, c.result_shm_offset, sizeof(*result));
     if (!result) {
       return error::kOutOfBounds;
     }
   }
 
   if (!validators_->read_pixel_format.IsValid(format)) {
@@ skipping 115 matching lines @@
 
   if (!CheckBoundFramebuffersValid("glReadPixels")) {
     return error::kNoError;
   }
 
   LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER("glReadPixels");
 
   ScopedResolvedFrameBufferBinder binder(this, false, true);
 
   if (x < 0 || y < 0 || max_x > max_size.width() || max_y > max_size.height()) {
+    // TODO(yunchao): need to handle the out-of-bounds case for reading pixels
+    // into PIXEL_PACK buffer.
+    if (c.pixels_shm_id == 0) {
+      LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",
+          "read pixels out of bounds into PIXEL_PACK buffer");
+      return error::kNoError;
+    }
     // The user requested an out of range area. Get the results 1 line
     // at a time.
     uint32 temp_size;
     uint32 unpadded_row_size;
     uint32 padded_row_size;
     if (!GLES2Util::ComputeImageDataSizes(
         width, 2, 1, format, type, state_.pack_alignment, &temp_size,
         &unpadded_row_size, &padded_row_size)) {
       LOCAL_SET_GL_ERROR(
           GL_INVALID_VALUE, "glReadPixels", "dimensions out of range");
@@ skipping 55 matching lines @@
         glBindBuffer(GL_PIXEL_PACK_BUFFER_ARB, 0);
         return error::kNoError;
       } else {
         // On error, unbind pack buffer and fall through to sync readpixels
         glBindBuffer(GL_PIXEL_PACK_BUFFER_ARB, 0);
         glDeleteBuffersARB(1, &buffer);
       }
     }
     glReadPixels(x, y, width, height, format, type, pixels);
   }
-  GLenum error = LOCAL_PEEK_GL_ERROR("glReadPixels");
-  if (error == GL_NO_ERROR) {
-    if (result != NULL) {
-      *result = true;
+  if (c.pixels_shm_id != 0) {
+    GLenum error = LOCAL_PEEK_GL_ERROR("glReadPixels");
+    if (error == GL_NO_ERROR) {
+      if (result != NULL) {
+        *result = true;
+      }
+      FinishReadPixels(c, 0);
     }
-    FinishReadPixels(c, 0);
   }
 
   return error::kNoError;
 }
 
 error::Error GLES2DecoderImpl::HandlePixelStorei(uint32 immediate_data_size,
                                                  const void* cmd_data) {
   const gles2::cmds::PixelStorei& c =
       *static_cast<const gles2::cmds::PixelStorei*>(cmd_data);
   GLenum pname = c.pname;
@@ skipping 6364 matching lines @@
   return error::kNoError;
 }
 
 // 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.
 #include "gpu/command_buffer/service/gles2_cmd_decoder_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu