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..832cb096945be36c91f2c20d31c4cee4c0912f5c 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,18 @@
 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;
 
+// Size of each of the grid cells for storing lazy pixel refs.
+const int kRegionWidth = 512;
+const int kRegionHeight = 512;

[reveman, 2013/04/22 19:56:06]

Can we avoid these constants and use SkTileGridPicture::TileGridInfo instead?

[vmpstr, 2013/04/22 22:38:16]

Done.

+
 class DisableLCDTextFilter : public SkDrawFilter {
  public:
   // SkDrawFilter interface.
@@ -101,10 +111,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 PixelRefsMap& lazy_pixel_refs) :

[enne (OOO), 2013/04/22 19:10:56]

PixelRefsMap => PixelRefMap?

[vmpstr, 2013/04/22 22:38:16]

Done.

     layer_rect_(layer_rect),
     opaque_rect_(opaque_rect),
-    picture_(picture) {
+    picture_(picture),
+    lazy_pixel_refs_(lazy_pixel_refs) {
 }
 
 Picture::~Picture() {
@@ -130,7 +142,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 +195,8 @@ void Picture::Record(ContentLayerClient* painter,
   picture_->endRecording();
 
   opaque_rect_ = gfx::ToEnclosedRect(opaque_layer_rect);
+
+  GatherAllPixelRefs();
 }
 
 void Picture::Raster(
@@ -207,12 +222,13 @@ 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& layer_rect,
+    std::list<skia::LazyPixelRef*>& pixel_ref_list) {
   DCHECK(picture_);
   SkData* pixel_refs = SkPictureUtils::GatherPixelRefs(
-      picture_.get(), SkRect::MakeXYWH(layer_rect.x(),
-                                       layer_rect.y(),
+      picture_.get(), SkRect::MakeXYWH(layer_rect.x() - layer_rect_.x(),
+                                       layer_rect.y() - layer_rect_.y(),

[enne (OOO), 2013/04/22 19:10:56]

I think you mean 0? ;)

Is the reason for this because of the translation applied at recording/raster
time?
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...

Do you think it would be more clear if all pictures were left in layer
coordinates, rather than being translated to some origin?

[vmpstr, 2013/04/22 22:38:16]

No, it's actually a different rect... I renamed the argument one to "query_rect"
to avoid confusion.
Yes, that's the reason. If we get rid of the translates, then we have to tell
skia that we need a picture and recording canvas of size layer+offset. I'm sure
that would work fine (since memory isn't actually allocated here), but it feels
like it is a worse solution than what we have now. I think the translates are
fairly clean... Wdyt?

                                        layer_rect.width(),
                                        layer_rect.height()));
   if (!pixel_refs)
@@ -262,4 +278,124 @@ 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 kRegionWidth by kRegionHeight cells.
+  for (int y = layer_rect_.y(); y < layer_rect_.bottom(); y += kRegionHeight) {
+    for (int x = layer_rect_.x(); x < layer_rect_.right(); x += kRegionWidth) {
+      gfx::Size extent(std::min(kRegionWidth, layer_rect_.right() - x),
+                       std::min(kRegionHeight, 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);

[enne (OOO), 2013/04/22 19:10:56]

If only Skia could just give us bounding rectangles directly...

[vmpstr, 2013/04/22 22:38:16]

Yep. Maybe they will in the future :D

+
+      // Only capture non-empty cells.
+      if (!lazy_pixel_refs.empty()) {
+        lazy_pixel_refs_[std::make_pair(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()

[enne (OOO), 2013/04/22 19:10:56]

Ok, this makes sense.  Thanks for the default ctor test.  :)

+  : picture_(0),

[reveman, 2013/04/22 19:56:06]

nit: indent 4 spaces

[vmpstr, 2013/04/22 22:38:16]

Done.

+    current_pixel_ref_(0),
+    min_point_(-1, -1),
+    max_point_(-1, -1),
+    current_x_(0),
+    current_y_(0) {
+}
+
+Picture::LazyPixelRefIterator::LazyPixelRefIterator(
+    gfx::Rect rect,
+    const Picture* picture)
+    : picture_(picture),
+      current_pixel_ref_(NULL) {
+  gfx::Rect layer_rect = picture->layer_rect_;
+
+  // We have to find a kRegionWidth/kRegionHeight aligned point that
+  // corresponds to the given rect. First, subtract the layer origin,
+  // then ensure the point is a multiple of kRegionWidth/kRegionHeight,
+  // and finally, add the layer origin back.
+  min_point_ = gfx::Point(
+      ((rect.x() - layer_rect.x()) / kRegionWidth) * kRegionWidth
+          + layer_rect.x(),
+      ((rect.y() - layer_rect.y()) / kRegionHeight) * kRegionHeight
+          + layer_rect.y());
+  max_point_ = gfx::Point(
+      ((rect.right() - layer_rect.x()) / kRegionWidth) * kRegionWidth
+          + layer_rect.x(),
+      ((rect.bottom() - layer_rect.y()) / kRegionHeight) * kRegionHeight
+          + layer_rect.y());
+
+  // Limit the points to knows pixel ref boundaries.
+  min_point_ = gfx::Point(
+      std::max(min_point_.x(), picture->min_lazy_pixel_cell_.x()),

[enne (OOO), 2013/04/22 19:10:56]

I think this is wrong.  lazy_pixel_cell is in layer space.  min_point has been
recentered to have layer_rect.origin() at (0, 0).  Can you add a test where this
would be caught?

(This increases my belief that we should not have that translation in
record/raster, since it's a confusing complication about what space things are
in.)

[vmpstr, 2013/04/22 22:38:16]

This is actually in layer space, since I add layer_rect back in. The way I think
about it is that all operations on cc::Picture are in layer space, and only when
we want to deal with SkPicture, we translate to have origin at 0,0. The
non-intersecting tests that I've added should catch these problems.

+      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 kRegionWidth less than min point, so that
+  // the first increment will point at min_point_.
+  current_x_ = min_point_.x() - kRegionWidth;
+  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_.empty()) {
+    current_pixel_ref_ = current_list_.front();
+    current_list_.pop_front();
+    return *this;
+  }
+
+  // If we already passed the max y, do nothing.
+  if (current_y_ > max_point_.y())
+    return *this;
+
+  while (true) {
+    // Advance the current grid cell.
+    current_x_ += kRegionWidth;
+    if (current_x_ > max_point_.x()) {
+      current_y_ += kRegionHeight;
+      current_x_ = min_point_.x();
+      if (current_y_ > max_point_.y()) {
+        current_pixel_ref_ = NULL;
+        break;
+      }
+    }
+
+    // If there are no pixel refs at this grid cell, keep incrementing.
+    PixelRefsMap::const_iterator iter =
+        picture_->lazy_pixel_refs_.find(std::make_pair(current_x_, current_y_));
+    if (iter == picture_->lazy_pixel_refs_.end() || iter->second.empty())

[reveman, 2013/04/22 19:56:06]

can "iter->second.empty()" ever be true?

[vmpstr, 2013/04/22 22:38:16]

Nope, I removed it.

+      continue;
+
+    // We found a non-empty list: store it and get the first pixel ref.
+    current_list_ = iter->second;

[reveman, 2013/04/22 19:56:06]

Can we avoid copying this list while iterating? looks like making current_list_
a reference and adding std::list<skia::LazyPixelRef*>::iterator would allow you
to do this. Being able to iterate over pixel refs without performing heap
allocations would be nice..

[vmpstr, 2013/04/22 22:38:16]

Done.

+    current_pixel_ref_ = current_list_.front();
+    current_list_.pop_front();
+    break;
+  }
+  return *this;
+}
+
 }  // namespace cc