Drop frontbuffers with ui-use-gpu-process, synchronized with browser, decoupled from backbuffer dro…

content/common/gpu/texture_image_transport_surface.cc

Index: content/common/gpu/texture_image_transport_surface.cc
diff --git a/content/common/gpu/texture_image_transport_surface.cc b/content/common/gpu/texture_image_transport_surface.cc
index de9d3357c228ed844581e5710786f2b608f67197..945b2493ac41f2e0f10768ae7de19993220f4fcb 100644
--- a/content/common/gpu/texture_image_transport_surface.cc
+++ b/content/common/gpu/texture_image_transport_surface.cc
@@ -137,7 +137,7 @@ bool TextureImageTransportSurface::OnMakeCurrent(gfx::GLContext* context) {
   if (!fbo_id_) {
     glGenFramebuffersEXT(1, &fbo_id_);
     glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_id_);
-    CreateBackTexture(gfx::Size(1, 1));
+    CreateTexture(back(), gfx::Size(1, 1));
 
 #ifndef NDEBUG
     GLenum status = glCheckFramebufferStatusEXT(GL_FRAMEBUFFER);
@@ -155,19 +155,34 @@ unsigned int TextureImageTransportSurface::GetBackingFrameBufferObject() {
   return fbo_id_;
 }
 
-void TextureImageTransportSurface::SetBufferAllocation(
-    BufferAllocationState state) {
+void TextureImageTransportSurface::SetBackbufferAllocation(bool allocation) {
+  DCHECK(textures_[back()].info);
+  if (static_cast<bool>(textures_[back()].info->service_id()) == allocation &&

[piman, 2012/05/04 00:03:49]

nit: use !! (or != 0) instead of static_cast<bool>

[mmocny, 2012/05/04 18:55:52]

Thanks.

On 2012/05/04 00:03:49, piman wrote:

+      textures_[back()].suggested_allocation == allocation)
+    return;
+  if (!helper_->MakeCurrent())
+    return;
+  if (allocation) {
+    CreateTexture(back(), textures_[back()].size);
+  } else {
+    textures_[back()].suggested_allocation = false;
+    ReleaseTexture(back());
+  }
+}
+
+void TextureImageTransportSurface::SetFrontbufferAllocation(bool allocation) {
+  DCHECK(textures_[front()].info);
+  if (static_cast<bool>(textures_[front()].info->service_id()) ==

[piman, 2012/05/04 00:03:49]

nit: use !! (or != 0) instead of static_cast<bool>

+      allocation && textures_[front()].suggested_allocation == allocation)
+    return;
   if (!helper_->MakeCurrent())
     return;

[piman, 2012/05/04 00:03:49]

should we set textures_[front()].suggested_allocation=allocation here?

[mmocny, 2012/05/04 18:55:52]

No -- hopefully my previous explanation of what this means helps clear things
up.  Upcoming patch should be a lot nicer.

On 2012/05/04 00:03:49, piman wrote:

-  switch (state) {
-    case BUFFER_ALLOCATION_FRONT_AND_BACK:
-      CreateBackTexture(textures_[back()].size);
-      break;
-    case BUFFER_ALLOCATION_FRONT_ONLY:
-    case BUFFER_ALLOCATION_NONE:
-      ReleaseBackTexture();
-      break;
-  };
+  // Note: we don't actually allocate frontbuffer now, we just fiddle the
+  // right state so that it is reallocated during the next swap.
+  if (allocation)
+    textures_[front()].size = gfx::Size();
+  else
+    RequestReleaseTexture(front());
 }
 
 void* TextureImageTransportSurface::GetShareHandle() {
@@ -183,7 +198,7 @@ void* TextureImageTransportSurface::GetConfig() {
 }
 
 void TextureImageTransportSurface::OnResize(gfx::Size size) {
-  CreateBackTexture(size);
+  CreateTexture(back(), size);
 }
 
 void TextureImageTransportSurface::OnWillDestroyStub(
@@ -203,19 +218,25 @@ void TextureImageTransportSurface::OnWillDestroyStub(
 }
 
 bool TextureImageTransportSurface::SwapBuffers() {
+  DCHECK(textures_[back()].info);
+  DCHECK(textures_[back()].info->service_id());
   if (!parent_stub_) {
     LOG(ERROR) << "SwapBuffers failed because no parent stub.";
     return false;
   }
+  DCHECK(textures_[front()].info);
+  if (!textures_[front()].info->service_id() &&
+      textures_[front()].size != gfx::Size())
+    return true;
 
   glFlush();
   front_ = back();
-  previous_damage_rect_ = gfx::Rect(textures_[front_].size);
+  previous_damage_rect_ = gfx::Rect(textures_[front()].size);
 
-  DCHECK(textures_[front_].client_id != 0);
+  DCHECK(textures_[front()].client_id != 0);
 
   GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params params;
-  params.surface_handle = textures_[front_].client_id;
+  params.surface_handle = textures_[front()].client_id;
   helper_->SendAcceleratedSurfaceBuffersSwapped(params);
   helper_->SetScheduled(false);
   return true;
@@ -223,19 +244,16 @@ bool TextureImageTransportSurface::SwapBuffers() {
 
 bool TextureImageTransportSurface::PostSubBuffer(
     int x, int y, int width, int height) {
+  DCHECK(textures_[back()].info);
+  DCHECK(textures_[back()].info->service_id());
   if (!parent_stub_) {
     LOG(ERROR) << "PostSubBuffer failed because no parent stub.";
     return false;
   }
-
-  DCHECK(textures_[back()].info);
-  int back_texture_service_id = textures_[back()].info->service_id();
-
-  DCHECK(textures_[front_].info);
-  int front_texture_service_id = textures_[front_].info->service_id();
-
-  gfx::Size expected_size = textures_[back()].size;
-  bool surfaces_same_size = textures_[front_].size == expected_size;
+  DCHECK(textures_[front()].info);
+  if (!textures_[front()].info->service_id() &&
+      textures_[front()].size != gfx::Size())
+    return true;
 
   const gfx::Rect new_damage_rect(x, y, width, height);
 
@@ -243,6 +261,12 @@ bool TextureImageTransportSurface::PostSubBuffer(
   if (new_damage_rect.IsEmpty())
     return true;
 
+  int back_texture_service_id = textures_[back()].info->service_id();
+  int front_texture_service_id = textures_[front()].info->service_id();
+
+  gfx::Size expected_size = textures_[back()].size;
+  bool surfaces_same_size = textures_[front()].size == expected_size;
+
   if (surfaces_same_size) {
     std::vector<gfx::Rect> regions_to_copy;
     GetRegionsToCopy(previous_damage_rect_, new_damage_rect, &regions_to_copy);
@@ -269,17 +293,18 @@ bool TextureImageTransportSurface::PostSubBuffer(
 
   glFlush();
   front_ = back();
+  previous_damage_rect_ = new_damage_rect;
+
+  DCHECK(textures_[front()].client_id != 0);
 
   GpuHostMsg_AcceleratedSurfacePostSubBuffer_Params params;
-  params.surface_handle = textures_[front_].client_id;
+  params.surface_handle = textures_[front()].client_id;
   params.x = x;
   params.y = y;
   params.width = width;
   params.height = height;
   helper_->SendAcceleratedSurfacePostSubBuffer(params);
   helper_->SetScheduled(false);
-
-  previous_damage_rect_ = new_damage_rect;
   return true;
 }
 
@@ -304,12 +329,18 @@ void TextureImageTransportSurface::OnNewSurfaceACK(
     uint64 surface_handle, TransportDIB::Handle /*shm_handle*/) {
 }
 
+void TextureImageTransportSurface::OnDiscardSurface(
+    uint64 surface_id) {

[piman, 2012/05/04 00:03:49]

Why do we need the id here? This is called by the UI. The UI only cares about
the front buffer.
When the UI calls this, it should be in a state where it will not use any of the
buffers. At that point, we could release whichever is the front (unless the
allocation has changed since then).

TBH I'm having a very hard time following the logic in this file regarding
allocation, especially front buffer.
SetFrontbufferAllocation, OnDiscardSurface and OnBuffersSwappedACK all can
happen in virtually arbitrary time/order relative to the rendering commands that
also affect allocation (mainly SwapBuffers/PostSubBuffers), so the state matrix
can be very big if not done properly, so it's super hard to reason about. Many
of the states are transient, but they exist and must be handled. Right now
there's few comments and no test.

The way I like to reason about it is this way: we have 2 reasons to have a front
buffer. We need to have a front buffer if either is true:
1- the browser says it needs a front buffer.
2- the renderer says it's visible (which I guess comes through
SetFrontbufferAllocation).

In steady state, 1 and 2 are the same, but transitions occur and during that
time they are not. You want to actually drop the front buffer when both of them
become false. While they are both false, any swap/post can be dropped to the
floor. When either one is true, the next swap/post will have regenerated the new
front buffer (that we can then send to the browser).

Right now, (1) is only one-sided (OnDiscardSurface), so we sort-of rely on the
fact that the renderer will create a new frame when the browser becomes visible
again. But it's not an explicit state, so it's hard to reason about. In
particular I don't know how to verify that when switching tabs rapidly, the
OnDiscardSurface won't come before a new frame that got kicked but is not
relevant any more (in which case we'll keep the front buffer, even though we
should really discard it).

I'd much rather see something like:
- a message from the browser indicating whether it wants a surface or not (i.e.
add a bool to the Discard message or something)
- 2 bools here: front_buffer_suggested_allocation, browser_needs_front_buffer
- in OnDiscardSurface (or whatever we rename it to) we set
browser_needs_front_buffer
- in SetFrontbufferAllocation we set front_buffer_suggested_allocation
- in both of them, we handle the transition if (browser_needs_front_buffer ||
front_buffer_suggested_allocation) changes. If it goes from true to false, we
drop the front buffer. If it goes from from false to true, there's nothing to do
yet until the next Swap/Post
- in Swap or Post, if (browser_needs_front_buffer ||
front_buffer_suggested_allocation) is false, we do nothing (i.e. we keep the
back buffer, the front buffer should already have been dropped). If it's true,
we send the Swap/Post to the browser, with the new id.

That's pretty much it (for the front buffer). The buffer we allocate is always
the back one (on Resize or on the Swap ACK), and that's already handled.
Whatever the order between OnDiscardSurface/SetFrontbufferAllocation, we can
verify we do the right thing.

[mmocny, 2012/05/04 18:55:52]

Originally, this was here to protect from releasing the wrong surface after a
swap.

After some cleanup and better control of state, this is no longer needed, and
I've removed the id.
Yes, exactly, I now make sure the allocation hasn't changed.
Sorry, you are correct, this is not very easy to follow or reason about.

I'm working to clean this up.
I don't understand how being explit about (1) could be useful?  I thought that
the ui does this implicitly by setting via visibility, and waiting for (2) to
happen.  visibility == true is equivalent to browser needs front buffer -- is it
possible this will change?

If the problem is that going through the renderer is too slow/not trustworthy to
relay visibility information, there is already an outstanding issue to have
browser/gpu communicate it directly (its good for other reasons as well).
If I understand correct, you agree that conceptually they both change at the
same time, and the only reason they get "out of sync" is because we have to
communicate the messages.

I think adding a new message for (1) will be equivalent to sending visibility to
gpu process directly (which will set FrontbufferAllocation and thus protect the
frontbuffer).
Yes, we do rely on this.  This is not related to frontbuffer protection
correctness.  Even if that is 100% clear and correct we still rely on renderer
pushing a frame when switching back to the tab.  I think this is currently
guaranteed, but its not explicitly requested by the ui.
If this is possible, that frontbuffer will still be cleared with the next Manage
call, so long as  global state does not get out of sync, memory will eventually
be reclaimed.
I will reason about this a bit longer to make sure.
Sending visibility directly to gpu would solve the same problem, if (visible ==
browser_needs_front_buffer).

+  ReleaseTexture(textures_[front()].client_id == surface_id ?
+                 front() : back());
+}
+
 void TextureImageTransportSurface::OnBuffersSwappedACK() {
   if (helper_->MakeCurrent()) {
-    if (textures_[front_].size != textures_[back()].size) {
-      CreateBackTexture(textures_[front_].size);
+    if (textures_[front()].size != textures_[back()].size) {
+      CreateTexture(back(), textures_[front()].size);
     } else {
-      AttachBackTextureToFBO();
+      AttachTextureToFBO(back());
     }
   }
 
@@ -326,36 +357,70 @@ void TextureImageTransportSurface::OnResizeViewACK() {
   NOTREACHED();
 }
 
-void TextureImageTransportSurface::ReleaseBackTexture() {
+void TextureImageTransportSurface::RequestReleaseTexture(int id) {
+  Texture& texture = textures_[id];
+  texture.suggested_allocation = false;
+  if (!texture.sent_to_client) {
+    ReleaseTexture(id);
+    return;
+  }
+  GpuHostMsg_AcceleratedSurfaceSuggestDiscard_Params params;
+  params.identifier = texture.client_id;
+  helper_->SendAcceleratedSurfaceSuggestDiscard(params);
+}
+
+void TextureImageTransportSurface::ReleaseTexture(int id) {
   if (!parent_stub_)
     return;
-  TextureInfo* info = textures_[back()].info;
+  Texture& texture = textures_[id];
+  TextureInfo* info = texture.info;
   DCHECK(info);
 
+  texture.sent_to_client = false;
+  if (texture.suggested_allocation)
+    return;
   GLuint service_id = info->service_id();
   if (!service_id)
     return;
   info->SetServiceId(0);
 
+
   {
     ScopedFrameBufferBinder fbo_binder(fbo_id_);
     glDeleteTextures(1, &service_id);
   }
   glFlush();
   CHECK_GL_ERROR();
+
+  if (id == front())
+    previous_damage_rect_ = gfx::Rect();
 }
 
-void TextureImageTransportSurface::CreateBackTexture(const gfx::Size& size) {
+void TextureImageTransportSurface::CreateTexture(
+    int id, const gfx::Size& size) {

[piman, 2012/05/04 00:03:49]

AFAICT you only call CreateTexture(back(), ...), which makes sense actually. Can
we leave it as CreateBackTexture to avoid improper usage?

[mmocny, 2012/05/04 18:55:52]

done.
On 2012/05/04 00:03:49, piman wrote:

   if (!parent_stub_)
     return;
-  Texture& texture = textures_[back()];
+  Texture& texture = textures_[id];
   TextureInfo* info = texture.info;
   DCHECK(info);
 
   GLuint service_id = info->service_id();
 
-  if (service_id && texture.size == size)
+  // If the suggested allocation is true, and we are already allocated and the
+  // right size, we can early exit.
+  if (service_id &&
+      texture.size == size &&
+      texture.suggested_allocation) {
+    DCHECK(texture.sent_to_client);
     return;
+  }
+  // If the suggested allocation is false, that means we've sent a suggestion to
+  // release surface, and may possibly soon get an ack to actually release, yet
+  // we now we have a request to create, and would thus like to ignore that
+  // release.  We do this by resettign suggested_allocation and sending an

[piman, 2012/05/04 00:03:49]

typo: resettign->resetting

[mmocny, 2012/05/04 18:55:52]

done.
On 2012/05/04 00:03:49, piman wrote:

+  // AcceleratedSurfaceNew to give the ui access to the surface again,
+  // though me may not need to generate textures or resize.
+  texture.suggested_allocation = true;
 
   if (!service_id) {
     glGenTextures(1, &service_id);
@@ -392,7 +457,7 @@ void TextureImageTransportSurface::CreateBackTexture(const gfx::Size& size) {
     CHECK_GL_ERROR();
   }
 
-  AttachBackTextureToFBO();
+  AttachTextureToFBO(id);
 
   GpuHostMsg_AcceleratedSurfaceNew_Params params;
   params.width = size.width();
@@ -402,16 +467,17 @@ void TextureImageTransportSurface::CreateBackTexture(const gfx::Size& size) {
   texture.sent_to_client = true;
 }
 
-void TextureImageTransportSurface::AttachBackTextureToFBO() {
+void TextureImageTransportSurface::AttachTextureToFBO(int id) {

[piman, 2012/05/04 00:03:49]

Having an id here smells fishy to me. Why would we attach anything but the back
buffer to the FBO?
AFAICT This is only called through AttachTextureToFBO(back()) or CreateTexture
which is only called for back() (see above).

[mmocny, 2012/05/04 18:55:52]

Yes, this was just a result of CreateTexture taking an id, and can change back
now.
On 2012/05/04 00:03:49, piman wrote:

   if (!parent_stub_)
     return;
-  DCHECK(textures_[back()].info);
+  TextureInfo* info = textures_[id].info;
+  DCHECK(textures_[id].info);
 
   ScopedFrameBufferBinder fbo_binder(fbo_id_);
   glFramebufferTexture2DEXT(GL_FRAMEBUFFER,
       GL_COLOR_ATTACHMENT0,
       GL_TEXTURE_2D,
-      textures_[back()].info->service_id(),
+      info->service_id(),
       0);
   glFlush();
   CHECK_GL_ERROR();