cc: Use BeginFrameArgs

cc/trees/layer_tree_host_impl.cc

Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index c61e6bec0d0c99f7f90801f421dd448b928bb9cb..6e666d24153a0f13d3f07b3582d697b49341e610 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -107,7 +107,6 @@ class LayerTreeHostImplTimeSourceAdapter : public TimeSourceClient {
           new DebugScopedSetImplThread(layer_tree_host_impl_->proxy()));
     }
 
-    // TODO(enne): This should probably happen post-animate.
     if (layer_tree_host_impl_->pending_tree()) {
       layer_tree_host_impl_->ActivatePendingTreeIfNeeded();
 
@@ -170,6 +169,7 @@ LayerTreeHostImpl::LayerTreeHostImpl(
     RenderingStatsInstrumentation* rendering_stats_instrumentation)
     : client_(client),
       proxy_(proxy),
+      output_surface_lost_(true),
       input_handler_client_(NULL),
       did_lock_scrolling_layer_(false),
       should_bubble_scrolls_(false),
@@ -252,7 +252,8 @@ void LayerTreeHostImpl::CommitComplete() {
     pending_tree_->set_needs_update_draw_properties();
     pending_tree_->UpdateDrawProperties();
     // Start working on newly created tiles immediately if needed.
-    ManageTiles();
+    if (!ManageTiles())

[enne (OOO), 2013/08/06 23:35:16]

It seems like TileManager should be able to activate here via
TileManagerClient::NotifyReadyToActivate without additional logic.

[brianderson, 2013/08/07 01:55:51]

In this patch, I avoid back to back activations by having the Scheduler
explicitly initiate activation. This code eventually notifies the Scheduler
which keeps track of whether or not the current active tree has been drawn.  If
the current active tree hasn't been drawn, it will not activate the pending tree
until after the draw.

+      client_->NotifyReadyToActivate();
   } else {
     active_tree_->set_needs_update_draw_properties();
   }
@@ -316,11 +317,11 @@ void LayerTreeHostImpl::Animate(base::TimeTicks monotonic_time,
   AnimateTopControls(monotonic_time);
 }
 
-void LayerTreeHostImpl::ManageTiles() {
+bool LayerTreeHostImpl::ManageTiles() {
   if (!tile_manager_)
-    return;
+    return false;
   if (!manage_tiles_needed_)
-    return;
+    return false;

[enne (OOO), 2013/08/06 23:35:16]

This false seems a little sketchy.  If you call ManageTiles and then immediately
call ManageTiles again, it will return false, but that doesn't mean the pending
tree can be activated.

I wonder if this would be conceptually cleaner if ManageTiles didn't return
anything.  The one caller that checks the return value is also guarded by an
impl-side painting setting which makes the tile_manager_ check redundant.  The
one caller is also right after an UpdateDrawProperties call which will always
indirectly call WillModifyTilePriorities.

[brianderson, 2013/08/07 01:55:51]

Ok. It seems like I need to fix some of the corner cases here.

If I do not return a value from this function to notify the Scheduler that we
are ready to activate immediately, then there are cases where
manage_tiles_needed_ is still false even after the call to UpdateDrawProperties.
 I think this is because of layers that aren't tiled (such as texture layers)
and when we are rasterizing on demand.

Is there a better way to check for those cases where we are not expecting a
NotifyReadyToActivate from the TileManager and can activate immediately.

[enne (OOO), 2013/08/07 15:44:19]

Maybe the problem here is just the comment.  ManageTiles returning false does
not mean that you can activate.  That's true during commit, but is not true in
general.  Returning true from ManageTiles here just means that the TileManager
is responsible for notifying about activation.

One option would be to change the comment on ManageTiles.  Alternatively, the
caller of ManageTiles during commit should just check manage_tiles_needed_
directly prior to calling ManageTiles to catch this case, rather than returning
a boolean value any time ManageTiles is called.

[brianderson, 2013/08/07 18:41:57]

Thanks for the background. I would like to avoid calling ManageTiles if we don't
need to because of the overhead
(https://code.google.com/p/chromium/issues/detail?id=269272).

Would another option be to revert changes to LayerTreeHostImpl::ManageTiles and
have pending_tree_->UpdateDrawProperties() somehow trigger the ReadyToActivate
if it doesn't set manage_tiles_needed_ to true?

[enne (OOO), 2013/08/07 19:48:48]

I wasn't suggesting to move the early out, just to add an additional check in
the commit function.

I think what you suggest could work for now, but I have been thinking about
making UpdateDrawProperties not always update tile priorities which would cause
that check to misfire.

I think it might be clearer to do it in the commit function, since not needing
to manage tiles is sort of a property of the tree, and can only be true once,
and so will always be true in commit when we have to add pending priorities for
all tiles on the pending tree.

[brianderson, 2013/08/07 20:12:29]

Ah, I understand your original comment now and it makes a lot of sense. Checking
in the commit works very well.

   manage_tiles_needed_ = false;
   tile_manager_->ManageTiles();
 
@@ -333,6 +334,7 @@ void LayerTreeHostImpl::ManageTiles() {
   SendManagedMemoryStats(memory_required_bytes,
                          memory_nice_to_have_bytes,
                          memory_used_bytes);
+  return true;
 }
 
 void LayerTreeHostImpl::StartPageScaleAnimation(gfx::Vector2d target_offset,
@@ -1062,10 +1064,7 @@ void LayerTreeHostImpl::DidInitializeVisibleTile() {
 }
 
 void LayerTreeHostImpl::NotifyReadyToActivate() {
-  if (pending_tree_) {
-    need_to_update_visible_tiles_before_draw_ = true;
-    ActivatePendingTree();
-  }
+  client_->NotifyReadyToActivate();
 }
 
 bool LayerTreeHostImpl::ShouldClearRootRenderPass() const {
@@ -1310,7 +1309,7 @@ const RendererCapabilities& LayerTreeHostImpl::GetRendererCapabilities() const {
 }
 
 bool LayerTreeHostImpl::SwapBuffers(const LayerTreeHostImpl::FrameData& frame) {
-  if (frame.has_no_damage)
+  if (frame.has_no_damage || output_surface_lost_)
     return false;
   renderer_->SwapBuffers();
   active_tree_->ClearLatencyInfo();
@@ -1353,6 +1352,7 @@ void LayerTreeHostImpl::DidLoseOutputSurface() {
   // TODO(jamesr): The renderer_ check is needed to make some of the
   // LayerTreeHostContextTest tests pass, but shouldn't be necessary (or
   // important) in production. We should adjust the test to not need this.
+  output_surface_lost_ = true;
   if (renderer_)
     client_->DidLoseOutputSurfaceOnImplThread();
 }
@@ -1447,6 +1447,8 @@ void LayerTreeHostImpl::ActivatePendingTree() {
   CHECK(pending_tree_);
   TRACE_EVENT_ASYNC_END0("cc", "PendingTree", pending_tree_.get());
 
+  need_to_update_visible_tiles_before_draw_ = true;
+
   active_tree_->SetRootLayerScrollOffsetDelegate(NULL);
   active_tree_->PushPersistedState(pending_tree_.get());
   if (pending_tree_->needs_full_tree_sync()) {
@@ -1475,6 +1477,10 @@ void LayerTreeHostImpl::ActivatePendingTree() {
       root_layer_scroll_offset_delegate_);
   active_tree_->DidBecomeActive();
 
+  client_->DidActivatePendingTree();
+  if (!tree_activation_callback_.is_null())
+    tree_activation_callback_.Run();
+
   // Reduce wasted memory now that unlinked resources are guaranteed not
   // to be used.
   client_->ReduceWastedContentsTextureMemoryOnImplThread();
@@ -1491,10 +1497,6 @@ void LayerTreeHostImpl::ActivatePendingTree() {
         stats.total_paint_time + stats.total_record_time +
             stats.total_rasterize_time_for_now_bins_on_pending_tree);
   }
-
-  client_->DidActivatePendingTree();
-  if (!tree_activation_callback_.is_null())
-    tree_activation_callback_.Run();
 }
 
 void LayerTreeHostImpl::SetVisible(bool visible) {
@@ -1602,6 +1604,7 @@ bool LayerTreeHostImpl::InitializeRenderer(
   tile_manager_.reset();
   resource_provider_.reset();
   output_surface_.reset();
+  output_surface_lost_ = true;
 
   if (!output_surface->BindToClient(this))
     return false;
@@ -1647,6 +1650,7 @@ bool LayerTreeHostImpl::InitializeRenderer(
 
   resource_provider_ = resource_provider.Pass();
   output_surface_ = output_surface.Pass();
+  output_surface_lost_ = false;
 
   client_->OnCanDrawStateChanged(CanDraw());
 
@@ -2197,6 +2201,8 @@ scoped_ptr<ScrollAndScaleSet> LayerTreeHostImpl::ProcessScrollDeltas() {
   CollectScrollDeltas(scroll_info.get(), active_tree_->root_layer());
   scroll_info->page_scale_delta = active_tree_->page_scale_delta();
   active_tree_->set_sent_page_scale_delta(scroll_info->page_scale_delta);
+  if (pending_tree_)
+    pending_tree_->set_sent_page_scale_delta(scroll_info->page_scale_delta);

[enne (OOO), 2013/08/06 23:35:16]

Sent page scale delta should always be 1 on the pending tree as it has not sent
anything.  I think this will especially not work with your changes to pass the
sent page scale delta from pending to active tree during activaiton.

[brianderson, 2013/08/07 01:55:51]

What prompted the page scale delta changes was that my changes (either back to
back activates or delaying the first draw after a new output surface until after
activation) were causing LayerTreeHostTestStartPageScaleAnimation to fail. I
think the best course of action will be to undo my changes regarding the page
scale and debug with Enne what's going wrong and the best way to fix it.

 
   return scroll_info.Pass();
 }
@@ -2216,6 +2222,12 @@ void LayerTreeHostImpl::AnimatePageScale(base::TimeTicks time) {
   active_tree_->SetPageScaleDelta(
       page_scale_animation_->PageScaleFactorAtTime(monotonic_time) /
       active_tree_->page_scale_factor());
+  if (pending_tree_) {
+    pending_tree_->SetPageScaleDelta(
+      page_scale_animation_->PageScaleFactorAtTime(monotonic_time) /
+      pending_tree_->page_scale_factor());
+  }
+
   gfx::Vector2dF next_scroll =
       page_scale_animation_->ScrollOffsetAtTime(monotonic_time);