cc: Plumb more details about pixel refs to tile manager.

cc/playback/pixel_ref_map.cc

Index: cc/playback/pixel_ref_map.cc
diff --git a/cc/playback/pixel_ref_map.cc b/cc/playback/pixel_ref_map.cc
index cf8c93503aad37f5f85b0c2bf04ef3b5038fac6e..a7df3042944bb86122b6a450450d2975d8d49467 100644
--- a/cc/playback/pixel_ref_map.cc
+++ b/cc/playback/pixel_ref_map.cc
@@ -21,7 +21,8 @@ PixelRefMap::PixelRefMap(const gfx::Size& cell_size) : cell_size_(cell_size) {
 PixelRefMap::~PixelRefMap() {
 }
 
-void PixelRefMap::GatherPixelRefsFromPicture(SkPicture* picture) {
+void PixelRefMap::GatherPixelRefsFromPicture(SkPicture* picture,
+                                             const gfx::Rect& layer_rect) {
   DCHECK(picture);
 
   int min_x = std::numeric_limits<int>::max();
@@ -45,10 +46,24 @@ void PixelRefMap::GatherPixelRefsFromPicture(SkPicture* picture) {
                        static_cast<int>(std::ceil(it->pixel_ref_rect.bottom())),
                        cell_size_.height()));
 
+    // We recorded the picture as if it was at (0, 0) by translating by layer

[reed1, 2015/08/10 17:58:38]

"We recorded the picture as if it was at (0, 0) by translating by layer rect
origin."

I like the comment, but it feels like there is implicit knowledge here, and that
if blink changed its behavior somehow, this assumption might break. Perhaps not,
and I am misunderstanding the larger picture.

On the topic of larger picture, is this an equivalent way to express the flow:?

- GatherPixelRefs job is to return a set of rectangles in global/device
coordinates, so they can be compared against tile boundaries
- To do this in a single picture, we can either transform the local bounds by
the CTM (per-call) or we can record the CTM per image.
- For nested pictures, that themselves might be transformed, we need to account
for the CTM at the time the sub-pictures was instantiated.

If this is approximately correct, perhaps a more obvious CL would be to pass the
CTM as a parameter to GatherPixelsRefs. Then on sub-pictures, it could update
that matrix before it does the recursive call on the sub-picture.

[weiliangc, 2015/08/10 18:09:45]

My understanding is that Pixel Refs are in layer/picture space, and we gather
them per SkPicture?

[vmpstr, 2015/08/11 21:00:49]

The translation happens during record time in the compositor, so it's not really
blink that's responsible for the offset. I think this part of the code is
sufficiently tied to the recording side of this that this should be OK (ie, if
the recording code changes it should be obvious that other parts of the same
system might need updating). 
GatherPixelRefs should return information relative to the given picture (ie as
if the picture was at (0, 0)). It does return a CTM for each of the pixel refs
in position_pixel_ref, as well as the rect. The rect is properly returned as
being at (0, 0)... I think it's nicer though to make it be relative to the
layer, since picture pile's calling code isn't aware of picture pile's
implementation details. I think technically CTM could be updated as well, but
I'm a bit hesitant to mess with it, since we really only need scale out of it
(for decoding), and the actual position doesn't really affect the decode.

The subpicture process (if this picture contains another picture) should also be
handled naturally by this, since the returning values are going to be relative
to the top level picture. 

+    // rect origin. However, now the matrix stores this offset, and the rect is

[weiliangc, 2015/08/10 18:09:45]

Overall comment is good. I was mostly confused by "However, now the matrix
stores this offset, and the rect is also offset." What is "the matrix" and "the
rect" here? Maybe just remove this sentence is ok?

[vmpstr, 2015/08/11 21:00:49]

Done.

+    // also offset. Add the rect origin back here. It really doesn't make much
+    // of a difference, since the query for pixel refs doesn't use this
+    // information. However, since picture pile / display list also returns this
+    // information, it would be nice to express it relative to the layer, not
+    // relative to the particular implementation of the raster source.
+    skia::PositionPixelRef position_pixel_ref = *it;
+    position_pixel_ref.pixel_ref_rect.setXYWH(
+        position_pixel_ref.pixel_ref_rect.x() + layer_rect.x(),
+        position_pixel_ref.pixel_ref_rect.y() + layer_rect.y(),
+        position_pixel_ref.pixel_ref_rect.width(),
+        position_pixel_ref.pixel_ref_rect.height());
+
     for (int y = min.y(); y <= max.y(); y += cell_size_.height()) {
       for (int x = min.x(); x <= max.x(); x += cell_size_.width()) {
         PixelRefMapKey key(x, y);
-        data_hash_map_[key].push_back(it->pixel_ref);
+        data_hash_map_[key].push_back(position_pixel_ref);
       }
     }