cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread().

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls corresponding function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818, 613529
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review-Url: https://codereview.chromium.org/1866043006
Cr-Commit-Position: refs/heads/master@{#396218}

cc/raster/raster_buffer_provider_perftest.cc

@@ -129,15 +129,7 @@ class PerfImageDecodeTaskImpl : public TileTask {
   void RunOnWorkerThread() override {}
 
   // Overridden from TileTask:
-  void ScheduleOnOriginThread(RasterBufferProvider* provider) override {}
-  void CompleteOnOriginThread(RasterBufferProvider* provider) override {
-    Reset();
-  }
-
-  void Reset() {
-    state().Reset();
-    did_complete_ = false;
-  }
+  void OnTaskCompleted() override { state().Reset(); }
 
  protected:
   ~PerfImageDecodeTaskImpl() override {}
@@ -146,34 +138,42 @@ class PerfImageDecodeTaskImpl : public TileTask {
   DISALLOW_COPY_AND_ASSIGN(PerfImageDecodeTaskImpl);
 };
 
+class PerfRasterBufferProviderHelper {
+ public:
+  virtual std::unique_ptr<RasterBuffer> AcquireBufferForRaster(
+      const Resource* resource,
+      uint64_t resource_content_id,
+      uint64_t previous_content_id) = 0;
+  virtual void ReleaseBufferForRaster(std::unique_ptr<RasterBuffer> buffer) = 0;
+};
+
 class PerfRasterTaskImpl : public TileTask {
  public:
-  PerfRasterTaskImpl(std::unique_ptr<ScopedResource> resource,
+  PerfRasterTaskImpl(PerfRasterBufferProviderHelper* helper,
+                     std::unique_ptr<ScopedResource> resource,
+                     std::unique_ptr<RasterBuffer> raster_buffer,
                      TileTask::Vector* dependencies)
-      : TileTask(true, dependencies), resource_(std::move(resource)) {}
+      : TileTask(true, dependencies),
+        helper_(helper),
+        resource_(std::move(resource)),
+        raster_buffer_(std::move(raster_buffer)) {}
 
   // Overridden from Task:
   void RunOnWorkerThread() override {}
 
   // Overridden from TileTask:
-  void ScheduleOnOriginThread(RasterBufferProvider* provider) override {
-    // No tile ids are given to support partial updates.
-    raster_buffer_ = provider->AcquireBufferForRaster(resource_.get(), 0, 0);
-  }
-  void CompleteOnOriginThread(RasterBufferProvider* provider) override {
-    provider->ReleaseBufferForRaster(std::move(raster_buffer_));
-    Reset();
-  }
+  void OnTaskCompleted() override {
+    if (helper_)
+      helper_->ReleaseBufferForRaster(std::move(raster_buffer_));
 
-  void Reset() {
     state().Reset();
-    did_complete_ = false;
   }
 
  protected:
   ~PerfRasterTaskImpl() override {}
 
  private:
+  PerfRasterBufferProviderHelper* helper_;
   std::unique_ptr<ScopedResource> resource_;
   std::unique_ptr<RasterBuffer> raster_buffer_;
 
@@ -202,7 +202,8 @@ class RasterBufferProviderPerfTestBase {
       image_decode_tasks->push_back(new PerfImageDecodeTaskImpl);
   }
 
-  void CreateRasterTasks(unsigned num_raster_tasks,
+  void CreateRasterTasks(PerfRasterBufferProviderHelper* helper,
+                         unsigned num_raster_tasks,
                          const TileTask::Vector& image_decode_tasks,
                          RasterTaskVector* raster_tasks) {
     const gfx::Size size(1, 1);
@@ -213,9 +214,14 @@ class RasterBufferProviderPerfTestBase {
       resource->Allocate(size, ResourceProvider::TEXTURE_HINT_IMMUTABLE,
                          RGBA_8888);
 
+      // No tile ids are given to support partial updates.
+      std::unique_ptr<RasterBuffer> raster_buffer;
+      if (helper)
+        raster_buffer = helper->AcquireBufferForRaster(resource.get(), 0, 0);
       TileTask::Vector dependencies = image_decode_tasks;
       raster_tasks->push_back(
-          new PerfRasterTaskImpl(std::move(resource), &dependencies));
+          new PerfRasterTaskImpl(helper, std::move(resource),
+                                 std::move(raster_buffer), &dependencies));
     }
   }
 
@@ -262,6 +268,7 @@ class RasterBufferProviderPerfTestBase {
 
 class RasterBufferProviderPerfTest
     : public RasterBufferProviderPerfTestBase,
+      public PerfRasterBufferProviderHelper,
       public testing::TestWithParam<RasterBufferProviderType> {
  public:
   // Overridden from testing::Test:
@@ -305,6 +312,20 @@ class RasterBufferProviderPerfTest
     tile_task_manager_->CheckForCompletedTasks();
   }
 
+  // Overridden from PerfRasterBufferProviderHelper:
+  std::unique_ptr<RasterBuffer> AcquireBufferForRaster(
+      const Resource* resource,
+      uint64_t resource_content_id,
+      uint64_t previous_content_id) override {
+    return tile_task_manager_->GetRasterBufferProvider()
+        ->AcquireBufferForRaster(resource, resource_content_id,
+                                 previous_content_id);
+  }
+  void ReleaseBufferForRaster(std::unique_ptr<RasterBuffer> buffer) override {
+    tile_task_manager_->GetRasterBufferProvider()->ReleaseBufferForRaster(
+        std::move(buffer));
+  }
+
   void RunMessageLoopUntilAllTasksHaveCompleted() {
     task_graph_runner_->RunUntilIdle();
     task_runner_->RunUntilIdle();
@@ -316,7 +337,8 @@ class RasterBufferProviderPerfTest
     TileTask::Vector image_decode_tasks;
     RasterTaskVector raster_tasks;
     CreateImageDecodeTasks(num_image_decode_tasks, &image_decode_tasks);
-    CreateRasterTasks(num_raster_tasks, image_decode_tasks, &raster_tasks);
+    CreateRasterTasks(this, num_raster_tasks, image_decode_tasks,
+                      &raster_tasks);
 
     // Avoid unnecessary heap allocations by reusing the same graph.
     TaskGraph graph;
@@ -346,7 +368,7 @@ class RasterBufferProviderPerfTest
     RasterTaskVector raster_tasks[kNumVersions];
     for (size_t i = 0; i < kNumVersions; ++i) {
       CreateImageDecodeTasks(num_image_decode_tasks, &image_decode_tasks[i]);
-      CreateRasterTasks(num_raster_tasks, image_decode_tasks[i],
+      CreateRasterTasks(this, num_raster_tasks, image_decode_tasks[i],
                         &raster_tasks[i]);
     }
 
@@ -378,7 +400,8 @@ class RasterBufferProviderPerfTest
     TileTask::Vector image_decode_tasks;
     RasterTaskVector raster_tasks;
     CreateImageDecodeTasks(num_image_decode_tasks, &image_decode_tasks);
-    CreateRasterTasks(num_raster_tasks, image_decode_tasks, &raster_tasks);
+    CreateRasterTasks(this, num_raster_tasks, image_decode_tasks,
+                      &raster_tasks);
 
     // Avoid unnecessary heap allocations by reusing the same graph.
     TaskGraph graph;
@@ -490,7 +513,8 @@ class RasterBufferProviderCommonPerfTest
     TileTask::Vector image_decode_tasks;
     RasterTaskVector raster_tasks;
     CreateImageDecodeTasks(num_image_decode_tasks, &image_decode_tasks);
-    CreateRasterTasks(num_raster_tasks, image_decode_tasks, &raster_tasks);
+    CreateRasterTasks(nullptr, num_raster_tasks, image_decode_tasks,
+                      &raster_tasks);
 
     // Avoid unnecessary heap allocations by reusing the same graph.
     TaskGraph graph;

cc/raster/raster_buffer_provider_unittest.cc

@@ -51,16 +51,26 @@ enum RasterBufferProviderType {
   RASTER_BUFFER_PROVIDER_TYPE_BITMAP
 };
 
-class TestRasterTaskImpl : public TileTask {
+class TestRasterTaskCompletionHandler {
  public:
-  typedef base::Callback<void(bool was_canceled)> Reply;
+  virtual void OnRasterTaskCompleted(
+      std::unique_ptr<RasterBuffer> raster_buffer,
+      unsigned id,
+      bool was_canceled) = 0;
+};
 
-  TestRasterTaskImpl(const Resource* resource,
-                     const Reply& reply,
+class TestRasterTaskImpl : public TileTask {
+ public:
+  TestRasterTaskImpl(TestRasterTaskCompletionHandler* completion_handler,
+                     std::unique_ptr<ScopedResource> resource,
+                     unsigned id,
+                     std::unique_ptr<RasterBuffer> raster_buffer,
                      TileTask::Vector* dependencies)
       : TileTask(true, dependencies),
-        resource_(resource),
-        reply_(reply),
+        completion_handler_(completion_handler),
+        resource_(std::move(resource)),
+        id_(id),
+        raster_buffer_(std::move(raster_buffer)),
         raster_source_(FakeRasterSource::CreateFilled(gfx::Size(1, 1))) {}
 
   // Overridden from Task:
@@ -72,22 +82,18 @@ class TestRasterTaskImpl : public TileTask {
   }
 
   // Overridden from TileTask:
-  void ScheduleOnOriginThread(RasterBufferProvider* provider) override {
-    // The raster buffer has no tile ids associated with it for partial update,
-    // so doesn't need to provide a valid dirty rect.
-    raster_buffer_ = provider->AcquireBufferForRaster(resource_, 0, 0);
-  }
-  void CompleteOnOriginThread(RasterBufferProvider* provider) override {
-    provider->ReleaseBufferForRaster(std::move(raster_buffer_));
-    reply_.Run(!state().IsFinished());
+  void OnTaskCompleted() override {
+    completion_handler_->OnRasterTaskCompleted(std::move(raster_buffer_), id_,
+                                               state().IsCanceled());
   }
 
  protected:
   ~TestRasterTaskImpl() override {}
 
  private:
-  const Resource* resource_;
-  const Reply reply_;
+  TestRasterTaskCompletionHandler* completion_handler_;
+  std::unique_ptr<ScopedResource> resource_;
+  unsigned id_;
   std::unique_ptr<RasterBuffer> raster_buffer_;
   scoped_refptr<RasterSource> raster_source_;
 
@@ -96,11 +102,19 @@ class TestRasterTaskImpl : public TileTask {
 
 class BlockingTestRasterTaskImpl : public TestRasterTaskImpl {
  public:
-  BlockingTestRasterTaskImpl(const Resource* resource,
-                             const Reply& reply,
-                             base::Lock* lock,
-                             TileTask::Vector* dependencies)
-      : TestRasterTaskImpl(resource, reply, dependencies), lock_(lock) {}
+  BlockingTestRasterTaskImpl(
+      TestRasterTaskCompletionHandler* completion_handler,
+      std::unique_ptr<ScopedResource> resource,
+      unsigned id,
+      std::unique_ptr<RasterBuffer> raster_buffer,
+      base::Lock* lock,
+      TileTask::Vector* dependencies)
+      : TestRasterTaskImpl(completion_handler,
+                           std::move(resource),
+                           id,
+                           std::move(raster_buffer),
+                           dependencies),
+        lock_(lock) {}
 
   // Overridden from Task:
   void RunOnWorkerThread() override {
@@ -118,7 +132,8 @@ class BlockingTestRasterTaskImpl : public TestRasterTaskImpl {
 };
 
 class RasterBufferProviderTest
-    : public testing::TestWithParam<RasterBufferProviderType> {
+    : public TestRasterTaskCompletionHandler,
+      public testing::TestWithParam<RasterBufferProviderType> {
  public:
   struct RasterTaskResult {
     unsigned id;
@@ -209,14 +224,15 @@ class RasterBufferProviderTest
         ScopedResource::Create(resource_provider_.get()));
     resource->Allocate(size, ResourceProvider::TEXTURE_HINT_IMMUTABLE,
                        RGBA_8888);
-    const Resource* const_resource = resource.get();
 
+    // The raster buffer has no tile ids associated with it for partial update,
+    // so doesn't need to provide a valid dirty rect.
+    std::unique_ptr<RasterBuffer> raster_buffer =
+        tile_task_manager_->GetRasterBufferProvider()->AcquireBufferForRaster(
+            resource.get(), 0, 0);
     TileTask::Vector empty;
-    tasks_.push_back(new TestRasterTaskImpl(
-        const_resource,
-        base::Bind(&RasterBufferProviderTest::OnTaskCompleted,
-                   base::Unretained(this), base::Passed(&resource), id),
-        &empty));
+    tasks_.push_back(new TestRasterTaskImpl(this, std::move(resource), id,
+                                            std::move(raster_buffer), &empty));
   }
 
   void AppendTask(unsigned id) { AppendTask(id, gfx::Size(1, 1)); }
@@ -228,14 +244,13 @@ class RasterBufferProviderTest
         ScopedResource::Create(resource_provider_.get()));
     resource->Allocate(size, ResourceProvider::TEXTURE_HINT_IMMUTABLE,
                        RGBA_8888);
-    const Resource* const_resource = resource.get();
 
+    std::unique_ptr<RasterBuffer> raster_buffer =
+        tile_task_manager_->GetRasterBufferProvider()->AcquireBufferForRaster(
+            resource.get(), 0, 0);
     TileTask::Vector empty;
     tasks_.push_back(new BlockingTestRasterTaskImpl(
-        const_resource,
-        base::Bind(&RasterBufferProviderTest::OnTaskCompleted,
-                   base::Unretained(this), base::Passed(&resource), id),
-        lock, &empty));
+        this, std::move(resource), id, std::move(raster_buffer), lock, &empty));
   }
 
   const std::vector<RasterTaskResult>& completed_tasks() const {
@@ -250,6 +265,17 @@ class RasterBufferProviderTest
     context_provider->ContextGL()->Flush();
   }
 
+  void OnRasterTaskCompleted(std::unique_ptr<RasterBuffer> raster_buffer,
+                             unsigned id,
+                             bool was_canceled) override {
+    tile_task_manager_->GetRasterBufferProvider()->ReleaseBufferForRaster(
+        std::move(raster_buffer));
+    RasterTaskResult result;
+    result.id = id;
+    result.canceled = was_canceled;
+    completed_tasks_.push_back(result);
+  }
+
  private:
   void Create3dOutputSurfaceAndResourceProvider() {
     output_surface_ = FakeOutputSurface::Create3d(context_provider_,
@@ -269,15 +295,6 @@ class RasterBufferProviderTest
         output_surface_.get(), &shared_bitmap_manager_, nullptr);
   }
 
-  void OnTaskCompleted(std::unique_ptr<ScopedResource> resource,
-                       unsigned id,
-                       bool was_canceled) {
-    RasterTaskResult result;
-    result.id = id;
-    result.canceled = was_canceled;
-    completed_tasks_.push_back(result);
-  }
-
   void OnTimeout() {
     timed_out_ = true;
     base::MessageLoop::current()->QuitWhenIdle();

cc/raster/task.cc

@@ -13,6 +13,9 @@ TaskState::TaskState() : value_(Value::NEW) {}
 TaskState::~TaskState() {
   DCHECK(value_ != Value::RUNNING)
       << "Running task should never get destroyed.";
+  // TODO(prashant.n): Remove NEW, once all the tests follow the task life
+  // cycle correctly. Few tests still do not take care of task states.
+  // crbug.com/613814.
   DCHECK(value_ == Value::NEW || value_ == Value::FINISHED ||
          value_ == Value::CANCELED)
       << "Task, if scheduled, should get concluded either in FINISHED or "
@@ -26,9 +29,11 @@ bool TaskState::IsScheduled() const {
 bool TaskState::IsRunning() const {
   return value_ == Value::RUNNING;
 }
+
 bool TaskState::IsFinished() const {
   return value_ == Value::FINISHED;
 }
+
 bool TaskState::IsCanceled() const {
   return value_ == Value::CANCELED;
 }
@@ -58,7 +63,7 @@ void TaskState::DidFinish() {
 
 void TaskState::DidCancel() {
   DCHECK(value_ == Value::NEW || value_ == Value::SCHEDULED)
-      << "Task should be scheduled and not running to get canceled.";
+      << "Task should be either new or scheduled to get canceled.";
   value_ = Value::CANCELED;
 }