Decoupling backbuffer allocation suggestion from frontbuffer allocation suggestion.

content/common/gpu/image_transport_surface_linux.cc

Index: content/common/gpu/image_transport_surface_linux.cc
diff --git a/content/common/gpu/image_transport_surface_linux.cc b/content/common/gpu/image_transport_surface_linux.cc
index 9fe3d80ef5b18f3b66ba90bd1b63fecde98c1af2..9f0835a0cf9a154a1cf0832186872dd4d828c979 100644
--- a/content/common/gpu/image_transport_surface_linux.cc
+++ b/content/common/gpu/image_transport_surface_linux.cc
@@ -93,7 +93,8 @@ class EGLImageTransportSurface
   virtual gfx::Size GetSize() OVERRIDE;
   virtual bool OnMakeCurrent(gfx::GLContext* context) OVERRIDE;
   virtual unsigned int GetBackingFrameBufferObject() OVERRIDE;
-  virtual void SetBufferAllocation(BufferAllocationState state) OVERRIDE;
+  virtual void SetBackbufferAllocation(bool allocated) OVERRIDE;
+  virtual void SetFrontbufferAllocation(bool allocated) OVERRIDE;
 
  protected:
   // ImageTransportSurface implementation
@@ -111,7 +112,11 @@ class EGLImageTransportSurface
   void SendPostSubBuffer(int x, int y, int width, int height);
 
   // Tracks the current buffer allocation state.
-  BufferAllocationState buffer_allocation_state_;
+  bool backbuffer_suggested_allocation_;
+  bool frontbuffer_suggested_allocation_;
+
+  // The expected size when visible.
+  gfx::Size visible_size_;
 
   uint32 fbo_id_;
 
@@ -145,7 +150,8 @@ class GLXImageTransportSurface
   virtual std::string GetExtensions();
   virtual gfx::Size GetSize() OVERRIDE;
   virtual bool OnMakeCurrent(gfx::GLContext* context) OVERRIDE;
-  virtual void SetBufferAllocation(BufferAllocationState state) OVERRIDE;
+  virtual void SetBackbufferAllocation(bool allocated) OVERRIDE;
+  virtual void SetFrontbufferAllocation(bool allocated) OVERRIDE;
 
  protected:
   // ImageTransportSurface implementation:
@@ -168,7 +174,8 @@ class GLXImageTransportSurface
   void ResizeSurface(gfx::Size size);
 
   // Tracks the current buffer allocation state.
-  BufferAllocationState buffer_allocation_state_;
+  bool backbuffer_suggested_allocation_;
+  bool frontbuffer_suggested_allocation_;
 
   XID dummy_parent_;
   gfx::Size size_;
@@ -270,7 +277,8 @@ EGLImageTransportSurface::EGLImageTransportSurface(
     GpuChannelManager* manager,
     GpuCommandBufferStub* stub)
     : gfx::PbufferGLSurfaceEGL(false, gfx::Size(16, 16)),
