Implement TextureImageTransportSurface using texture mailbox

content/browser/renderer_host/render_widget_host_view_aura.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_view_aura.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 259 matching lines @@
       in_shutdown_(false),
       is_fullscreen_(false),
       popup_parent_host_view_(NULL),
       popup_child_host_view_(NULL),
       is_loading_(false),
       text_input_type_(ui::TEXT_INPUT_TYPE_NONE),
       can_compose_inline_(true),
       has_composition_text_(false),
       device_scale_factor_(1.0f),
       current_surface_(0),
-      current_surface_is_protected_(true),
-      current_surface_in_use_by_compositor_(true),
-      protection_state_id_(0),
-      surface_route_id_(0),
       paint_canvas_(NULL),
       synthetic_move_sent_(false),
       accelerated_compositing_state_changed_(false),
       can_lock_compositor_(YES) {
   host_->SetView(this);
   window_observer_.reset(new WindowObserver(this));
   window_->AddObserver(window_observer_.get());
   aura::client::SetTooltipText(window_, &tooltip_);
   aura::client::SetActivationDelegate(window_, this);
 }
@@ skipping 66 matching lines @@
 void RenderWidgetHostViewAura::WasShown() {
   if (!host_->is_hidden())
     return;
   host_->WasShown();
 
   if (!current_surface_ && host_->is_accelerated_compositing_active() &&
       !released_front_lock_.get()) {
     released_front_lock_ = GetCompositor()->GetCompositorLock();
   }
 
-  AdjustSurfaceProtection();
-
 #if defined(OS_WIN)
   LPARAM lparam = reinterpret_cast<LPARAM>(this);
   EnumChildWindows(ui::GetHiddenWindow(), ShowWindowsCallback, lparam);
 #endif
 }
 
 void RenderWidgetHostViewAura::WasHidden() {
   if (host_->is_hidden())
     return;
   host_->WasHidden();
 
   released_front_lock_ = NULL;
 
-  if (ShouldReleaseFrontSurface() &&
-      host_->is_accelerated_compositing_active()) {
-    current_surface_ = 0;
-    UpdateExternalTexture();
-  }
-
-  AdjustSurfaceProtection();
-
 #if defined(OS_WIN)
   aura::RootWindow* root_window = window_->GetRootWindow();
   if (root_window) {
     HWND parent = root_window->GetAcceleratedWidget();
     LPARAM lparam = reinterpret_cast<LPARAM>(this);
 
     EnumChildWindows(parent, HideWindowsCallback, lparam);
   }
 #endif
 }
@@ skipping 268 matching lines @@
     const gfx::Size& dst_size,
     const base::Callback<void(bool)>& callback,
     skia::PlatformBitmap* output) {
   base::ScopedClosureRunner scoped_callback_runner(base::Bind(callback, false));
 
   std::map<uint64, scoped_refptr<ui::Texture> >::iterator it =
       image_transport_clients_.find(current_surface_);
   if (it == image_transport_clients_.end())
     return;
 
-  ui::Texture* container = it->second;
+  scoped_refptr<ui::Texture> container = it->second;
   DCHECK(container);
 
   gfx::Size dst_size_in_pixel = ConvertSizeToPixel(this, dst_size);
   if (!output->Allocate(
       dst_size_in_pixel.width(), dst_size_in_pixel.height(), true))
     return;
 
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   GLHelper* gl_helper = factory->GetGLHelper();
   if (!gl_helper)
     return;
 
   unsigned char* addr = static_cast<unsigned char*>(
       output->GetBitmap().getPixels());
   scoped_callback_runner.Release();
   // Wrap the callback with an internal handler so that we can inject our
-  // own completion handlers (where we can call AdjustSurfaceProtection).
+  // own completion handlers (where we can try to free the frontbuffer).
   base::Callback<void(bool)> wrapper_callback = base::Bind(
       &RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinished,
       AsWeakPtr(),
-      callback);
+      callback,
+      container); // Hold a reference to the texture

[piman, 2012/11/09 22:02:01]

I think we don't need this. By the time CropScaleReadbackAndCleanTexture
returns, the texture won't be used any more (it internally copies/scales it
before running the asynchronous part).
I think the thumbnailer-related asynchronous code is overly conservative and can
be simplified.

[no sievers, 2012/11/19 20:30:44]

Right it does the copy first on the UI thread. Removed.

   ++pending_thumbnail_tasks_;
 
   // Convert |src_subrect| from the views coordinate (upper-left origin) into
   // the OpenGL coordinate (lower-left origin).
   gfx::Rect src_subrect_in_gl = src_subrect;
   src_subrect_in_gl.set_y(GetViewBounds().height() - src_subrect.bottom());
 
   gfx::Rect src_subrect_in_pixel = ConvertRectToPixel(this, src_subrect_in_gl);
   gl_helper->CropScaleReadbackAndCleanTexture(container->PrepareTexture(),
                                               container->size(),
                                               src_subrect_in_pixel,
                                               dst_size_in_pixel,
                                               addr,
                                               wrapper_callback);
 }
 
 void RenderWidgetHostViewAura::OnAcceleratedCompositingStateChange() {
   // Delay processing the state change until we either get a software frame if
   // switching to software mode or receive a buffers swapped notification
   // if switching to accelerated mode.
   // Sometimes (e.g. on a page load) the renderer will spuriously disable then
   // re-enable accelerated compositing, causing us to flash.
   // TODO(piman): factor the enable/disable accelerated compositing message into
   // the UpdateRect/AcceleratedSurfaceBuffersSwapped messages so that we have
   // fewer inconsistent temporary states.
   accelerated_compositing_state_changed_ = true;
 }
 
-bool RenderWidgetHostViewAura::ShouldFastACK(uint64 surface_id) {
-  ui::Texture* container = image_transport_clients_[surface_id];
-  DCHECK(container);
-
+bool RenderWidgetHostViewAura::ShouldFastACK(const gfx::Size& size) {
   if (can_lock_compositor_ == NO_PENDING_RENDERER_FRAME ||
       can_lock_compositor_ == NO_PENDING_COMMIT ||
       resize_locks_.empty())
     return false;
 
-  gfx::Size container_size = ConvertSizeToDIP(this, container->size());
+  gfx::Size container_size = ConvertSizeToDIP(this, size);
   ResizeLockList::iterator it = resize_locks_.begin();
   while (it != resize_locks_.end()) {
     if ((*it)->expected_size() == container_size)
       break;
     ++it;
   }
 
   // We could be getting an unexpected frame due to an animation
   // (i.e. we start resizing but we get an old size frame first).
   return it == resize_locks_.end() || ++it != resize_locks_.end();
 }
 
 void RenderWidgetHostViewAura::UpdateExternalTexture() {
   // Delay processing accelerated compositing state change till here where we
   // act upon the state change. (Clear the external texture if switching to
   // software mode or set the external texture if going to accelerated mode).
   if (accelerated_compositing_state_changed_)
     accelerated_compositing_state_changed_ = false;
 
   if (current_surface_ != 0 && host_->is_accelerated_compositing_active()) {
     ui::Texture* container = image_transport_clients_[current_surface_];
     window_->SetExternalTexture(container);
-    current_surface_in_use_by_compositor_ = true;
 
     if (!container) {
       resize_locks_.clear();
     } else {
       ResizeLockList::iterator it = resize_locks_.begin();
       while (it != resize_locks_.end()) {
         gfx::Size container_size = ConvertSizeToDIP(this,
             container->size());
         if ((*it)->expected_size() == container_size)
           break;
@@ skipping 15 matching lines @@
             it2->get()->UnlockCompositor();
           }
           if (!compositor->HasObserver(this))
             compositor->AddObserver(this);
         }
         resize_locks_.erase(resize_locks_.begin(), it);
       }
     }
   } else {
     window_->SetExternalTexture(NULL);
-    if (ShouldReleaseFrontSurface() &&
-        host_->is_accelerated_compositing_active()) {
-      // We need to wait for a commit to clear to guarantee that all we
-      // will not issue any more GL referencing the previous surface.
-      ui::Compositor* compositor = GetCompositor();
-      if (compositor) {
-        can_lock_compositor_ = NO_PENDING_COMMIT;
-        on_compositing_did_commit_callbacks_.push_back(
-            base::Bind(&RenderWidgetHostViewAura::
-                           SetSurfaceNotInUseByCompositor,
-                       AsWeakPtr()));
-        if (!compositor->HasObserver(this))
-          compositor->AddObserver(this);
-      }
-    }
     resize_locks_.clear();
   }
 }
 
