Implement client side PBOs for glReadPixel

content/common/gpu/client/gl_helper.cc

Index: content/common/gpu/client/gl_helper.cc
diff --git a/content/common/gpu/client/gl_helper.cc b/content/common/gpu/client/gl_helper.cc
index 0d295f03643b12961487d78454d2fbca199874fb..4ddc549b0895c5c44fbf5109719b8094ba49410b 100644
--- a/content/common/gpu/client/gl_helper.cc
+++ b/content/common/gpu/client/gl_helper.cc
@@ -4,9 +4,10 @@
 
 #include "content/common/gpu/client/gl_helper.h"
 
-#include <queue>
+#include <set>
 
 #include "base/bind.h"
+#include "base/debug/trace_event.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
@@ -15,6 +16,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/time.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebCString.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebString.h"
 #include "third_party/skia/include/core/SkRegion.h"
@@ -27,24 +29,6 @@ using WebKit::WebGraphicsContext3D;
 
 namespace {
 
-const char kGLHelperThreadName[] = "GLHelperThread";
-
-class GLHelperThread : public base::Thread {
- public:
-  GLHelperThread() : base::Thread(kGLHelperThreadName) {
-    Start();
-  }
-  virtual ~GLHelperThread() {
-    Stop();
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(GLHelperThread);
-};
-
-base::LazyInstance<GLHelperThread> g_gl_helper_thread =
-    LAZY_INSTANCE_INITIALIZER;
-
 class ScopedWebGLId {
  public:
   typedef void (WebGraphicsContext3D::*DeleteFunc)(WebGLId);
@@ -239,10 +223,8 @@ namespace content {
 class GLHelper::CopyTextureToImpl {
  public:
   CopyTextureToImpl(WebGraphicsContext3D* context,
-                    WebGraphicsContext3D* context_for_thread,
                     GLHelper* helper)
       : context_(context),
-        context_for_thread_(context_for_thread),
         helper_(helper),
         flush_(context),
         program_(context, context->createProgram()),
@@ -253,7 +235,6 @@ class GLHelper::CopyTextureToImpl {
   }
   ~CopyTextureToImpl() {
     CancelRequests();
-    DeleteContextForThread();
   }
 
   void InitBuffer();
@@ -323,13 +304,8 @@ class GLHelper::CopyTextureToImpl {
                        const gfx::Size& dst_size,
                        bool vertically_flip_texture);
 
-  // Deletes the context for GLHelperThread.
-  void DeleteContextForThread();
-  static void ReadBackFramebuffer(scoped_refptr<Request> request,
-                                  WebGraphicsContext3D* context,
-                                  scoped_refptr<base::TaskRunner> reply_loop);
   static void ReadBackFramebufferComplete(scoped_refptr<Request> request,
-                                          bool result);
+                                          GLuint buffer, GLuint query);
   void FinishRequest(scoped_refptr<Request> request);
   void CancelRequests();
 
@@ -345,7 +321,6 @@ class GLHelper::CopyTextureToImpl {
   static const WebKit::WGC3Dchar kCopyFragmentShader[];
 
   WebGraphicsContext3D* context_;
-  WebGraphicsContext3D* context_for_thread_;
   GLHelper* helper_;
 
   // A scoped flush that will ensure all resource deletions are flushed when
@@ -366,7 +341,7 @@ class GLHelper::CopyTextureToImpl {
   WebKit::WGC3Dint texture_location_;
   // The location of the texture coordinate of the sub-rectangle in the program.
   WebKit::WGC3Dint src_subrect_location_;
-  std::queue<scoped_refptr<Request> > request_queue_;
+  std::set<scoped_refptr<Request> > request_set_;
 };
 
 const WebKit::WGC3Dfloat GLHelper::CopyTextureToImpl::kVertexAttributes[] = {
@@ -394,6 +369,20 @@ const WebKit::WGC3Dchar GLHelper::CopyTextureToImpl::kCopyVertexShader[] =
     "  v_texcoord = src_subrect.xy + a_texcoord * src_subrect.zw;"
     "}";
 
+
+#if (SK_R32_SHIFT == 16) && !SK_B32_SHIFT
+const WebKit::WGC3Dchar GLHelper::CopyTextureToImpl::kCopyFragmentShader[] =
+    "precision mediump float;"
+    "varying vec2 v_texcoord;"
+    "uniform sampler2D s_texture;"
+    "const mediump mat4 swizzle = mat4( 0.0, 0.0, 1.0, 0.0,  "
+    "                                   0.0, 1.0, 0.0, 0.0,  "
+    "                                   1.0, 0.0, 0.0, 0.0,  "
+    "                                   0.0, 0.0, 0.0, 1.0 );"
+    "void main() {"
+    "  gl_FragColor = swizzle * texture2D(s_texture, v_texcoord);"

[apatrick_chromium, 2013/03/19 18:34:48]

Will this work? Seems a bit simpler.

gl_FragColor = texture2D(s_texture, v_texcoord).bgra;

[hubbe, 2013/03/19 22:42:42]

Done.

+    "}";
+#else
 const WebKit::WGC3Dchar GLHelper::CopyTextureToImpl::kCopyFragmentShader[] =
     "precision mediump float;"
     "varying vec2 v_texcoord;"
@@ -401,6 +390,8 @@ const WebKit::WGC3Dchar GLHelper::CopyTextureToImpl::kCopyFragmentShader[] =
     "void main() {"
     "  gl_FragColor = texture2D(s_texture, v_texcoord);"
     "}";
+#endif
+
 
 void GLHelper::CopyTextureToImpl::InitBuffer() {
   ScopedBufferBinder<GL_ARRAY_BUFFER> buffer_binder(
@@ -517,17 +508,6 @@ WebGLId GLHelper::CopyTextureToImpl::ScaleTexture(
   return dst_texture;
 }
 
-void GLHelper::CopyTextureToImpl::DeleteContextForThread() {
-  if (!context_for_thread_)
-    return;
-
-  g_gl_helper_thread.Pointer()->message_loop_proxy()->PostTask(
-      FROM_HERE,
-      base::Bind(&DeleteContext,
-                 context_for_thread_));
-  context_for_thread_ = NULL;
-}
-
 void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
     WebGLId src_texture,
     const gfx::Size& src_size,
@@ -535,26 +515,54 @@ void GLHelper::CopyTextureToImpl::CropScaleReadbackAndCleanTexture(
     const gfx::Size& dst_size,
     unsigned char* out,
     const base::Callback<void(bool)>& callback) {
-  if (!context_for_thread_) {
-    callback.Run(false);
-    return;
-  }
-
   WebGLId texture = ScaleTexture(src_texture,
                                  src_size,
                                  src_subrect,
                                  dst_size,
-                                 false);
+#ifdef USE_SKIA
+                                 true
+#else
+                                 false
+#endif
+                                 );
   context_->flush();
   scoped_refptr<Request> request =
       new Request(this, texture, dst_size, out, callback);
-  request_queue_.push(request);
+  request_set_.insert(request);
 
-  g_gl_helper_thread.Pointer()->message_loop_proxy()->PostTask(FROM_HERE,
-      base::Bind(&ReadBackFramebuffer,
-                 request,
-                 context_for_thread_,
-                 base::MessageLoopProxy::current()));
+  ScopedFlush flush(context_);
+  ScopedFramebuffer dst_framebuffer(context_, context_->createFramebuffer());
+  gfx::Size size;
+  base::AutoLock auto_lock(request->lock);
+  size = request->size;
+
+  ScopedFramebufferBinder<GL_FRAMEBUFFER> framebuffer_binder(
+      context_, dst_framebuffer);
+  ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(
+      context_, request->texture);
+  context_->framebufferTexture2D(GL_FRAMEBUFFER,
+                                 GL_COLOR_ATTACHMENT0,
+                                 GL_TEXTURE_2D,
+                                 request->texture,
+                                 0);
+  GLuint buffer = context_->createBuffer();
+  context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM,
+                       buffer);
+  context_->bufferData(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM,
+                       4 * size.GetArea(),
+                       NULL,
+                       GL_STREAM_READ);
+
+  GLuint query = context_->createQueryEXT();
+  context_->beginQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM, query);
+  context_->readPixels(0, 0, size.width(), size.height(),
+                       GL_RGBA, GL_UNSIGNED_BYTE, 0);
+  context_->endQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM);

[apatrick_chromium, 2013/03/19 18:34:48]

You might need to call flush here to guarantee that the query eventually ends.
Gregg will know is endQueryEXT does an implicit flush.

[greggman, 2013/03/19 20:54:11]

yes, you need to call flush. endQueryEXT may or may not do a flush but it's not
guaranteed.

[hubbe, 2013/03/19 22:42:42]

Does the flush need to happen before endQueryExt?
If not, there is a ScopedFlush higher up which should work, no?

[apatrick_chromium, 2013/03/19 23:31:20]

I missed the ScopedFlush. It will work.

+  context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0);

[apatrick_chromium, 2013/03/19 18:34:48]

What if a different buffer was bound to this target previously?

[greggman, 2013/03/19 20:54:11]

You can't use UNPACK_TRANSFER_BUFFER for READing. Only for WRITEing. I know it
happens to work but the semantic for GL is UNPACK for for providing data to GL,
PACK is for getting data from GL.

[hubbe, 2013/03/19 22:42:42]

Good question.
Should I document what buffers are used, or should I attempt to set it back? (If
set it back, what is the proper way to get the buffer given a binding?)

[hubbe, 2013/03/19 22:42:42]

I guess I have to implement that extension then, right?
Is there a good resource for this kind of knowledge? Learning openGL piece by
piece is not exactly fun.

[apatrick_chromium, 2013/03/19 23:31:20]

I think the issue is, before this code was using context_for_thread_ as the
context for the read-back. So it was free to modify the context state without
affecting the state of context_. Now it is changing the state of context_. This
might be okay but I don't know what state it is safe to change on it.

BTW, I noticed that there might be a similar issue with the framebuffer and
texture bindings since ScopedFramebufferBinder etc always restore ID zero, which
might not have been what was previously bound on context_.

Generally, unless these bindings are cached somewhere on the client side, there
is not going to be an efficient way to find out what they were before because
you would need to do a synchronous round-trip to the GPU process to get that
information.

Perhaps what you could do is retain the second GL context (but rename the
variable because you don't need the helper thread) so that you can safely modify
its state. Specifically, you would be able to reset all the bindings to zero.

I seem to remember this code had exactly this issue before and that was why the
second context was introduced.

[hubbe, 2013/03/19 23:39:27]

This things are cached in the client side, so restoring them would not be slow. 
Is this a general concern, or are we simply trying to make sure that callers to
this particular function are not caught unaware?

(Currently, there is exactly one call to this function in the Chrome code base.)

[apatrick_chromium, 2013/03/19 23:53:59]

Yes it looks like getInteger(GL_PIXEL_PACK_TRANSFER_BUFFER_BINDING_CHROMIUM,
...) will not round-trip to the GPU process so I think you can just call it
prior to the first bindBuffer and then restore the previous buffer rather than
buffer zero.

Likewise, now that this code is no longer using the special helper context as it
was before, modify ScopedFramebufferBinder and ScopedTextureBinder to work
similarly with GL_DRAW_FRAMEBUFFER_BINDING, GL_READ_FRAMEBUFFER_BINDING and
GL_TEXTURE_BINDING as the argument to getInteger.

+  base::MessageLoopProxy::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&ReadBackFramebufferComplete, request, buffer, query),
+      base::TimeDelta::FromMilliseconds(2));
 }
 
 void GLHelper::CopyTextureToImpl::ReadbackTextureSync(WebGLId texture,
@@ -590,75 +598,60 @@ WebKit::WebGLId GLHelper::CopyTextureToImpl::CopyAndScaleTexture(
                       vertically_flip_texture);
 }
 
-void GLHelper::CopyTextureToImpl::ReadBackFramebuffer(
+void GLHelper::CopyTextureToImpl::ReadBackFramebufferComplete(
     scoped_refptr<Request> request,
-    WebGraphicsContext3D* context,
-    scoped_refptr<base::TaskRunner> reply_loop) {
-  DCHECK(context);
-  if (!context->makeContextCurrent() || context->isContextLost()) {
-    base::AutoLock auto_lock(request->lock);
-    if (request->pixels) {
-      // Only report failure if the request wasn't canceled (otherwise the
-      // failure has already been reported).
-      request->pixels = NULL;
-      reply_loop->PostTask(
-          FROM_HERE, base::Bind(ReadBackFramebufferComplete, request, false));
-    }
+    GLuint buffer, GLuint query) {
+
+  TRACE_EVENT0("mirror",
+               "GLHelper::CopyTextureToImpl::ReadBackFramebufferComplete");
+
+  if (!request->copy_texture_impl) {
+    request->callback.Run(false);
     return;
   }
-  ScopedFlush flush(context);
-  ScopedFramebuffer dst_framebuffer(context, context->createFramebuffer());
-  unsigned char* pixels = NULL;
-  gfx::Size size;
-  {
-    // Note: We don't want to keep the lock while doing the readBack (since we
-    // don't want to block the UI thread). We rely on the fact that once the
-    // texture is bound to a FBO (that isn't current), deleting the texture is
-    // delayed until the FBO is deleted. We ensure ordering by flushing while
-    // the lock is held. Either the main thread cancelled before we get the
-    // lock, and we'll exit early, or we ensure that the texture is bound to the
-    // framebuffer before the main thread has a chance to delete it.
-    base::AutoLock auto_lock(request->lock);
-    if (!request->texture || !request->pixels)
+
+  WebKit::WebGraphicsContext3D* context = request->copy_texture_impl->context_;
+  bool result = false;
+
+  if (buffer != 0) {
+    unsigned int done = 1;
+    context->getQueryObjectuivEXT(query, GL_QUERY_RESULT_AVAILABLE_EXT, &done);
+    if (!done) {
+      TRACE_EVENT0("mirror",
+                   "GLHelper::CopyTextureToImpl::Reschedule");
+      base::MessageLoopProxy::current()->PostDelayedTask(
+          FROM_HERE,
+          base::Bind(&ReadBackFramebufferComplete, request, buffer, query),
+          base::TimeDelta::FromMilliseconds(2));
       return;
-    pixels = request->pixels;
-    request->pixels = NULL;
-    size = request->size;
-    {
-      ScopedFlush flush(context);
-      ScopedFramebufferBinder<GL_FRAMEBUFFER> framebuffer_binder(
-          context, dst_framebuffer);
-      ScopedTextureBinder<GL_TEXTURE_2D> texture_binder(
-          context, request->texture);
-      context->framebufferTexture2D(GL_FRAMEBUFFER,
-                                    GL_COLOR_ATTACHMENT0,
-                                    GL_TEXTURE_2D,
-                                    request->texture,
-                                    0);
     }
+    context->deleteQueryEXT(query);
+    context->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM,
+                        buffer);
+    void* data = context->mapBufferCHROMIUM(
+        GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, GL_READ_ONLY);
+
+    if (data) {
+      result = true;
+      memcpy(request->pixels, data, request->size.GetArea() * 4);
+      context->unmapBufferCHROMIUM(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM);
+    } else {
+      fprintf(stderr,"mapBuffer Returns NULL\n");

[apatrick_chromium, 2013/03/19 18:34:48]

Use LOG / VLOG instead of fprintf.

[hubbe, 2013/03/19 22:42:42]

Old useless debugging junk. Gone.

+    }
+    context->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0);

[apatrick_chromium, 2013/03/19 18:34:48]

Same as above. There might have been a different buffer bound in the target.

[hubbe, 2013/03/19 22:42:42]

This is in an async callback. Is it really reasonable for someone to expect a
buffer binding to persist between callbacks?

[apatrick_chromium, 2013/03/19 23:31:20]

The assumption might have been made somewhere. Need to find out.

[hubbe, 2013/03/19 23:39:27]

While it is possible, the browser still works when using this code, so at least
that assumption has not been made anywhere in the critical path on linux-aura.

[apatrick_chromium, 2013/03/19 23:53:59]

You can do the same thing to get the buffer binding here and there should be no
issues.

+    context->deleteBuffer(buffer);
+  } else {
+    fprintf(stderr,"Buffer is negative!\n");

[apatrick_chromium, 2013/03/19 18:34:48]

Use LOG / VLOG instead of fprintf.

[hubbe, 2013/03/19 22:42:42]

Gone.

   }
-  bool result = context->readBackFramebuffer(
-        pixels,
-        4 * size.GetArea(),
-        dst_framebuffer.id(),
-        size.width(),
-        size.height());
-  reply_loop->PostTask(
-      FROM_HERE, base::Bind(ReadBackFramebufferComplete, request, result));
-}
 
-void GLHelper::CopyTextureToImpl::ReadBackFramebufferComplete(
-    scoped_refptr<Request> request,
-    bool result) {
   request->callback.Run(result);
-  if (request->copy_texture_impl)
-    request->copy_texture_impl->FinishRequest(request);
+  request->copy_texture_impl->FinishRequest(request);
 }
 
 void GLHelper::CopyTextureToImpl::FinishRequest(
     scoped_refptr<Request> request) {
-  CHECK(request_queue_.front() == request);
-  request_queue_.pop();
+  CHECK(request_set_.find(request) != request_set_.end());
+  request_set_.erase(request);
   base::AutoLock auto_lock(request->lock);
   if (request->texture != 0) {
     context_->deleteTexture(request->texture);
@@ -668,9 +661,9 @@ void GLHelper::CopyTextureToImpl::FinishRequest(
 }
 
 void GLHelper::CopyTextureToImpl::CancelRequests() {
-  while (!request_queue_.empty()) {
-    scoped_refptr<Request> request = request_queue_.front();
-    request_queue_.pop();
+  while (!request_set_.empty()) {
+    scoped_refptr<Request> request = *request_set_.begin();
+    request_set_.erase(request);
     request->copy_texture_impl = NULL;
     bool cancelled = false;
     {
@@ -689,34 +682,11 @@ void GLHelper::CopyTextureToImpl::CancelRequests() {
   }
 }
 
-base::subtle::Atomic32 GLHelper::count_ = 0;
-
-GLHelper::GLHelper(WebGraphicsContext3D* context,
-                   WebGraphicsContext3D* context_for_thread)
-    : context_(context),
-      context_for_thread_(context_for_thread) {
-  base::subtle::NoBarrier_AtomicIncrement(&count_, 1);
+GLHelper::GLHelper(WebGraphicsContext3D* context)
+    : context_(context) {
 }
 
 GLHelper::~GLHelper() {
-  DCHECK_NE(MessageLoop::current(),
-            g_gl_helper_thread.Pointer()->message_loop());
-  base::subtle::Atomic32 decremented_count =
-    base::subtle::NoBarrier_AtomicIncrement(&count_, -1);
-  if (decremented_count == 0) {
-    // When this is the last instance, we synchronize with the pending
-    // operations on GLHelperThread. Otherwise on shutdown we may kill the GPU
-    // process infrastructure (BrowserGpuChannelHostFactory) before they have
-    // a chance to complete, likely leading to a crash.
-    base::WaitableEvent event(false, false);
-    g_gl_helper_thread.Pointer()->message_loop_proxy()->PostTask(
-        FROM_HERE,
-        base::Bind(&SignalWaitableEvent,
-                   &event));
-    // http://crbug.com/125415
-    base::ThreadRestrictions::ScopedAllowWait allow_wait;
-    event.Wait();
-  }
 }
 
 WebGraphicsContext3D* GLHelper::context() const {
@@ -786,9 +756,7 @@ WebGLId GLHelper::CompileShaderFromSource(
 void GLHelper::InitCopyTextToImpl() {
   // Lazily initialize |copy_texture_to_impl_|
   if (!copy_texture_to_impl_.get())
-    copy_texture_to_impl_.reset(new CopyTextureToImpl(context_,
-                                                      context_for_thread_,
-                                                      this));
+    copy_texture_to_impl_.reset(new CopyTextureToImpl(context_, this));
 }
 
 void GLHelper::CopySubBufferDamage(WebKit::WebGLId texture,