Mac: Make resize more smooth

gpu/ipc/service/image_transport_surface_overlay_mac.mm

Index: gpu/ipc/service/image_transport_surface_overlay_mac.mm
diff --git a/gpu/ipc/service/image_transport_surface_overlay_mac.mm b/gpu/ipc/service/image_transport_surface_overlay_mac.mm
index 84ab18d138ac46bb081239eab49620c997fa367c..465626c66877fbfce9def8e092e4f00b92c8cfd5 100644
--- a/gpu/ipc/service/image_transport_surface_overlay_mac.mm
+++ b/gpu/ipc/service/image_transport_surface_overlay_mac.mm
@@ -43,6 +43,11 @@
 
 namespace {
 
+// Keep previously-used CAContexts around until 5 subsequent swaps have been
+// issued, to ensure that the CAContext is no longer being displayed. This
+// value was determined empirically.
+const size_t kSwapsToKeepDiscardedSizeDependentState = 5;
+
 void CheckGLErrors(const char* msg) {
   GLenum gl_error;
   while ((gl_error = glGetError()) != GL_NO_ERROR) {
@@ -70,11 +75,8 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
     : stub_(stub->AsWeakPtr()),
       handle_(handle),
       use_remote_layer_api_(ui::RemoteLayerAPISupported()),
-      scale_factor_(1),
       gl_renderer_id_(0) {
   ui::GpuSwitchingManager::GetInstance()->AddObserver(this);
-  ca_layer_tree_coordinator_.reset(
-      new ui::CALayerTreeCoordinator(use_remote_layer_api_));
 }
 
 ImageTransportSurfaceOverlayMac::~ImageTransportSurfaceOverlayMac() {
@@ -94,24 +96,13 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
       base::Bind(&ImageTransportSurfaceOverlayMac::SetLatencyInfo,
                  base::Unretained(this)));
 
-  // Create the CAContext to send this to the GPU process, and the layer for
-  // the context.
-  if (use_remote_layer_api_) {
-    CGSConnectionID connection_id = CGSMainConnectionID();
-    ca_context_.reset([
-        [CAContext contextWithCGSConnection:connection_id options:@{}] retain]);
-    [ca_context_ setLayer:ca_layer_tree_coordinator_->GetCALayerForDisplay()];
-
-    fullscreen_low_power_ca_context_.reset([
-        [CAContext contextWithCGSConnection:connection_id options:@{}] retain]);
-    [fullscreen_low_power_ca_context_ setLayer:
-        ca_layer_tree_coordinator_->GetFullscreenLowPowerLayerForDisplay()];
-  }
+  if (!size_dependent_state_)

[tapted, 2016/06/17 00:23:38]

should this always be true? (e.g. can we DCHECK(!size_dependent_state_)

+    Resize(gfx::Size(1, 1), 1, false);
   return true;
 }
 
 void ImageTransportSurfaceOverlayMac::Destroy() {
-  ca_layer_tree_coordinator_.reset();
+  discarded_size_dependent_state_.push_back(std::move(size_dependent_state_));

[tapted, 2016/06/17 00:23:38]

why not

discarded_size_dependent_state_.clear()
size_dependent_state_.reset()

?

   if (previous_frame_fence_) {
     gl::ScopedSetGLToRealGLApi scoped_set_gl_api;
     // Ensure we are using the context with which the fence was created.
@@ -232,7 +223,7 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
   base::TimeTicks finish_time = base::TimeTicks::Now();
 
   bool fullscreen_low_power_layer_valid = false;
-  ca_layer_tree_coordinator_->CommitPendingTreesToCA(
+  size_dependent_state_->ca_layer_tree_coordinator_->CommitPendingTreesToCA(
       pixel_damage_rect, &fullscreen_low_power_layer_valid);
   // TODO(ccameron): Plumb the fullscreen low power layer through to the
   // appropriate window.
@@ -251,19 +242,32 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
   CAContextID fullscreen_low_power_ca_context_id = 0;
   gfx::ScopedRefCountedIOSurfaceMachPort io_surface_mach_port;
   if (use_remote_layer_api_) {
-    ca_context_id = [ca_context_ contextId];
+    ca_context_id = [size_dependent_state_->ca_context_ contextId];
     fullscreen_low_power_ca_context_id =
-        [fullscreen_low_power_ca_context_ contextId];
+        [size_dependent_state_->fullscreen_low_power_ca_context_ contextId];
   } else {
-    IOSurfaceRef io_surface =
-        ca_layer_tree_coordinator_->GetIOSurfaceForDisplay();
+    IOSurfaceRef io_surface = size_dependent_state_->ca_layer_tree_coordinator_
+                                  ->GetIOSurfaceForDisplay();
     if (io_surface)
       io_surface_mach_port.reset(IOSurfaceCreateMachPort(io_surface));
   }
   SendAcceleratedSurfaceBuffersSwapped(
       handle_, ca_context_id, fullscreen_low_power_layer_valid,
-      fullscreen_low_power_ca_context_id, io_surface_mach_port, pixel_size_,
-      scale_factor_, std::move(latency_info_));
+      fullscreen_low_power_ca_context_id, io_surface_mach_port,
+      size_dependent_state_->pixel_size_, size_dependent_state_->scale_factor_,
+      std::move(latency_info_));
+
+  // Free state from previous resizes only after several swaps.
+  for (auto& discarded_state : discarded_size_dependent_state_)
+    discarded_state->swaps_since_discarded_ += 1;
+  while (!discarded_size_dependent_state_.empty()) {
+    if (discarded_size_dependent_state_.front()->swaps_since_discarded_ >
+        kSwapsToKeepDiscardedSizeDependentState) {
+      discarded_size_dependent_state_.pop_front();
+    } else {
+      break;

[tapted, 2016/06/17 00:23:38]

optional nit: Maybe just move the `if` into the `while` condition. And (for
readability) perhaps something like

  auto& discards = discarded_size_dependent_state_;
  while (!discards.empty() && discards.front()->swaps_since_discarded_ > 
         kSwapsToKeepDiscardedSizeDependentState)
    discards.pop_front();


(My old-stylee brain wanted to combine these two loops, but it requires knowing
too much about interator-invalidation on std::deque - e.g. the following
probably works, but explaining, e.g., why you need to check against .empty() as
well as .end() makes it feel a little too much like voodoo

  auto& discards = discarded_size_dependent_state_;
  for (auto i = discards.begin(); !discards.empty() && i != discards.end(); ) {
    if (i->swaps_since_discarded_++ == kSwapsToKeepDiscardedSizeDependentState)
      discarded_size_dependent_state_.erase(i++);
    else
      ++i;
  }

+    }
+  }
 
   // Reset all state for the next frame.
   latency_info_.clear();
@@ -271,8 +275,7 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
 }
 
 gfx::SwapResult ImageTransportSurfaceOverlayMac::SwapBuffers() {
-  return SwapBuffersInternal(
-      gfx::Rect(0, 0, pixel_size_.width(), pixel_size_.height()));
+  return SwapBuffersInternal(gfx::Rect(size_dependent_state_->pixel_size_));
 }
 
 gfx::SwapResult ImageTransportSurfaceOverlayMac::PostSubBuffer(int x,
@@ -316,8 +319,9 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
     DLOG(ERROR) << "Invalid non-zero Z order.";
     return false;
   }
-  return ca_layer_tree_coordinator_->SetPendingGLRendererBackbuffer(
-      static_cast<gl::GLImageIOSurface*>(image)->io_surface());
+  return size_dependent_state_->ca_layer_tree_coordinator_
+      ->SetPendingGLRendererBackbuffer(
+          static_cast<gl::GLImageIOSurface*>(image)->io_surface());
 }
 
 bool ImageTransportSurfaceOverlayMac::ScheduleCALayer(
@@ -340,7 +344,8 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
     io_surface = io_surface_image->io_surface();
     cv_pixel_buffer = io_surface_image->cv_pixel_buffer();
   }
-  return ca_layer_tree_coordinator_->GetPendingCARendererLayerTree()
+  return size_dependent_state_->ca_layer_tree_coordinator_
+      ->GetPendingCARendererLayerTree()
       ->ScheduleCALayer(is_clipped, gfx::ToEnclosingRect(clip_rect),
                         sorting_context_id, transform, io_surface,
                         cv_pixel_buffer, contents_rect,
@@ -369,9 +374,38 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
 bool ImageTransportSurfaceOverlayMac::Resize(const gfx::Size& pixel_size,
                                              float scale_factor,
                                              bool has_alpha) {
-  pixel_size_ = pixel_size;
-  scale_factor_ = scale_factor;
-  ca_layer_tree_coordinator_->Resize(pixel_size, scale_factor);
+  if (size_dependent_state_ &&
+      size_dependent_state_->pixel_size_ == pixel_size &&
+      size_dependent_state_->scale_factor_ == scale_factor) {
+    return true;
+  }
+  if (size_dependent_state_)
+    discarded_size_dependent_state_.push_back(std::move(size_dependent_state_));
+
+  size_dependent_state_.reset(new SizeDependentState);
+  size_dependent_state_->pixel_size_ = pixel_size;
+  size_dependent_state_->scale_factor_ = scale_factor;
+  size_dependent_state_->ca_layer_tree_coordinator_.reset(
+      new ui::CALayerTreeCoordinator(use_remote_layer_api_));
+  size_dependent_state_->ca_layer_tree_coordinator_->Resize(pixel_size,
+                                                            scale_factor);
+
+  // Create the CAContext to send this to the GPU process, and the layer for
+  // the context.
+  if (use_remote_layer_api_) {
+    CGSConnectionID connection_id = CGSMainConnectionID();
+    size_dependent_state_->ca_context_.reset([
+        [CAContext contextWithCGSConnection:connection_id options:@{}] retain]);

[tapted, 2016/06/17 00:23:38]

nit: use base::scoped_policy::RETAIN ?

+    [size_dependent_state_->ca_context_
+        setLayer:size_dependent_state_->ca_layer_tree_coordinator_
+                     ->GetCALayerForDisplay()];
+
+    size_dependent_state_->fullscreen_low_power_ca_context_.reset([
+        [CAContext contextWithCGSConnection:connection_id options:@{}] retain]);

[tapted, 2016/06/17 00:23:38]

nit: base::scoped_policy::RETAIN

+    [size_dependent_state_->fullscreen_low_power_ca_context_
+        setLayer:size_dependent_state_->ca_layer_tree_coordinator_
+                     ->GetFullscreenLowPowerLayerForDisplay()];
+  }

[tapted, 2016/06/17 00:23:38]

I think it would be good to move all this into the SizeDependentState
constructor -- makes it clear what properties it's using from
ImageTransportSurfaceOverlayMac to create the state (I think just
use_remote_layer_api_?), and you get to remove some occurrences of
`size_dependent_state_->`

   return true;
 }
 
@@ -406,4 +440,9 @@ void IOSurfaceContextNoOp(scoped_refptr<ui::IOSurfaceContext>) {
 ImageTransportSurfaceOverlayMac::IOSurfaceInUseQuery::~IOSurfaceInUseQuery() =
     default;
 
+ImageTransportSurfaceOverlayMac::SizeDependentState::SizeDependentState() =

[tapted, 2016/06/17 00:23:38]

nit: (if you want to keep the declaration in the header) move before
IOSurfaceInUseQuery, to match header order

+    default;
+ImageTransportSurfaceOverlayMac::SizeDependentState::~SizeDependentState() =
+    default;
+
 }  // namespace gpu