Perform glReadPixels with PBOs in the gpu, if PBOs are available.

gpu/command_buffer/client/gles2_implementation.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.
 
 // A class to emulate GLES2 over command buffers.
 
 #include "gpu/command_buffer/client/gles2_implementation.h"
 
 #include <algorithm>
 #include <map>
@@ skipping 2218 matching lines @@
   }
 
   if (bound_pixel_pack_transfer_buffer_id_) {
     GLuint offset = ToGLuint(pixels);
     BufferTracker::Buffer* buffer = GetBoundPixelUnpackTransferBufferIfValid(
         bound_pixel_pack_transfer_buffer_id_,
         "glReadPixels", offset, padded_row_size * height);
     if (buffer && buffer->shm_id() != -1) {
       helper_->ReadPixels(xoffset, yoffset, width, height, format, type,
                           buffer->shm_id(), buffer->shm_offset(),
-                          0, 0);
-      buffer->set_transfer_ready_token(helper_->InsertToken());
+                          0, 0, true);
       CheckGLError();
     }
     return;
   }
 
   if (!pixels) {
     SetGLError(GL_INVALID_OPERATION, "glReadPixels", "pixels = NULL");
     return;
   }
 
@@ skipping 11 matching lines @@
     // NOTE: We must look up the address of the result area AFTER allocation
     // of the transfer buffer since the transfer buffer may be reallocated.
     Result* result = GetResultAs<Result*>();
     if (!result) {
       return;
     }
     *result = 0;  // mark as failed.
     helper_->ReadPixels(
         xoffset, yoffset, width, num_rows, format, type,
         buffer.shm_id(), buffer.offset(),
-        GetResultShmId(), GetResultShmOffset());
+        GetResultShmId(), GetResultShmOffset(),
+        false);
     WaitForCmd();
     if (*result != 0) {
       // when doing a y-flip we have to iterate through top-to-bottom chunks
       // of the dst. The service side handles reversing the rows within a
       // chunk.
       int8* rows_dst;
       if (pack_reverse_row_order_) {
           rows_dst = dest + (height - num_rows) * padded_row_size;
       } else {
           rows_dst = dest;
@@ skipping 891 matching lines @@
         "glDeleteTextures", "id not created by this context.");
     return;
   }
   // When you delete a query you can't mark its memory as unused until it's
   // completed.
   // Note: If you don't do this you won't mess up the service but you will mess
   // up yourself.
 
   // TODO(gman): Consider making this faster by putting pending quereies
   // on some queue to be removed when they are finished.
+  // TODO(hubbe): Consider using signalQuery callback to delete pending
+  //              queries.
   bool query_pending = false;
   for (GLsizei ii = 0; ii < n; ++ii) {
     QueryTracker::Query* query = query_tracker_->GetQuery(queries[ii]);
-    if (query && query->Pending()) {
+    if (query && !query->CheckResultsAvailable(helper_)) {
       query_pending = true;
       break;
     }
   }
 
   if (query_pending) {
     WaitForCmd();
+
+    query_pending = false;
+    for (GLsizei ii = 0; ii < n; ++ii) {
+      QueryTracker::Query* query = query_tracker_->GetQuery(queries[ii]);
+      if (query && !query->CheckResultsAvailable(helper_)) {
+        query_pending = true;
+        break;
+      }
+    }
+    if (query_pending) {
+      // Some queries may still not be done even after the GPU process
+      // has sent the command to the driver. When this happens we need
+      // to use glFinish() to wait for the query to finish.
+      // Moral of this story: Don't delete pending queries!
+      helper_->Finish();

[piman, 2013/06/26 22:17:08]

Why do we need to do this?
DeleteQueries + WaitForCmd would ensure the query was destroyed & removed
server-side. After that point it won't write to the shared memory any more,
regardless of whether it completed or not at the GL level.

[hubbe, 2013/06/28 22:17:49]

That does sound perfectly reasonable, but then why doesn't the code work that
already? 

[piman, 2013/06/28 23:35:46]

I think you accidentally a word, and I don't understand what you're asking
exactly.

But I think you're right that the current code doesn't actually work correctly,
and that's probably what's causing crbug.com/253488

If you move the original WaitForCmd until after the DeleteQueriesEXTImmediate
(and remove the check below with the MustBeContextLost() ) it should be fine
though:
- if any query is pending, we delete it and finish. At this point the service
won't write to the shm any more.
- if no query is pending, we know the service won't write to the shm any more,
so no need to finish.

[hubbe, 2013/06/29 00:13:56]

Ok, I'm changing it. Seems to work in my tests at least. :)

+      WaitForCmd();
+    }
   }
 
   for (GLsizei ii = 0; ii < n; ++ii) {
     QueryTracker::Query* query = query_tracker_->GetQuery(queries[ii]);
-    if (query && query->Pending()) {
-      if (!query->CheckResultsAvailable(helper_)) {
-        // Should only get here on context lost.
-        MustBeContextLost();
-      }
+    if (query && !query->CheckResultsAvailable(helper_)) {
+      // Should only get here on context lost.
+      MustBeContextLost();
     }
     query_tracker_->RemoveQuery(queries[ii], helper_->IsContextLost());
   }
   helper_->DeleteQueriesEXTImmediate(n, queries);
 }
 
 // TODO(gman): Remove this. Queries are not shared resources.
 void GLES2Implementation::DeleteQueriesStub(
     GLsizei /* n */, const GLuint* /* queries */) {
 }
@@ skipping 637 matching lines @@
   CheckGLError();
 }
 
 // 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/client/gles2_implementation_impl_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu