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

content/browser/renderer_host/render_widget_host_view_aura.cc

Index: content/browser/renderer_host/render_widget_host_view_aura.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.cc b/content/browser/renderer_host/render_widget_host_view_aura.cc
index c3405a6197858354e26d9f73adfeb68a1c8df457..2729cefeedcca88526f32001b6b4d407e15e7b93 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -9,7 +9,6 @@
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
-#include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
 #include "base/string_number_conversions.h"
 #include "content/browser/renderer_host/backing_store_skia.h"
@@ -215,6 +214,10 @@ RenderWidgetHostViewAura::RenderWidgetHostViewAura(RenderWidgetHost* host)
       can_compose_inline_(true),
       has_composition_text_(false),
       current_surface_(0),
+      route_id_(0),
+      gpu_host_id_(0),
+      weak_factory_(this),

[piman, 2012/06/11 21:32:24]

ALLOW_THIS_IN_INITIALIZER_LIST

[mmocny, 2012/06/12 18:26:18]

Not needed once using SupportsWeakPtr.

On 2012/06/11 21:32:24, piman wrote:

+      is_thumbnailing_(false),
       paint_canvas_(NULL),
       synthetic_move_sent_(false),
       needs_update_texture_(false) {
@@ -238,6 +241,7 @@ RenderWidgetHostViewAura::~RenderWidgetHostViewAura() {
     popup_parent_host_view_->popup_child_host_view_ = NULL;
   }
   aura::client::SetTooltipText(window_, NULL);
+  weak_factory_.InvalidateWeakPtrs();

[piman, 2012/06/11 21:32:24]

Do you need this? weak_factory_ will invalidate the WeakPtrs when it gets
destroyed.
You can actually make this class derive from SupportsWeakPtr instead, which is
equivalent.

[mmocny, 2012/06/12 18:26:18]

Done.

 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -287,10 +291,31 @@ RenderWidgetHost* RenderWidgetHostViewAura::GetRenderWidgetHost() const {
 
 void RenderWidgetHostViewAura::DidBecomeSelected() {
   host_->WasRestored();
+
+  if (!current_surface_ && host_->is_accelerated_compositing_active() &&
+      !released_front_lock_.get())
+    released_front_lock_ = window_->GetRootWindow()->GetCompositorLock();
+
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableUIReleaseFrontSurface)) {

[piman, 2012/06/11 21:32:24]

Rather than parsing the command line every time this is called, better to store
this as a bool.

[mmocny, 2012/06/12 18:26:18]

Done, using static bool inside anon namespace helper so as not to bloat RWHVA
(modeled after ShouldSendPinchGesture).
I know there are issues with static initializers and destruction handlers, but I
suspect static bool is fine?

On 2012/06/11 21:32:24, piman wrote:

+    SendSurfaceIsProtectedBasedOnVisibilityWhenReady();
+    // Reset this here in case there is an outstanding callback to remove
+    // frontbuffer protection. This protects against ABA issues.
+    on_thumbnailing_ended_callback_.Reset();
+  }
 }
 
 void RenderWidgetHostViewAura::WasHidden() {
   host_->WasHidden();
+
+  released_front_lock_ = NULL;
+
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableUIReleaseFrontSurface)) {
+    current_surface_ = 0;
+    UpdateExternalTexture();
+    SendSurfaceIsProtectedBasedOnVisibilityWhenReady();
+  }
 }
 
 void RenderWidgetHostViewAura::SetSize(const gfx::Size& size) {
@@ -453,6 +478,7 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurface(
     skia::PlatformCanvas* output,
     base::Callback<void(bool)> callback) {
   base::ScopedClosureRunner scoped_callback_runner(base::Bind(callback, false));
+
   ui::Compositor* compositor = GetCompositor();
   if (!compositor)
     return;
@@ -478,11 +504,31 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurface(
   unsigned char* addr = static_cast<unsigned char*>(
       output->getTopDevice()->accessBitmap(true).getPixels());
   scoped_callback_runner.Release();
+  // on_thumbnailing_ended_callback_ is not set up to handle multiple concurrent
+  // thumbnail requests, nor would doing that be a good idea. Could early exit
+  // instead of DCHECK if this is not already handled by thumbnailer generater.
+  DCHECK(!is_thumbnailing_);

[piman, 2012/06/11 21:32:24]

Please check with mazda that this is ensured. That code is pretty far from here,
and baking this assumption may not be safe against refactoring there.
Probably better is to fail if one is already in progress (by calling the
callback with failure).

[mmocny, 2012/06/12 18:26:18]

Had a talk with @mazda.  It is not correct to ignore, or callback(false) because
the thumbnailer already handles multiple outstanding thumbnail requests by
ignoring all but the last one.  So, if we fail request B while request A is
pending, the thumbnailer code will ignore request A even when its the only
success one.

Instead, I just track the number of pending thumbnail requests and don't
AdjustSurfaceAllocation until there is 0 pending, and let all of them do what
they always did.

On 2012/06/11 21:32:24, piman wrote:

+  is_thumbnailing_ = true;
+  // Wrap the callback with an internal handler so that we can inject our
+  // own completion handlers. Used to delay informing gpu process that it can
+  // discard frontbuffer until after we finish thumbnailing.
+  base::Callback<void(bool)> wrapper_callback = base::Bind(
+      &RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinished,
+      weak_factory_.GetWeakPtr(),
+      callback);
   gl_helper->CopyTextureTo(container->texture_id(),
                            container->size(),
                            size_in_pixel,
                            addr,
-                           callback);
+                           wrapper_callback);
+}
+
+void RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinished(
+    base::Callback<void(bool)> callback, bool result) {
+  is_thumbnailing_ = false;
+  if(!on_thumbnailing_ended_callback_.is_null())
+    on_thumbnailing_ended_callback_.Run();
+  callback.Run(result);
 }
 
 void RenderWidgetHostViewAura::OnAcceleratedCompositingStateChange() {
@@ -499,13 +545,16 @@ void RenderWidgetHostViewAura::UpdateExternalTexture() {
   needs_update_texture_ = false;
   if (current_surface_ != 0 &&
       host_->is_accelerated_compositing_active()) {
-
+    DCHECK(image_transport_clients_.find(current_surface_) !=
+           image_transport_clients_.end());
     ImageTransportClient* container =
         image_transport_clients_[current_surface_];
     if (container)
       container->Update();
     window_->SetExternalTexture(container);
 
+    released_front_lock_ = NULL;
+
     if (!container) {
       resize_locks_.clear();
     } else {
@@ -548,7 +597,16 @@ void RenderWidgetHostViewAura::UpdateExternalTexture() {
 void RenderWidgetHostViewAura::AcceleratedSurfaceBuffersSwapped(
     const GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params& params_in_pixel,
     int gpu_host_id) {
+  // If we are not visible then this swap is stale. We must still ACK but do not

[piman, 2012/06/11 21:32:24]

I think there's still ABA issues.
Say at some point, at around the same time:
- gpu sends swap
- tab got invisible and all other conditions were good, so it sends
IsProtected(false)

A fraction later, the tab got visible again before the swap was received. When
the swap is received, the browser will start using it, and send a
IsProtected(true), but it may be too late and the GPU process may delete the
buffer.

I'm pretty sure you need a state id for the IsProtected message (increasing
every time it is sent) and attach that to the swap/post so that you can verify
that the swap isn't stale (hence the buffer at risk of being destroyed).

[mmocny, 2012/06/11 21:45:09]

Keen observation!  I will add this.

On 2012/06/11 21:32:24, piman wrote:

[mmocny, 2012/06/12 18:26:18]

Done.

+  // reset current_surface_ so that we do not try to display this surface later.
+  if (!host_->IsVisible()) {
+    DCHECK(!current_surface_);
+    RenderWidgetHostImpl::AcknowledgeSwapBuffers(params_in_pixel.route_id,
+        gpu_host_id);
+    return;
+  }
   current_surface_ = params_in_pixel.surface_handle;
+  DCHECK(current_surface_);
   UpdateExternalTexture();
 
   ui::Compositor* compositor = GetCompositor();
@@ -558,6 +616,8 @@ void RenderWidgetHostViewAura::AcceleratedSurfaceBuffersSwapped(
     RenderWidgetHostImpl::AcknowledgeSwapBuffers(params_in_pixel.route_id,
         gpu_host_id);
   } 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 = content::ConvertSizeToDIP(this,
@@ -585,7 +645,16 @@ void RenderWidgetHostViewAura::AcceleratedSurfaceBuffersSwapped(
 void RenderWidgetHostViewAura::AcceleratedSurfacePostSubBuffer(
     const GpuHostMsg_AcceleratedSurfacePostSubBuffer_Params& params_in_pixel,
     int gpu_host_id) {
+  // If we are not visible then this PSB is stale. We must still ACK but do not
+  // reset current_surface_ so that we do not try to display this surface later.
+  if (!host_->IsVisible()) {
+    DCHECK(!current_surface_);
+    RenderWidgetHostImpl::AcknowledgePostSubBuffer(params_in_pixel.route_id,
+        gpu_host_id);
+    return;
+  }
   current_surface_ = params_in_pixel.surface_handle;
+  DCHECK(current_surface_);
   UpdateExternalTexture();
 
   ui::Compositor* compositor = GetCompositor();
@@ -595,6 +664,8 @@ void RenderWidgetHostViewAura::AcceleratedSurfacePostSubBuffer(
     RenderWidgetHostImpl::AcknowledgePostSubBuffer(
         params_in_pixel.route_id, gpu_host_id);
   } 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();
 
@@ -641,7 +712,9 @@ void RenderWidgetHostViewAura::AcceleratedSurfaceNew(
       int32 width_in_pixel,
       int32 height_in_pixel,
       uint64* surface_handle,
-      TransportDIB::Handle* shm_handle) {
+      TransportDIB::Handle* shm_handle,
+      int32 route_id,
+      int gpu_host_id) {
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   scoped_refptr<ImageTransportClient> surface(factory->CreateTransportClient(
         gfx::Size(width_in_pixel, height_in_pixel), surface_handle));
@@ -652,10 +725,15 @@ void RenderWidgetHostViewAura::AcceleratedSurfaceNew(
   *shm_handle = surface->Handle();
 
   image_transport_clients_[*surface_handle] = surface;
+
+  route_id_ = route_id;
+  gpu_host_id_ = gpu_host_id;
 }
 
 void RenderWidgetHostViewAura::AcceleratedSurfaceRelease(
     uint64 surface_handle) {
+  DCHECK(image_transport_clients_.find(surface_handle) !=
+         image_transport_clients_.end());
   if (current_surface_ == surface_handle) {
     current_surface_ = 0;
     UpdateExternalTexture();
@@ -663,6 +741,62 @@ void RenderWidgetHostViewAura::AcceleratedSurfaceRelease(
   image_transport_clients_.erase(surface_handle);
 }
 
+void RenderWidgetHostViewAura::
+    SendSurfaceIsProtectedBasedOnVisibilityWhenReady() {
+  if (!route_id_ || !gpu_host_id_)
+    return;
+  if (host_->IsVisible()) {
+    RenderWidgetHostImpl::SendFrontSurfaceIsProtected(true,
+                                                      route_id_,
+                                                      gpu_host_id_);
+    return;
+  }
+  DCHECK(!current_surface_);
+
+  ui::Compositor* compositor = GetCompositor();
+  // If we are currently compositing, schedule this function to be called again
+  // after we finish. This function must handle changes to state due to this
+  // delay.
+  // Note: There is no ABA issue because if visibility changes to true and back,
+  // we know we didn't schedule a composite during that time.
+  if (compositor->DrawPending()) {
+    on_compositing_ended_callbacks_.push_back(
+        base::Bind(&RenderWidgetHostViewAura::
+                       SendSurfaceIsProtectedFalseAfterThumbnailerFinished,
+                   weak_factory_.GetWeakPtr()));
+    if (!compositor->HasObserver(this))
+      compositor->AddObserver(this);
+    return;
+  }
+
+  SendSurfaceIsProtectedFalseAfterThumbnailerFinished();
+}
+
+void RenderWidgetHostViewAura::
+    SendSurfaceIsProtectedFalseAfterThumbnailerFinished() {
+  if (!route_id_ || !gpu_host_id_)
+    return;
+  if (host_->IsVisible())
+    return;
+  DCHECK(!current_surface_);
+  // If we are currently thumbnailing, schedule this function to be called again
+  // after we finish. This function must handle changes to state due to this
+  // delay.
+  // Note: We guard against ABA issue with visibility changes by resetting the
+  // on_thumbnailing_ended_callback_ in DidBecomeSelected.
+  if (is_thumbnailing_) {
+    on_thumbnailing_ended_callback_ =

[piman, 2012/06/11 21:32:24]

Do you need to make this a callback? You could just set a bool state and call
SendSurfaceIsProtectedFalseAfterThumbnailerFinished from
CopyFromCompositingSurfaceFinished if it is set.

Similarly to what we do in TextureImageTransportSurface, maybe you could make
that logic as:
- IsProtected is a virtual state of (!visible && !surface_in_use_by_thumbnailer
&& !surface_in_use_by_compositor)
- tracking individual states separately
  * visible through DidBecomeSelected/WasHidden,
  * surface_in_use_by_thumbnailer through
CopyFromCompositingSurface/CopyFromCompositingSurfaceFinished,
  * surface_in_use_by_compositor through the CompositorObserver callbacks
- send the message any time IsProtected() changes (e.g. have a common function
that checks the current state vs the last sent state and send a message when
they're different - and call that function every time any of the above changes).

I think that way it'd be much clearer about the logic (tracing callbacks is
hard).

[mmocny, 2012/06/11 21:45:09]

Sounds great, will do.

On 2012/06/11 21:32:24, piman wrote:

[mmocny, 2012/06/12 18:26:18]

Done.

+        base::Bind(&RenderWidgetHostViewAura::
+                       SendSurfaceIsProtectedFalseAfterThumbnailerFinished,
+                   weak_factory_.GetWeakPtr());
+    return;
+  }
+
+  RenderWidgetHostImpl::SendFrontSurfaceIsProtected(false,
+                                                    route_id_,
+                                                    gpu_host_id_);
+}
+
 void RenderWidgetHostViewAura::SetBackground(const SkBitmap& background) {
   content::RenderWidgetHostViewBase::SetBackground(background);
   host_->SetBackground(background);
@@ -1203,6 +1337,9 @@ void RenderWidgetHostViewAura::OnCompositingAborted(
 void RenderWidgetHostViewAura::OnLostResources(ui::Compositor* compositor) {
   image_transport_clients_.clear();
   current_surface_ = 0;
+  route_id_ = 0;
+  gpu_host_id_ = 0;
+  is_thumbnailing_ = 0;

[piman, 2012/06/11 21:32:24]

The thumbnailing callback should be called either way (possibly with failure),
so there shouldn't be a need to do this.

[mmocny, 2012/06/12 18:26:18]

Done.

   UpdateExternalTexture();
   locks_pending_draw_.clear();