cc: Do GatherPixelRefs from skia at record time

cc/resources/picture.cc

Index: cc/resources/picture.cc
diff --git a/cc/resources/picture.cc b/cc/resources/picture.cc
index b54e08f31d0234911c3de6cdd0625609284e10ce..f6b508783f764c1171cddf0700b89ae215935032 100644
--- a/cc/resources/picture.cc
+++ b/cc/resources/picture.cc
@@ -4,6 +4,10 @@
 
 #include "cc/resources/picture.h"
 
+#include <algorithm>
+#include <limits>
+#include <set>
+
 #include "base/base64.h"
 #include "base/debug/trace_event.h"
 #include "cc/debug/rendering_stats.h"
@@ -21,12 +25,20 @@
 namespace {
 // URI label for a lazily decoded SkPixelRef.
 const char kLabelLazyDecoded[] = "lazy";
+
 // Version ID; to be used in serialization.
 const int kPictureVersion = 1;
+
 // Minimum size of a decoded stream that we need.
 // 4 bytes for version, 4 * 4 for each of the 2 rects.
 const unsigned int kMinPictureSizeBytes = 36;
 
+int ToMultiple(int value, int multiple) {

[enne (OOO), 2013/04/23 01:59:39]

Should this be moved to cc/base/util.h as RoundDown, equivalent to RoundUp?

[vmpstr, 2013/04/23 20:02:23]

Done.

+  return (value < 0) ?
+      (value / multiple - 1) * multiple
+      : (value / multiple) * multiple;
+}
+
 class DisableLCDTextFilter : public SkDrawFilter {
  public:
   // SkDrawFilter interface.
@@ -101,10 +113,12 @@ Picture::Picture(const std::string& encoded_string, bool* success) {
 
 Picture::Picture(const skia::RefPtr<SkPicture>& picture,
                  gfx::Rect layer_rect,
-                 gfx::Rect opaque_rect) :
+                 gfx::Rect opaque_rect,
+                 const PixelRefMap& lazy_pixel_refs) :
     layer_rect_(layer_rect),
     opaque_rect_(opaque_rect),
-    picture_(picture) {
+    picture_(picture),
+    lazy_pixel_refs_(lazy_pixel_refs) {
 }
 
 Picture::~Picture() {
@@ -130,7 +144,8 @@ void Picture::CloneForDrawing(int num_threads) {
     scoped_refptr<Picture> clone = make_scoped_refptr(
         new Picture(skia::AdoptRef(new SkPicture(clones[i])),
                     layer_rect_,
-                    opaque_rect_));
+                    opaque_rect_,
+                    lazy_pixel_refs_));
     clones_.push_back(clone);
   }
 }
@@ -182,6 +197,15 @@ void Picture::Record(ContentLayerClient* painter,
   picture_->endRecording();
 
   opaque_rect_ = gfx::ToEnclosedRect(opaque_layer_rect);
+
+  cell_size_ = gfx::Size(

[enne (OOO), 2013/04/23 01:59:39]

For what it's worth, this isn't going to line up with the tile grid.  You're
going to be off-by-one, and so will hit four tile grid cells for every one cell
here.

That said, I don't think it really matters.  Honestly, any resolution that is
about the average size of images will probably work out ok.  Maybe you'll decode
something you don't need, but most of the time some other high priority tile
will need it and it won't be a waste.

[reveman, 2013/04/23 14:33:24]

I would vote for getting this to line up properly in this patch so we only hit
one cell when using an identity transform.

I think it's worth getting the common case as fast as possible on the cc thread
as we'll be iterating over a large set of tiles and all their pixel refs inside
AssignGpuMemoryToTiles in the near future.

[vmpstr, 2013/04/23 20:02:23]

I think I fixed this. Right now, when we do gather pixels from skia, we do it
for a rect x,y w by h. When computing the indecies, we can just use x,y w-1 by
h-1 rect. That way, if the only overlap is the right/bottom boundary, then we
will only check one index (but still capture refs in w by h). If the overlap is
bigger than one pixel, then we need to (correctly) check more indecies.

+      tile_grid_info.fTileInterval.width() +
+          2 * tile_grid_info.fMargin.width(),
+      tile_grid_info.fTileInterval.height() +
+          2 * tile_grid_info.fMargin.height());
+  DCHECK_GT(cell_size_.width(), 0);
+  DCHECK_GT(cell_size_.height(), 0);
+  GatherAllPixelRefs();
 }
 
 void Picture::Raster(
@@ -207,14 +231,15 @@ void Picture::Raster(
   canvas->restore();
 }
 
-void Picture::GatherPixelRefs(const gfx::Rect& layer_rect,
-                              std::list<skia::LazyPixelRef*>& pixel_ref_list) {
+void Picture::GatherPixelRefsFromSkia(
+    const gfx::Rect& query_rect,

[enne (OOO), 2013/04/23 01:59:39]

const gfx::Rect& => gfx::Rect

[vmpstr, 2013/04/23 20:02:23]

Done.

+    std::list<skia::LazyPixelRef*>& pixel_ref_list) {

[reveman, 2013/04/23 14:33:24]

nit: while you're here. I think the style guide recommends passing return values
by pointer rather than reference.

[vmpstr, 2013/04/23 20:02:23]

Done.

   DCHECK(picture_);
   SkData* pixel_refs = SkPictureUtils::GatherPixelRefs(
-      picture_.get(), SkRect::MakeXYWH(layer_rect.x(),
-                                       layer_rect.y(),
-                                       layer_rect.width(),
-                                       layer_rect.height()));
+      picture_.get(), SkRect::MakeXYWH(query_rect.x() - layer_rect_.x(),

[enne (OOO), 2013/04/23 01:59:39]

Haha.  Sorry for misreading earlier.  Calling it query_rect helps a lot.

[vmpstr, 2013/04/23 20:02:23]

Yeah, I vote in general to avoid naming completely different things with names
that differ only by an underscore :P

+                                       query_rect.y() - layer_rect_.y(),
+                                       query_rect.width(),
+                                       query_rect.height()));
   if (!pixel_refs)
     return;
 
@@ -262,4 +287,138 @@ void Picture::AsBase64String(std::string* output) const {
                      output);
 }
 
+void Picture::GatherAllPixelRefs() {
+  int min_x = std::numeric_limits<int>::max();
+  int min_y = std::numeric_limits<int>::max();
+  int max_x = 0;
+  int max_y = 0;
+
+  // Capture pixel refs for this picture in a grid
+  // with cell_size_ sized cells.
+  for (int y = layer_rect_.y();
+      y < layer_rect_.bottom();
+      y += cell_size_.height()) {
+    for (int x = layer_rect_.x();
+        x < layer_rect_.right();
+        x += cell_size_.width()) {
+      gfx::Size extent(std::min(cell_size_.width(), layer_rect_.right() - x),

[enne (OOO), 2013/04/23 01:59:39]

I think these lines would be more clearly put as:

gfx::Rect rect(gfx::Point(x, y), cell_size_);
rect.intersect(layer_rect_);

[vmpstr, 2013/04/23 20:02:23]

Done.

+                       std::min(cell_size_.height(), layer_rect_.bottom() - y));
+      gfx::Rect rect(x, y, extent.width(), extent.height());
+
+      std::list<skia::LazyPixelRef*> lazy_pixel_refs;
+      GatherPixelRefsFromSkia(rect, lazy_pixel_refs);
+
+      // Only capture non-empty cells.
+      if (!lazy_pixel_refs.empty()) {
+        lazy_pixel_refs_[PixelRefMapKey(x, y)].swap(lazy_pixel_refs);
+        min_x = std::min(min_x, x);
+        min_y = std::min(min_y, y);
+        max_x = std::max(max_x, x);
+        max_y = std::max(max_y, y);
+      }
+    }
+  }
+
+  min_lazy_pixel_cell_ = gfx::Point(min_x, min_y);
+  max_lazy_pixel_cell_ = gfx::Point(max_x, max_y);
+}
+
+Picture::LazyPixelRefIterator::LazyPixelRefIterator()
+    : picture_(NULL),
+      current_list_(NULL),
+      current_pixel_ref_(NULL),
+      min_point_(-1, -1),
+      max_point_(-1, -1),
+      current_x_(0),
+      current_y_(0) {
+}
+
+Picture::LazyPixelRefIterator::LazyPixelRefIterator(
+    gfx::Rect rect,

[enne (OOO), 2013/04/23 01:59:39]

rect => query_rect?

[vmpstr, 2013/04/23 20:02:23]

Done.

+    const Picture* picture)
+    : picture_(picture),
+      current_list_(NULL),
+      current_pixel_ref_(NULL) {
+  gfx::Rect layer_rect = picture->layer_rect_;
+  gfx::Size cell_size = picture->cell_size_;
+
+  // We have to find a cell_size aligned point that
+  // corresponds to the given rect. First, subtract the layer origin,
+  // then ensure the point is a multiple of cell_size,
+  // and finally, add the layer origin back.
+  min_point_ = gfx::Point(
+      ToMultiple(rect.x() - layer_rect.x(), cell_size.width())
+          + layer_rect.x(),
+      ToMultiple(rect.y() - layer_rect.y(), cell_size.height())
+          + layer_rect.y());
+  max_point_ = gfx::Point(
+      ToMultiple(rect.right() - layer_rect.x(), cell_size.width())
+          + layer_rect.x(),
+      ToMultiple(rect.bottom() - layer_rect.y(), cell_size.height())
+          + layer_rect.y());
+
+  // Limit the points to knows pixel ref boundaries.

[enne (OOO), 2013/04/23 01:59:39]

knows => known?

[vmpstr, 2013/04/23 20:02:23]

Oops. Fixed.

+  min_point_ = gfx::Point(
+      std::max(min_point_.x(), picture->min_lazy_pixel_cell_.x()),
+      std::max(min_point_.y(), picture->min_lazy_pixel_cell_.y()));
+  max_point_ = gfx::Point(
+      std::min(max_point_.x(), picture->max_lazy_pixel_cell_.x()),
+      std::min(max_point_.y(), picture->max_lazy_pixel_cell_.y()));
+
+  // Make the current x be cell_size.width() less than min point, so that
+  // the first increment will point at min_point_.
+  current_x_ = min_point_.x() - cell_size.width();
+  current_y_ = min_point_.y();
+  ++(*this);
+}
+
+Picture::LazyPixelRefIterator::~LazyPixelRefIterator() {
+}
+
+Picture::LazyPixelRefIterator& Picture::LazyPixelRefIterator::operator++() {
+  // If we're not at the end of the list, then just get the next item.
+  if (current_list_ &&
+      current_list_iterator_ != current_list_->end()) {

[reveman, 2013/04/23 14:33:24]

I think this would be cleaner if you used a sentinel node here instead of
allowing current_list_ to be NULL. you can get rid of |current_pixel_ref_| and
|current_list_iterator_| would be the current position instead of being one step
ahead, which I find awkward. should also make this a bit more efficient.

[vmpstr, 2013/04/23 20:02:23]

Done.

+    current_pixel_ref_ = *current_list_iterator_;
+    ++current_list_iterator_;
+    return *this;
+  }
+
+  // If we already passed the max y, do nothing.
+  if (current_y_ > max_point_.y())
+    return *this;
+
+  while (true) {
+    gfx::Size cell_size = picture_->cell_size_;
+
+    // Advance the current grid cell.
+    current_x_ += cell_size.width();
+    if (current_x_ > max_point_.x()) {
+      current_y_ += cell_size.height();
+      current_x_ = min_point_.x();
+      // Min x can be greater than max x if the query rect
+      // does not intersect the layer rect.
+      if (current_y_ > max_point_.y() ||
+          current_x_ > max_point_.x()) {
+        current_pixel_ref_ = NULL;
+        break;
+      }
+    }
+
+    // If there are no pixel refs at this grid cell, keep incrementing.
+    PixelRefMap::const_iterator iter =
+        picture_->lazy_pixel_refs_.find(PixelRefMapKey(current_x_, current_y_));

[reveman, 2013/04/23 14:33:24]

nit: think it might be a bit cleaner to put "PixelRefMapKey key(current_x_,
current_y_)" on a separate line.

[vmpstr, 2013/04/23 20:02:23]

Done.

+    if (iter == picture_->lazy_pixel_refs_.end())
+      continue;
+
+    // We found a non-empty list: store it and get the first pixel ref.
+    current_list_ = &iter->second;
+    current_list_iterator_ = current_list_->begin();
+    current_pixel_ref_ = *current_list_iterator_;
+    ++current_list_iterator_;
+    break;
+  }
+  return *this;
+}
+
 }  // namespace cc