cc: Avoid redraw for missing tile outside visible rect

cc/layers/picture_layer_impl.cc

Index: cc/layers/picture_layer_impl.cc
diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
index 8dbfeddcb3678952a21b3b7d7df368b5c9c3ca45..83218dd60ef9361e681a791c0a62f7382efe7a48 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -264,6 +264,9 @@ void PictureLayerImpl::AppendQuads(
   // unused can be considered for removal.
   std::vector<PictureLayerTiling*> seen_tilings;
 
+  gfx::Rect tile_rect = GetVisibleRectForTilePriorityInContentSpace();

[danakj, 2014/08/15 22:09:08]

scaled_tile_priority_rect?

[enne (OOO), 2014/08/15 22:33:50]

"ForTilePriority" also seems not true anymore.  Is there a better name?

[boliu, 2014/08/15 23:07:26]

How about something with activation, external_activation_rect? Implies tile
priority etc.

+  tile_rect = gfx::ScaleToEnclosingRect(tile_rect, max_contents_scale);
+
   size_t missing_tile_count = 0u;
   size_t on_demand_missing_tile_count = 0u;
   for (PictureLayerTilingSet::CoverageIterator iter(tilings_.get(),
@@ -290,8 +293,10 @@ void PictureLayerImpl::AppendQuads(
           gfx::Rect opaque_rect = iter->opaque_rect();
           opaque_rect.Intersect(geometry_rect);
 
-          if (iter->contents_scale() != ideal_contents_scale_)
+          if (iter->contents_scale() != ideal_contents_scale_ &&
+              geometry_rect.Intersects(tile_rect)) {

[danakj, 2014/08/15 21:43:44]

We only iterate over the visible_content_rect in this method, so I don't get
this, won't everything intersect?

[boliu, 2014/08/15 21:56:26]

Yes in chrome. No in webview.

In chrome, draw viewport and "visible rect for tile priority" is the same thing.

In webview the draw viewport can be bigger than the visible rect.

[danakj, 2014/08/15 22:09:08]

Maybe we need separate counts (or a bool) in AppendQuadsData for tiles that are
inside vs outside the tile priority rect then. Just not counting them seems
awkward?

[hush (inactive), 2014/08/15 22:12:37]

I'm not exactly sure about the scales of the rects here:
tile_rect is scaled to the max contents scale, but I'm not sure what the scale
is for "geometry_rect". Can you explain?

[danakj, 2014/08/15 22:20:45]

geometry_rect is in the "geometry space" of the iterator, its the space we work
in to keep things in integers, and it is the space scaled by the max contents
scale. The scale given for that space was given to the iterator as the
max_contents_scale on L273, given to the PictureLayerTiling::CoverageIterator
here
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...
and you can see how the geometry rect is computed in there as it's scaled by the
tiling this_geometry_scale/contents_scale when moving from the content space of
the tiling back to the "geometry space" input. 

[boliu, 2014/08/15 22:33:42]

I think we'll need a count then. One counts missing, one counts missing but
outside of the "tile priority rect". Only redraw if they are not equal.

Sounds ok?

[hush (inactive), 2014/08/15 22:35:36]

Thanks Dana!
So I suppose the dest_scale_ in PictureLayerTiling just means geometry scale.

On 2014/08/15 22:20:45, danakj wrote:

[danakj, 2014/08/15 22:36:06]

Yeh
 

             append_quads_data->num_incomplete_tiles++;
+          }
 
           TileDrawQuad* quad =
               render_pass->CreateAndAppendDrawQuad<TileDrawQuad>();
@@ -364,10 +369,12 @@ void PictureLayerImpl::AppendQuads(
                      false);
       }
 
-      append_quads_data->num_missing_tiles++;
+      if (geometry_rect.Intersects(tile_rect)) {
+        append_quads_data->num_missing_tiles++;

[danakj, 2014/08/15 21:43:44]

num missing tiles is used for UMA metrics to tell how much checkerboarding was
seen, so we can't do this optionally.

[boliu, 2014/08/15 21:56:26]

In chrome, they will always intersect, so no change there.

In webview, I think the number of missing tiles in draw viewport but outside of
the visible rect isn't really a meaningful number. Also we don't collect uma
data, although I really wish we did.

[danakj, 2014/08/15 22:09:08]

This makes it hard to reason about when we're counting tiles in Chrome. "Always
but it looks like we're not"

+        ++missing_tile_count;
+      }
       append_quads_data->approximated_visible_content_area +=
           visible_geometry_rect.width() * visible_geometry_rect.height();
-      ++missing_tile_count;
       continue;
     }
 
@@ -472,6 +479,26 @@ void PictureLayerImpl::UpdateTilePriorities(
   if (!tiling_needs_update)
     return;
 
+  gfx::Rect visible_layer_rect = gfx::ScaleToEnclosingRect(
+      GetVisibleRectForTilePriorityInContentSpace(), 1.f / contents_scale_x());
+  WhichTree tree =
+      layer_tree_impl()->IsActiveTree() ? ACTIVE_TREE : PENDING_TREE;
+  for (size_t i = 0; i < tilings_->num_tilings(); ++i) {
+    tilings_->tiling_at(i)->UpdateTilePriorities(tree,
+                                                 visible_layer_rect,
+                                                 ideal_contents_scale_,
+                                                 current_frame_time_in_seconds,
+                                                 occlusion_tracker,
+                                                 render_target(),
+                                                 draw_transform());
+  }
+
+  // Tile priorities were modified.
+  layer_tree_impl()->DidModifyTilePriorities();
+}
+
+gfx::Rect PictureLayerImpl::GetVisibleRectForTilePriorityInContentSpace()
+    const {
   // If visible_rect_for_tile_priority_ is empty or
   // viewport_rect_for_tile_priority_ is set to be different from the device
   // viewport, try to inverse project the viewport into layer space and use
@@ -492,22 +519,7 @@ void PictureLayerImpl::UpdateTilePriorities(
     }
   }
 
-  gfx::Rect visible_layer_rect = gfx::ScaleToEnclosingRect(
-      visible_rect_in_content_space, 1.f / contents_scale_x());
-  WhichTree tree =
-      layer_tree_impl()->IsActiveTree() ? ACTIVE_TREE : PENDING_TREE;
-  for (size_t i = 0; i < tilings_->num_tilings(); ++i) {
-    tilings_->tiling_at(i)->UpdateTilePriorities(tree,
-                                                 visible_layer_rect,
-                                                 ideal_contents_scale_,
-                                                 current_frame_time_in_seconds,
-                                                 occlusion_tracker,
-                                                 render_target(),
-                                                 draw_transform());
-  }
-
-  // Tile priorities were modified.
-  layer_tree_impl()->DidModifyTilePriorities();
+  return visible_rect_in_content_space;
 }
 
 void PictureLayerImpl::NotifyTileStateChanged(const Tile* tile) {
@@ -764,6 +776,7 @@ void PictureLayerImpl::MarkVisibleResourcesAsRequired() const {
     return;
 
   gfx::Rect rect(visible_content_rect());
+  rect.Intersect(GetVisibleRectForTilePriorityInContentSpace());

[hush (inactive), 2014/08/15 21:53:18]

I think you need to early out when intersection is empty

[danakj, 2014/08/15 22:09:08]

Why are we intersecting instead of just using the tile priority rect? For cases
where visible_content_rect is empty and we made something up? We early out above
in empty case already.

[boliu, 2014/08/15 22:33:42]

MarkRequiredOffscreenTiles test uses a
GetVisibleRectForTilePriorityInContentSpace() that's bigger than
visible_content_rect(), so using the first would cause more tile to be marked
required.

I'm trying to think if that can happen in practice though... It would mean the
draw viewport is smaller than the tile priority rect, which I think can actually
happen in webview *in theory*.

But I think we can guarantee tile priority rect is contained in the draw rect
outside of cc, then it should be ok, and would just need to fix that test.

Umm...thoughts?

[boliu, 2014/08/15 23:07:26]

This is actually kinda hard to do in webview code. Draw viewport and tile
priority rect are provided in difference spaces.

Also I think we should be intersecting with visible_content_rect() in the
UpdateTilePriorities too.

 
   float min_acceptable_scale =
       std::min(raster_contents_scale_, ideal_contents_scale_);