cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread().

cc/tiles/tile_manager.cc

 // Copyright 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 "cc/tiles/tile_manager.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 23 matching lines @@
 
 // Flag to indicate whether we should try and detect that
 // a tile is of solid color.
 const bool kUseColorEstimator = true;
 
 DEFINE_SCOPED_UMA_HISTOGRAM_AREA_TIMER(
     ScopedRasterTaskTimer,
     "Compositing.%s.RasterTask.RasterUs",
     "Compositing.%s.RasterTask.RasterPixelsPerMs");
 
 class RasterTaskImpl : public TileTask {

[vmpstr, 2016/05/21 00:02:14]

We might want to restructure this task a bit. Specifically, since we're passing
a tile, we can have a private struct like ThreadSafeState and just pull out all
of the relevant information from the tile into ThreadSafeState at construction
time. Then, it's clear what can and cannot be accessed in RunOnWorkerThread and
we pass far fewer parameters into RasterTaskImpl...

Can you leave a TODO here?

[prashant.n, 2016/05/21 00:33:53]

Yes. I'll create two structures one for running task and one for processing
completion in next patch. For this I'll leave the comment in current patch.

  public:
-  RasterTaskImpl(const Resource* resource,
+  RasterTaskImpl(TileManager* tile_manager,
+                 Resource* resource,
                  scoped_refptr<RasterSource> raster_source,
                  const gfx::Rect& content_rect,
                  const gfx::Rect& invalid_content_rect,
                  float contents_scale,
                  const RasterSource::PlaybackSettings& playback_settings,
                  TileResolution tile_resolution,
                  int layer_id,
                  uint64_t source_prepare_tiles_id,
-                 const void* tile,
+                 Tile* tile,
                  uint64_t new_content_id,
                  uint64_t previous_content_id,
                  uint64_t resource_content_id,
                  int source_frame_number,
-                 const base::Callback<void(bool)>& reply,
+                 std::unique_ptr<RasterBuffer> raster_buffer,
                  TileTask::Vector* dependencies,
                  bool supports_concurrent_execution)
       : TileTask(supports_concurrent_execution, dependencies),
+        tile_manager_(tile_manager),
         resource_(resource),
         raster_source_(std::move(raster_source)),
         content_rect_(content_rect),
         invalid_content_rect_(invalid_content_rect),
         contents_scale_(contents_scale),
         playback_settings_(playback_settings),
         tile_resolution_(tile_resolution),
         layer_id_(layer_id),
         source_prepare_tiles_id_(source_prepare_tiles_id),
         tile_(tile),
         new_content_id_(new_content_id),
         previous_content_id_(previous_content_id),
         resource_content_id_(resource_content_id),
         source_frame_number_(source_frame_number),
-        reply_(reply) {}
+        raster_buffer_(std::move(raster_buffer)) {}
 
   // Overridden from Task:
   void RunOnWorkerThread() override {
     TRACE_EVENT1("cc", "RasterizerTaskImpl::RunOnWorkerThread",
                  "source_prepare_tiles_id", source_prepare_tiles_id_);
 
     DCHECK(raster_source_.get());
     DCHECK(raster_buffer_);
 
     frame_viewer_instrumentation::ScopedRasterTask raster_task(
         tile_, tile_resolution_, source_frame_number_, layer_id_);
     ScopedRasterTaskTimer timer;
     timer.SetArea(content_rect_.size().GetArea());
 
     DCHECK(raster_source_);
 
     raster_buffer_->Playback(raster_source_.get(), content_rect_,
                              invalid_content_rect_, new_content_id_,
                              contents_scale_, playback_settings_);
   }
 
   // Overridden from TileTask:
-  void ScheduleOnOriginThread(RasterBufferProvider* provider) override {
-    DCHECK(!raster_buffer_);
-    raster_buffer_ = provider->AcquireBufferForRaster(
-        resource_, resource_content_id_, previous_content_id_);
-  }
-  void CompleteOnOriginThread(RasterBufferProvider* provider) override {
-    provider->ReleaseBufferForRaster(std::move(raster_buffer_));
-    reply_.Run(!state().IsFinished());
+  void OnTaskCompleted() override {
+    tile_manager_->OnRasterTaskCompleted(std::move(raster_buffer_), tile_,
+                                         resource_, state().IsCanceled());
   }
 
  protected:
   ~RasterTaskImpl() override { DCHECK(!raster_buffer_); }
 
  private:
-  const Resource* resource_;
+  TileManager* tile_manager_;
+  Resource* resource_;
   scoped_refptr<RasterSource> raster_source_;
   gfx::Rect content_rect_;
   gfx::Rect invalid_content_rect_;
   float contents_scale_;
   RasterSource::PlaybackSettings playback_settings_;
   TileResolution tile_resolution_;
   int layer_id_;
   uint64_t source_prepare_tiles_id_;
-  const void* tile_;
+  Tile* tile_;

[vmpstr, 2016/05/21 00:02:14]

The reason we had this as a void* is that it would only be used as an id for
tracing. That is, we have to be careful not to be accessing tiles from different
threads. I think the patch you have now is fine, but can you leave a comment
here to say that tile object should not be accessed in any function that runs on
a different thread (it's not a thread safe object)

[prashant.n, 2016/05/21 00:33:53]

Yes.

I was thinking to add clousres here one for running and one for completing. But
passing members is better than having closures.

   uint64_t new_content_id_;
   uint64_t previous_content_id_;
   uint64_t resource_content_id_;
   int source_frame_number_;
-  const base::Callback<void(bool)> reply_;
   std::unique_ptr<RasterBuffer> raster_buffer_;
 
   DISALLOW_COPY_AND_ASSIGN(RasterTaskImpl);
 };
 
 TaskCategory TaskCategoryForTileTask(TileTask* task,
                                      bool use_foreground_category) {
   if (!task->supports_concurrent_execution())
     return TASK_CATEGORY_NONCONCURRENT_FOREGROUND;
 
@@ skipping 41 matching lines @@
 }
 
 void InsertNodeForDecodeTask(TaskGraph* graph,
                              TileTask* task,
                              bool use_foreground_category,
                              uint16_t priority) {
   uint32_t dependency_count = 0u;
   if (task->dependencies().size()) {
     DCHECK_EQ(task->dependencies().size(), 1u);
     auto* dependency = task->dependencies()[0].get();
-    if (!dependency->HasCompleted()) {
+    if (!dependency->state().IsFinished()) {
       InsertNodeForDecodeTask(graph, dependency, use_foreground_category,
                               priority);
       graph->edges.push_back(TaskGraph::Edge(dependency, task));
       dependency_count = 1u;
     }
   }
   InsertNodeForTask(graph, task,
                     TaskCategoryForTileTask(task, use_foreground_category),
                     priority, dependency_count);
 }
 
 void InsertNodesForRasterTask(TaskGraph* graph,
                               TileTask* raster_task,
                               const TileTask::Vector& decode_tasks,
                               size_t priority,
                               bool use_foreground_category) {
   size_t dependencies = 0u;
 
   // Insert image decode tasks.
   for (TileTask::Vector::const_iterator it = decode_tasks.begin();
        it != decode_tasks.end(); ++it) {
     TileTask* decode_task = it->get();
 
     // Skip if already decoded.
-    if (decode_task->HasCompleted())
+    if (decode_task->state().IsFinished())
       continue;
 
     dependencies++;
 
     // Add decode task if it doesn't already exist in graph.
     TaskGraph::Node::Vector::iterator decode_it =
         std::find_if(graph->nodes.begin(), graph->nodes.end(),
                      [decode_task](const TaskGraph::Node& node) {
                        return node.task == decode_task;
                      });
@@ skipping 17 matching lines @@
 
     graph->edges.push_back(TaskGraph::Edge(decode_task, raster_task));
   }
 
   InsertNodeForTask(
       graph, raster_task,
       TaskCategoryForTileTask(raster_task, use_foreground_category), priority,
       dependencies);
 }
 
+bool CheckDependenciesForFinishedRasterTask(TileTask* raster_task) {

[vmpstr, 2016/05/21 00:02:14]

Our pattern is usually:

void CheckFoo(Bar* bar) {
#if DCHECK_IS_ON()
  for (...)
    ...
#endif
}

and then in the calling code:

CheckFoo(bar);

Also can you move this function after the class below?

[prashant.n, 2016/05/21 00:33:53]

Yes.

+  // If there is any dependency which is not finished, add a DCHECK for it.
+  for (auto& decode_task : raster_task->dependencies())
+    DCHECK(decode_task->state().IsFinished());
+
+  return true;
+}
+
 class TaskSetFinishedTaskImpl : public TileTask {
  public:
   explicit TaskSetFinishedTaskImpl(
       base::SequencedTaskRunner* task_runner,
       const base::Closure& on_task_set_finished_callback)
       : TileTask(true),
         task_runner_(task_runner),
         on_task_set_finished_callback_(on_task_set_finished_callback) {}
 
   // Overridden from Task:
   void RunOnWorkerThread() override {
     TRACE_EVENT0("cc", "TaskSetFinishedTaskImpl::RunOnWorkerThread");
     TaskSetFinished();
   }
 
-  // Overridden from TileTask:
-  void ScheduleOnOriginThread(RasterBufferProvider* provider) override {}
-  void CompleteOnOriginThread(RasterBufferProvider* provider) override {}
+  void OnTaskCompleted() override{};
 
  protected:
   ~TaskSetFinishedTaskImpl() override {}
 
   void TaskSetFinished() {
     task_runner_->PostTask(FROM_HERE, on_task_set_finished_callback_);
   }
 
  private:
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
@@ skipping 219 matching lines @@
 
 void TileManager::Flush() {
   TRACE_EVENT0("cc", "TileManager::Flush");
 
   if (!tile_task_manager_) {
     TRACE_EVENT_INSTANT0("cc", "Flush aborted", TRACE_EVENT_SCOPE_THREAD);
     return;
   }
 
   tile_task_manager_->CheckForCompletedTasks();
-
   did_check_for_completed_tasks_since_last_schedule_tasks_ = true;
 
   TRACE_EVENT_INSTANT1("cc", "DidFlush", TRACE_EVENT_SCOPE_THREAD, "stats",
                        RasterTaskCompletionStatsAsValue(flush_stats_));
   flush_stats_ = RasterTaskCompletionStats();
 }
 
 std::unique_ptr<base::trace_event::ConvertableToTraceFormat>
 TileManager::BasicStateAsValue() const {
   std::unique_ptr<base::trace_event::TracedValue> value(
@@ skipping 266 matching lines @@
   for (auto& prioritized_tile : tiles_that_need_to_be_rasterized) {
     Tile* tile = prioritized_tile.tile();
 
     DCHECK(tile->draw_info().requires_resource());
     DCHECK(!tile->draw_info().resource_);
 
     if (!tile->raster_task_)
       tile->raster_task_ = CreateRasterTask(prioritized_tile);
 
     TileTask* task = tile->raster_task_.get();
-    DCHECK(!task->HasCompleted());
+
+    // Skip already finished raster task. Finished raster task should have all
+    // its dependencies finished.
+    if (task->state().IsFinished()) {

[vmpstr, 2016/05/21 00:02:14]

In what situation can we be getting a finished task here? Also, is cancelled a
type of finished or does finish imply it ran to completion?

[prashant.n, 2016/05/21 00:33:53]

See crbug.com/613529 for more details. I'll modify the comment to depict the
situation.
Only finished state implies to it ran to completion. The canceled state implies
it completed as canceled. The task task is modified to canceled state in
TaskGraphWorkQueue::ScheduleTasks(). So this won't arise but I'll check further
- canceled task must be collected before next scheduling happens, so that if
tile which had task canceled should get new raster task.

+      DCHECK(CheckDependenciesForFinishedRasterTask(task));
+      continue;
+    }
 
     if (tile->required_for_activation()) {
       required_for_activate_count++;
       graph_.edges.push_back(
           TaskGraph::Edge(task, required_for_activation_done_task.get()));
     }
     if (tile->required_for_draw()) {
       required_for_draw_count++;
       graph_.edges.push_back(
           TaskGraph::Edge(task, required_for_draw_done_task.get()));
@@ skipping 99 matching lines @@
     if (task)
       decode_tasks.push_back(task);
 
     if (need_to_unref_when_finished)
       ++it;
     else
       it = images.erase(it);
   }
 
   bool supports_concurrent_execution = !use_gpu_rasterization_;
+  std::unique_ptr<RasterBuffer> raster_buffer =
+      tile_task_manager_->GetRasterBufferProvider()->AcquireBufferForRaster(
+          resource, resource_content_id, tile->invalidated_id());
   return make_scoped_refptr(new RasterTaskImpl(
-      resource, prioritized_tile.raster_source(), tile->content_rect(),
+      this, resource, prioritized_tile.raster_source(), tile->content_rect(),
       tile->invalidated_content_rect(), tile->contents_scale(),
       playback_settings, prioritized_tile.priority().resolution,
-      tile->layer_id(), prepare_tiles_count_, static_cast<const void*>(tile),
-      tile->id(), tile->invalidated_id(), resource_content_id,
-      tile->source_frame_number(),
-      base::Bind(&TileManager::OnRasterTaskCompleted, base::Unretained(this),
-                 tile->id(), resource),
-      &decode_tasks, supports_concurrent_execution));
+      tile->layer_id(), prepare_tiles_count_, tile, tile->id(),
+      tile->invalidated_id(), resource_content_id, tile->source_frame_number(),
+      std::move(raster_buffer), &decode_tasks, supports_concurrent_execution));
 }
 
 void TileManager::OnRasterTaskCompleted(
-    Tile::Id tile_id,
+    std::unique_ptr<RasterBuffer> raster_buffer,
+    Tile* tile,
     Resource* resource,
     bool was_canceled) {
-  DCHECK(tiles_.find(tile_id) != tiles_.end());
+  DCHECK(tile);
+  DCHECK(tiles_.find(tile->id()) != tiles_.end());
+  tile_task_manager_->GetRasterBufferProvider()->ReleaseBufferForRaster(
+      std::move(raster_buffer));
 
-  Tile* tile = tiles_[tile_id];
   TileDrawInfo& draw_info = tile->draw_info();
   DCHECK(tile->raster_task_.get());
   orphan_tasks_.push_back(tile->raster_task_);
   tile->raster_task_ = nullptr;
 
   // Unref all the images.
   auto images_it = scheduled_draw_images_.find(tile->id());
   const std::vector<DrawImage>& images = images_it->second;
   for (const auto& image : images)
     image_decode_controller_->UnrefImage(image);
@@ skipping 309 matching lines @@
 void TileManager::Signals::reset() {
   ready_to_activate = false;
   did_notify_ready_to_activate = false;
   ready_to_draw = false;
   did_notify_ready_to_draw = false;
   all_tile_tasks_completed = false;
   did_notify_all_tile_tasks_completed = false;
 }
 
 }  // namespace cc