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

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"
 #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"
 #include "content/browser/renderer_host/dip_util.h"
 #include "content/browser/renderer_host/image_transport_client.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/browser/renderer_host/web_input_event_aura.h"
 #include "content/common/gpu/client/gl_helper.h"
 #include "content/common/gpu/gpu_messages.h"
 #include "content/port/browser/render_widget_host_view_port.h"
@@ skipping 185 matching lines @@
       ALLOW_THIS_IN_INITIALIZER_LIST(window_(new aura::Window(this))),
       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),
       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) {
   host_->SetView(this);
   window_observer_.reset(new WindowObserver(this));
   window_->AddObserver(window_observer_.get());
   aura::client::SetTooltipText(window_, &tooltip_);
   aura::client::SetActivationDelegate(window_, this);
 }
 
 RenderWidgetHostViewAura::~RenderWidgetHostViewAura() {
   if (!shared_surface_handle_.is_null()) {
     ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
     factory->DestroySharedSurfaceHandle(shared_surface_handle_);
     factory->RemoveObserver(this);
   }
   window_->RemoveObserver(window_observer_.get());
   UnlockMouse();
   if (popup_type_ != WebKit::WebPopupTypeNone) {
     DCHECK(popup_parent_host_view_);
     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.

 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewAura, RenderWidgetHostView implementation:
 
 void RenderWidgetHostViewAura::InitAsChild(
     gfx::NativeView parent_view) {
   window_->Init(ui::LAYER_TEXTURED);
   window_->SetName("RenderWidgetHostViewAura");
   window_->layer()->set_scale_content(false);
@@ skipping 29 matching lines @@
   Show();
   Focus();
 }
 
 RenderWidgetHost* RenderWidgetHostViewAura::GetRenderWidgetHost() const {
   return host_;
 }
 
 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) {
   SetBounds(gfx::Rect(window_->bounds().origin(), size));
 }
 
 void RenderWidgetHostViewAura::SetBounds(const gfx::Rect& rect) {
   if (window_->bounds().size() != rect.size() &&
       host_->is_accelerated_compositing_active()) {
     resize_locks_.push_back(make_linked_ptr(
@@ skipping 142 matching lines @@
 BackingStore* RenderWidgetHostViewAura::AllocBackingStore(
     const gfx::Size& size) {
   return new BackingStoreSkia(host_, size);
 }
 
 void RenderWidgetHostViewAura::CopyFromCompositingSurface(
     const gfx::Size& size,
     skia::PlatformCanvas* output,
     base::Callback<void(bool)> callback) {
   base::ScopedClosureRunner scoped_callback_runner(base::Bind(callback, false));
+
   ui::Compositor* compositor = GetCompositor();
   if (!compositor)
     return;
 
   std::map<uint64, scoped_refptr<ImageTransportClient> >::iterator it =
       image_transport_clients_.find(current_surface_);
   if (it == image_transport_clients_.end())
     return;
 
   ImageTransportClient* container = it->second;
   DCHECK(container);
 
   gfx::Size size_in_pixel = content::ConvertSizeToPixel(this, size);
   if (!output->initialize(
       size_in_pixel.width(), size_in_pixel.height(), true))
     return;
 
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   content::GLHelper* gl_helper = factory->GetGLHelper(compositor);
   if (!gl_helper)
     return;
 
   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() {
   // Delay UpdateExternalTexture until we actually got a software frame.
   // 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.
   needs_update_texture_ = true;
 }
 
 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 {
       typedef std::vector<linked_ptr<ResizeLock> > ResizeLockList;
       ResizeLockList::iterator it = resize_locks_.begin();
       while (it != resize_locks_.end()) {
         gfx::Size container_size = content::ConvertSizeToDIP(this,
             container->size());
         if ((*it)->expected_size() == container_size)
           break;
@@ skipping 22 matching lines @@
     }
   } else {
     window_->SetExternalTexture(NULL);
     resize_locks_.clear();
   }
 }
 
 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();
   if (!compositor) {
     // We have no compositor, so we have no way to display the surface.
     // Must still send the ACK.
     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,
                                                        surface_size_in_pixel);
     window_->SchedulePaintInRect(gfx::Rect(surface_size));
 
     if (!resize_locks_.empty() && !compositor->DrawPending()) {
       // If we are waiting for the resize, fast-track the ACK.
       // However only do so if we're not between the Draw() and the
       // OnCompositingEnded(), because out-of-order execution in the GPU process
       // might corrupt the "front buffer" for the currently issued frame.
       RenderWidgetHostImpl::AcknowledgeSwapBuffers(
           params_in_pixel.route_id, gpu_host_id);
     } else {
       // Add sending an ACK to the list of things to do OnCompositingEnded
       on_compositing_ended_callbacks_.push_back(
           base::Bind(&RenderWidgetHostImpl::AcknowledgeSwapBuffers,
                      params_in_pixel.route_id, gpu_host_id));
       if (!compositor->HasObserver(this))
         compositor->AddObserver(this);
     }
   }
 }
 
 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();
   if (!compositor) {
     // We have no compositor, so we have no way to display the surface
     // Must still send the ACK
     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();
 
     // Co-ordinates come in OpenGL co-ordinate space.
     // We need to convert to layer space.
     gfx::Rect rect_to_paint = content::ConvertRectToDIP(this, gfx::Rect(
         params_in_pixel.x,
         surface_size_in_pixel.height() - params_in_pixel.y -
             params_in_pixel.height,
         params_in_pixel.width,
@@ skipping 26 matching lines @@
   // 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;
 }
 
 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));
   if (!surface) {
     LOG(ERROR) << "Failed to create ImageTransportClient";
     return;
   }
   *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();
   }
   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);
   window_->layer()->SetFillsBoundsOpaquely(background.isOpaque());
 }
 
 void RenderWidgetHostViewAura::GetScreenInfo(WebKit::WebScreenInfo* results) {
   GetScreenInfoForWindow(results, window_);
 }
 
@@ skipping 520 matching lines @@
 void RenderWidgetHostViewAura::OnCompositingAborted(
     ui::Compositor* compositor) {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewAura, ImageTransportFactoryObserver implementation:
 
 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();
 
   DCHECK(!shared_surface_handle_.is_null());
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   factory->DestroySharedSurfaceHandle(shared_surface_handle_);
   shared_surface_handle_ = factory->CreateSharedSurfaceHandle(compositor);
   host_->CompositingSurfaceUpdated();
   host_->ScheduleComposite();
 }
@@ skipping 126 matching lines @@
 RenderWidgetHostView* RenderWidgetHostView::CreateViewForWidget(
     RenderWidgetHost* widget) {
   return new RenderWidgetHostViewAura(widget);
 }
 
 // static
 void content::RenderWidgetHostViewPort::GetDefaultScreenInfo(
     WebKit::WebScreenInfo* results) {
   GetScreenInfoForWindow(results, NULL);
 }