Delay activation/draw on GPU tile work completion

cc/tiles/tile_manager.cc

Index: cc/tiles/tile_manager.cc
diff --git a/cc/tiles/tile_manager.cc b/cc/tiles/tile_manager.cc
index 5a9d737e491e057ada0830fc93f0eb39110e5ed4..eb3b8fa761ae227b1b8b6a1ae2f60117a0e9870e 100644
--- a/cc/tiles/tile_manager.cc
+++ b/cc/tiles/tile_manager.cc
@@ -366,7 +366,8 @@ TileManager::TileManager(TileManagerClient* client,
       has_scheduled_tile_tasks_(false),
       prepare_tiles_count_(0u),
       next_tile_id_(0u),
-      task_set_finished_weak_ptr_factory_(this) {}
+      task_set_finished_weak_ptr_factory_(this),
+      ready_to_draw_callback_weak_ptr_factory_(this) {}
 
 TileManager::~TileManager() {
   FinishTasksAndCleanUp();
@@ -394,6 +395,7 @@ void TileManager::FinishTasksAndCleanUp() {
   more_tiles_need_prepare_check_notifier_.Cancel();
   signals_check_notifier_.Cancel();
   task_set_finished_weak_ptr_factory_.InvalidateWeakPtrs();
+  ready_to_draw_callback_weak_ptr_factory_.InvalidateWeakPtrs();
 
   image_controller_.SetImageDecodeCache(nullptr);
   locked_image_tasks_.clear();
@@ -512,6 +514,12 @@ bool TileManager::PrepareTiles(
   FreeResourcesForReleasedTiles();
   CleanUpReleasedTiles();
 
+  // The required for activate/draw status of tiles may have changed any time
+  // PrepareTiles is called. If we have any |pending_ready_to_draw_tiles_|,
+  // set a flag so we know to re-compute these requirements.
+  if (!pending_ready_for_draw_tiles_.empty())

[vmpstr, 2016/12/14 18:03:31]

If it's empty, then we won't do much work anyway. How about we just set it
unconditionally?

[ericrk, 2016/12/14 19:53:48]

true enough...

+    pending_tile_requirements_dirty_ = true;
+
   PrioritizedWorkToSchedule prioritized_work = AssignGpuMemoryToTiles();
 
   // Inform the client that will likely require a draw if the highest priority
@@ -538,6 +546,7 @@ void TileManager::Flush() {
 
   tile_task_manager_->CheckForCompletedTasks();
   did_check_for_completed_tasks_since_last_schedule_tasks_ = true;
+  CheckPendingReadyToDrawTiles(true /* issue_signals */);
 
   TRACE_EVENT_INSTANT1("cc", "DidFlush", TRACE_EVENT_SCOPE_THREAD, "stats",
                        RasterTaskCompletionStatsAsValue(flush_stats_));
@@ -681,7 +690,6 @@ TileManager::PrioritizedWorkToSchedule TileManager::AssignGpuMemoryToTiles() {
               tile->content_rect(), tile->raster_scales(), &color);
       if (is_solid_color) {
         tile->draw_info().set_solid_color(color);
-        tile->draw_info().set_was_ever_ready_to_draw();
         if (!tile_is_needed_now)
           tile->draw_info().set_was_a_prepaint_tile();
         client_->NotifyTileStateChanged(tile);
@@ -781,7 +789,7 @@ void TileManager::FreeResourcesForTile(Tile* tile) {
   TileDrawInfo& draw_info = tile->draw_info();
   if (draw_info.resource_) {
     resource_pool_->ReleaseResource(draw_info.resource_);

[vmpstr, 2016/12/14 18:03:31]

Can you add a TakeResource to the draw info that automatically sets it to null?
I'd like to move towards having draw_info.resource_ not be accessible directly
by the tile manager, since it's becoming easy to get the usage wrong. 

[ericrk, 2016/12/14 19:53:48]

sure, added.

-    draw_info.resource_ = nullptr;
+    draw_info.set_resource(nullptr);
   }
 }
 
@@ -1036,14 +1044,17 @@ void TileManager::OnRasterTaskCompleted(
   resource_pool_->OnContentReplaced(resource->id(), tile->id());
   ++flush_stats_.completed_count;
 
-  draw_info.set_use_resource();
-  draw_info.resource_ = resource;
+  draw_info.set_resource(resource);
   draw_info.contents_swizzled_ = DetermineResourceRequiresSwizzle(tile);
-
-  DCHECK(draw_info.IsReadyToDraw());
-  draw_info.set_was_ever_ready_to_draw();
-
-  client_->NotifyTileStateChanged(tile);
+  if (global_state_.tree_priority == SMOOTHNESS_TAKES_PRIORITY &&
+      !raster_buffer_provider_->IsResourceReadyToDraw(resource->id())) {
+    // In SMOOTHNESS_TAKES_PRIORITY mode, we wait for GPU work to complete
+    // for a tile before setting it as ready to draw.
+    pending_ready_for_draw_tiles_.push_back(tile);
+  } else {
+    draw_info.set_resource_ready_for_draw();
+    client_->NotifyTileStateChanged(tile);
+  }
 }
 
 ScopedTilePtr TileManager::CreateTile(const Tile::CreateInfo& info,
@@ -1092,14 +1103,16 @@ bool TileManager::AreRequiredTilesReadyToDraw(
 
 bool TileManager::IsReadyToActivate() const {
   TRACE_EVENT0("cc", "TileManager::IsReadyToActivate");
-  return AreRequiredTilesReadyToDraw(
-      RasterTilePriorityQueue::Type::REQUIRED_FOR_ACTIVATION);
+  return pending_required_for_activation_callback_id_ == 0 &&
+         AreRequiredTilesReadyToDraw(
+             RasterTilePriorityQueue::Type::REQUIRED_FOR_ACTIVATION);
 }
 
 bool TileManager::IsReadyToDraw() const {
   TRACE_EVENT0("cc", "TileManager::IsReadyToDraw");
-  return AreRequiredTilesReadyToDraw(
-      RasterTilePriorityQueue::Type::REQUIRED_FOR_DRAW);
+  return pending_required_for_draw_callback_id_ == 0 &&
+         AreRequiredTilesReadyToDraw(
+             RasterTilePriorityQueue::Type::REQUIRED_FOR_DRAW);
 }
 
 void TileManager::CheckAndIssueSignals() {
@@ -1107,6 +1120,8 @@ void TileManager::CheckAndIssueSignals() {
   tile_task_manager_->CheckForCompletedTasks();
   did_check_for_completed_tasks_since_last_schedule_tasks_ = true;
 
+  CheckPendingReadyToDrawTiles(false /* issue_signals */);
+
   // Ready to activate.
   if (signals_.ready_to_activate && !signals_.did_notify_ready_to_activate) {
     signals_.ready_to_activate = false;
@@ -1260,6 +1275,77 @@ bool TileManager::UsePartialRaster() const {
          raster_buffer_provider_->CanPartialRasterIntoProvidedResource();
 }
 
+void TileManager::CheckPendingReadyToDrawTiles(bool issue_signals) {
+  bool in_smoothness_mode =
+      global_state_.tree_priority == SMOOTHNESS_TAKES_PRIORITY;
+  ResourceProvider::ResourceIdArray required_for_activation_ids;
+  ResourceProvider::ResourceIdArray required_for_draw_ids;
+
+  for (auto it = pending_ready_for_draw_tiles_.begin();
+       it != pending_ready_for_draw_tiles_.end();) {
+    Tile* tile = *it;
+    Resource* resource = tile->draw_info().resource_;

[vmpstr, 2016/12/14 18:03:31]

Can you change this to a getter instead?

[ericrk, 2016/12/14 19:53:48]

Done.

+    if (!resource) {
+      // The tile has been evicted or deleted. Remove it from our list.
+      it = pending_ready_for_draw_tiles_.erase(it);
+      continue;
+    }
+
+    if (!in_smoothness_mode ||
+        raster_buffer_provider_->IsResourceReadyToDraw(resource->id())) {
+      // Tile is ready to draw.
+      tile->draw_info().set_resource_ready_for_draw();
+      client_->NotifyTileStateChanged(tile);
+      it = pending_ready_for_draw_tiles_.erase(it);
+      continue;
+    }
+
+    if (pending_tile_requirements_dirty_)
+      tile->tiling()->UpdateRequiredStatesOnTile(tile);
+    if (tile->required_for_activation())
+      required_for_activation_ids.push_back(resource->id());
+    if (tile->required_for_draw())
+      required_for_draw_ids.push_back(resource->id());
+
+    ++it;
+  }
+
+  if (required_for_activation_ids.empty()) {
+    pending_required_for_activation_callback_id_ = 0;
+  } else {
+    pending_required_for_activation_callback_id_ =
+        raster_buffer_provider_->SetReadyToDrawCallback(
+            required_for_activation_ids,
+            base::Bind(&TileManager::CheckPendingReadyToDrawTiles,
+                       ready_to_draw_callback_weak_ptr_factory_.GetWeakPtr(),
+                       true /* issue_signals */),
+            pending_required_for_activation_callback_id_);
+  }
+
+  if (required_for_draw_ids.empty()) {
+    pending_required_for_draw_callback_id_ = 0;
+  } else {
+    pending_required_for_draw_callback_id_ =
+        raster_buffer_provider_->SetReadyToDrawCallback(
+            required_for_draw_ids,
+            base::Bind(&TileManager::CheckPendingReadyToDrawTiles,
+                       ready_to_draw_callback_weak_ptr_factory_.GetWeakPtr(),
+                       true /* issue_signals */),
+            pending_required_for_draw_callback_id_);
+  }
+
+  // Update our signals now that we know whether we have pending resources.
+  signals_.ready_to_activate =

[vmpstr, 2016/12/14 18:03:31]

I think there might be some counter-intuitive interaction here:

Suppose we get a DidFinishRunningTileTasksRequiredForDraw, with
DidFinishRunningTileTasksRequiredForActivation still pending (but about to run):

- The calling code would set ready_to_draw to true, ready_to_activate would be
false and we're in CheckAndIssueSignals
- This function is called and it determines (say we're not in smoothness mode)
that ready_to_activate is true and sets as such
- The AreTilesReady code runs and issues an activation signal
- Then, DidFinishRunningTileTasksRequiredForActivation runs and unconditionally
sets ready_to_activate to true again and runs the same code again
- This would issue a second activation (bad?)

I wonder if these setters should be ... &= ... and in the sync token callback
set both signals to true... although that might still issue a second activation
if there's a pending raster task signal, no?

[ericrk, 2016/12/14 19:53:48]

Looked through the code. I believe this is prevented by
Signals::did_notify_ready_to_activate_, which blocks further activations until
it is reset. We only reset it in PrepareTiles, so I think this is fine.

+      (pending_required_for_activation_callback_id_ == 0);
+  signals_.ready_to_draw = (pending_required_for_draw_callback_id_ == 0);
+
+  if (issue_signals && (signals_.ready_to_activate || signals_.ready_to_draw))
+    signals_check_notifier_.Schedule();
+
+  // We've just updated all pending tile requirements if necessary.
+  pending_tile_requirements_dirty_ = false;
+}
+
 // Utility function that can be used to create a "Task set finished" task that
 // posts |callback| to |task_runner| when run.
 scoped_refptr<TileTask> TileManager::CreateTaskSetFinishedTask(
@@ -1278,8 +1364,8 @@ TileManager::MemoryUsage::MemoryUsage(size_t memory_bytes,
       resource_count_(static_cast<int>(resource_count)) {
   // MemoryUsage is constructed using size_ts, since it deals with memory and
   // the inputs are typically size_t. However, during the course of usage (in
-  // particular operator-=) can cause internal values to become negative. Thus,
-  // member variables are signed.
+  // particular operator-=) can cause internal values to become negative.
+  // Thus, member variables are signed.
   DCHECK_LE(memory_bytes,
             static_cast<size_t>(std::numeric_limits<int64_t>::max()));
   DCHECK_LE(resource_count,
@@ -1291,7 +1377,8 @@ TileManager::MemoryUsage TileManager::MemoryUsage::FromConfig(
     const gfx::Size& size,
     ResourceFormat format) {
   // We can use UncheckedSizeInBytes here since this is used with a tile
-  // size which is determined by the compositor (it's at most max texture size).
+  // size which is determined by the compositor (it's at most max texture
+  // size).
   return MemoryUsage(ResourceUtil::UncheckedSizeInBytes<size_t>(size, format),
                      1);
 }