Makes tile-creation lazy

cc/resources/picture_layer_tiling.cc

Index: cc/resources/picture_layer_tiling.cc
diff --git a/cc/resources/picture_layer_tiling.cc b/cc/resources/picture_layer_tiling.cc
index e25b86b90b31c8ab7d83b212623b3e106e4cc414..c0c458b92e8b8367284c319711d1f1f7fc55dd1f 100644
--- a/cc/resources/picture_layer_tiling.cc
+++ b/cc/resources/picture_layer_tiling.cc
@@ -22,7 +22,15 @@ scoped_ptr<PictureLayerTiling> PictureLayerTiling::Create(
 }
 
 scoped_ptr<PictureLayerTiling> PictureLayerTiling::Clone() const {
-  return make_scoped_ptr(new PictureLayerTiling(*this));
+  PictureLayerTiling* out = new PictureLayerTiling(contents_scale_);

[enne (OOO), 2013/03/29 20:50:58]

style nit: please don't ever use unwrapped raw pointers with no owner.

You could instead say:
scoped_ptr<PictureLayerTiling> clone(new PictureLayerTiling(contents_scale_));
// ...
return clone.Pass();

[whunt, 2013/03/29 21:52:04]

Okay, this is a style nit through right?  Due to out being local to the stack
before the return and no exceptions occurring the current code cannot ever lose
the pointer before calling make_scoped_ptr, corret?

[enne (OOO), 2013/03/29 22:34:20]

Yes.  Your code as written is logically correct.

+  out->contents_scale_ = contents_scale_;
+  out->layer_bounds_ = layer_bounds_;
+  out->resolution_ = resolution_;
+  out->client_ = client_;
+  out->tiling_data_ = tiling_data_;
+  out->last_source_frame_number_ = last_source_frame_number_;
+  out->last_impl_frame_time_ = last_impl_frame_time_;
+  return make_scoped_ptr(out);
 }
 
 PictureLayerTiling::PictureLayerTiling(float contents_scale)
@@ -57,10 +65,42 @@ Tile* PictureLayerTiling::TileAt(int i, int j) const {
 }
 
 void PictureLayerTiling::CreateTile(int i, int j) {
-  gfx::Rect tile_rect = tiling_data_.TileBoundsWithBorder(i, j);
-  tile_rect.set_size(tiling_data_.max_texture_size());
   TileMapKey key(i, j);
   DCHECK(tiles_.find(key) == tiles_.end());
+
+  gfx::Rect paint_rect = tiling_data_.TileBoundsWithBorder(i, j);
+  gfx::Rect tile_rect = paint_rect;
+  tile_rect.set_size(tiling_data_.max_texture_size());
+
+  // Check our twin for a valid tile.
+  const PictureLayerTiling* twin = client_->GetTwinTiling(this);
+  if (twin) {
+    Tile* candidate_tile = twin->TileAt(i, j);
+    if (candidate_tile && tiling_data_.max_texture_size() ==

[enne (OOO), 2013/03/29 20:50:58]

These two conditions aren't really tied together.  Maybe you should check the
max texture size up front before you even get the twin tiling?

[whunt, 2013/03/29 21:52:04]

done.

+        twin->tiling_data_.max_texture_size()) {
+
+      // Loop over the invalidation region explicitly because we have

[enne (OOO), 2013/03/29 20:50:58]

As a simplification suggestion, what about scaling the tile bounds to layer
space and then just using Region::Intersects?

[whunt, 2013/03/29 21:52:04]

Is that safe?  We did some rounding when generated these tile bounds in the
first place.  I'm not sure if the inverse scaling would be conservative given we
push the points of the tiles around to avoid cracks and overlap?  I guess we're
comparing scaled values and scales *shouldn't* affect the comparison.  I was
following what was done before (which looked like this), but you know this
better than I do.

[enne (OOO), 2013/03/29 22:34:20]

I think this is safe to do in both directions if you do it conservatively using
ToEnclosingRect.

+      // to scale each rect from layer space to content space.
+      bool invalid = false;
+      for (Region::Iterator region_iter(*client_->GetInvalidation());
+           !invalid && region_iter.has_rect();
+           region_iter.next()) {
+
+        gfx::Rect layer_rect = region_iter.rect();
+        gfx::Rect rect =
+            gfx::ToEnclosingRect(ScaleRect(layer_rect, contents_scale_));
+
+        if (rect.Intersects(paint_rect))
+          invalid = true;
+      }
+      if (!invalid) {
+        tiles_[key] = candidate_tile;
+        return;
+      }
+    }
+  }
+
+  // Create a new tile because our twin didn't have a valid one.
   scoped_refptr<Tile> tile = client_->CreateTile(this, tile_rect);
   if (tile)
     tiles_[key] = tile;
@@ -79,95 +119,30 @@ void PictureLayerTiling::SetLayerBounds(gfx::Size layer_bounds) {
 
   gfx::Size old_layer_bounds = layer_bounds_;
   layer_bounds_ = layer_bounds;
+
+  // Calculate and set the new content bounds.
   gfx::Size old_content_bounds = tiling_data_.total_size();
   gfx::Size content_bounds =
       gfx::ToCeiledSize(gfx::ScaleSize(layer_bounds_, contents_scale_));
-
   tiling_data_.SetTotalSize(content_bounds);
-  if (layer_bounds_.IsEmpty()) {
-    tiles_.clear();
-    return;
-  }
 
+  // Handle the case where the size of our tiling has changed.
   gfx::Size tile_size = client_->CalculateTileSize(
       tiling_data_.max_texture_size(),
       content_bounds);
   if (tile_size != tiling_data_.max_texture_size()) {
     tiling_data_.SetMaxTextureSize(tile_size);
     tiles_.clear();
-    CreateTilesFromLayerRect(gfx::Rect(layer_bounds_));
+    interest_rect_ = gfx::Rect();

[enne (OOO), 2013/03/29 20:50:58]

https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti...

[whunt, 2013/03/29 21:52:04]

I think my previous response was just incorrect.  If we shrink the tile slightly
and let the old tile remain the lazy walk will discover no new area and generate
no new tiles.  We need to set the interest_rect_ to null if we clear the tiles,
that way they get re-created correctly.  This can be tested for.

[enne (OOO), 2013/03/29 22:34:20]

Ah, that makes a lot of sense.  Thanks for the explanation.

     return;
   }
 
-  // Any tiles outside our new bounds are invalid and should be dropped.
-  if (old_content_bounds.width() > content_bounds.width() ||
-      old_content_bounds.height() > content_bounds.height()) {
-    int right =
-        tiling_data_.TileXIndexFromSrcCoord(content_bounds.width() - 1);
-    int bottom =
-        tiling_data_.TileYIndexFromSrcCoord(content_bounds.height() - 1);
-
-    std::vector<TileMapKey> invalid_tile_keys;
-    for (TileMap::const_iterator it = tiles_.begin();
-         it != tiles_.end(); ++it) {
-      if (it->first.first > right || it->first.second > bottom)
-        invalid_tile_keys.push_back(it->first);
-    }
-    for (size_t i = 0; i < invalid_tile_keys.size(); ++i)
-      tiles_.erase(invalid_tile_keys[i]);
-  }
-
-  // Create tiles for newly exposed areas.
-  Region layer_region((gfx::Rect(layer_bounds_)));
-  layer_region.Subtract(gfx::Rect(old_layer_bounds));
-  for (Region::Iterator iter(layer_region); iter.has_rect(); iter.next()) {
-    Invalidate(iter.rect());
-    CreateTilesFromLayerRect(iter.rect());
-  }
-}
-
-void PictureLayerTiling::Invalidate(const Region& layer_invalidation) {
-  std::vector<TileMapKey> new_tiles;
-
-  for (Region::Iterator region_iter(layer_invalidation);
-       region_iter.has_rect();
-       region_iter.next()) {
-    gfx::Rect layer_invalidation = region_iter.rect();
-    layer_invalidation.Intersect(gfx::Rect(layer_bounds_));
-    gfx::Rect rect =
-        gfx::ToEnclosingRect(ScaleRect(layer_invalidation, contents_scale_));
-
-    for (PictureLayerTiling::Iterator tile_iter(this, contents_scale_, rect);
-         tile_iter;
-         ++tile_iter) {
-      TileMapKey key(tile_iter.tile_i_, tile_iter.tile_j_);
-      TileMap::iterator found = tiles_.find(key);
-      if (found == tiles_.end())
-        continue;
-
-      tiles_.erase(found);
-      new_tiles.push_back(key);
-    }
-  }
-
-  for (size_t i = 0; i < new_tiles.size(); ++i)
-    CreateTile(new_tiles[i].first, new_tiles[i].second);
-}
-
-void PictureLayerTiling::CreateTilesFromLayerRect(gfx::Rect layer_rect) {
-  gfx::Rect content_rect =
-      gfx::ToEnclosingRect(ScaleRect(layer_rect, contents_scale_));
-  CreateTilesFromContentRect(content_rect);
-}
-
-void PictureLayerTiling::CreateTilesFromContentRect(gfx::Rect content_rect) {
-  for (TilingData::Iterator iter(&tiling_data_, content_rect); iter; ++iter) {
-    TileMap::iterator found =
-        tiles_.find(TileMapKey(iter.index_x(), iter.index_y()));
-    // Ignore any tiles that already exist.
-    if (found != tiles_.end())
-      continue;
-    CreateTile(iter.index_x(), iter.index_y());
+  // Delete all tiles that contained previously unpainted regions.
+  for (TilingData::DifferenceIterator iter(&tiling_data_,

[enne (OOO), 2013/03/29 20:50:58]

I think this loop will be a no-op because you have already set the tiling data's
total size above to something smaller than old_content_bounds.  I think if you
write a unit test for this and you will see what I am trying to get at.

[whunt, 2013/03/29 21:52:04]

I don't understand.  If the new content bounds are bigger than the old content
bounds, we need to invalidate the partially pained edge tiles.  This difference
iterator should cover those tiles.

[enne (OOO), 2013/03/29 22:34:20]

Put another way, what is the maximum index that TilingData::Iterator or
TilingData::DifferenceIterator will consider?

+                                           gfx::Rect(content_bounds),
+                                           gfx::Rect(old_content_bounds));
+      iter; ++iter) {
+    tiles_.erase(TileMapKey(iter.index()));
   }
 }
 
@@ -337,6 +312,23 @@ void PictureLayerTiling::UpdateTilePriorities(
   if (ContentRect().IsEmpty())
     return;
 
+  gfx::Rect viewport_in_content_space =
+      gfx::ToEnclosingRect(gfx::ScaleRect(viewport_in_layer_space,
+                                          contents_scale_));
+
+  gfx::Size tile_size = tiling_data_.max_texture_size();
+  int64 interest_rect_area =
+      max_tiles_for_interest_area * tile_size.width() * tile_size.height();
+
+  gfx::Rect interest_rect = ExpandRectEquallyToAreaBoundedBy(
+      viewport_in_content_space,
+      interest_rect_area,
+      ContentRect());
+  DCHECK(ContentRect().Contains(interest_rect));
+
+  ManageTiles(interest_rect_, interest_rect);
+  interest_rect_ = interest_rect;
+
   bool first_update_in_new_source_frame =
       current_source_frame_number != last_source_frame_number_;
 
@@ -353,43 +345,11 @@ void PictureLayerTiling::UpdateTilePriorities(
   if (!first_update_in_new_impl_frame && !first_update_in_new_source_frame)
     return;
 
-  double time_delta = 0;
-  if (last_impl_frame_time_ != 0 && last_layer_bounds == current_layer_bounds)
+  double time_delta = 0.0;
+  if (last_impl_frame_time_ != 0.0 &&
+      last_layer_bounds == current_layer_bounds)
     time_delta = current_frame_time - last_impl_frame_time_;
 
-  gfx::Rect viewport_in_content_space =
-      gfx::ToEnclosingRect(gfx::ScaleRect(viewport_in_layer_space,
-                                          contents_scale_));
-
-  gfx::Size tile_size = tiling_data_.max_texture_size();
-  int64 prioritized_rect_area =
-      max_tiles_for_interest_area *
-      tile_size.width() * tile_size.height();
-
-  gfx::Rect prioritized_rect = ExpandRectEquallyToAreaBoundedBy(
-      viewport_in_content_space,
-      prioritized_rect_area,
-      ContentRect());
-  DCHECK(ContentRect().Contains(prioritized_rect));
-
-  // Iterate through all of the tiles that were live last frame but will
-  // not be live this frame, and mark them as being dead.
-  for (TilingData::DifferenceIterator iter(&tiling_data_,
-                                           last_prioritized_rect_,
-                                           prioritized_rect);
-       iter;
-       ++iter) {
-    TileMap::iterator find = tiles_.find(iter.index());
-    if (find == tiles_.end())
-      continue;
-
-    TilePriority priority;
-    DCHECK(!priority.is_live);

[enne (OOO), 2013/03/29 20:50:58]

Can you also remove is_live from tile priority and the tile manager in this
patch, as all tiles will be marked live with your change?

[whunt, 2013/03/29 21:52:04]

From my understanding, all tiles should be live (we no longer have a loop that
sets them to dead either).  Would you like me to roll that into this patch?  I
talked to Nat and decided that removing the live_or_allocated_tiles from
tile-manager was outside the scope of this cl but is_live seems closer.  It's
your call.

[enne (OOO), 2013/03/29 22:34:20]

That's fine to leave it out and follow up later.  I just wasn't privy to that
conversation where that was decided and just wanted to know what your plan was.

[whunt, 2013/03/29 23:17:36]

It was just an informal discussion in the cubes like 3 hours ago.  I offered to
pull in more to this patch and his suggestion was to keep it more manageable for
the reviewers (which is mostly you, so is you're call =) )

-    Tile* tile = find->second.get();
-    tile->SetPriority(tree, priority);
-  }
-  last_prioritized_rect_ = prioritized_rect;
-
   gfx::Rect view_rect(device_viewport);
   float current_scale = current_layer_contents_scale / contents_scale_;
   float last_scale = last_layer_contents_scale / contents_scale_;
@@ -404,7 +364,7 @@ void PictureLayerTiling::UpdateTilePriorities(
         last_screen_transform.matrix().get(0, 3),
         last_screen_transform.matrix().get(1, 3));
 
-    for (TilingData::Iterator iter(&tiling_data_, prioritized_rect);
+    for (TilingData::Iterator iter(&tiling_data_, interest_rect);
          iter; ++iter) {
       TileMap::iterator find = tiles_.find(iter.index());
       if (find == tiles_.end())
@@ -437,7 +397,7 @@ void PictureLayerTiling::UpdateTilePriorities(
       tile->SetPriority(tree, priority);
     }
   } else {
-    for (TilingData::Iterator iter(&tiling_data_, prioritized_rect);
+    for (TilingData::Iterator iter(&tiling_data_, interest_rect);
          iter; ++iter) {
       TileMap::iterator find = tiles_.find(iter.index());
       if (find == tiles_.end())
@@ -485,6 +445,34 @@ void PictureLayerTiling::UpdateTilePriorities(
   last_impl_frame_time_ = current_frame_time;
 }
 
+void PictureLayerTiling::ManageTiles(const gfx::Rect& old_interest_rect,
+                                     const gfx::Rect& new_interest_rect) {
+  if (old_interest_rect == new_interest_rect)
+    return;
+
+  // Iterate to delete all tiles outside of our new interest rect.
+  for (TilingData::DifferenceIterator iter(&tiling_data_,
+                                           old_interest_rect,
+                                           new_interest_rect);
+      iter; ++iter) {

[enne (OOO), 2013/03/29 20:50:58]

style nit: Could you put "iter" and "++iter" on different lines if your for loop
doesn't fit on one line, here and elsewhere? I find that easier to read, and
it's what clang-format does automatically.

[whunt, 2013/03/29 21:52:04]

Sure.

+    TileMapKey key(iter.index());
+    TileMap::iterator found = tiles_.find(key);
+    if (found != tiles_.end() &&
+        !new_interest_rect.Intersects(found->second->content_rect()))

[enne (OOO), 2013/03/29 20:50:58]

What are you trying to get at with this condition?

[whunt, 2013/03/29 21:52:04]

we're trying to find tiles that are entirely outside of our new interest rect. 
The difference iterator between the new and old interest rects can find border
tiles that are not outside the new interest rect and do not need to be erased.

+      tiles_.erase(found);
+  }
+
+  // Iterate to allocate new tiles for all regions with newly exposed area.
+  for (TilingData::DifferenceIterator iter(&tiling_data_,
+                                           new_interest_rect,
+                                           old_interest_rect);
+      iter; ++iter) {
+    TileMapKey key(iter.index());
+    TileMap::iterator found = tiles_.find(key);
+    CreateTile(key.first, key.second);
+  }
+}
+
 void PictureLayerTiling::DidBecomeActive() {
   for (TileMap::const_iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
     it->second->SetPriority(ACTIVE_TREE, it->second->priority(PENDING_TREE));