Draw the offscreen texture to reflector's surface without extra copy

content/browser/compositor/offscreen_browser_compositor_output_surface.cc

Index: content/browser/compositor/offscreen_browser_compositor_output_surface.cc
diff --git a/content/browser/compositor/offscreen_browser_compositor_output_surface.cc b/content/browser/compositor/offscreen_browser_compositor_output_surface.cc
index 9fa502684c74517388c4566f60e116647be7f92f..2fda692159b55d177f9ff1aa874b1ad96a6eac76 100644
--- a/content/browser/compositor/offscreen_browser_compositor_output_surface.cc
+++ b/content/browser/compositor/offscreen_browser_compositor_output_surface.cc
@@ -11,8 +11,10 @@
 #include "cc/output/output_surface_client.h"
 #include "cc/resources/resource_provider.h"
 #include "content/browser/compositor/browser_compositor_overlay_candidate_validator.h"
+#include "content/browser/compositor/owned_mailbox.h"
 #include "content/browser/compositor/reflector_impl.h"
 #include "content/common/gpu/client/context_provider_command_buffer.h"
+#include "content/common/gpu/client/gl_helper.h"
 #include "content/public/browser/browser_thread.h"
 #include "gpu/command_buffer/client/context_support.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
@@ -38,6 +40,7 @@ OffscreenBrowserCompositorOutputSurface::
       fbo_(0),
       is_backbuffer_discarded_(false),
       backing_texture_id_(0),
+      needs_update_reflector_(false),
       weak_ptr_factory_(this) {
   capabilities_.max_frames_pending = 1;
   capabilities_.uses_default_gl_framebuffer = false;
@@ -52,10 +55,19 @@ void OffscreenBrowserCompositorOutputSurface::EnsureBackbuffer() {
   is_backbuffer_discarded_ = false;
 
   if (!backing_texture_id_) {
+    GLHelper* shared_helper =
+        ImageTransportFactory::GetInstance()->GetGLHelper();
+    mailbox_ = new OwnedMailbox(shared_helper);

[piman, 2015/05/07 01:56:45]

Why not creating mailbox_ with the offline_compositor_gl_helper_? That way you
don't need to ConsumeMailboxToTexture it - you can simply use
mailbox_->texture_id(). (but then you want to release mailbox_ in
DiscardBackbuffer)


I wonder if it might be possible to converge both paths. Especially now that the
thread is gone, it may be easier to always have the
BrowserCompositorOutputSurface produce the mailbox (i.e. in the old path, do the
readback), and then the Reflector just manages the interaction with the
mirroring layers, forwarding the mailbox and damage. What do you think?

[oshima, 2015/05/07 16:43:24]

ok, let me rework on it.

[oshima, 2015/05/13 07:47:03]

Looks like I still need to use shared helper because mailbox needs to be alive
until
the release callback is called, which is set in Layer::SetTextureMailbox.

I confirmed this with danakj@. 

     GLES2Interface* gl = context_provider_->ContextGL();
-    cc::ResourceFormat format = cc::RGBA_8888;
 
-    gl->GenTextures(1, &backing_texture_id_);
+    offline_compositor_gl_helper_.reset(
+        new GLHelper(gl, context_provider()->ContextSupport()));
+
+    backing_texture_id_ =
+        offline_compositor_gl_helper_->ConsumeMailboxToTexture(
+            mailbox_->mailbox(), mailbox_->sync_point());
+
+    cc::ResourceFormat format = cc::RGBA_8888;
     gl->BindTexture(GL_TEXTURE_2D, backing_texture_id_);
     gl->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
     gl->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
@@ -64,6 +76,7 @@ void OffscreenBrowserCompositorOutputSurface::EnsureBackbuffer() {
     gl->TexImage2D(GL_TEXTURE_2D, 0, GLInternalFormat(format),
                    surface_size_.width(), surface_size_.height(), 0,
                    GLDataFormat(format), GLDataType(format), nullptr);
+    needs_update_reflector_ = true;
   }
 }
 
@@ -110,10 +123,13 @@ void OffscreenBrowserCompositorOutputSurface::BindFramebuffer() {
 
 void OffscreenBrowserCompositorOutputSurface::SwapBuffers(
     cc::CompositorFrame* frame) {
-  // TODO(oshima): Pass the texture to the reflector so that the
-  // reflector can simply use and draw it on their surface instead of
-  // copying the texture.
   if (reflector_) {
+    if (needs_update_reflector_) {
+      needs_update_reflector_ = false;
+      reflector_->OnSourceTextureMailboxUpdated();
+    }
+    offline_compositor_gl_helper_->InsertOrderingBarrier();

[piman, 2015/05/07 01:56:45]

The InsertOrderingBarrier is needed to order between 2 contexts. Here we just
used the output surface's context. In OnSourceSwapBuffers, it's also the same
context that we use, so I don't think the InsertOrderingBarrier is needed here.
It is needed in OnSourceSwapBuffers to order wrt the mirroring compositor's
context.

[oshima, 2015/05/13 07:47:03]

Removed

+
     if (frame->gl_frame_data->sub_buffer_rect ==
         gfx::Rect(frame->gl_frame_data->size))
       reflector_->OnSourceSwapBuffers();
@@ -122,13 +138,21 @@ void OffscreenBrowserCompositorOutputSurface::SwapBuffers(
   }
 
   client_->DidSwapBuffers();
+}
+
+scoped_refptr<OwnedMailbox>
+OffscreenBrowserCompositorOutputSurface::GetTextureMailbox() {
+  EnsureBackbuffer();

[piman, 2015/05/07 01:56:45]

Is this needed? The compositor is not allowed to draw / call SwapBuffers if it
hasn't called EnsureBackbuffer first. Then maybe you can just call
OnSourceTextureMailboxUpdated directly from EnsureBackbuffer, and not need
needs_update_reflector_.

[oshima, 2015/05/07 16:43:24]

I need GetTextureMailbox call so that I can get the mailbox in
OnSourceOutputSurfaceReady. I'll rework to consolidate two path and hopefully we
can clean this up.

+  // This is called from the reflector, so no need to update
+  // the refletor.
+  needs_update_reflector_ = false;
+  return mailbox_;
+}
 
-  // TODO(oshima): sync with the reflector's SwapBuffersComplete.
-  uint32_t sync_point =
-      context_provider_->ContextGL()->InsertSyncPointCHROMIUM();
-  context_provider_->ContextSupport()->SignalSyncPoint(
-      sync_point, base::Bind(&OutputSurface::OnSwapBuffersComplete,
-                             weak_ptr_factory_.GetWeakPtr()));
+base::Closure
+OffscreenBrowserCompositorOutputSurface::CreateCompositionStartedCallback() {
+  return base::Bind(&OutputSurface::OnSwapBuffersComplete,
+                    weak_ptr_factory_.GetWeakPtr());
 }
 
 #if defined(OS_MACOSX)