Fixes ReadPixel to have a result so the client

side can know if it should or should not copy
the results.

This is the way the spec is. If ReadPixels fails
it's not supposed to modify client side memory.

Unfortunately that makes it slow because it stalls
the GPU pipeline 3 times.

The service side has to  call glGetError to save up 
any current errors. That's one stall. Then it calls
ReadPixels, another stall, finally calls glGetError 
a 3rd time to see if  ReadPixels succeeded so that 
it can tell the client side implementation of
ReadPixels whether or not to copy memory.

We could decide to change the behavior of ReadPixels
so it always copies. That wouldn't be spec complient
but suspect most developers never expect it to fail
and don't count on it not always writing to their
memory.

TEST=none
BUG=none

Review URL: http://codereview.chromium.org/652213

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@39930 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 86 insertions, 20 deletions

diff --git a/gpu/command_buffer/build_gles2_cmd_buffer.py b/gpu/command_buffer/build_gles2_cmd_buffer.py
index 0b19010..a1010ec 100755
--- a/gpu/command_buffer/build_gles2_cmd_buffer.py
+++ b/gpu/command_buffer/build_gles2_cmd_buffer.py
@@ -820,6 +820,7 @@ _ENUM_LISTS = {
 # This table specifies types and other special data for the commands that
 # will be generated.
 #
+# cmd_comment:  A comment added to the cmd format.
 # type:         defines which handler will be used to generate code.
 # DecoderFunc:  defines which function to call in the decoder to execute the
 #               corresponding GL command. If not specified the GL command will
@@ -832,6 +833,8 @@ _ENUM_LISTS = {
 # immediate:    Whether or not to generate an immediate command for the GL
 #               function. The default is if there is exactly 1 pointer argument
 #               in the GL function an immediate command is generated.
+# impl_func:    Whether or not to generate the GLES2Implementation part of this
+#               command.
 # needs_size:   If true a data_size field is added to the command.
 # data_type:    The type of data the command uses. For PUTn or PUT types.
 # count:        The number of units per element. For PUTn or PUT types.
@@ -872,8 +875,8 @@ _FUNCTION_INFO = {
     'cmd_args': 'GLenum mode, GLsizei count, GLenum type, GLuint index_offset',
   },
   'EnableVertexAttribArray': {'DecoderFunc': 'DoEnableVertexAttribArray'},
-  'Finish': {'ImplFunc': False},
-  'Flush': {'ImplFunc': False},
+  'Finish': {'impl_func': False},
+  'Flush': {'impl_func': False},
   'FramebufferRenderbuffer': {'DecoderFunc': 'glFramebufferRenderbufferEXT'},
   'FramebufferTexture2D': {'DecoderFunc': 'glFramebufferTexture2DEXT'},
   'GenerateMipmap': {
@@ -988,7 +991,20 @@ _FUNCTION_INFO = {
   'LinkProgram': {'DecoderFunc': 'DoLinkProgram'},
   'PixelStorei': {'type': 'Manual'},
   'RenderbufferStorage': {'DecoderFunc': 'glRenderbufferStorageEXT'},
-  'ReadPixels': {'type': 'Custom', 'immediate': False},
+  'ReadPixels': {
+    'cmd_comment':
+        '// ReadPixels has the result separated from the pixel buffer so that\n'
+        '// it is easier to specify the result going to some specific place\n'
+        '// that exactly fits the rectangle of pixels.\n',
+    'type': 'Custom',
+    'immediate': False,
+    'impl_func': False,
+    'cmd_args':
+        'GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, '
+        'GLenum type, uint32 pixels_shm_id, uint32 pixels_shm_offset, '
+        'uint32 result_shm_id, uint32 result_shm_offset',
+    'result': ['uint32'],
+  },
   'ReleaseShaderCompiler': {'type': 'Noop'},
   'ShaderBinary': {'type': 'Noop'},
   'ShaderSource': {
@@ -1024,7 +1040,7 @@ _FUNCTION_INFO = {
                   'GLsizei stride, GLuint offset',
   },
   'SwapBuffers': {
-    'ImplFunc': False,
+    'impl_func': False,
     'DecoderFunc': 'DoSwapBuffers',
     'unit_test': False,
   },
@@ -1149,6 +1165,9 @@ class TypeHandler(object):
 
   def WriteStruct(self, func, file):
     """Writes a structure that matches the arguments to a function."""
+    comment = func.GetInfo('cmd_comment')
+    if not comment == None:
+      file.Write(comment)
     file.Write("struct %s {\n" % func.name)
     file.Write("  typedef %s ValueType;\n" % func.name)
     file.Write("  static const CommandId kCmdId = k%s;\n" % func.name)
@@ -1413,7 +1432,7 @@ TEST_F(GLES2DecoderTest, %(name)sInvalidArgs%(arg_index)d_%(value_index)d) {
 
   def WriteGLES2ImplementationHeader(self, func, file):
     """Writes the GLES2 Implemention declaration."""
-    impl_func = func.GetInfo('ImplFunc')
+    impl_func = func.GetInfo('impl_func')
     if func.can_auto_generate and (impl_func == None or impl_func == True):
       file.Write("%s %s(%s) {\n" %
                  (func.return_type, func.original_name,
diff --git a/gpu/command_buffer/client/gles2_cmd_helper_autogen.h b/gpu/command_buffer/client/gles2_cmd_helper_autogen.h
index 2b6185a..df3ed0e 100644
--- a/gpu/command_buffer/client/gles2_cmd_helper_autogen.h
+++ b/gpu/command_buffer/client/gles2_cmd_helper_autogen.h
@@ -667,10 +667,12 @@
 
   void ReadPixels(
       GLint x, GLint y, GLsizei width, GLsizei height, GLenum format,
-      GLenum type, uint32 pixels_shm_id, uint32 pixels_shm_offset) {
+      GLenum type, uint32 pixels_shm_id, uint32 pixels_shm_offset,
+      uint32 result_shm_id, uint32 result_shm_offset) {
     gles2::ReadPixels& c = GetCmdSpace<gles2::ReadPixels>();
     c.Init(
-        x, y, width, height, format, type, pixels_shm_id, pixels_shm_offset);
+        x, y, width, height, format, type, pixels_shm_id, pixels_shm_offset,
+        result_shm_id, result_shm_offset);
   }
 
   void RenderbufferStorage(
diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
index cc895fe..450e48d 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -495,6 +495,8 @@ void GLES2Implementation::ReadPixels(
   if (width == 0 || height == 0) {
     return;
   }
+  typedef gles2::ReadPixels::Result Result;
+  Result* result = static_cast<Result*>(result_buffer_);
   int8* dest = reinterpret_cast<int8*>(pixels);
   GLsizeiptr max_size = transfer_buffer_.GetLargestFreeOrPendingSize();
   GLsizeiptr unpadded_row_size = GLES2Util::ComputeImageDataSize(
@@ -508,12 +510,18 @@ void GLES2Implementation::ReadPixels(
       GLint num_rows = std::min(height, max_rows);
       GLsizeiptr part_size = num_rows * padded_row_size;
       void* buffer = transfer_buffer_.Alloc(part_size);
-      // TODO(gman): handle errors.
+      *result = 0;  // mark as failed.
       helper_->ReadPixels(
           xoffset, yoffset, width, num_rows, format, type,
-          transfer_buffer_id_, transfer_buffer_.GetOffset(buffer));
+          transfer_buffer_id_, transfer_buffer_.GetOffset(buffer),
+          result_shm_id(), result_shm_offset());
+      WaitForCmd();
+      // If it was not marked as successful exit.
+      if (*result == 0) {
+        return;
+      }
       memcpy(dest, buffer, part_size);
-      transfer_buffer_.FreePendingToken(buffer, helper_->InsertToken());
+      transfer_buffer_.Free(buffer);
       yoffset += num_rows;
       dest += part_size;
       height -= num_rows;
@@ -532,12 +540,18 @@ void GLES2Implementation::ReadPixels(
         GLint num_pixels = std::min(width, max_sub_row_pixels);
         GLsizeiptr part_size = num_pixels * element_size;
         void* buffer = transfer_buffer_.Alloc(part_size);
-        // TODO(gman): handle errors.
+        *result = 0;  // mark as failed.
         helper_->ReadPixels(
             temp_xoffset, yoffset, temp_width, 1, format, type,
-            transfer_buffer_id_, transfer_buffer_.GetOffset(buffer));
+            transfer_buffer_id_, transfer_buffer_.GetOffset(buffer),
+            result_shm_id(), result_shm_offset());
+        WaitForCmd();
+        // If it was not marked as successful exit.
+        if (*result == 0) {
+          return;
+        }
         memcpy(row_dest, buffer, part_size);
-        transfer_buffer_.FreePendingToken(buffer, helper_->InsertToken());
+        transfer_buffer_.Free(buffer);
         row_dest += part_size;
         temp_xoffset += num_pixels;
         temp_width -= num_pixels;
diff --git a/gpu/command_buffer/common/gles2_cmd_format_autogen.h b/gpu/command_buffer/common/gles2_cmd_format_autogen.h
index 57a8a4e..19824a4 100644
--- a/gpu/command_buffer/common/gles2_cmd_format_autogen.h
+++ b/gpu/command_buffer/common/gles2_cmd_format_autogen.h
@@ -4705,11 +4705,16 @@ COMPILE_ASSERT(offsetof(PolygonOffset, factor) == 4,
 COMPILE_ASSERT(offsetof(PolygonOffset, units) == 8,
                OffsetOf_PolygonOffset_units_not_8);
 
+// ReadPixels has the result separated from the pixel buffer so that
+// it is easier to specify the result going to some specific place
+// that exactly fits the rectangle of pixels.
 struct ReadPixels {
   typedef ReadPixels ValueType;
   static const CommandId kCmdId = kReadPixels;
   static const cmd::ArgFlags kArgFlags = cmd::kFixed;
 
+  typedef uint32 Result;
+
   static uint32 ComputeSize() {
     return static_cast<uint32>(sizeof(ValueType));  // NOLINT
   }
@@ -4720,7 +4725,8 @@ struct ReadPixels {
 
   void Init(
       GLint _x, GLint _y, GLsizei _width, GLsizei _height, GLenum _format,
-      GLenum _type, uint32 _pixels_shm_id, uint32 _pixels_shm_offset) {
+      GLenum _type, uint32 _pixels_shm_id, uint32 _pixels_shm_offset,
+      uint32 _result_shm_id, uint32 _result_shm_offset) {
     SetHeader();
     x = _x;
     y = _y;
@@ -4730,16 +4736,19 @@ struct ReadPixels {
     type = _type;
     pixels_shm_id = _pixels_shm_id;
     pixels_shm_offset = _pixels_shm_offset;
+    result_shm_id = _result_shm_id;
+    result_shm_offset = _result_shm_offset;
   }
 
   void* Set(
       void* cmd, GLint _x, GLint _y, GLsizei _width, GLsizei _height,
       GLenum _format, GLenum _type, uint32 _pixels_shm_id,
-      uint32 _pixels_shm_offset) {
+      uint32 _pixels_shm_offset, uint32 _result_shm_id,
+      uint32 _result_shm_offset) {
     static_cast<ValueType*>(
         cmd)->Init(
             _x, _y, _width, _height, _format, _type, _pixels_shm_id,
-            _pixels_shm_offset);
+            _pixels_shm_offset, _result_shm_id, _result_shm_offset);
     return NextCmdAddress<ValueType>(cmd);
   }
 
@@ -4752,10 +4761,12 @@ struct ReadPixels {
   uint32 type;
   uint32 pixels_shm_id;
   uint32 pixels_shm_offset;
+  uint32 result_shm_id;
+  uint32 result_shm_offset;
 };
 
-COMPILE_ASSERT(sizeof(ReadPixels) == 36,
-               Sizeof_ReadPixels_is_not_36);
+COMPILE_ASSERT(sizeof(ReadPixels) == 44,
+               Sizeof_ReadPixels_is_not_44);
 COMPILE_ASSERT(offsetof(ReadPixels, header) == 0,
                OffsetOf_ReadPixels_header_not_0);
 COMPILE_ASSERT(offsetof(ReadPixels, x) == 4,
@@ -4774,6 +4785,10 @@ COMPILE_ASSERT(offsetof(ReadPixels, pixels_shm_id) == 28,
                OffsetOf_ReadPixels_pixels_shm_id_not_28);
 COMPILE_ASSERT(offsetof(ReadPixels, pixels_shm_offset) == 32,
                OffsetOf_ReadPixels_pixels_shm_offset_not_32);
+COMPILE_ASSERT(offsetof(ReadPixels, result_shm_id) == 36,
+               OffsetOf_ReadPixels_result_shm_id_not_36);
+COMPILE_ASSERT(offsetof(ReadPixels, result_shm_offset) == 40,
+               OffsetOf_ReadPixels_result_shm_offset_not_40);
 
 struct RenderbufferStorage {
   typedef RenderbufferStorage ValueType;
diff --git a/gpu/command_buffer/common/gles2_cmd_format_test_autogen.h b/gpu/command_buffer/common/gles2_cmd_format_test_autogen.h
index 949cb77..381fea1 100644
--- a/gpu/command_buffer/common/gles2_cmd_format_test_autogen.h
+++ b/gpu/command_buffer/common/gles2_cmd_format_test_autogen.h
@@ -1754,7 +1754,9 @@ TEST(GLES2FormatTest, ReadPixels) {
       static_cast<GLenum>(15),
       static_cast<GLenum>(16),
       static_cast<uint32>(17),
-      static_cast<uint32>(18));
+      static_cast<uint32>(18),
+      static_cast<uint32>(19),
+      static_cast<uint32>(20));
   EXPECT_EQ(static_cast<uint32>(ReadPixels::kCmdId),
             cmd.header.command);
   EXPECT_EQ(sizeof(cmd), cmd.header.size * 4u);
@@ -1768,6 +1770,8 @@ TEST(GLES2FormatTest, ReadPixels) {
   EXPECT_EQ(static_cast<GLenum>(16), cmd.type);
   EXPECT_EQ(static_cast<uint32>(17), cmd.pixels_shm_id);
   EXPECT_EQ(static_cast<uint32>(18), cmd.pixels_shm_offset);
+  EXPECT_EQ(static_cast<uint32>(19), cmd.result_shm_id);
+  EXPECT_EQ(static_cast<uint32>(20), cmd.result_shm_offset);
 }
 
 TEST(GLES2FormatTest, RenderbufferStorage) {
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index df18f39..c018356 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -1942,19 +1942,31 @@ error::Error GLES2DecoderImpl::HandleReadPixels(
   GLsizei height = c.height;
   GLenum format = c.format;
   GLenum type = c.type;
+  // TODO(gman): Handle out of range rectangles.
+  typedef gles2::ReadPixels::Result Result;
   uint32 pixels_size = GLES2Util::ComputeImageDataSize(
       width, height, format, type, pack_alignment_);
   void* pixels = GetSharedMemoryAs<void*>(
       c.pixels_shm_id, c.pixels_shm_offset, pixels_size);
-  if (!pixels) {
+  Result* result = GetSharedMemoryAs<Result*>(
+        c.result_shm_id, c.result_shm_offset, sizeof(*result));
+  if (!pixels || !result) {
     return error::kOutOfBounds;
   }
+
   if (!ValidateGLenumReadPixelFormat(format) ||
       !ValidateGLenumPixelType(type)) {
     SetGLError(GL_INVALID_VALUE);
     return error::kNoError;
   }
+  CopyRealGLErrorsToWrapper();
   glReadPixels(x, y, width, height, format, type, pixels);
+  GLenum error = glGetError();
+  if (error == GL_NO_ERROR) {
+    *result = true;
+  } else {
+    SetGLError(error);
+  }
   return error::kNoError;
 }