-      buffer_allocation_state_(BUFFER_ALLOCATION_FRONT_AND_BACK),
+      backbuffer_suggested_allocation_(true),
+      frontbuffer_suggested_allocation_(true),
       fbo_id_(0),
       made_current_(false) {
   helper_.reset(new ImageTransportHelper(this,
@@ -336,31 +344,21 @@ unsigned int EGLImageTransportSurface::GetBackingFrameBufferObject() {
   return fbo_id_;
 }
 
-void EGLImageTransportSurface::SetBufferAllocation(
-    BufferAllocationState state) {
-  if (buffer_allocation_state_ == state)
+void EGLImageTransportSurface::SetBackbufferAllocation(bool allocated) {
+  if (backbuffer_suggested_allocation_ == allocated)
     return;
-  buffer_allocation_state_ = state;
-
-  switch (state) {
-    case BUFFER_ALLOCATION_FRONT_AND_BACK:
-      if (!back_surface_.get() && front_surface_.get())
-        OnResize(front_surface_->size());
-      break;
-
-    case BUFFER_ALLOCATION_FRONT_ONLY:
-      if (back_surface_.get() && front_surface_.get())
-        ReleaseSurface(&back_surface_);
-      break;
+  backbuffer_suggested_allocation_ = allocated;
 
-    case BUFFER_ALLOCATION_NONE:
-      if (back_surface_.get() && front_surface_.get())
-        ReleaseSurface(&back_surface_);
-      break;
+  if (backbuffer_suggested_allocation_)
+    OnResize(visible_size_);
+  else
+    ReleaseSurface(&back_surface_);
+}
 
-    default:
-      NOTREACHED();
-  }
+void EGLImageTransportSurface::SetFrontbufferAllocation(bool allocated) {
+  if (frontbuffer_suggested_allocation_ == allocated)
+    return;
+  frontbuffer_suggested_allocation_ = allocated;
 }
 
 void EGLImageTransportSurface::ReleaseSurface(
@@ -374,6 +372,7 @@ void EGLImageTransportSurface::ReleaseSurface(
 }
 
 void EGLImageTransportSurface::OnResize(gfx::Size size) {
+  visible_size_ = size;
   back_surface_ = new EGLAcceleratedSurface(size);
 
   GLint previous_fbo_id = 0;
@@ -399,6 +398,9 @@ void EGLImageTransportSurface::OnResize(gfx::Size size) {
 }
 
 bool EGLImageTransportSurface::SwapBuffers() {
+  DCHECK(backbuffer_suggested_allocation_);
+  if (!frontbuffer_suggested_allocation_)

[jonathan.backer, 2012/05/15 15:53:31]

Need to set |previous_damage_rect_| here. AFAIK, we don't alternate b/w full
swaps and partial swaps (always just one or the other), but I'd like our code to
support that use case.

[mmocny, 2012/05/15 20:02:28]

Actually, I think I need to set previous damage to full frame when the
frontbuffer suggestion changes.  Otherwise, dropping and recreating frontbuffer
without a swap will lead to corruption.
On 2012/05/15 15:53:31, jonathan.backer wrote:

[jonathan.backer, 2012/05/16 02:39:01]

We use |previous_damage_rect_| for two frame damage tracking --- it is the
damage on the current front buffer and we get the damage for the backbuffer
passed in to PostSubBuffer.

The region outside of the |previous_damage_rect_| on the frontbuffer is assumed
to be common to the front and backbuffer. In order to make the backbuffer
complete, we copy over the difference between |preview_damage_rect_| and the
|new_damage_rect| passed in to PostSubBuffer from the front to the backbuffer.

Hope this clarifies the role.

And yes, it is probably sufficient to do this reset when the front buffer is
dropped (although it is somewhat inconsistent with how the code flows... at the
end of SwapBuffers and PostSubBuffer we assume that we have created a new front
buffer and adjusted the previous_damage_rect_ accordingly).

IIRC, |previous_damage_rect_| refers to On 2012/05/15 20:02:28, mmocny wrote:

+    return true;
   front_surface_.swap(back_surface_);
   DCHECK_NE(front_surface_.get(), static_cast<EGLAcceleratedSurface*>(NULL));
   helper_->DeferToFence(base::Bind(
@@ -428,7 +430,9 @@ void EGLImageTransportSurface::SendBuffersSwapped() {
 
 bool EGLImageTransportSurface::PostSubBuffer(
     int x, int y, int width, int height) {
-
+  DCHECK(backbuffer_suggested_allocation_);
+  if (!frontbuffer_suggested_allocation_)

[jonathan.backer, 2012/05/15 15:53:31]

I think that we need to set |previous_damage_rect_| here. Plus we should
probably be asserting that update region is the full backbuffer --- we rely on
being able to read from the frontbuffer for partial updates and we can't in this
case.

[mmocny, 2012/05/15 20:02:28]

I dont think we do read from the frontbuffer (right?), nor should we receive
full frame updates when we dont have one.

We copy from the backbuffer to the frontbuffer all the regions which changed,
for the purpose of ensure that once the front swaps to back it will have the
same contents as the previous backbuffer did.

Since we are not actually swapping buffers when !front, the current backbuffer
will always have the right contents.

The only time we need a full frame update, is right after we re-create the
backbuffer.  To test that, we would probably need a
"backbuffer_was_just_recreated_" flag and then test for that case here.  Is that
worth adding?

On 2012/05/15 15:53:31, jonathan.backer wrote:

[jonathan.backer, 2012/05/16 02:39:01]

We often do a copy from the front buffer to the back buffer in this function
(see comment above). However, we wouldn't if the damage passed in to this
function covered the whole buffer (which is why I suggested that we DCHECK this
if we exit early due to lack of frontbuffer to read from).
The copy goes the other way: glCopyTexSubImage reads from the current FBO  (with
front buffer as colour attachment) to the currently bound texture (the
backbuffer). We make the backbuffer correct by copying the necessary pieces from
the front just before the swap.

[mmocny, 2012/05/16 14:41:50]

I see, thank you for clearing up the direction of the copy, I had thought of it
going the other direction.  I guess we do it this way because we save a region
copy when the same region is dirty each frame?


This actually has me a little worried about the state of the buffer when
rendering frames in the background (for example, for thumbnailer).  The renderer
is not guaranteed to push a full frame when we do not have a frontbuffer (it
actually has no idea about what state frontbuffer is in).

I put the DCHECK in and ran a bunch of tests and this was not an issue, since we
push a full frame after dropping backbuffer, and we only paint a single frame
for thumbnailer which ends up the whole frame.  So, at the moment, we
coincidentally always push a full frame when drawing without a frontbuffer.

However, I left the DCHECK out of this patch since it is not currently
guaranteed, and I will instead file a separate bug to figure this out.

Also, I think that as long as the buffer contents outside the PostSubBuff region
aren't clobbered, things should work fine without the copy from front in this
eary exit step.

+    return true;
   DCHECK_NE(back_surface_.get(), static_cast<EGLAcceleratedSurface*>(NULL));
   gfx::Size expected_size = back_surface_->size();
   bool surfaces_same_size = front_surface_.get() &&
@@ -527,7 +531,8 @@ GLXImageTransportSurface::GLXImageTransportSurface(
     GpuChannelManager* manager,
     GpuCommandBufferStub* stub)
     : gfx::NativeViewGLSurfaceGLX(),
-      buffer_allocation_state_(BUFFER_ALLOCATION_FRONT_AND_BACK),
+      backbuffer_suggested_allocation_(true),
+      frontbuffer_suggested_allocation_(true),
       dummy_parent_(0),
       size_(1, 1),
       bound_(false),
@@ -611,30 +616,26 @@ void GLXImageTransportSurface::ReleaseSurface() {
   bound_ = false;
 }
 
-void GLXImageTransportSurface::SetBufferAllocation(
-    BufferAllocationState state) {
-  if (buffer_allocation_state_ == state)
+void GLXImageTransportSurface::SetBackbufferAllocation(bool allocated) {
+  if (backbuffer_suggested_allocation_ == allocated)
     return;
-  buffer_allocation_state_ = state;
+  backbuffer_suggested_allocation_ = allocated;
 
-  switch (state) {
-    case BUFFER_ALLOCATION_FRONT_AND_BACK: {
-      ResizeSurface(size_);
-      break;
-    }
-    case BUFFER_ALLOCATION_FRONT_ONLY: {
-      ResizeSurface(gfx::Size(1,1));
-      break;
-    }
-    case BUFFER_ALLOCATION_NONE: {
-      ResizeSurface(gfx::Size(1,1));
-      if (bound_)
-        ReleaseSurface();
-      break;
-    }
-    default:
-      NOTREACHED();
-  }
+  if (backbuffer_suggested_allocation_)
+    ResizeSurface(size_);
+  else
+    ResizeSurface(gfx::Size(1,1));
+}
+
+void GLXImageTransportSurface::SetFrontbufferAllocation(bool allocated) {
+  if (frontbuffer_suggested_allocation_ == allocated)
+    return;
+  frontbuffer_suggested_allocation_ = allocated;
+
+  // We recreate frontbuffer by recreating backbuffer and swapping.
+  // But we release frontbuffer by telling UI to release its handle on it.
+  if (!frontbuffer_suggested_allocation_ && bound_)
+    ReleaseSurface();
 }
 
 void GLXImageTransportSurface::ResizeSurface(gfx::Size size) {
@@ -656,6 +657,9 @@ void GLXImageTransportSurface::OnResize(gfx::Size size) {
 }
 
 bool GLXImageTransportSurface::SwapBuffers() {
+  DCHECK(backbuffer_suggested_allocation_);
+  if (!frontbuffer_suggested_allocation_)
+    return true;
   gfx::NativeViewGLSurfaceGLX::SwapBuffers();
   helper_->DeferToFence(base::Bind(
       &GLXImageTransportSurface::SendBuffersSwapped,
@@ -682,6 +686,9 @@ void GLXImageTransportSurface::SendBuffersSwapped() {
 
 bool GLXImageTransportSurface::PostSubBuffer(
     int x, int y, int width, int height) {
+  DCHECK(backbuffer_suggested_allocation_);
+  if (!frontbuffer_suggested_allocation_)
+    return true;
   gfx::NativeViewGLSurfaceGLX::PostSubBuffer(x, y, width, height);
   helper_->DeferToFence(base::Bind(
       &GLXImageTransportSurface::SendPostSubBuffer,