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 90341b5b0c25d83a8289dd37413aaaf76fbf8f53..3ae8c2893fb4a6ef2605ef3e76a4467f7c4e20d2 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
@@ -109,9 +110,11 @@ class EGLImageTransportSurface
   void ReleaseSurface(scoped_refptr<EGLAcceleratedSurface>* surface);
   void SendBuffersSwapped();
   void SendPostSubBuffer(int x, int y, int width, int height);
+  void AdjustBufferAllocations();
 
   // Tracks the current buffer allocation state.
-  BufferAllocationState buffer_allocation_state_;
+  bool backbuffer_suggested_allocation_;
+  bool frontbuffer_suggested_allocation_;
 
   uint32 fbo_id_;
 
@@ -145,7 +148,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:
@@ -166,9 +170,11 @@ class GLXImageTransportSurface
   void SendPostSubBuffer(int x, int y, int width, int height);
 
   void ResizeSurface(gfx::Size size);
+  void AdjustBufferAllocations();
 
   // 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 +276,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,33 +343,37 @@ 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;
+  AdjustBufferAllocations();
+}
 
-    case BUFFER_ALLOCATION_NONE:
-      if (back_surface_.get() && front_surface_.get())
-        ReleaseSurface(&back_surface_);
-      break;
+void EGLImageTransportSurface::SetFrontbufferAllocation(bool allocated) {
+  if (frontbuffer_suggested_allocation_ == allocated)
+    return;
+  frontbuffer_suggested_allocation_ = allocated;
+  AdjustBufferAllocations();
+}
 
-    default:
-      NOTREACHED();
+void EGLImageTransportSurface::AdjustBufferAllocations() {
+  if (backbuffer_suggested_allocation_) {
+    if (back_surface_.get())
+      return;
+    if (!front_surface_.get())
+      return;
+    OnResize(front_surface_->size());

[jonathan.backer, 2012/05/09 18:13:01]

AFAICT, we're only managing the backbuffer here. So why are we calling it when
the frontbuffer allocation changes?

The only reason we need a front_surface_ to create a back_surface_ is to get a
size. Maybe we should keep this state separately to simplify the code?

If not, maybe some of these early exits should become DCHECKS so that it's
easier to reason about.

[mmocny, 2012/05/09 18:59:15]

Yes, we need front only to read size, and we should just be caching it.  I was
attempting to limit drive by fixme's and just replicate existing functionality,
but this is probably worth the fix, especially given the next comment..

As for why this gets called on frontbuffer allocation change:  Just for
consistency with the other paths.  It is a noop for now, but that may change.

On 2012/05/09 18:13:01, jonathan.backer wrote:

+  } else {
+    if (!back_surface_.get())
+      return;
+    if (!front_surface_.get())
+      return;

[jonathan.backer, 2012/05/09 18:13:01]

Why do we exit early here? Perhaps we should never be in this state (in which
case a DCHECK is appropriate). But I don't understand early exit.

[mmocny, 2012/05/09 18:59:15]

Its possible that this is testing for a case that isn't expected and a DCHECK
would suffice.  This is only needed because we expect a front when we recreate,
and if we just cache the size that wont be needed.

On 2012/05/09 18:13:01, jonathan.backer wrote:

+    ReleaseSurface(&back_surface_);
   }
 }
 
+
 void EGLImageTransportSurface::ReleaseSurface(
     scoped_refptr<EGLAcceleratedSurface>* surface) {
   if (surface->get()) {
@@ -399,6 +410,8 @@ void EGLImageTransportSurface::OnResize(gfx::Size size) {
 }
 
 bool EGLImageTransportSurface::SwapBuffers() {
+  DCHECK(backbuffer_suggested_allocation_);
+  DCHECK(frontbuffer_suggested_allocation_);
   front_surface_.swap(back_surface_);
   DCHECK_NE(front_surface_.get(), static_cast<EGLAcceleratedSurface*>(NULL));
   helper_->DeferToFence(base::Bind(
@@ -428,7 +441,8 @@ void EGLImageTransportSurface::SendBuffersSwapped() {
 
 bool EGLImageTransportSurface::PostSubBuffer(
     int x, int y, int width, int height) {
-
+  DCHECK(backbuffer_suggested_allocation_);
+  DCHECK(frontbuffer_suggested_allocation_);
   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 +541,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,29 +626,27 @@ 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;
+  AdjustBufferAllocations();
+}
 
-  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();
+void GLXImageTransportSurface::SetFrontbufferAllocation(bool allocated) {
+  if (frontbuffer_suggested_allocation_ == allocated)
+    return;
+  frontbuffer_suggested_allocation_ = allocated;
+  AdjustBufferAllocations();
+}
+
+void GLXImageTransportSurface::AdjustBufferAllocations() {
+  if (backbuffer_suggested_allocation_) {
+    ResizeSurface(size_);
+  } else {
+    ResizeSurface(gfx::Size(1,1));
+    if (!frontbuffer_suggested_allocation_ && bound_)
+      ReleaseSurface();
   }
 }
 
@@ -656,6 +669,8 @@ void GLXImageTransportSurface::OnResize(gfx::Size size) {
 }
 
 bool GLXImageTransportSurface::SwapBuffers() {
+  DCHECK(backbuffer_suggested_allocation_);
+  DCHECK(frontbuffer_suggested_allocation_);
   gfx::NativeViewGLSurfaceGLX::SwapBuffers();
   helper_->DeferToFence(base::Bind(
       &GLXImageTransportSurface::SendBuffersSwapped,
@@ -682,6 +697,8 @@ void GLXImageTransportSurface::SendBuffersSwapped() {
 
 bool GLXImageTransportSurface::PostSubBuffer(
     int x, int y, int width, int height) {
+  DCHECK(backbuffer_suggested_allocation_);
+  DCHECK(frontbuffer_suggested_allocation_);
   gfx::NativeViewGLSurfaceGLX::PostSubBuffer(x, y, width, height);
   helper_->DeferToFence(base::Bind(
       &GLXImageTransportSurface::SendPostSubBuffer,