cc: Start using raster/eviction iterators.

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 0229879da606154bf09e76d505383be0bb7c2178..d69a9e014b1db5a28e577806b4873a8af747a618 100644
--- a/cc/resources/picture_layer_tiling.cc
+++ b/cc/resources/picture_layer_tiling.cc
@@ -54,13 +54,6 @@ class TileEvictionOrder {
   TreePriority tree_priority_;
 };
 
-void ReleaseTile(Tile* tile, WhichTree tree) {
-  // Reset priority as tile is ref-counted and might still be used
-  // even though we no longer hold a reference to it here anymore.
-  tile->SetPriority(tree, TilePriority());
-  tile->set_shared(false);
-}
-
 }  // namespace
 
 scoped_ptr<PictureLayerTiling> PictureLayerTiling::Create(
@@ -81,6 +74,7 @@ PictureLayerTiling::PictureLayerTiling(float contents_scale,
       client_(client),
       tiling_data_(gfx::Size(), gfx::Size(), true),
       last_impl_frame_time_in_seconds_(0.0),
+      content_to_screen_scale_(1.0f),
       has_visible_rect_tiles_(false),
       has_skewport_rect_tiles_(false),
       has_soon_border_rect_tiles_(false),
@@ -107,7 +101,7 @@ PictureLayerTiling::PictureLayerTiling(float contents_scale,
 
 PictureLayerTiling::~PictureLayerTiling() {
   for (TileMap::const_iterator it = tiles_.begin(); it != tiles_.end(); ++it)
-    ReleaseTile(it->second.get(), client_->GetTree());
+    it->second->set_shared(false);
 }
 
 void PictureLayerTiling::SetClient(PictureLayerTilingClient* client) {
@@ -133,6 +127,8 @@ Tile* PictureLayerTiling::CreateTile(int i,
           gfx::ScaleToEnclosingRect(paint_rect, 1.0f / contents_scale_);
       if (!client_->GetInvalidation()->Intersects(rect)) {
         DCHECK(!candidate_tile->is_shared());
+        DCHECK_EQ(i, candidate_tile->tiling_i_index());
+        DCHECK_EQ(j, candidate_tile->tiling_j_index());
         candidate_tile->set_shared(true);
         tiles_[key] = candidate_tile;
         return candidate_tile;
@@ -144,6 +140,7 @@ Tile* PictureLayerTiling::CreateTile(int i,
   scoped_refptr<Tile> tile = client_->CreateTile(this, tile_rect);
   if (tile.get()) {
     DCHECK(!tile->is_shared());
+    tile->set_tiling_index(i, j);
     tiles_[key] = tile;
   }
   return tile.get();
@@ -466,7 +463,7 @@ bool PictureLayerTiling::RemoveTileAt(int i,
   TileMap::iterator found = tiles_.find(TileMapKey(i, j));
   if (found == tiles_.end())
     return false;
-  ReleaseTile(found->second.get(), client_->GetTree());
+  found->second->set_shared(false);
   tiles_.erase(found);
   if (recycled_twin) {
     // Recycled twin does not also have a recycled twin, so pass NULL.
@@ -479,7 +476,7 @@ void PictureLayerTiling::Reset() {
   live_tiles_rect_ = gfx::Rect();
   PictureLayerTiling* recycled_twin = client_->GetRecycledTwinTiling(this);
   for (TileMap::const_iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
-    ReleaseTile(it->second.get(), client_->GetTree());
+    it->second->set_shared(false);
     if (recycled_twin)
       recycled_twin->RemoveTileAt(it->first.first, it->first.second, NULL);
   }
@@ -555,16 +552,18 @@ void PictureLayerTiling::UpdateTilePriorities(
     return;
   }
 
+  // Calculate the skewport.
+  gfx::Rect skewport = ComputeSkewport(current_frame_time_in_seconds,
+                                       visible_rect_in_content_space);
+  DCHECK(skewport.Contains(visible_rect_in_content_space));
+
+  // Calculate the eventually/live tiles rect.
   size_t max_tiles_for_interest_area = client_->GetMaxTilesForInterestArea();
 
   gfx::Size tile_size = tiling_data_.max_texture_size();
   int64 eventually_rect_area =
       max_tiles_for_interest_area * tile_size.width() * tile_size.height();
 
-  gfx::Rect skewport = ComputeSkewport(current_frame_time_in_seconds,
-                                       visible_rect_in_content_space);
-  DCHECK(skewport.Contains(visible_rect_in_content_space));
-
   gfx::Rect eventually_rect =
       ExpandRectEquallyToAreaBoundedBy(visible_rect_in_content_space,
                                        eventually_rect_area,
@@ -576,6 +575,13 @@ void PictureLayerTiling::UpdateTilePriorities(
       << "tiling_size: " << tiling_size().ToString()
       << " eventually_rect: " << eventually_rect.ToString();
 
+  // Calculate the soon border rect.
+  content_to_screen_scale_ = ideal_contents_scale / contents_scale_;
+  gfx::Rect soon_border_rect = visible_rect_in_content_space;
+  float border = kSoonBorderDistanceInScreenPixels / content_to_screen_scale_;
+  soon_border_rect.Inset(-border, -border, -border, -border);
+
+  // Update the tiling state.
   SetLiveTilesRect(eventually_rect);
 
   last_impl_frame_time_in_seconds_ = current_frame_time_in_seconds;
@@ -583,104 +589,20 @@ void PictureLayerTiling::UpdateTilePriorities(
 
   eviction_tiles_cache_valid_ = false;
 
-  TilePriority now_priority(resolution_, TilePriority::NOW, 0);
-  float content_to_screen_scale = ideal_contents_scale / contents_scale_;
-
-  // Assign now priority to all visible tiles.
-  bool include_borders = false;
-  has_visible_rect_tiles_ = false;
-  for (TilingData::Iterator iter(
-           &tiling_data_, visible_rect_in_content_space, include_borders);
-       iter;
-       ++iter) {
-    TileMap::iterator find = tiles_.find(iter.index());
-    if (find == tiles_.end())
-      continue;
-    has_visible_rect_tiles_ = true;
-    Tile* tile = find->second.get();
-
-    tile->SetPriority(tree, now_priority);
-
-    // Set whether tile is occluded or not.
-    gfx::Rect tile_query_rect = ScaleToEnclosingRect(
-        IntersectRects(tile->content_rect(), visible_rect_in_content_space),
-        1.0f / contents_scale_);
-    bool is_occluded = occlusion_in_layer_space.IsOccluded(tile_query_rect);
-    tile->set_is_occluded(tree, is_occluded);
-  }
-
-  // Assign soon priority to skewport tiles.
-  has_skewport_rect_tiles_ = false;
-  for (TilingData::DifferenceIterator iter(
-           &tiling_data_, skewport, visible_rect_in_content_space);
-       iter;
-       ++iter) {
-    TileMap::iterator find = tiles_.find(iter.index());
-    if (find == tiles_.end())
-      continue;
-    has_skewport_rect_tiles_ = true;
-    Tile* tile = find->second.get();
-
-    gfx::Rect tile_bounds =
-        tiling_data_.TileBounds(iter.index_x(), iter.index_y());
-
-    float distance_to_visible =
-        visible_rect_in_content_space.ManhattanInternalDistance(tile_bounds) *
-        content_to_screen_scale;
-
-    TilePriority priority(resolution_, TilePriority::SOON, distance_to_visible);
-    tile->SetPriority(tree, priority);
-  }
-
-  // Assign eventually priority to interest rect tiles.
-  has_eventually_rect_tiles_ = false;
-  for (TilingData::DifferenceIterator iter(
-           &tiling_data_, eventually_rect, skewport);
-       iter;
-       ++iter) {
-    TileMap::iterator find = tiles_.find(iter.index());
-    if (find == tiles_.end())
-      continue;
-    has_eventually_rect_tiles_ = true;
-    Tile* tile = find->second.get();
-
-    gfx::Rect tile_bounds =
-        tiling_data_.TileBounds(iter.index_x(), iter.index_y());
-
-    float distance_to_visible =
-        visible_rect_in_content_space.ManhattanInternalDistance(tile_bounds) *
-        content_to_screen_scale;
-    TilePriority priority(
-        resolution_, TilePriority::EVENTUALLY, distance_to_visible);
-    tile->SetPriority(tree, priority);
-  }
-
-  // Upgrade the priority on border tiles to be SOON.
-  gfx::Rect soon_border_rect = visible_rect_in_content_space;
-  float border = kSoonBorderDistanceInScreenPixels / content_to_screen_scale;
-  soon_border_rect.Inset(-border, -border, -border, -border);
-  has_soon_border_rect_tiles_ = false;
-  for (TilingData::DifferenceIterator iter(
-           &tiling_data_, soon_border_rect, skewport);
-       iter;
-       ++iter) {
-    TileMap::iterator find = tiles_.find(iter.index());
-    if (find == tiles_.end())
-      continue;
-    has_soon_border_rect_tiles_ = true;
-    Tile* tile = find->second.get();
-
-    TilePriority priority(resolution_,
-                          TilePriority::SOON,
-                          tile->priority(tree).distance_to_visible);
-    tile->SetPriority(tree, priority);
-  }
-
-  // Update iteration rects.
   current_visible_rect_ = visible_rect_in_content_space;
   current_skewport_rect_ = skewport;
   current_soon_border_rect_ = soon_border_rect;
   current_eventually_rect_ = eventually_rect;
+  current_occlusion_in_layer_space_ = occlusion_in_layer_space;
+
+  // Update has_*_tiles state.
+  const gfx::Rect& tiling_rect = gfx::Rect(tiling_size());

[danakj, 2014/09/19 01:41:47]

why const&? just gfx::Rect tiling_rect(tiling_size()); ?

[vmpstr, 2014/09/19 21:22:53]

Done.

+
+  has_visible_rect_tiles_ = tiling_rect.Intersects(current_visible_rect_);
+  has_skewport_rect_tiles_ = tiling_rect.Intersects(current_skewport_rect_);
+  has_soon_border_rect_tiles_ =
+      tiling_rect.Intersects(current_soon_border_rect_);
+  has_eventually_rect_tiles_ = tiling_rect.Intersects(current_eventually_rect_);
 }
 
 void PictureLayerTiling::SetLiveTilesRect(
@@ -741,29 +663,103 @@ void PictureLayerTiling::VerifyLiveTilesRect() {
 #endif
 }
 
-void PictureLayerTiling::DidBecomeRecycled() {
-  // DidBecomeActive below will set the active priority for tiles that are
-  // still in the tree. Calling this first on an active tiling that is becoming
-  // recycled takes care of tiles that are no longer in the active tree (eg.
-  // due to a pending invalidation).
-  for (TileMap::const_iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
-    it->second->SetPriority(ACTIVE_TREE, TilePriority());
+bool PictureLayerTiling::IsTileOccluded(const Tile* tile) const {
+  DCHECK(tile);
+  gfx::Rect tile_query_rect = ScaleToEnclosingRect(

[danakj, 2014/09/19 01:41:47]

This does int->float->int work. Can you do this only if contents_scale_ != 1?

Maybe also consider a current_occlusion_.HasOcclusion() early out so the
renderer doesn't pay for the browser compositor's requirements here? Or TODO?

[vmpstr, 2014/09/19 21:22:52]

Done.
Added a TODO

+      IntersectRects(tile->content_rect(), current_visible_rect_),
+      1.0f / contents_scale_);
+  // Explicitly check if the tile is outside the viewport. If so, we need to

[danakj, 2014/09/19 01:41:47]

This isn't really right if current_visible_rect_ is the layer's
visible_content_rect() which it is.. sometimes.

i'm not sure why we ever use the visible_content_rect() in tile prioritization.
the viewport projected to the layer seems strictly better.

Can you put a TODO that this is wrong for tiles clipped out but inside the
viewport if current_visible_rect_ is not the viewport?

[vmpstr, 2014/09/19 21:22:52]

Done.

+  // return false, since occlusion for this tile is unknown.
+  if (tile_query_rect.IsEmpty())
+    return false;
+  bool is_occluded =
+      current_occlusion_in_layer_space_.IsOccluded(tile_query_rect);

[danakj, 2014/09/19 01:41:47]

just return this instead of using a variable?

[vmpstr, 2014/09/19 21:22:52]

Done.

+  return is_occluded;
+}
+
+bool PictureLayerTiling::IsTileRequiredForActivation(const Tile* tile) const {
+  if (resolution_ != HIGH_RESOLUTION)

[danakj, 2014/09/19 01:41:47]

we used to consider tiles that were higher res than raster scale or ideal scale
as enough or so. can you add a TODO about we might want to do that?

also we used to require low res if the active tree had unshared low res there.
what about that?

do we not have tests for these cases?

[vmpstr, 2014/09/19 21:22:53]

Non ideal tilings won't be rasterizing any new content (as they are not
rasterizing right now either), so I'm not sure if it's worth it. It would only
really prevent the tiles from being evicted _somtimes_, which I feel should be a
different mechanism anyway.
The problem is that when we did this all from PLI, it knew about
low-res/high-res at the same time. In PLT, it's kind of awkward to ask about
other tilings. So I'm not exactly sure what the correct approach is here.

[danakj, 2014/09/19 22:30:06]

Is that implemented as a different mechanism now? Can you point me to it?
This function is called from PictureLayerImpl.. so we should be able to pass in
what info we need or make the decision at that callsite.

Right now PLI::AllTilesRequiredReady is using a CoverageIterator on each tiling.
If any high res tile is missing, it could iterate on the low res and check them.
If all the high res tiles are ready it could just not call this method at all
for LOW_RES. So maybe this resolution check doesn't belong in this method at
all, and PLI should be making choices based on the resolution. WDYT?

It could also do the missing_region thing for non-ideal resolutions like we do
in mark visible to maintain letting us activate early with those resources at
the right times?

[vmpstr, 2014/09/22 18:01:59]

This is where we don't rasterize new non-ideal tiles:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/tile_...

As for eviction, required for activation would bump the tiles up in the priority
order, which would make them be evicted less frequently. But other than that, if
we're out of memory then we're out of memory; we'll evict anything to stay
within budget.
The call to check whether we are ready to activate comes from PLI. However, the
thing to actually set it and schedule it with required for activation flag on
(so we get the signal from the rasterizer) is done via the raster iterator.
Granted, that raster iterator is also created from a layer raster iterator, but
I think it's a bit more involved that just passing the uncovered area.

I'm a bit conflicted here. I don't want to have only half of the functionality
(ie required checks do low res, but required setters don't). Also, I feel like
this would complicate the patch a bit, mostly because the layer iterator right
now doesn't care about required for activation -- it just uses the tiling
iterator. 

+    return false;
+
+  WhichTree tree = client_->GetTree();

[danakj, 2014/09/19 01:41:47]

the temp variable not needed

[danakj, 2014/09/19 01:44:53]

can we DCHECK(PENDING_TREE) instead actually? the callsite already checks this.

[vmpstr, 2014/09/19 21:22:52]

Done.

+  if (tree != PENDING_TREE)
+    return false;
+
+  if (IsTileOccluded(tile))
+    return false;
+
+  if (client_->RequiresHighResToDraw())
+    return true;
+
+  const PictureLayerTiling* twin_tiling = client_->GetTwinTiling(this);
+  if (!twin_tiling)
+    return true;
+
+  if (twin_tiling->layer_bounds() != layer_bounds())
+    return true;
+
+  if (twin_tiling->current_visible_rect_ != current_visible_rect_)
+    return true;
+
+  Tile* twin_tile =
+      twin_tiling->TileAt(tile->tiling_i_index(), tile->tiling_j_index());
+  if (!twin_tile || twin_tile == tile)

[danakj, 2014/09/19 01:41:47]

there should be some readytodraw checks in here no? if the tile isn't ready to
draw we should allow low res to be required?

https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...

I changed that code recently, are the tests not testing anything anymore with
this change? https://codereview.chromium.org/540663002

[vmpstr, 2014/09/19 21:22:52]

See above.. We did that to mark low res. Suggestions are welcome.

+    return false;
+
+  return true;
+}
+
+void PictureLayerTiling::UpdateTileAndTwinPriority(Tile* tile) const {
+  UpdateTilePriority(tile);
+
+  const PictureLayerTiling* twin_tiling = client_->GetTwinTiling(this);
+  if (!tile->is_shared() || !twin_tiling) {
+    WhichTree tree = client_->GetTree();
+    WhichTree twin_tree = tree == ACTIVE_TREE ? PENDING_TREE : ACTIVE_TREE;
+    tile->SetPriority(twin_tree, TilePriority());
+    tile->set_is_occluded(twin_tree, false);
+    if (twin_tree == PENDING_TREE)
+      tile->set_required_for_activation(false);
+    return;
   }
+
+  twin_tiling->UpdateTilePriority(tile);
 }
 
-void PictureLayerTiling::DidBecomeActive() {
-  PicturePileImpl* active_pile = client_->GetPile();
-  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 to get deleted once the corresponding
-    // PictureLayerImpl and any in flight raster jobs go out of scope.
-    it->second->set_picture_pile(active_pile);
+void PictureLayerTiling::UpdateTilePriority(Tile* tile) const {
+  WhichTree tree = client_->GetTree();
+
+  if (tree == PENDING_TREE)
+    tile->set_required_for_activation(false);
+  tile->set_is_occluded(tree, false);
+
+  DCHECK_EQ(TileAt(tile->tiling_i_index(), tile->tiling_j_index()), tile);
+  gfx::Rect tile_bounds =
+      tiling_data_.TileBounds(tile->tiling_i_index(), tile->tiling_j_index());
+
+  if (current_visible_rect_.Intersects(tile_bounds)) {

[danakj, 2014/09/19 01:41:47]

here i think we want the actual visible rect of the layer, not the viewport
projected into the layer without any clipping.. i think this is currently bad at
overflow scroll for example. can you add a TODO about this?

[vmpstr, 2014/09/19 21:22:52]

I think this is OK? Tile bounds here are guaranteed to be within the content
bounds of the layer, so if this check passes then at least part of the viewport
in layer space is overlapping the content bounds. Does that make sense?

[danakj, 2014/09/19 22:30:06]

Yes but consider a small overflow scroll div clipping stuff in teh middle of the
viewport, and a larger layer being clipped by that. It's all inside the
viewport, but it's mostly not visible at all, so we say NOW for tiles that are
entirely clipped away by the overflow scroll div.

[vmpstr, 2014/09/22 18:01:59]

(\sciencedog) But wouldn't we have the visible layer rect be the same thing as
the projected viewport clipped to this tiling bounds? Like I'm trying to
understand if this is different from taking current_visible_rect_ and then
clipping it to tiling bounds. I don't think it makes a difference, since
tile_bounds is already naturally clipped to those bounds so the intersection
check will not change value... Let me know if I completely misunderstand what
you mean :P

+    tile->SetPriority(tree, TilePriority(resolution_, TilePriority::NOW, 0));

[danakj, 2014/09/19 01:41:47]

This priority logic is duplicated from the iterators, they decide the order and
the priority based on these rects. Then they call this to "SetPriority" to what
they decided. They should be the one setting this stuff or passing it in here
based on their stage.

I think you commented elsewhere similarly about the is_occluded() and
required_for_activation() being problematic similarly. We should definitely
iterate (hah get it?) on this and make this better.

Can we improve this now, or leave a TODO about this?

[vmpstr, 2014/09/19 21:22:53]

I've added a general todo here. I want to move the priority to the iterators
altogether, which is a big change (mostly because all of the unittests will
simultaneously explode). I can move the setting code to the iterators, if that's
better. Let me know.

+    if (tree == PENDING_TREE)
+      tile->set_required_for_activation(IsTileRequiredForActivation(tile));
+    tile->set_is_occluded(tree, IsTileOccluded(tile));
+    return;
+  }
+
+  float distance_to_visible =
+      current_visible_rect_.ManhattanInternalDistance(tile_bounds) *
+      content_to_screen_scale_;
+
+  if (current_soon_border_rect_.Intersects(tile_bounds) ||
+      current_skewport_rect_.Intersects(tile_bounds)) {
+    tile->SetPriority(
+        tree,
+        TilePriority(resolution_, TilePriority::SOON, distance_to_visible));
+    return;
   }
+
+  tile->SetPriority(
+      tree,
+      TilePriority(resolution_, TilePriority::EVENTUALLY, distance_to_visible));
 }
 
 void PictureLayerTiling::GetAllTilesForTracing(
@@ -954,9 +950,8 @@ void PictureLayerTiling::UpdateEvictionCacheIfNeeded(
   eviction_tiles_eventually_and_required_for_activation_.clear();
 
   for (TileMap::iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
-    // TODO(vmpstr): This should update the priority if UpdateTilePriorities
-    // changes not to do this.
     Tile* tile = it->second.get();
+    UpdateTileAndTwinPriority(tile);

[danakj, 2014/09/19 01:41:47]

the tile and it's twin are both in this map right? does this mean we update each
of their priorities twice? is that worth worrying about?

[vmpstr, 2014/09/19 21:22:53]

Well.. you're right. But I'm not worried about it too much. I don't really know
of a better alternative aside from having some sort of "this tile priority has
been updated during this frame/manage tiles" sort of thing, which I'd like to
avoid.

     const TilePriority& priority =
         tile->priority_for_tree_priority(tree_priority);
     switch (priority.priority_bin) {
@@ -1049,8 +1044,11 @@ PictureLayerTiling::TilingRasterTileIterator::TilingRasterTileIterator(
 
   current_tile_ =
       tiling_->TileAt(visible_iterator_.index_x(), visible_iterator_.index_y());
-  if (!current_tile_ || !TileNeedsRaster(current_tile_))
+  if (!current_tile_ || !TileNeedsRaster(current_tile_)) {
     ++(*this);
+    return;
+  }
+  tiling_->UpdateTileAndTwinPriority(current_tile_);
 }
 
 PictureLayerTiling::TilingRasterTileIterator::~TilingRasterTileIterator() {}
@@ -1111,6 +1109,9 @@ void PictureLayerTiling::TilingRasterTileIterator::AdvancePhase() {
       break;
     }
   } while (!spiral_iterator_);
+
+  if (current_tile_)
+    tiling_->UpdateTileAndTwinPriority(current_tile_);
 }
 
 PictureLayerTiling::TilingRasterTileIterator&
@@ -1148,6 +1149,9 @@ operator++() {
     }
     current_tile_ = tiling_->TileAt(next_index.first, next_index.second);
   }
+
+  if (current_tile_)
+    tiling_->UpdateTileAndTwinPriority(current_tile_);
   return *this;
 }