Revert 209625 "Perform glReadPixels with PBOs in the gpu, if PBO..."

Looks like it causes OutOfProcessPPAPITest.Graphics3D failure on XP. http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%28dbg%29%282%29/builds/31095 > Perform glReadPixels with PBOs in the gpu, if PBOs are available. > Make GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM wait for readpixel transfers. > Add signalQuery to get a callback when the transfer is done. > PLEASE NOTE: glMapBuffer does not wait for the readpixels transfer to complete anymore. > Nobody is currently relying on that behaviour. > Update gl_helper.cc and gl_renderer.cc to use queries. > > BUG=249925 > > Review URL: https://chromiumcodereview.appspot.com/16831004 TBR=hubbe@chromium.org Review URL: https://codereview.chromium.org/18445004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@209654 0039d316-1c4b-4281-b951-d872f2087c98
marcosholgado · Jul 2, 2013 · 1c1ef68 · 1c1ef68
1 parent d2c08b7
commit 1c1ef68
Show file tree

Hide file tree

Showing 25 changed files with 131 additions and 402 deletions.
diff --git a/cc/output/gl_renderer.cc b/cc/output/gl_renderer.cc
@@ -2301,14 +2301,6 @@ void GLRenderer::DoGetFramebufferPixels(
                                      NULL,
                                      GL_STREAM_READ));
 
