Implement the logic to kick off image decoding jobs for TileManager

cc/tile_manager.cc

Index: cc/tile_manager.cc
diff --git a/cc/tile_manager.cc b/cc/tile_manager.cc
index 5eface4e7d9bfc6fa4e6fdadcd503ed2f02d368e..1b8237fc36bca99aa65d71efe55680eb5090a51e 100644
--- a/cc/tile_manager.cc
+++ b/cc/tile_manager.cc
@@ -12,11 +12,13 @@
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "cc/image.h"
 #include "cc/platform_color.h"
 #include "cc/rendering_stats.h"
 #include "cc/resource_pool.h"
 #include "cc/switches.h"
 #include "cc/tile.h"
+#include "skia/ext/lazy_pixel_ref.h"
 #include "third_party/skia/include/core/SkDevice.h"
 
 namespace {
@@ -160,6 +162,8 @@ void TileManager::ManageTiles() {
 
   const bool smoothness_takes_priority = global_state_.smoothness_takes_priority;
 
+  decoding_progress_.clear();

[reveman, 2012/12/05 19:48:46]

ManageTiles() might be called while images are being decoded. I assume it would
be incorrect to clear all image decoding progress in that case.

[qinmin, 2012/12/07 05:06:28]

Since this variable is changed to keep track of all pending decoding tasks,
removed this line here.

On 2012/12/05 19:48:46, David Reveman wrote:

+
   // Bin into three categories of tiles: things we need now, things we need soon, and eventually
   for (TileVector::iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
     Tile* tile = *it;
@@ -196,6 +200,15 @@ void TileManager::ManageTiles() {
     }
 
     mts.bin = EVENTUALLY_BIN;
+
+    // Update all the SkPixelRefs this tile intersects.
+    mts.pixel_refs.clear();
+    mts.pixel_refs =
+        const_cast<PicturePileImpl *>(tile->picture_pile())->GatherPixelRefs(
+            tile->rect_inside_picture_);
+    for (std::vector<SkPixelRef*>::iterator it = mts.pixel_refs.begin();
+        it != mts.pixel_refs.end(); ++it)
+      decoding_progress_[*it] = NOT_STARTED;

[reveman, 2012/12/05 19:48:46]

The pixel refs for a tile should never change, right? It seems unnecessary to
gather them every time ManagedTiles() is called. Looking at the skia
implementation, GatherPixelRefs can be a bit costly. It would be nice to not
make this call until we know that the tile is going to be rasterized.

[qinmin, 2012/12/07 05:06:28]

Changed this to only get the skpixelref information for tiles in NOW_BIN.
Delayed it for other tiles until we start to kick off image decoding jobs.

On 2012/12/05 19:48:46, David Reveman wrote:

   }
 
   // Memory limit policy works by mapping some bin states to the NEVER bin.
@@ -300,17 +313,91 @@ void TileManager::FreeResourcesForTile(Tile* tile) {
 }
 
 void TileManager::DispatchMoreRasterTasks() {
-  while (!tiles_that_need_to_be_rasterized_.empty()) {
-    int max_pending_tasks = kNumPendingRasterTasksPerThread *
-        kMaxRasterThreads;
-
+  int max_pending_tasks = kNumPendingRasterTasksPerThread *
+      kMaxRasterThreads;
+  bool all_tiles_waiting_for_decoder = false;
+  while (!tiles_that_need_to_be_rasterized_.empty() &&
+      !all_tiles_waiting_for_decoder) {
     // Stop dispatching raster tasks when too many are pending.
     if (pending_raster_tasks_ >= max_pending_tasks)
       break;
+    all_tiles_waiting_for_decoder = true;
+    // TODO(qinmin): Use a separate queue for tiles that are doing image
+    // decoding. However, we need to find a good way to keep track of
+    // the tile priority. Otherwise, one queue may starve the other queue.
+    TileVector::reverse_iterator it;
+    for (it = tiles_that_need_to_be_rasterized_.rbegin();
+        it != tiles_that_need_to_be_rasterized_.rend(); ++it) {
+      std::vector<SkPixelRef*> pixel_refs;
+      if (HasUndecodedImages(*it, pixel_refs)) {
+        if (pixel_refs.empty())
+          continue;
+        for (std::vector<SkPixelRef*>::iterator it = pixel_refs.begin();
+            it != pixel_refs.end(); ++it) {
+          SpawnImageDecodingTask(*it);
+          // If we are reaching the task limit, just finish here and wait for
+          // tasks to finish.
+          if (pending_raster_tasks_ >= max_pending_tasks)
+            return;
+        }
+      } else {
+        DispatchOneRasterTask(*it);
+        all_tiles_waiting_for_decoder = false;
+        // Be cautious that we are erasing a reverse iterator.
+        tiles_that_need_to_be_rasterized_.erase(--it.base());
+        break;
+      }
+    }

[reveman, 2012/12/05 19:48:46]

I think we're starting to do too much in this function. Could we add an utility
function to make this easier to read? Maybe DispatchMoreImageDecodingTasks.

[qinmin, 2012/12/07 05:06:28]

Done. Now all the jobs waiting on image decoding goes to
tiles_waiting_for_image_decoding_. When one image decoding task finishes, we
check that queue first, before we check the remaining task in the
tiles_that_need_to_be_rasterized_ queue.

+  }
+}
 
-    DispatchOneRasterTask(tiles_that_need_to_be_rasterized_.back());
-    tiles_that_need_to_be_rasterized_.pop_back();
+bool TileManager::HasUndecodedImages(Tile* tile,

[reveman, 2012/12/05 19:48:46]

Could we just remove SkPixelRefs from managed_state.pixel_refs as they are
started? It looks to me like we could replace decoding_progress_ with a
managed_state.pending_image_decoding_tasks member if we remove SkPixelRefs as we
post decoding tasks.

[reveman, 2012/12/05 20:28:35]

I guess we still need a map with all the pixel refs currently being decoded so
we don't call PrepareToDecode on a pixel ref currently being decoded.

[qinmin, 2012/12/07 05:06:28]

we cannot remove SkPixelRef if the decoding task just started. Since if a
decoding job finishes, we need that information to check whether all decoding
tasks have finished. But we can remove a SkPixelRef from the pixel_refs when an
image decoding is done. 
And we still need the decoding_progress_ as a new tile can figure out whether an
image is currently in decoding, or has finished decoding.

+                                     std::vector<SkPixelRef*>& unstarted) {
+  bool has_undecoded_images = false;
+  SkPixelRef* decode_not_started = 0;
+  ManagedTileState& managed_state = tile->managed_state();
+  for (std::vector<SkPixelRef*>::iterator it = managed_state.pixel_refs.begin();
+      it != managed_state.pixel_refs.end(); ++it) {
+    bool not_started = false;
+    switch(decoding_progress_[*it]) {
+    case FINISHED:
+      break;
+    case NOT_STARTED:
+      not_started = true;
+    case IN_PROGRESS:
+      scoped_refptr<Image> image(Image::Create(*it));
+      // If image is already in cache, don't spawn decode task.
+      if (!image->PrepareToDecode()) {
+        has_undecoded_images = true;
+        if (not_started)
+          unstarted.push_back(*it);
+      }
+    }
   }
+  return has_undecoded_images;
+}
+
+void TileManager::SpawnImageDecodingTask(SkPixelRef* pixel_ref) {
+  TRACE_EVENT0("cc", "TileManager::SpawnImageDecodingTask");
+  scoped_refptr<Image> image(Image::Create(pixel_ref));

[reveman, 2012/12/05 19:48:46]

Doesn't look like image need to be ref counted. Can you make this scoped_ptr<>
or just use Image* and delete the image in OnImageDecoded().

[reveman, 2012/12/05 20:28:35]

Would it make sense to add a ScopedPtrHashMap<SkPixelRef*, Image> to hold images
during decode and track what pixel refs are currently in use?

[qinmin, 2012/12/07 05:06:28]

Removed the image class. However, I am not sure whether skPixelRef* is safe to
be posted on to the raster thread.  Should we use scoped_refptr to get hold of a
tile, or a picture before posting the task?

On 2012/12/05 20:28:35, David Reveman wrote:

+  // Kicking off a image decode job.
+  ++pending_raster_tasks_;
+  worker_pool_->GetTaskRunnerWithShutdownBehavior(
+      base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)->PostTaskAndReply(
+          FROM_HERE,
+          base::Bind(&Image::Decode,

[Alpha Left Google, 2012/12/05 20:24:28]

This is fine now but just be aware that after this is done the cache is not
guaranteed to stick. We can get more stats later to see how many re-decodings
are happening.

[qinmin, 2012/12/07 05:06:28]

Yes, redecoding can be problematic if the cache got cleared before we put the
tile onto rasterization.

On 2012/12/05 20:24:28, Alpha wrote:

+                     image),
+          base::Bind(&TileManager::OnImageDecoded,
+                     base::Unretained(this),

[reveman, 2012/12/05 19:48:46]

Each decoding task is associated with a tile and decoding the tile region of
pixel ref doesn't mean it's decoded for another tile region, right? Maybe you
should add scoped_refptr<Tile> to OnImageDecoded parameters.

[qinmin, 2012/12/07 05:06:28]

I think when we kick off the image decoder, we always decode the whole picture.
Although image decoder can cache part of the picture. This is just to make the
implementation easier.
Otherwise, we have to have knowledge of whether another tile is decoding the
same image, and what is the decoding region.

So prepareToDecode() can return whether the particular region is cached, but
decode() always decode the whole image. Also, this makes the life of image
decoder much easier. Otherwise, 4 tiles may lauch 4 decoding tasks on the same
image with different rects.

On 2012/12/05 19:48:46, David Reveman wrote:

+                     image));
+  decoding_progress_[pixel_ref] = IN_PROGRESS;
+}
+
+void TileManager::OnImageDecoded(scoped_refptr<Image> image) {
+  TRACE_EVENT0("cc", "TileManager::OnImageDecoded");
+  --pending_raster_tasks_;
+  decoding_progress_[image->pixel_ref()] = FINISHED;
+  DispatchMoreRasterTasks();
 }
 
 void TileManager::DispatchOneRasterTask(scoped_refptr<Tile> tile) {