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, params, &pixels_size, NULL, NULL, NULL)) {

[Zhenyao Mo, 2015/12/15 23:50:56]

nit: use nullptr instead.

[yunchao, 2015/12/16 11:04:25]

Done.

     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;

[Zhenyao Mo, 2015/12/15 23:50:56]

nit: use nullptr instead.

[yunchao, 2015/12/16 11:04:25]

Done.

+  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) {
+        LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",
+            "pixel pack buffer is not large enough");
+        return error::kNoError;
+      }
+      if (c.pixels_shm_offset != 0) {
+        return error::kInvalidArguments;
+      }
+      pixels = reinterpret_cast<void *>(0);

[Zhenyao Mo, 2015/12/15 23:50:56]

Here you will need to consider the offset parameter.

Also, you need to validate that offset + pixels_size do not overflow and is the
same or smaller than buffer->size()

[yunchao, 2015/12/16 11:04:25]

Done.

+    } else {
+      LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels",

[Zhenyao Mo, 2015/12/15 23:50:56]

Here you should return error::kInvalidArguments. No GL error.

[yunchao, 2015/12/16 11:04:25]

Done.

+          "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");

[Zhenyao Mo, 2015/12/15 23:50:56]

Here you should return error::kInvalidArguments. No GL error.

[yunchao, 2015/12/16 11:04:25]

Done.

+      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*>(
         c.result_shm_id, c.result_shm_offset, sizeof(*result));
     if (!result) {
       return error::kOutOfBounds;
     }
   }
 
   if (!validators_->read_pixel_format.IsValid(format)) {
@@ skipping 114 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()) {
+  if ((x < 0 || y < 0 || max_x > max_size.width() || max_y > max_size.height())
+      && c.pixels_shm_id != 0) {

[Zhenyao Mo, 2015/12/15 23:50:56]

We do want to handle the out-of-bounds readPixels with PIXEL_PACK_BUFFER case as
I mentioned previously.  So for now, at least could you add a TODO and generate
an INVALID_OPERATION to avoid undefined results?

[yunchao, 2015/12/16 11:04:25]

Done.

     // 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