Makes tile-creation lazy

cc/resources/picture_layer_tiling.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/resources/picture_layer_tiling.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/debug/trace_event.h"
 #include "cc/base/math_util.h"
 #include "ui/gfx/point_conversions.h"
 #include "ui/gfx/rect_conversions.h"
 #include "ui/gfx/safe_integer_conversions.h"
 #include "ui/gfx/size_conversions.h"
 
 namespace cc {
 
 scoped_ptr<PictureLayerTiling> PictureLayerTiling::Create(
     float contents_scale) {
   return make_scoped_ptr(new PictureLayerTiling(contents_scale));
 }
 
 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)
     : client_(NULL),
       contents_scale_(contents_scale),
       tiling_data_(gfx::Size(), gfx::Size(), true),
       resolution_(NON_IDEAL_RESOLUTION),
       last_source_frame_number_(0),
       last_impl_frame_time_(0) {
 }
@@ skipping 14 matching lines @@
 }
 
 Tile* PictureLayerTiling::TileAt(int i, int j) const {
   TileMap::const_iterator iter = tiles_.find(TileMapKey(i, j));
   if (iter == tiles_.end())
     return NULL;
   return iter->second.get();
 }
 
 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;
 }
 
 Region PictureLayerTiling::OpaqueRegionInContentRect(
     const gfx::Rect& content_rect) const {
   Region opaque_region;
   // TODO(enne): implement me
   return opaque_region;
 }
 
 void PictureLayerTiling::SetLayerBounds(gfx::Size layer_bounds) {
   if (layer_bounds_ == layer_bounds)
     return;
 
   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);
 
-  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());
+  // 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()));
   }
 }
 
-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());
-  }
-}
-
 PictureLayerTiling::Iterator::Iterator()
     : tiling_(NULL),
       current_tile_(NULL),
       tile_i_(0),
       tile_j_(0),
       left_(0),
       top_(0),
       right_(-1),
       bottom_(-1) {
 }
@@ skipping 146 matching lines @@
     float current_layer_contents_scale,
     const gfx::Transform& last_screen_transform,
     const gfx::Transform& current_screen_transform,
     int current_source_frame_number,
     double current_frame_time,
     bool store_screen_space_quads_on_tiles,
     size_t max_tiles_for_interest_area) {
   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_;
 
   bool first_update_in_new_impl_frame =
       current_frame_time != last_impl_frame_time_;
 
   // In pending tree, this is always called. We update priorities:
   // - Immediately after a commit (first_update_in_new_source_frame).
   // - On animation ticks after the first frame in the tree
   //   (first_update_in_new_impl_frame).
   // In active tree, this is only called during draw. We update priorities:
   // - On draw if properties were not already computed by the pending tree
   //   and activated for the frame (first_update_in_new_impl_frame).
   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_;
 
   // Fast path tile priority calculation when both transforms are translations.
   if (last_screen_transform.IsIdentityOrTranslation() &&
       current_screen_transform.IsIdentityOrTranslation()) {
     gfx::Vector2dF current_offset(
         current_screen_transform.matrix().get(0, 3),
         current_screen_transform.matrix().get(1, 3));
     gfx::Vector2dF last_offset(
         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())
         continue;
       Tile* tile = find->second.get();
 
       gfx::Rect tile_bounds =
           tiling_data_.TileBounds(iter.index_x(), iter.index_y());
       gfx::RectF current_screen_rect = gfx::ScaleRect(
           tile_bounds,
@@ skipping 12 matching lines @@
               last_screen_rect, current_screen_rect, time_delta, view_rect);
       TilePriority priority(
           resolution_,
           time_to_visible_in_seconds,
           distance_to_visible_in_pixels);
       if (store_screen_space_quads_on_tiles)
         priority.set_current_screen_quad(gfx::QuadF(current_screen_rect));
       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())
         continue;
       Tile* tile = find->second.get();
 
       gfx::Rect tile_bounds =
           tiling_data_.TileBounds(iter.index_x(), iter.index_y());
       gfx::RectF current_layer_content_rect = gfx::ScaleRect(
           tile_bounds,
@@ skipping 27 matching lines @@
                               &clipped));
       }
       tile->SetPriority(tree, priority);
     }
   }
 
   last_source_frame_number_ = current_source_frame_number;
   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));
     it->second->SetPriority(PENDING_TREE, TilePriority());
 
     // Tile holds a ref onto a picture pile. If the tile never gets invalidated
     // and recreated, then that picture pile ref could exist indefinitely.  To
     // prevent this, ask the client to update the pile to its own ref.  This
     // will cause PicturePileImpls and their clones to get deleted once the
     // corresponding PictureLayerImpl and any in flight raster jobs go out of
@@ skipping 105 matching lines @@
 
     // If our delta is less then our event distance, we're done.
     if (delta < event.distance)
       break;
   }
 
   return gfx::Rect(origin_x, origin_y, width, height);
 }
 
 }  // namespace cc