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 425cfe8cc7f0d2bd7d89ab2ca66770083eca35b6..4fc5feaf6bfb64905675b5488ead992abca906f3 100644
--- a/cc/tiles/tile_manager.cc
+++ b/cc/tiles/tile_manager.cc
@@ -368,7 +368,8 @@ TileManager::TileManager(TileManagerClient* client,
       prepare_tiles_count_(0u),
       next_tile_id_(0u),
       check_tile_priority_inversion_(check_tile_priority_inversion),
-      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();
@@ -396,6 +397,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();
   raster_buffer_provider_ = nullptr;
 
   image_controller_.SetImageDecodeCache(nullptr);
@@ -421,6 +423,10 @@ void TileManager::SetResources(ResourcePool* resource_pool,
 
 void TileManager::Release(Tile* tile) {
   released_tiles_.push_back(tile);
+  // The |tile|'s owning PictureLayerTiling has released ownership and turned
+  // the

[vmpstr, 2017/01/05 22:00:46]

Reformat plz

[ericrk, 2017/01/09 23:05:21]

Done.

+  // |tile| over to TileManager for deletion.
+  tile->set_tiling(nullptr);
 }
 
 void TileManager::FreeResourcesForReleasedTiles() {
@@ -439,6 +445,7 @@ void TileManager::CleanUpReleasedTiles() {
     DCHECK(!tile->draw_info().has_resource());
     DCHECK(tiles_.find(tile->id()) != tiles_.end());
     tiles_.erase(tile->id());
+    pending_ready_for_draw_tiles_.erase(tile);
 
     delete tile;
   }
@@ -515,6 +522,10 @@ bool TileManager::PrepareTiles(
   FreeResourcesForReleasedTiles();
   CleanUpReleasedTiles();
 
+  // The required for activate/draw status of tiles may have changed any time
+  // PrepareTiles is called.
+  pending_tile_requirements_dirty_ = true;
+
   PrioritizedWorkToSchedule prioritized_work = AssignGpuMemoryToTiles();
 
   // Inform the client that will likely require a draw if the highest priority
@@ -541,6 +552,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_));
@@ -684,7 +696,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);
@@ -817,10 +828,9 @@ TileManager::PrioritizedWorkToSchedule TileManager::AssignGpuMemoryToTiles() {
 
 void TileManager::FreeResourcesForTile(Tile* tile) {
   TileDrawInfo& draw_info = tile->draw_info();
-  if (draw_info.resource_) {
-    resource_pool_->ReleaseResource(draw_info.resource_);
-    draw_info.resource_ = nullptr;
-  }
+  Resource* resource = draw_info.TakeResource();
+  if (resource)
+    resource_pool_->ReleaseResource(resource);
 }
 
 void TileManager::FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(
@@ -878,7 +888,7 @@ void TileManager::ScheduleTasks(
     Tile* tile = prioritized_tile.tile();
 
     DCHECK(tile->draw_info().requires_resource());
-    DCHECK(!tile->draw_info().resource_);
+    DCHECK(!tile->draw_info().resource());
 
     if (!tile->raster_task_)
       tile->raster_task_ = CreateRasterTask(
@@ -1074,14 +1084,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

[vmpstr, 2017/01/05 22:00:46]

Move this comment outside of the if plz

Also consider doing this pattern, but it's really up to you.

// If SMOOTHNESS ...
if (...) {
 pending_ready_for_draw_tiles_.insert(tile);
 return;
}

draw_info.set_resource_ready_for_draw();
client_->NotifyTileStateChanged(tile);

[ericrk, 2017/01/09 23:05:21]

good suggestion.

+    // for a tile before setting it as ready to draw.
+    pending_ready_for_draw_tiles_.insert(tile);
+  } else {
+    draw_info.set_resource_ready_for_draw();
+    client_->NotifyTileStateChanged(tile);
+  }
 }
 
 ScopedTilePtr TileManager::CreateTile(const Tile::CreateInfo& info,
@@ -1130,14 +1143,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() {
@@ -1145,6 +1160,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;
@@ -1298,6 +1315,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;
+    const Resource* resource = tile->draw_info().resource();
+    if (!tile->tiling()) {
+      // The tile has been evicted or deleted. Remove it from our list.

[vmpstr, 2017/01/05 22:00:46]

nit: or is pending deletion. (you remove it when it's proper deleted)

[ericrk, 2017/01/09 23:05:22]

now that tile deletion has been cleaned up, this is gone.

+      it = pending_ready_for_draw_tiles_.erase(it);
+      continue;
+    }
+
+    if (!in_smoothness_mode ||

[vmpstr, 2017/01/05 22:00:46]

You can just do the gobal_state_ == ... check here and remove the variable.

[ericrk, 2017/01/09 23:05:21]

Done.

+        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_)

[vmpstr, 2017/01/05 22:00:46]

I know we talked about this a bit, but is there a race here?
UpdateTilePriorities might have run and the scheduler is pending a PrepareTiles,
but one hasn't happened yet. Is that possible? What are the implications? It
seems that some times that _are_ now required might be missed.

[ericrk, 2017/01/09 23:05:21]

Ok, I think there was a race here (mainly because of the scheduler sometimes
skipping PrepareTiles) - added a TileManager::DidModifyTilePriorities to handle
this.

+      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;

[vmpstr, 2017/01/05 22:00:46]

Maybe

pending_required_for_activation_callback_id_ = 0;
if (!required_for_activation_ids.empty()) {
  ...
}

WDYT?

[ericrk, 2017/01/09 23:05:21]

Done.

+  } 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 =
+      (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(
@@ -1316,8 +1404,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,
@@ -1329,7 +1417,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);
 }
@@ -1337,9 +1426,9 @@ TileManager::MemoryUsage TileManager::MemoryUsage::FromConfig(
 // static
 TileManager::MemoryUsage TileManager::MemoryUsage::FromTile(const Tile* tile) {
   const TileDrawInfo& draw_info = tile->draw_info();
-  if (draw_info.resource_) {
-    return MemoryUsage::FromConfig(draw_info.resource_->size(),
-                                   draw_info.resource_->format());
+  if (draw_info.resource()) {
+    return MemoryUsage::FromConfig(draw_info.resource()->size(),
+                                   draw_info.resource()->format());
   }
   return MemoryUsage();
 }