+void RenderWidgetHostViewAura::SwapBuffersCompleted(
+    int gpu_host_id,
+    int route_id,
+    uint64 surface_handle,
+    scoped_refptr<ui::Texture> texture) {
+  ui::Compositor* compositor = GetCompositor();
+  if (!compositor) {
+    InsertSyncPointAndACK(route_id, gpu_host_id, surface_handle, texture);
+  } else {
+    // Add sending an ACK to the list of things to do OnCompositingDidCommit
+    can_lock_compositor_ = NO_PENDING_COMMIT;
+    on_compositing_did_commit_callbacks_.push_back(
+        base::Bind(&RenderWidgetHostViewAura::InsertSyncPointAndACK,
+                   route_id,
+                   gpu_host_id,
+                   surface_handle,
+                   texture));
+    if (!compositor->HasObserver(this))
+      compositor->AddObserver(this);
+  }
+}
+
 void RenderWidgetHostViewAura::AcceleratedSurfaceBuffersSwapped(
     const GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params& params_in_pixel,
     int gpu_host_id) {
-  surface_route_id_ = params_in_pixel.route_id;
-  // If protection state changed, then this swap is stale. We must still ACK but
-  // do not update current_surface_ since it may have been discarded.
-  if (params_in_pixel.protection_state_id &&
-      params_in_pixel.protection_state_id != protection_state_id_) {
-    DCHECK(!current_surface_);
-    if (!params_in_pixel.skip_ack)
-      InsertSyncPointAndACK(params_in_pixel.route_id, gpu_host_id, false, NULL);
+  const gfx::Rect surface_rect = gfx::Rect(gfx::Point(), params_in_pixel.size);
+  if (ShouldFastACK(params_in_pixel.size)) {

[jonathan.backer, 2012/11/12 16:52:15]

I'm torn as to handle the detached (no compositor state). The decision on how to
handle it was made before the current implementation of fast-ACKing (when the UI
compositor was single-threaded).

It's clear from RWHVA::WasShown that we're expecting a new frame to come in when
a tab was made visible (we're reattached). Here are the advantages of fast-ACK
when detached:
- no copy on PostSubBuffer
- no insertion of SyncPoint
- no commit (in threaded case)

WDYT?

[no sievers, 2012/11/19 20:30:44]

When exactly would this happen?
It sounds though like it would be harmless then to skip the frame also when
!compositor?

On 2012/11/12 16:52:15, jonathan.backer wrote:

[piman, 2012/11/19 22:09:52]

When detached, there'll be no commit either since you don't have a compositor to
commit to.

On tab switch, prior to this patch, we have the case where we don't wait for the
renderer to redraw everything (including uploading textures), where the GPU
process had kept the front buffer valid, and sends it immediately. It's critical
for fast tab switch.
On this new version, it requires us to handle the swaps coming from the GPU
process, even when not in the tree.

[piman, 2012/11/19 22:31:54]

GetCompositor() walks the window tree to find the compositor. If it's not in the
tree, it'll return NULL, but we still need to handle updates for when we get
back into the tree. I'm not 100% sure which UI operations exactly cause this to
happen, but they happen at a higher level. Among others, I believe instant and
prerender do this. Also, we can't strongly associate a RWHVA with a compositor
because of multiple screens.

At a high level, I've been considering this:
1- if we have a compositor, changing textures has some implications to that
compositor: it needs to know which area to redraw, and we need to synchronize
with the rendering logic (on another thread or not) so that we don't modify the
texture that is rendered from.
2- whether we have a compositor or not, the RWHVA still needs to keep track of
the renderer state, whether coming from the renderer or the GPU process on
behalf of it. That state includes the "front buffer", i.e. which texture it is,
and whether we need to copy dirty rects (with this patch).

I think executing the parts in (2) only when we have a compositor would be
wrong.

+    skipped_damage_ = UnionRects(skipped_damage_, surface_rect);
+    InsertSyncPointAndACK(params_in_pixel.route_id,
+                          gpu_host_id,
+                          params_in_pixel.surface_handle,
+                          NULL);
     return;
   }
 
-  if (ShouldFastACK(params_in_pixel.surface_handle)) {
-    if (!params_in_pixel.skip_ack)
-      InsertSyncPointAndACK(params_in_pixel.route_id, gpu_host_id, false, NULL);
-    return;
+  const uint64 previous_surface = current_surface_;
+  scoped_refptr<ui::Texture> texture_to_return;
+  if (previous_surface)
+    texture_to_return = image_transport_clients_[previous_surface];
+
+  current_surface_ = params_in_pixel.surface_handle;
+  DCHECK(image_transport_clients_.find(params_in_pixel.surface_handle) !=
+      image_transport_clients_.end());
+
+  image_transport_clients_[current_surface_]->Consume(params_in_pixel.size);
+  released_front_lock_ = NULL;
+  UpdateExternalTexture();
+
+  previous_damage_rect_ = surface_rect;
+  if (!skipped_damage_.IsEmpty())

[piman, 2012/11/09 22:02:01]

The test isn't useful, since you always end up with the empty rect in either
case.

[no sievers, 2012/11/19 20:30:44]

Done.

+    skipped_damage_ = gfx::Rect();
+
+  ui::Compositor* compositor = GetCompositor();
+  if (compositor) {
+    gfx::Size surface_size = ConvertSizeToDIP(this, params_in_pixel.size);
+    window_->SchedulePaintInRect(gfx::Rect(surface_size));
   }
 
-  current_surface_ = params_in_pixel.surface_handle;
-  // If we don't require an ACK that means the content is not a fresh updated
-  // new frame, rather we are just resetting our handle to some old content
-  // that we still hadn't discarded. Although we could display immediately,
-  // by not resetting the compositor lock here, we give us some time to get
-  // a fresh frame which means fewer content flashes.
-  if (!params_in_pixel.skip_ack)
-    released_front_lock_ = NULL;
-
-  UpdateExternalTexture();
-
-  ui::Compositor* compositor = GetCompositor();
-  if (!compositor) {
-    if (!params_in_pixel.skip_ack)
-      InsertSyncPointAndACK(params_in_pixel.route_id, gpu_host_id, true, NULL);
-  } else {
-    DCHECK(image_transport_clients_.find(params_in_pixel.surface_handle) !=
-           image_transport_clients_.end());
-    gfx::Size surface_size_in_pixel =
-        image_transport_clients_[params_in_pixel.surface_handle]->size();
-    gfx::Size surface_size = ConvertSizeToDIP(this, surface_size_in_pixel);
-    window_->SchedulePaintInRect(gfx::Rect(surface_size));
-
-    if (!params_in_pixel.skip_ack) {
-      // Add sending an ACK to the list of things to do OnCompositingDidCommit
-      can_lock_compositor_ = NO_PENDING_COMMIT;
-      on_compositing_did_commit_callbacks_.push_back(
-          base::Bind(&RenderWidgetHostViewAura::InsertSyncPointAndACK,
-                     params_in_pixel.route_id,
-                     gpu_host_id,
-                     true));
-      if (!compositor->HasObserver(this))
-        compositor->AddObserver(this);
-    }
-  }
+  SwapBuffersCompleted(gpu_host_id,
+                       params_in_pixel.route_id,
+                       previous_surface,
+                       texture_to_return);
 }
 
 void RenderWidgetHostViewAura::AcceleratedSurfacePostSubBuffer(
     const GpuHostMsg_AcceleratedSurfacePostSubBuffer_Params& params_in_pixel,
     int gpu_host_id) {
-  surface_route_id_ = params_in_pixel.route_id;
-  // If visible state changed, then this PSB is stale. We must still ACK but
-  // do not update current_surface_.
-  if (params_in_pixel.protection_state_id &&
-      params_in_pixel.protection_state_id != protection_state_id_) {
-    DCHECK(!current_surface_);
-    InsertSyncPointAndACK(params_in_pixel.route_id, gpu_host_id, false, NULL);
+  const gfx::Rect surface_rect =
+      gfx::Rect(gfx::Point(), params_in_pixel.surface_size);
+  gfx::Rect damage_rect(params_in_pixel.x,
+                        params_in_pixel.y,
+                        params_in_pixel.width,
+                        params_in_pixel.height);
+  if (ShouldFastACK(params_in_pixel.surface_size)) {
+    skipped_damage_ = UnionRects(skipped_damage_, damage_rect);
+    InsertSyncPointAndACK(params_in_pixel.route_id,
+                          gpu_host_id,
+                          params_in_pixel.surface_handle,
+                          NULL);
     return;
   }
 
-  if (ShouldFastACK(params_in_pixel.surface_handle)) {
-    InsertSyncPointAndACK(params_in_pixel.route_id, gpu_host_id, false, NULL);
-    return;
+  const uint64 previous_surface = current_surface_;
+  scoped_refptr<ui::Texture> texture_to_return;
+  if (previous_surface)
+    texture_to_return = image_transport_clients_[previous_surface];
+
+  current_surface_ = params_in_pixel.surface_handle;
+  DCHECK(image_transport_clients_.find(params_in_pixel.surface_handle) !=
+      image_transport_clients_.end());
+
+  scoped_refptr<ui::Texture>& current_texture =

[piman, 2012/11/09 22:02:01]

scoped_refptr<ui::Texture>& -> ui::Texture*? You're not modifying the map so no
need for the extra indirection.

[no sievers, 2012/11/19 20:30:44]

Done.

+      image_transport_clients_[current_surface_];
+  current_texture->Consume(params_in_pixel.surface_size);
+  released_front_lock_ = NULL;
+  UpdateExternalTexture();
+
+  if (!surface_rect.Contains(skipped_damage_)) {
+    // The surface could have shrunk since we skipped an update, in which
+    // case we can expect a full update.
+    DCHECK(damage_rect == surface_rect);

[piman, 2012/11/09 22:02:01]

mmh, you're really asserting something that's coming from a (potentially
malicious) renderer.
What happens if it's not true? Sounds like we only get bad pixels (which is ok
for a malicious renderer). change to a DLOG_IF?

[no sievers, 2012/11/19 20:30:44]

Done.

+    skipped_damage_ = gfx::Rect();
   }
 
-  current_surface_ = params_in_pixel.surface_handle;
-  released_front_lock_ = NULL;
-  DCHECK(current_surface_);
-  UpdateExternalTexture();
+  if (!skipped_damage_.IsEmpty()) {
+    damage_rect = UnionRects(skipped_damage_, damage_rect);

[piman, 2012/11/09 22:02:01]

So the problem here (and when we union the skipped_damage_ on fast acks), is
that we keep the bounding box of the damage instead of the actual rects. So when
we try to copy previous_damage-damage, we may end up forgetting to copy some
pixels  that were not made up-to-date.
Should we make all the damage tracking structs regions?

[jonathan.backer, 2012/11/12 16:52:15]

Good catch.

[no sievers, 2012/11/19 20:30:44]

Done.

+    skipped_damage_ = gfx::Rect();
+  }
 
+  DCHECK(surface_rect.Contains(damage_rect));
   ui::Compositor* compositor = GetCompositor();
-  if (!compositor) {
-    InsertSyncPointAndACK(params_in_pixel.route_id, gpu_host_id, true, NULL);
-  } else {
-    DCHECK(image_transport_clients_.find(params_in_pixel.surface_handle) !=
-           image_transport_clients_.end());
-    gfx::Size surface_size_in_pixel =
-        image_transport_clients_[params_in_pixel.surface_handle]->size();
+  if (compositor) {

[piman, 2012/11/09 22:02:01]

I think you should do this always. At least the copy. You may not have a
compositor if you're not in the tree, but in this case you still want to ensure
that the damage is copied so that when we come back into the tree the texture is
sane.

[jonathan.backer, 2012/11/12 16:52:15]

As I've mentioned above, I have a slight preference for going the other way:
treat the lack of compositor as an opportunity for a fast-ACK.

[no sievers, 2012/11/19 20:30:44]

Done, I let it skip the frame if !GetCompositor(). Anything I should test for
this?

+    const gfx::Size surface_size_in_pixel = params_in_pixel.surface_size;
+    DCHECK(!texture_to_return ||
+           texture_to_return->size() == current_texture->size() ||
+           damage_rect == surface_rect);
+    if (texture_to_return && !previous_damage_rect_.IsEmpty() &&
+        texture_to_return->size() == current_texture->size()) {
+      ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
+      GLHelper* gl_helper = factory->GetGLHelper();
+      gl_helper->CopySubBufferDamage(current_texture->PrepareTexture(),
+                                     texture_to_return->PrepareTexture(),
+                                     damage_rect,
+                                     previous_damage_rect_);

[jonathan.backer, 2012/11/12 16:52:15]

Which context is current here? Maybe it doesn't matter because they are all in
the same share group?

[no sievers, 2012/11/19 20:30:44]

Yes, the thumbnailer also uses the GLHelper and relies on having access to the
frontbuffer texture in the context the helper was initialized in. I put a
flush() in that routine to make sure the copy is issued before the compositor
uses the texture.

[piman, 2012/11/19 22:09:52]

On the client side, makeContextCurrent is essentially a noop after the first one
(which creates the context), so I think we've been sloppy with it. If anything
I'd expect the GLHelper to take care of it.

+    }
+    previous_damage_rect_ = damage_rect;
 
     // Co-ordinates come in OpenGL co-ordinate space.
     // We need to convert to layer space.
     gfx::Rect rect_to_paint = ConvertRectToDIP(this, gfx::Rect(
         params_in_pixel.x,
         surface_size_in_pixel.height() - params_in_pixel.y -
-            params_in_pixel.height,
+        params_in_pixel.height,
         params_in_pixel.width,
         params_in_pixel.height));
 
     // Damage may not have been DIP aligned, so inflate damage to compensate
     // for any round-off error.
     rect_to_paint.Inset(-1, -1);
     rect_to_paint.Intersect(window_->bounds());
 
     window_->SchedulePaintInRect(rect_to_paint);
+  }
 
-    // Add sending an ACK to the list of things to do OnCompositingDidCommit
-    can_lock_compositor_ = NO_PENDING_COMMIT;
-    on_compositing_did_commit_callbacks_.push_back(
-        base::Bind(&RenderWidgetHostViewAura::InsertSyncPointAndACK,
-                   params_in_pixel.route_id,
-                   gpu_host_id,
-                   true));
-    if (!compositor->HasObserver(this))
-      compositor->AddObserver(this);
-  }
+  SwapBuffersCompleted(gpu_host_id,
+                       params_in_pixel.route_id,
+                       previous_surface,
+                       texture_to_return);
 }
 
 void RenderWidgetHostViewAura::AcceleratedSurfaceSuspend() {
 }
 
 bool RenderWidgetHostViewAura::HasAcceleratedSurface(
       const gfx::Size& desired_size) {
   // Aura doesn't use GetBackingStore for accelerated pages, so it doesn't
   // matter what is returned here as GetBackingStore is the only caller of this
   // method. TODO(jbates) implement this if other Aura code needs it.
   return false;
 }
 
 // TODO(backer): Drop the |shm_handle| once I remove some unused service side
 // code.
 void RenderWidgetHostViewAura::AcceleratedSurfaceNew(
       int32 width_in_pixel,
       int32 height_in_pixel,
-      uint64 surface_handle) {
+      uint64 surface_handle,
+      const std::vector<signed char>& mailbox_name) {
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   scoped_refptr<ui::Texture> surface(factory->CreateTransportClient(
       gfx::Size(width_in_pixel, height_in_pixel), device_scale_factor_,
-      surface_handle));
+      mailbox_name));
   if (!surface) {
     LOG(ERROR) << "Failed to create ImageTransport texture";
     return;
   }
-
   image_transport_clients_[surface_handle] = surface;
 }
 
 void RenderWidgetHostViewAura::AcceleratedSurfaceRelease(
     uint64 surface_handle) {
-  DCHECK(image_transport_clients_.find(surface_handle) !=
-         image_transport_clients_.end());
-  if (current_surface_ == surface_handle) {
+  // This really tells us to release the frontbuffer.
+  if (current_surface_ && ShouldReleaseFrontSurface() &&

[piman, 2012/11/09 22:02:01]

We should only do this if current_surface_==surface_handle

[no sievers, 2012/11/19 20:30:44]

Well, I guess I changed the semantics of the message now to indicate that we
should drop the frontbuffer. I'm not sure if it's necessary for the surface to
tell RWHVA when it freed the backbuffer, because the actual GL texture is owned
by the surface and the 'other' texture here should not refer to any real GL
texture (with the exception of the transition - I probably assumed that the
surface would not send a release message while an ack is pending, but maybe I
have to revisit that assumption and see if it's a problem).

+      host_->is_accelerated_compositing_active()) {

[piman, 2012/11/09 22:02:01]

Why is this test needed? Note, it's almost positively racy (it's a state that
comes from the renderer whereas AcceleratedSurfaceRelease comes from the GPU
process).

[no sievers, 2012/11/19 20:30:44]

Done.

+    ui::Compositor* compositor = GetCompositor();
+    if (compositor) {
+      // We need to wait for a commit to clear to guarantee that all we
+      // will not issue any more GL referencing the previous surface.
+      can_lock_compositor_ = NO_PENDING_COMMIT;
+      scoped_refptr<ui::Texture> surface_ref =
+          image_transport_clients_[surface_handle];
+      on_compositing_did_commit_callbacks_.push_back(
+          base::Bind(&RenderWidgetHostViewAura::
+                     SetSurfaceNotInUseByCompositor,
+                     AsWeakPtr(),
+                     surface_ref));
+      if (!compositor->HasObserver(this))
+        compositor->AddObserver(this);
+    }
+    image_transport_clients_.erase(current_surface_);

[piman, 2012/11/09 22:02:01]

This should still happen always

     current_surface_ = 0;
     UpdateExternalTexture();
   }
-  image_transport_clients_.erase(surface_handle);
 }
 
-void RenderWidgetHostViewAura::SetSurfaceNotInUseByCompositor(ui::Compositor*) {
-  if (current_surface_ || !host_->is_hidden())
-    return;
-  current_surface_in_use_by_compositor_ = false;
-  AdjustSurfaceProtection();
-}
-
-void RenderWidgetHostViewAura::AdjustSurfaceProtection() {
-  // If the current surface is non null, it is protected.
-  // If we are visible, it is protected.
-  // Otherwise, change to not proctected once done thumbnailing and compositing.
-  bool surface_is_protected =
-      current_surface_ ||
-      !host_->is_hidden() ||
-      (current_surface_is_protected_ &&
-          (pending_thumbnail_tasks_ > 0 ||
-              current_surface_in_use_by_compositor_));
-  if (current_surface_is_protected_ == surface_is_protected)
-    return;
-  current_surface_is_protected_ = surface_is_protected;
-  ++protection_state_id_;
-
-  if (!surface_route_id_ || !shared_surface_handle_.parent_gpu_process_id)
-    return;
-
-  RenderWidgetHostImpl::SendFrontSurfaceIsProtected(
-      surface_is_protected,
-      protection_state_id_,
-      surface_route_id_,
-      shared_surface_handle_.parent_gpu_process_id);
+void RenderWidgetHostViewAura::SetSurfaceNotInUseByCompositor(
+    scoped_refptr<ui::Texture>) {
 }
 
 void RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinished(
     base::WeakPtr<RenderWidgetHostViewAura> render_widget_host_view,
     const base::Callback<void(bool)>& callback,
+    scoped_refptr<ui::Texture>,
     bool result) {
   callback.Run(result);
 
   if (!render_widget_host_view.get())
     return;
   --render_widget_host_view->pending_thumbnail_tasks_;
-  render_widget_host_view->AdjustSurfaceProtection();
 }
 
 void RenderWidgetHostViewAura::SetBackground(const SkBitmap& background) {
   RenderWidgetHostViewBase::SetBackground(background);
   host_->SetBackground(background);
   window_->layer()->SetFillsBoundsOpaquely(background.isOpaque());
 }
 
 void RenderWidgetHostViewAura::GetScreenInfo(WebScreenInfo* results) {
   GetScreenInfoForWindow(results, window_);
@@ skipping 660 matching lines @@
 
 void RenderWidgetHostViewAura::OnCompositingDidCommit(
     ui::Compositor* compositor) {
   if (can_lock_compositor_ == NO_PENDING_COMMIT) {
     can_lock_compositor_ = YES;
     for (ResizeLockList::iterator it = resize_locks_.begin();
         it != resize_locks_.end(); ++it)
       if ((*it)->GrabDeferredLock())
         can_lock_compositor_ = YES_DID_LOCK;
   }
-  RunCompositingDidCommitCallbacks(compositor);
+  RunCompositingDidCommitCallbacks();
   locks_pending_commit_.clear();
 }
 
 void RenderWidgetHostViewAura::OnCompositingStarted(
     ui::Compositor* compositor) {
 }
 
 void RenderWidgetHostViewAura::OnCompositingEnded(
     ui::Compositor* compositor) {
 }
@@ skipping 10 matching lines @@
     can_lock_compositor_ = NO_PENDING_RENDERER_FRAME;
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewAura, ImageTransportFactoryObserver implementation:
 
 void RenderWidgetHostViewAura::OnLostResources() {
   image_transport_clients_.clear();
   current_surface_ = 0;
-  protection_state_id_ = 0;
-  current_surface_is_protected_ = true;
-  current_surface_in_use_by_compositor_ = true;
-  surface_route_id_ = 0;
   UpdateExternalTexture();
   locks_pending_commit_.clear();
 
   DCHECK(!shared_surface_handle_.is_null());
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   factory->DestroySharedSurfaceHandle(shared_surface_handle_);
   shared_surface_handle_ = factory->CreateSharedSurfaceHandle();
   host_->CompositingSurfaceUpdated();
   host_->ScheduleComposite();
 }
@@ skipping 114 matching lines @@
   gfx::Rect rect = window_->bounds();
   int border_x = rect.width() * kMouseLockBorderPercentage / 100;
   int border_y = rect.height() * kMouseLockBorderPercentage / 100;
 
   return global_mouse_position_.x() < rect.x() + border_x ||
       global_mouse_position_.x() > rect.right() - border_x ||
       global_mouse_position_.y() < rect.y() + border_y ||
       global_mouse_position_.y() > rect.bottom() - border_y;
 }
 
-void RenderWidgetHostViewAura::RunCompositingDidCommitCallbacks(
-    ui::Compositor* compositor) {
-  for (std::vector< base::Callback<void(ui::Compositor*)> >::const_iterator
+void RenderWidgetHostViewAura::RunCompositingDidCommitCallbacks() {
+  for (std::vector<base::Closure>::const_iterator
       it = on_compositing_did_commit_callbacks_.begin();
       it != on_compositing_did_commit_callbacks_.end(); ++it) {
-    it->Run(compositor);
+    it->Run();
   }
   on_compositing_did_commit_callbacks_.clear();
 }
 
 // static
 void RenderWidgetHostViewAura::InsertSyncPointAndACK(
-    int32 route_id, int gpu_host_id, bool presented,
-    ui::Compositor* compositor) {
+    int32 route_id,
+    int gpu_host_id,
+    uint64 surface_handle,
+    scoped_refptr<ui::Texture> texture_to_produce) {
+  if (texture_to_produce)
+    texture_to_produce->Produce();
+
+  ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
+
   uint32 sync_point = 0;
-  // If we have no compositor, so we must still send the ACK. A zero
-  // sync point will not be waited for in the GPU process.
-  if (compositor) {
-    ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
-    sync_point = factory->InsertSyncPoint();
-  }
+  // If we did at least consume a texture (not skipped a frame), we have
+  // to insert a sync point.
+  if (texture_to_produce || !surface_handle)
+      sync_point = factory->InsertSyncPoint();
 
   RenderWidgetHostImpl::AcknowledgeBufferPresent(
-      route_id, gpu_host_id, presented, sync_point);
+      route_id, gpu_host_id, surface_handle, sync_point);
 }
 
 void RenderWidgetHostViewAura::AddingToRootWindow() {
   host_->ParentChanged(GetNativeViewId());
 }
 
 void RenderWidgetHostViewAura::RemovingFromRootWindow() {
   host_->ParentChanged(0);
   // We are about to disconnect ourselves from the compositor, we need to issue
   // the callbacks now, because we won't get notified when the frame is done.
   // TODO(piman): this might in theory cause a race where the GPU process starts
   // drawing to the buffer we haven't yet displayed. This will only show for 1
   // frame though, because we will reissue a new frame right away without that
   // composited data.
   ui::Compositor* compositor = GetCompositor();
-  RunCompositingDidCommitCallbacks(compositor);
+  RunCompositingDidCommitCallbacks();
   locks_pending_commit_.clear();
   if (compositor && compositor->HasObserver(this))
     compositor->RemoveObserver(this);
   DetachFromInputMethod();
 }
 
 ui::Compositor* RenderWidgetHostViewAura::GetCompositor() {
   aura::RootWindow* root_window = window_->GetRootWindow();
   return root_window ? root_window->compositor() : NULL;
 }
@@ skipping 12 matching lines @@
     RenderWidgetHost* widget) {
   return new RenderWidgetHostViewAura(widget);
 }
 
 // static
 void RenderWidgetHostViewPort::GetDefaultScreenInfo(WebScreenInfo* results) {
   GetScreenInfoForWindow(results, NULL);
 }
 
 }  // namespace content