cc: Fix incorrect completion of tiles and missing "ready to activate" signal.

cc/resources/pixel_buffer_raster_worker_pool.cc

 // Copyright 2013 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/resources/pixel_buffer_raster_worker_pool.h"
 
 #include "base/containers/stack_container.h"
 #include "base/debug/trace_event.h"
 #include "base/values.h"
 #include "cc/debug/traced_value.h"
@@ skipping 79 matching lines @@
     const NodeVector::ContainerType& dependencies) {
   for (NodeVector::ContainerType::const_iterator it = dependencies.begin();
        it != dependencies.end(); ++it) {
     internal::GraphNode* dependency = *it;
 
     node->add_dependency();
     dependency->add_dependent(node);
   }
 }
 
+bool WasCanceled(const internal::RasterWorkerPoolTask* task) {

[vmpstr, 2013/07/10 16:46:49]

If this is only for a DCHECK, maybe we can name it something like
WasCanceledForDebug... I'm not sure, I would just like to discourage future use
of this outside of std::find_if and things like that.

[reveman, 2013/07/11 00:37:21]

ForDebug in the name makes it sound like it's checking something else. I added a
comment that makes it clear this is only for debugging purposes.

+  return task->WasCanceled();
+}
+
 }  // namespace
 
 PixelBufferRasterWorkerPool::PixelBufferRasterWorkerPool(
     ResourceProvider* resource_provider,
     size_t num_threads)
     : RasterWorkerPool(resource_provider, num_threads),
       shutdown_(false),
       scheduled_raster_task_count_(0),
       bytes_pending_upload_(0),
       has_performed_uploads_since_last_flush_(false),
@@ skipping 83 matching lines @@
   }
 
   tasks_required_for_activation_.clear();
   for (TaskMap::iterator it = new_pixel_buffer_tasks.begin();
        it != new_pixel_buffer_tasks.end(); ++it) {
     internal::RasterWorkerPoolTask* task = it->first;
     if (IsRasterTaskRequiredForActivation(task))
       tasks_required_for_activation_.insert(task);
   }
 
+  // |tasks_required_for_activation_| contains all tasks that need to
+  // complete before we can send a "ready to activate" signal. Tasks
+  // that have already completed should not be part of this set.
+  for (TaskDeque::const_iterator it = completed_tasks_.begin();

[vmpstr, 2013/07/10 16:46:49]

I think we can fold this loop and the one above into the remaining two loops in
this function. Having separate loops is a bit clearer, but I'm just wary that
we're doing multiple iterations through the tasks when we really don't have to.
Wdyt?

[reveman, 2013/07/11 00:37:21]

This and ScheduleMoreTasks needs a rewrite at some point but I'd rather not mess
with any of that in this patch but instead keep this as simple as possible for
now.

+       it != completed_tasks_.end(); ++it) {
+    tasks_required_for_activation_.erase(*it);
+  }
+
   pixel_buffer_tasks_.swap(new_pixel_buffer_tasks);
 
   // Check for completed tasks when ScheduleTasks() is called as
   // priorities might have changed and this maximizes the number
   // of top priority tasks that are scheduled.
   RasterWorkerPool::CheckForCompletedTasks();
   CheckForCompletedUploads();
   FlushUploads();
 
   // Schedule new tasks.
@@ skipping 189 matching lines @@
   TRACE_EVENT_ASYNC_STEP1(
       "cc", "ScheduledTasks", this, StateName(),
       "state", TracedValue::FromValue(StateAsValue().release()));
 
   // Schedule another check for completed raster tasks while there are
   // pending raster tasks or pending uploads.
   if (HasPendingTasks())
     ScheduleCheckForCompletedRasterTasks();
 
   // Generate client notifications.
-  if (will_notify_client_that_no_tasks_required_for_activation_are_pending)
+  if (will_notify_client_that_no_tasks_required_for_activation_are_pending) {
+    DCHECK(std::find_if(raster_tasks_required_for_activation().begin(),
+                        raster_tasks_required_for_activation().end(),
+                        WasCanceled) ==
+          raster_tasks_required_for_activation().end());
     client()->DidFinishedRunningTasksRequiredForActivation();
+  }
   if (will_notify_client_that_no_tasks_are_pending) {
     TRACE_EVENT_ASYNC_END0("cc", "ScheduledTasks", this);
+    DCHECK(!HasPendingTasksRequiredForActivation());
     client()->DidFinishedRunningTasks();
   }
 }
 
 void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
   TRACE_EVENT0("cc", "PixelBufferRasterWorkerPool::ScheduleMoreTasks");
 
   enum RasterTaskType {
     PREPAINT_TYPE = 0,
     REQUIRED_FOR_ACTIVATION_TYPE = 1,
@@ skipping 149 matching lines @@
                "was_canceled", was_canceled,
                "needs_upload", needs_upload);
 
   DCHECK(pixel_buffer_tasks_.find(task.get()) != pixel_buffer_tasks_.end());
 
   // Balanced with MapPixelBuffer() call in ScheduleMoreTasks().
   resource_provider()->UnmapPixelBuffer(task->resource()->id());
 
   if (!needs_upload) {
     resource_provider()->ReleasePixelBuffer(task->resource()->id());
+
+    if (was_canceled) {

[vmpstr, 2013/07/10 16:46:49]

Does was_canceled guarantee that needs_upload is false?

[reveman, 2013/07/11 00:37:21]

Yes, see PixelBufferWorkerPoolTaskImpl above. I added a DCHECK below in in
latest patch that verifies this.

+      // When priorites change, a raster task can be canceled as a result of
+      // no longer being of high enough priority to fit in our throttled
+      // raster task budget. The task has not yet completed in this case.
+      if (std::find(raster_tasks().begin(),

[vmpstr, 2013/07/10 16:46:49]

I prefer you save the iterator first, then do the if check

[reveman, 2013/07/11 00:37:21]

Done.

+                    raster_tasks().end(),
+                    task) != raster_tasks().end()) {
+        pixel_buffer_tasks_[task.get()] = NULL;
+        return;
+      }
+    }
+
     task->DidRun(was_canceled);
     DCHECK(std::find(completed_tasks_.begin(),
                      completed_tasks_.end(),
                      task) == completed_tasks_.end());
     completed_tasks_.push_back(task);
     tasks_required_for_activation_.erase(task);
     return;
   }
 
   resource_provider()->BeginSetPixels(task->resource()->id());
@@ skipping 48 matching lines @@
 
   throttle_state->SetInteger("bytes_available_for_upload",
                              kMaxPendingUploadBytes - bytes_pending_upload_);
   throttle_state->SetInteger("bytes_pending_upload", bytes_pending_upload_);
   throttle_state->SetInteger("scheduled_raster_task_count",
                              scheduled_raster_task_count_);
   return throttle_state.PassAs<base::Value>();
 }
 
 }  // namespace cc