cc: Do GatherPixelRefs from skia at record time

cc/resources/picture_pile_impl.cc

Index: cc/resources/picture_pile_impl.cc
diff --git a/cc/resources/picture_pile_impl.cc b/cc/resources/picture_pile_impl.cc
index dd30ede82a80d8a7fe20e416d671725fde8a66c4..b3d711f5e74789ef74121067b42c500724c00948 100644
--- a/cc/resources/picture_pile_impl.cc
+++ b/cc/resources/picture_pile_impl.cc
@@ -198,31 +198,6 @@ int64 PicturePileImpl::Raster(
   return total_pixels_rasterized;
 }
 
-void PicturePileImpl::GatherPixelRefs(
-    gfx::Rect content_rect,
-    float contents_scale,
-    std::list<skia::LazyPixelRef*>& pixel_refs) {
-  std::list<skia::LazyPixelRef*> result;
-
-  gfx::Rect layer_rect = gfx::ToEnclosingRect(
-      gfx::ScaleRect(content_rect, 1.f / contents_scale));
-
-  for (TilingData::Iterator tile_iter(&tiling_, layer_rect);
-       tile_iter; ++tile_iter) {
-    PictureListMap::iterator map_iter =
-        picture_list_map_.find(tile_iter.index());
-    if (map_iter == picture_list_map_.end())
-      continue;
-
-    PictureList& pic_list = map_iter->second;
-    for (PictureList::const_iterator i = pic_list.begin();
-         i != pic_list.end(); ++i) {
-      (*i)->GatherPixelRefs(layer_rect, result);
-      pixel_refs.splice(pixel_refs.end(), result);
-    }
-  }
-}
-
 skia::RefPtr<SkPicture> PicturePileImpl::GetFlattenedPicture() {
   TRACE_EVENT0("cc", "PicturePileImpl::GetFlattenedPicture");
 
@@ -277,4 +252,62 @@ PicturePileImpl::Analysis::Analysis()
 PicturePileImpl::Analysis::~Analysis() {
 }
 
+PicturePileImpl::PixelRefIterator::PixelRefIterator(
+    gfx::Rect content_rect,
+    float contents_scale,
+    const PicturePileImpl* picture_pile)
+    : picture_pile_(picture_pile),
+      layer_rect_(gfx::ToEnclosingRect(
+          gfx::ScaleRect(content_rect, 1.f / contents_scale))),
+      tile_iterator_(&picture_pile_->tiling_, layer_rect_),
+      picture_list_(NULL),
+      current_pixel_ref_(NULL) {
+  ++(*this);
+}
+
+PicturePileImpl::PixelRefIterator::~PixelRefIterator() {
+}
+
+PicturePileImpl::PixelRefIterator&
+    PicturePileImpl::PixelRefIterator::operator++() {

[reveman, 2013/04/24 14:10:52]

I feel like this could be cleaner. It's hard to follow the flow of this function
and I don't like that we have a current_pixel_ref_ and keep the iterators one
step ahead of the current position. Maybe splitting it up into functions by
doing something like this:

bool PixelRefIterator::AdvanceToValidTile() {
  DCHECK(tile_iterator_);
  do {
    PictureListMap::const_iterator map_iter =
        picture_list_map_.find(tile_iterator_.index());
    if (map_iter != picture_list_map_.end()) {
      picture_list_ = map_iter->second;
      picture_list_iterator_ = picture_list_->begin();
      return true;
    }
  } while (++tile_iterator_);

  return false;
}

bool PixelRefIterator::AdvanceToValidPicture() {
  do {
    if (picture_list_iterator_ != picture_list_->end()) {
      pixel_ref_iterator_ = Picture::PixelRefIterator(
          layer_rect_,
          *picture_list_iterator_);
      return true;
    }

    ++tile_iterator_;
  } while (AdvanceToValidTile());

  return false;
}

PixelRefIterator& PixelRefIterator::operator++() {
  ++pixel_ref_iterator_;
  do {
    if (pixel_ref_iterator_)
      return *this;

    ++picture_list_iterator_;
  } while (AdvanceToValidPicture());

  return *this;
}

and we do something like the following in ctor rather than ++(*this):

PixelRefIterator::PixelRefIterator() {
  if (AdvanceToValidTile()) {
    while (AdvanceToValidPicture()) {
      if (pixel_ref_iterator_)
        break;

      ++picture_list_iterator_;
    }
  }
}

Wdyt?

[vmpstr, 2013/04/24 21:11:10]

I played around with it, and settled to something that is very similar to your
proposal.

+  while (true) {
+    // If there are still pixel refs on the current picture,
+    // advance the picture refs iterator.
+    if (picture_refs_iterator_) {
+      current_pixel_ref_ = *picture_refs_iterator_;
+      ++picture_refs_iterator_;
+      break;
+    }
+
+    // If there are pictures left in the current tile's list, process them.
+    if (picture_list_ &&
+        picture_list_iterator_ != picture_list_->end()) {
+      scoped_refptr<Picture> picture = *picture_list_iterator_;
+      ++picture_list_iterator_;
+      picture_refs_iterator_ = Picture::PixelRefIterator(
+          layer_rect_,
+          picture.get());
+      continue;
+    }
+
+    // If we're out of tiles, we're done.
+    if (!tile_iterator_) {
+      current_pixel_ref_ = NULL;
+      break;
+    }
+
+    // Grab a new picture list from the current tile,
+    // and advance the tile iterator.
+    PictureListMap::const_iterator map_iterator =
+        picture_pile_->picture_list_map_.find(tile_iterator_.index());
+    ++tile_iterator_;
+    if (map_iterator == picture_pile_->picture_list_map_.end())
+      continue;
+
+    picture_list_ = &map_iterator->second;
+    picture_list_iterator_ = picture_list_->begin();
+  }
+  return *this;
+}
+
 }  // namespace cc