-  WebKit::WebGLId query = 0;
-  if (is_async) {
-    query = context_->createQueryEXT();
-    GLC(context_, context_->beginQueryEXT(
-        GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM,
-        query));
-  }
-
   GLC(context_,
       context_->readPixels(window_rect.x(),
                            window_rect.y(),
@@ -2334,7 +2326,6 @@ void GLRenderer::DoGetFramebufferPixels(
                  base::Unretained(this),
                  cleanup_callback,
                  buffer,
-                 query,
                  dest_pixels,
                  window_rect.size());
   // Save the finished_callback so it can be cancelled.
@@ -2345,11 +2336,10 @@ void GLRenderer::DoGetFramebufferPixels(
   pending_async_read_pixels_.front()->buffer = buffer;
 
   if (is_async) {
-    GLC(context_, context_->endQueryEXT(
-        GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM));
-    SyncPointHelper::SignalQuery(
+    unsigned sync_point = context_->insertSyncPoint();
+    SyncPointHelper::SignalSyncPoint(
         context_,
-        query,
+        sync_point,
         finished_callback);
   } else {
     resource_provider_->Finish();
@@ -2362,15 +2352,10 @@ void GLRenderer::DoGetFramebufferPixels(
 void GLRenderer::FinishedReadback(
     const AsyncGetFramebufferPixelsCleanupCallback& cleanup_callback,
     unsigned source_buffer,
-    unsigned query,
     uint8* dest_pixels,
     gfx::Size size) {
   DCHECK(!pending_async_read_pixels_.empty());
 
-  if (query != 0) {
-    GLC(context_, context_->deleteQueryEXT(query));
-  }
-
   PendingAsyncReadPixels* current_read = pending_async_read_pixels_.back();
   // Make sure we service the readbacks in order.
   DCHECK_EQ(source_buffer, current_read->buffer);

diff --git a/cc/output/gl_renderer.h b/cc/output/gl_renderer.h
@@ -201,7 +201,6 @@ class CC_EXPORT GLRenderer : public DirectRenderer {
   void FinishedReadback(
       const AsyncGetFramebufferPixelsCleanupCallback& cleanup_callback,
       unsigned source_buffer,
-      unsigned query,
       uint8_t* dest_pixels,
       gfx::Size size);
   void PassOnSkBitmap(scoped_ptr<SkBitmap> bitmap,

diff --git a/cc/test/test_web_graphics_context_3d.cc b/cc/test/test_web_graphics_context_3d.cc
@@ -364,12 +364,6 @@ void TestWebGraphicsContext3D::signalSyncPoint(
   sync_point_callbacks_.push_back(callback);
 }
 
-void TestWebGraphicsContext3D::signalQuery(
-    WebKit::WebGLId query,
-    WebGraphicsSyncPointCallback* callback) {
-  sync_point_callbacks_.push_back(callback);
-}
-
 void TestWebGraphicsContext3D::setSwapBuffersCompleteCallbackCHROMIUM(
     WebGraphicsSwapBuffersCompleteCallbackCHROMIUM* callback) {
   if (support_swapbuffers_complete_callback_)

diff --git a/cc/test/test_web_graphics_context_3d.h b/cc/test/test_web_graphics_context_3d.h
@@ -110,8 +110,6 @@ class TestWebGraphicsContext3D : public FakeWebGraphicsContext3D {
   // Takes ownership of the |callback|.
   virtual void signalSyncPoint(unsigned sync_point,
                                WebGraphicsSyncPointCallback* callback);
-  virtual void signalQuery(WebKit::WebGLId query,
-                           WebGraphicsSyncPointCallback* callback);
 
   virtual void setSwapBuffersCompleteCallbackCHROMIUM(
       WebGraphicsSwapBuffersCompleteCallbackCHROMIUM* callback);

diff --git a/content/common/gpu/client/gl_helper.cc b/content/common/gpu/client/gl_helper.cc
@@ -188,24 +188,20 @@ class GLHelper::CopyTextureToImpl :
             int32 row_stride_bytes_,
             unsigned char* pixels_,
             const base::Callback<void(bool)>& callback_)
-        : done(false),
-          size(size_),
+        : size(size_),
           bytes_per_row(bytes_per_row_),
           row_stride_bytes(row_stride_bytes_),
           pixels(pixels_),
           callback(callback_),
-          buffer(0),
-          query(0) {
+          buffer(0) {
     }
 
-    bool done;
     gfx::Size size;
     int bytes_per_row;
     int row_stride_bytes;
     unsigned char* pixels;
     base::Callback<void(bool)> callback;
     GLuint buffer;
-    WebKit::WebGLId query;
   };
 
   // A readback pipeline that also converts the data to YUV before
@@ -296,7 +292,7 @@ class GLHelper::CopyTextureToImpl :
                        GLHelper::ScalerQuality quality);
 
   static void nullcallback(bool success) {}
-  void ReadbackDone(Request *request);
+  void ReadbackDone(Request* request);
   void FinishRequest(Request* request, bool result);
   void CancelRequests();
 
@@ -384,16 +380,12 @@ void GLHelper::CopyTextureToImpl::ReadbackAsync(
                        NULL,
                        GL_STREAM_READ);
 
-  request->query = context_->createQueryEXT();
-  context_->beginQueryEXT(GL_ASYNC_READ_PIXELS_COMPLETED_CHROMIUM,
-                          request->query);
   context_->readPixels(0, 0, dst_size.width(), dst_size.height(),
                        GL_RGBA, GL_UNSIGNED_BYTE, NULL);
-  context_->endQueryEXT(GL_ASYNC_READ_PIXELS_COMPLETED_CHROMIUM);
   context_->bindBuffer(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM, 0);
-  cc::SyncPointHelper::SignalQuery(
+  cc::SyncPointHelper::SignalSyncPoint(
       context_,
-      request->query,
+      context_->insertSyncPoint(),
       base::Bind(&CopyTextureToImpl::ReadbackDone, AsWeakPtr(), request));
 }
 
@@ -471,59 +463,45 @@ WebKit::WebGLId GLHelper::CopyTextureToImpl::CopyAndScaleTexture(
                       quality);
 }
 
-void GLHelper::CopyTextureToImpl::ReadbackDone(Request* finished_request) {
+void GLHelper::CopyTextureToImpl::ReadbackDone(Request* request) {
   TRACE_EVENT0("mirror",
                "GLHelper::CopyTextureToImpl::CheckReadbackFramebufferComplete");
-  finished_request->done = true;
+  DCHECK(request == request_queue_.front());
 
-  // We process transfer requests in the order they were received, regardless
-  // of the order we get the callbacks in.
-  while (!request_queue_.empty()) {
-    Request* request = request_queue_.front();
-    if (!request->done) {
-      break;
-    }
-
-    bool result = false;
-    if (request->buffer != 0) {
-      context_->bindBuffer(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM,
-                           request->buffer);
-      unsigned char* data = static_cast<unsigned char *>(
-          context_->mapBufferCHROMIUM(
-              GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM, GL_READ_ONLY));
-      if (data) {
-        result = true;
-        if (request->bytes_per_row == request->size.width() * 4 &&
-            request->bytes_per_row == request->row_stride_bytes) {
-          memcpy(request->pixels, data, request->size.GetArea() * 4);
-        } else {
-          unsigned char* out = request->pixels;
-          for (int y = 0; y < request->size.height(); y++) {
-            memcpy(out, data, request->bytes_per_row);
-            out += request->row_stride_bytes;
-            data += request->size.width() * 4;
-          }
+  bool result = false;
+  if (request->buffer != 0) {
+    context_->bindBuffer(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM,
+                         request->buffer);
+    unsigned char* data = static_cast<unsigned char *>(
+        context_->mapBufferCHROMIUM(
+            GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM, GL_READ_ONLY));
+    if (data) {
+      result = true;
+      if (request->bytes_per_row == request->size.width() * 4 &&
+          request->bytes_per_row == request->row_stride_bytes) {
+        memcpy(request->pixels, data, request->size.GetArea() * 4);
+      } else {
+        unsigned char* out = request->pixels;
+        for (int y = 0; y < request->size.height(); y++) {
+          memcpy(out, data, request->bytes_per_row);
+          out += request->row_stride_bytes;
+          data += request->size.width() * 4;
         }
-        context_->unmapBufferCHROMIUM(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM);
       }
-      context_->bindBuffer(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM, 0);
+      context_->unmapBufferCHROMIUM(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM);
     }
-
-    FinishRequest(request, result);
+    context_->bindBuffer(GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM, 0);
   }
+
+  FinishRequest(request, result);
 }
 
 void GLHelper::CopyTextureToImpl::FinishRequest(Request* request,
                                                 bool result) {
-  TRACE_EVENT0("mirror", "GLHelper::CopyTextureToImpl::FinishRequest");
   DCHECK(request_queue_.front() == request);
   request_queue_.pop();
   request->callback.Run(result);
   ScopedFlush flush(context_);
-  if (request->query != 0) {
-    context_->deleteQueryEXT(request->query);
-    request->query = 0;
-  }
   if (request->buffer != 0) {
     context_->deleteBuffer(request->buffer);
     request->buffer = 0;

diff --git a/gpu/GLES2/gl2extchromium.h b/gpu/GLES2/gl2extchromium.h
@@ -339,9 +339,6 @@ typedef void (GL_APIENTRYP PFNGLBINDUNIFORMLOCATIONCHROMIUMPROC) (
 #ifndef GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM
 #define GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM 0x84F5
 #endif
-#ifndef GL_ASYNC_READ_PIXELS_COMPLETED_CHROMIUM
-#define GL_ASYNC_READ_PIXELS_COMPLETED_CHROMIUM 0x84F6
-#endif
 #endif  /* GL_CHROMIUM_async_pixel_transfers */
 
 /* GL_CHROMIUM_copy_texture */

diff --git a/gpu/command_buffer/build_gles2_cmd_buffer.py b/gpu/command_buffer/build_gles2_cmd_buffer.py
@@ -826,7 +826,6 @@
       'GL_COMMANDS_ISSUED_CHROMIUM',
       'GL_LATENCY_QUERY_CHROMIUM',
       'GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM',
-      'GL_ASYNC_READ_PIXELS_COMPLETED_CHROMIUM',
     ],
   },
   'RenderBufferParameter': {
@@ -1902,8 +1901,7 @@
         'GLint x, GLint y, GLsizei width, GLsizei height, '
         'GLenumReadPixelFormat format, GLenumReadPixelType type, '
         'uint32 pixels_shm_id, uint32 pixels_shm_offset, '
-        'uint32 result_shm_id, uint32 result_shm_offset, '
-        'GLboolean async',
+        'uint32 result_shm_id, uint32 result_shm_offset',
     'result': ['uint32'],
     'defer_reads': True,
   },

diff --git a/gpu/command_buffer/client/gles2_cmd_helper_autogen.h b/gpu/command_buffer/client/gles2_cmd_helper_autogen.h
@@ -923,12 +923,12 @@
   void ReadPixels(
       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, GLboolean async) {
+      uint32 result_shm_id, uint32 result_shm_offset) {
     gles2::cmds::ReadPixels* c = GetCmdSpace<gles2::cmds::ReadPixels>();
     if (c) {
       c->Init(
           x, y, width, height, format, type, pixels_shm_id, pixels_shm_offset,
-          result_shm_id, result_shm_offset, async);
+          result_shm_id, result_shm_offset);
     }
   }
 

diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
@@ -2236,7 +2236,8 @@ void GLES2Implementation::ReadPixels(
     if (buffer && buffer->shm_id() != -1) {
       helper_->ReadPixels(xoffset, yoffset, width, height, format, type,
                           buffer->shm_id(), buffer->shm_offset(),
-                          0, 0, true);
+                          0, 0);
+      buffer->set_transfer_ready_token(helper_->InsertToken());
       CheckGLError();
     }
     return;
@@ -2268,8 +2269,7 @@ void GLES2Implementation::ReadPixels(
     helper_->ReadPixels(
         xoffset, yoffset, width, num_rows, format, type,
         buffer.shm_id(), buffer.offset(),
-        GetResultShmId(), GetResultShmOffset(),
-        false);
+        GetResultShmId(), GetResultShmOffset());
     WaitForCmd();
     if (*result != 0) {
       // when doing a y-flip we have to iterate through top-to-bottom chunks
@@ -3175,31 +3175,36 @@ void GLES2Implementation::DeleteQueriesEXTHelper(
     return;
   }
   // When you delete a query you can't mark its memory as unused until it's
-  // either completed, or deleted in the gpu process.
+  // 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.
   bool query_pending = false;
   for (GLsizei ii = 0; ii < n; ++ii) {
     QueryTracker::Query* query = query_tracker_->GetQuery(queries[ii]);
-    if (query && !query->CheckResultsAvailable(helper_)) {
+    if (query && query->Pending()) {
       query_pending = true;
       break;
     }
   }
 
-  helper_->DeleteQueriesEXTImmediate(n, queries);
-
   if (query_pending) {
-    // This should make sure that the GPU process have deleted the queries
-    // and given up any claim on the shared memory that goes along with
-    // those queries so that we can safely re-use the shared memory.
     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();
+      }
+    }
     query_tracker_->RemoveQuery(queries[ii], helper_->IsContextLost());
   }
+  helper_->DeleteQueriesEXTImmediate(n, queries);
 }
 
 // TODO(gman): Remove this. Queries are not shared resources.

diff --git a/gpu/command_buffer/client/gles2_implementation_unittest.cc b/gpu/command_buffer/client/gles2_implementation_unittest.cc
@@ -1438,12 +1438,11 @@ TEST_F(GLES2ImplementationTest, ReadPixels2Reads) {
   Cmds expected;
   expected.read1.Init(
       0, 0, kWidth, kHeight / 2, kFormat, kType,
-      mem1.id, mem1.offset, result1.id, result1.offset,
-      false);
+      mem1.id, mem1.offset, result1.id, result1.offset);
   expected.set_token1.Init(GetNextToken());
   expected.read2.Init(
       0, kHeight / 2, kWidth, kHeight / 2, kFormat, kType,
-      mem2.id, mem2.offset, result2.id, result2.offset, false);
+      mem2.id, mem2.offset, result2.id, result2.offset);
   expected.set_token2.Init(GetNextToken());
   scoped_ptr<int8[]> buffer(new int8[kWidth * kHeight * kBytesPerPixel]);
 
@@ -1475,7 +1474,7 @@ TEST_F(GLES2ImplementationTest, ReadPixelsBadFormatType) {
   Cmds expected;
   expected.read.Init(
       0, 0, kWidth, kHeight, kFormat, kType,
-      mem1.id, mem1.offset, result1.id, result1.offset, false);
+      mem1.id, mem1.offset, result1.id, result1.offset);
   expected.set_token.Init(GetNextToken());
   scoped_ptr<int8[]> buffer(new int8[kWidth * kHeight * kBytesPerPixel]);
 

diff --git a/gpu/command_buffer/client/query_tracker.cc b/gpu/command_buffer/client/query_tracker.cc
@@ -113,7 +113,9 @@ void QueryTracker::Query::Begin(GLES2Implementation* gl) {
       gl->helper()->BeginQueryEXT(target(), id(), shm_id(), shm_offset());
       break;
     case GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM:
-    case GL_ASYNC_READ_PIXELS_COMPLETED_CHROMIUM:
+      // tell service about id, shared memory and count
+      gl->helper()->BeginQueryEXT(target(), id(), shm_id(), shm_offset());
+      break;
     default:
       // tell service about id, shared memory and count
       gl->helper()->BeginQueryEXT(target(), id(), shm_id(), shm_offset());
@@ -163,7 +165,8 @@ bool QueryTracker::Query::CheckResultsAvailable(
                              static_cast<uint64>(0xFFFFFFFFL));
           break;
         case GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM:
-        case GL_ASYNC_READ_PIXELS_COMPLETED_CHROMIUM:
+          result_ = info_.sync->result;
+          break;
         default:
           result_ = info_.sync->result;
           break;