cc: Simplify picture layer tiling update tile priorities.

cc: Simplify picture layer tiling update tile priorities.

This patch simplifies the priority math calculation and moves it
into layers space. This makes the priority to be less correct,
since we're inverse transforming the viewport into layer space
instead of forward transforming tiles into screen space in all cases.

In particular, with perspective and rotated layers, we will be marking
more tiles as visible than before. This should be OK, since the amount
of those layers in practice is limited.

BUG=339142
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251064

[vmpstr, 2014-01-31 20:53:37]

Hi. Please take a look. This is an initial patch that roughs in the skewport and
removes time to visible stuff. I'm in the process of finding/making interesting
perspective layer pages to fully test this. Also, I didn't yet add any new
unittests for this, but will do that once we're more or less OK with the
approach.

Thanks!

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:21: const double
kSkewportTargetTimeInSeconds = 0.5f;
This is kind of arbitrary. I'm more than happy to change it after some
experimentation.

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling_unittest.cc:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
I'm still working on this file. I'll be adding more unittests, updating
comments, and figuring out whether the perspective thing we will do is correct.

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:121: const float kBackflingGuardDistancePixels =
314.0f;
Note that this patch eliminates backfling thing. We can add it back using some
skewport adjustments. However, all tiles outside of the viewport will now be
marked as eventually and sorted by distance, so they should be higher priority
than other non-skewport tiles.

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.cc
File cc/resources/tile_priority.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.c...
cc/resources/tile_priority.cc:106: state->Set("priority_bin",
TilePriorityBinAsValue(priority_bin).release());
This will also require some traceviewer changes...

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority_u...
File cc/resources/tile_priority_unittest.cc (left):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority_u...
cc/resources/tile_priority_unittest.cc:12: TEST(TilePriorityTest,
TimeForBoundsToIntersectWithScroll) {
This function is gone, since we're not using time to visible anymore.

[enne (OOO), 2014-01-31 23:07:46]

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:21: const double
kSkewportTargetTimeInSeconds = 0.5f;
On 2014/01/31 20:53:38, vmpstr wrote:
> This is kind of arbitrary. I'm more than happy to change it after some
> experimentation.

Should we put this in LayerSettings?

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:346: gfx::Rect
PictureLayerTiling::ComputeVisibleRect(
Is it possible that something higher in the stack, like the tiling set or the
layer can check that the visible_layer_rect is empty and pass the
viewport_in_layer_space instead, and then the PictureLayerTiling only has to
deal with a single rect.

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:359: gfx::Rect
PictureLayerTiling::ComputeSkewport(
You're probably already planning this, but I'd love a few targeted unit tests
for this.

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:371: double extrapolationMultiplier =
kSkewportTargetTimeInSeconds / time_delta;
_multiplier

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:473: // Assign eventually priority to all
tiles in the eventually rect that are not
Can you DCHECK that the skewport always contains the visible rect, to increase
confidence that all tiles have been prioritized with this last loop?

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.h:142: size_t max_tiles_for_interest_area);
This isn't part of your patch, but we don't change this at runtime in practice,
right? Could this be a constructor or other setter parameter?

(I'm happy with how few parameters get passed to this function and so I'm
itching to remove even more.)

https://codereview.chromium.org/140513006/diff/1/cc/resources/prioritized_til...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/prioritized_til...
cc/resources/prioritized_tile_set.cc:21: if (ams.priority_bin !=
bms.priority_bin)
Isn't the bin comparator for tiles in the same bin? Shouldn't it always be the
case that the bins are the same?

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:120: const float kPrepaintingWindowTimeSeconds =
1.0f;
Should the soon bin skewport window be 1 second instead of 0.5 to try to
minimize behavior changes in this patch?

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:124: if (prio.distance_to_visible ==
When does this happen?

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:933: if
(tile->priority(ACTIVE_TREE).distance_to_visible == 0)
style nit: 0.f

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.cc
File cc/resources/tile_priority.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.c...
cc/resources/tile_priority.cc:106: state->Set("priority_bin",
TilePriorityBinAsValue(priority_bin).release());
On 2014/01/31 20:53:38, vmpstr wrote:
> This will also require some traceviewer changes...

Do you know if TraceViewer will break in the short term because of the missing
time_to_visible_in_seconds or just not display this new priority_bin value?

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.h
File cc/resources/tile_priority.h (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.h...
cc/resources/tile_priority.h:45: NOW = 0,
No need to assign these values.

[enne (OOO), 2014-01-31 23:33:36]

This looks like a good incremental approach.  Can you delete the rotated and
perspective perf tests for prioritization since they all go down the same code
path?

It also seems like the perf wins here are going to be rotated/perspective tile
prioritization gets faster (and more incorrect) and possibly a little bit less
setup work per layer and per tiling.  Am I missing anything?

https://codereview.chromium.org/140513006/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:342: if
(screen_space_transform().GetInverse(&screen_to_layer)) {
Maybe we only need this projection if the viewport_in_content_space is empty? 
Also, if the viewport in content space is empty, then that means that the layer
is clipped away.  Right now (and previously) we'd mark those tiles as visible if
they were clipped away but also inside the projection of the viewport.

If we substitute the projected viewport for the viewport in content space,
should we skip marking anything as NOW and just mark everything in the skewport
+ viewport as SOON?

https://codereview.chromium.org/140513006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:363: last_screen_space_transform_ =
current_screen_space_transform;
Do we need this still?

https://codereview.chromium.org/140513006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:723: if
(iter->priority(PENDING_TREE).distance_to_visible != 0)
I think this can go away as we only mark tiles in the visible content rect as
visible and this loop is only iterating over tiles in that rect.

[vmpstr, 2014-02-03 20:27:23]

Hi,

I have uploaded a second version of the patch, which addresses most (if not all)
of your comments. Please take a look. Meanwhile, I will start implementing
unittests for the new functionality that I have added.

Thanks!

On 2014/01/31 23:33:36, enne wrote:
> This looks like a good incremental approach.  Can you delete the rotated and
> perspective perf tests for prioritization since they all go down the same code
> path?

Done. 

> 
> It also seems like the perf wins here are going to be rotated/perspective tile
> prioritization gets faster (and more incorrect) and possibly a little bit less
> setup work per layer and per tiling.  Am I missing anything?

Nope, I think you're correct here.

I've also done some manual verification. Chrome behaves as I would expect on
poster circle as well as on
http://www.webkit.org/blog-files/3d-transforms/perspective-by-example.html after
changing the perspective layers to be different sizes (ie 4k by 4k or so).

https://codereview.chromium.org/140513006/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:342: if
(screen_space_transform().GetInverse(&screen_to_layer)) {
On 2014/01/31 23:33:36, enne wrote:
> Maybe we only need this projection if the viewport_in_content_space is empty? 

Done.

> Also, if the viewport in content space is empty, then that means that the
layer
> is clipped away.  Right now (and previously) we'd mark those tiles as visible
if
> they were clipped away but also inside the projection of the viewport.
> 
> If we substitute the projected viewport for the viewport in content space,
> should we skip marking anything as NOW and just mark everything in the
skewport
> + viewport as SOON?

I'd prefer to keep this as now for now. My reasoning being that it more closely
resembles current behaviour. On top of that, piping through the fact we replaced
the rect requires extra plumbing. Given that this is more of an optimization,
I'd prefer to leave that for a follow-up. Wdyt?

https://codereview.chromium.org/140513006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:363: last_screen_space_transform_ =
current_screen_space_transform;
On 2014/01/31 23:33:36, enne wrote:
> Do we need this still?

Nope. Removed.

https://codereview.chromium.org/140513006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:723: if
(iter->priority(PENDING_TREE).distance_to_visible != 0)
On 2014/01/31 23:33:36, enne wrote:
> I think this can go away as we only mark tiles in the visible content rect as
> visible and this loop is only iterating over tiles in that rect.

Good point. Done.

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:21: const double
kSkewportTargetTimeInSeconds = 0.5f;
On 2014/01/31 23:07:46, enne wrote:
> On 2014/01/31 20:53:38, vmpstr wrote:
> > This is kind of arbitrary. I'm more than happy to change it after some
> > experimentation.
> 
> Should we put this in LayerSettings?

Done.

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:346: gfx::Rect
PictureLayerTiling::ComputeVisibleRect(
On 2014/01/31 23:07:46, enne wrote:
> Is it possible that something higher in the stack, like the tiling set or the
> layer can check that the visible_layer_rect is empty and pass the
> viewport_in_layer_space instead, and then the PictureLayerTiling only has to
> deal with a single rect.

Good idea. Done.

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:359: gfx::Rect
PictureLayerTiling::ComputeSkewport(
On 2014/01/31 23:07:46, enne wrote:
> You're probably already planning this, but I'd love a few targeted unit tests
> for this.

Yeah, I definitely plan to have good test coverage of the new functions.

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:371: double extrapolationMultiplier =
kSkewportTargetTimeInSeconds / time_delta;
On 2014/01/31 23:07:46, enne wrote:
> _multiplier

Done.

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:473: // Assign eventually priority to all
tiles in the eventually rect that are not
On 2014/01/31 23:07:46, enne wrote:
> Can you DCHECK that the skewport always contains the visible rect, to increase
> confidence that all tiles have been prioritized with this last loop?

I have that on line 421 right after computing the skewport

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.h:142: size_t max_tiles_for_interest_area);
On 2014/01/31 23:07:46, enne wrote:
> This isn't part of your patch, but we don't change this at runtime in
practice,
> right? Could this be a constructor or other setter parameter?
> 
> (I'm happy with how few parameters get passed to this function and so I'm
> itching to remove even more.)

Done. But to avoid passing this in other functions that create a tiling, I made
this a getter on the client. Personally, I like that better, but let me know.

https://codereview.chromium.org/140513006/diff/1/cc/resources/prioritized_til...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/prioritized_til...
cc/resources/prioritized_tile_set.cc:21: if (ams.priority_bin !=
bms.priority_bin)
On 2014/01/31 23:07:46, enne wrote:
> Isn't the bin comparator for tiles in the same bin? Shouldn't it always be the
> case that the bins are the same?

Not exactly. I mean, I hope that in the near future the priority bins stuff
(NOW_BIN/SOON_BIN/EVENTUALLY_BIN/AT_LAST_BIN/NEVER_BIN/and all active versions
as well) will go away. Until that time, it just happens that TilePriority::NOW
maps to NOW_BIN, but it doesn't have to be that way. The prioritized tile set
splits into bins as in ManagedTileBin, not as in TilePriority::PriorityBin.

It's somewhat confusing to have two different types that represent bins, but
ManagedTileBin has a few more bins based on tree priority and whether the tile
is currently being rasterized. I would prefer to keep it that way for now. Wdyt?

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:120: const float kPrepaintingWindowTimeSeconds =
1.0f;
On 2014/01/31 23:07:46, enne wrote:
> Should the soon bin skewport window be 1 second instead of 0.5 to try to
> minimize behavior changes in this patch?

Done.

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:124: if (prio.distance_to_visible ==
On 2014/01/31 23:07:46, enne wrote:
> When does this happen?

This is magic :P. distance_to_visible == inf means that we cleared that tile's
priority to default which means that we haven't updated tile priorities which
means it's an active tree priority for a tile only on the pending tree, or maybe
something on the recycle tree.

If you look at around 425-426 in this file, we still keep the tile around if it
was prioritized on one of the trees but at the lowest priority.

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:933: if
(tile->priority(ACTIVE_TREE).distance_to_visible == 0)
On 2014/01/31 23:07:46, enne wrote:
> style nit: 0.f

Done.

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.cc
File cc/resources/tile_priority.cc (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.c...
cc/resources/tile_priority.cc:106: state->Set("priority_bin",
TilePriorityBinAsValue(priority_bin).release());
On 2014/01/31 23:07:46, enne wrote:
> On 2014/01/31 20:53:38, vmpstr wrote:
> > This will also require some traceviewer changes...
> 
> Do you know if TraceViewer will break in the short term because of the missing
> time_to_visible_in_seconds or just not display this new priority_bin value?

A bit of both. It will "break" in the sense that selecting time to visible
heatmap will not produce any results. It won't error or anything, just have
blank data. And, of course it won't display the new bin in a heatmap

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.h
File cc/resources/tile_priority.h (right):

https://codereview.chromium.org/140513006/diff/1/cc/resources/tile_priority.h...
cc/resources/tile_priority.h:45: NOW = 0,
On 2014/01/31 23:07:46, enne wrote:
> No need to assign these values.

Done.

https://codereview.chromium.org/140513006/diff/210001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/140513006/diff/210001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1196: TEST_F(PictureLayerImplTest,
MarkRequiredOffscreenTiles) {
This change is due to the mark required function no longer checking distance
information.

[epennerAtGoogle, 2014-02-03 23:43:03]

Cool! Adding a couple (long winded) comments. Some should be addressed if we
want this to function as it's own patch with no regressions, while some could
just be possible improvements down the road.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/managed_ti...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/managed_ti...
cc/resources/managed_tile_state.h:115: float distance_to_visible;
Since we plan on 'augmenting' this distance in various ways to cram most of our
priority into it, should we put that into the name?  I can't think of a better
name though ('distance_based_metric'?), so I have no strong preference on the
name.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:342: gfx::Rect
PictureLayerTiling::ComputeSkewport(
This could benefit from a high-level comment describing what this is doing (eg
This extrapolates each edge's location using it's velocity and the time elapsed
since last frame - except, it strictly grows and never shrinks).

Also, I think this needs a clamp on how much we extrapolate (using another
constant, unfortunately). If we are relying on finite differences, there is
problems with that type of velocity estimate that could result in massive
skewports.

Last, shrinking and growing viewports don't really change our priority ordering
within a layer. We would still just paint 'outwards'. However, a large skewport
might change how much memory we used compared to just sitting still on a page.
Maybe we should keep the larger skewport as it is here (it makes the most
sense), but if so I think we should limit the extra 'outwards' painting it
causes eventually (to make sure it doesn't spike our memory too much).

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:121: const float kBackflingGuardDistancePixels =
314.0f;
As is, some platforms use a 'soon-only-memory-policy'. When we remove the
'fling-guard' and use skewport->soon in this patch, we will have effectively no
pre-painted tiles unless you are scrolling when using that policy.

To not regress painting behavior, we should somehow maintain the 'fling-guard'
part of the SOON_BIN, until the binning process can be sorted out or removed.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:125: const float
kMaxPrepaintingDistancePixelsHighRes = 2000.0f;
This limit was effectively a velocity limit. I think if we remove this we should
have a limit on the viewport edge velocities, or a limit on the skewport's
extruded size.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_prior...
File cc/resources/tile_priority.h (left):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_prior...
cc/resources/tile_priority.h:84: static float TimeForBoundsToIntersect(const
gfx::RectF& previous_bounds,
There is a tidbit in here that it might be nice to capture in the new solution
somehow. The new distance calculation is not skewed in any way by different x/y
velocities. So if we are scrolling downwards with just a touch of movement to
the left, we'll paint the left tiles much more eagerly than we should. The
bigger the 'time window' is for the skewport, the worse this will get.

One approach I can think of is stretching the x/y dimensions where the Manhattan
distance takes place. Or getting both X/Y components from the Manhattan
distance, and scaling those accordingly before adding them.

[enne (OOO), 2014-02-04 00:38:22]

https://codereview.chromium.org/140513006/diff/240001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/140513006/diff/240001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:354: visible_rect_in_content_space,
This caller passes visible_rect_in_content_space but the function signature says
visible_layer_rect.  Which is it?

https://codereview.chromium.org/140513006/diff/240001/cc/resources/managed_ti...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/managed_ti...
cc/resources/managed_tile_state.h:115: float distance_to_visible;
On 2014/02/03 23:43:03, epennerAtGoogle wrote:
> Since we plan on 'augmenting' this distance in various ways to cram most of
our
> priority into it, should we put that into the name?  I can't think of a better
> name though ('distance_based_metric'?), so I have no strong preference on the
> name.

I thought we would be able to use this really as distance to visible but use an
explicit bin as the higher order sorting bit.  So, something in the apron might
be closer but in the EVENTUALLY bin than something farther away in the
skewport+SOON bin, and we'll sort the SOON before the EVENTUALLY.

[epennerAtGoogle, 2014-02-04 00:57:48]

> I thought we would be able to use this really as distance to visible but use
an
> explicit bin as the higher order sorting bit.  So, something in the apron
might
> be closer but in the EVENTUALLY bin than something farther away in the
> skewport+SOON bin, and we'll sort the SOON before the EVENTUALLY.

If we just added some 'bonus' to the skewport, it wouldn't achieve the same? And
then we would have no bins at all... If it's an argument that it's too messy to
encode everything into one value, I buy that and using BINs sounds fine.

However, see my note about the X/Y velocities vs X/Y contribution to the final
distance. It would be nice to at least be able to fudge the distance to make
better ordering decisions... It can still be called 'distance' but I hope we
won't be too strictly tied to how we calculate it (if fudging it helps achieve
the ordering we want).

Also, we shouldn't use the BINs the way they were used before (memory-policy
enables/disables bins). The SOON bin will be too tight for any memory policy, if
it only contains the skewport.

[vmpstr, 2014-02-04 19:50:49]

Hi, please take another look. Thank you.

https://codereview.chromium.org/140513006/diff/240001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/140513006/diff/240001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:354: visible_rect_in_content_space,
On 2014/02/04 00:38:22, enne wrote:
> This caller passes visible_rect_in_content_space but the function signature
says
> visible_layer_rect.  Which is it?

This is PictureLayerTilingSet::UpdateTilePriorities which takes
visible_content_rect and converts it to visible_layer_rect before passing it to
PictureLayerTiling::UpdateTilePriorities. I think as a part of doing the whole
incremental thing, I'd like to remove
PictureLayerTilingSet::UpdateTilePriorities and just iterate over each tiling
here.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/managed_ti...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/managed_ti...
cc/resources/managed_tile_state.h:115: float distance_to_visible;
On 2014/02/04 00:38:22, enne wrote:
> On 2014/02/03 23:43:03, epennerAtGoogle wrote:
> > Since we plan on 'augmenting' this distance in various ways to cram most of
> our
> > priority into it, should we put that into the name?  I can't think of a
better
> > name though ('distance_based_metric'?), so I have no strong preference on
the
> > name.
> 
> I thought we would be able to use this really as distance to visible but use
an
> explicit bin as the higher order sorting bit.  So, something in the apron
might
> be closer but in the EVENTUALLY bin than something farther away in the
> skewport+SOON bin, and we'll sort the SOON before the EVENTUALLY.

I initially considered doing a single metric, by boosting the skewport (like
dividing the distance by some value or something like that). However, I think
that makes it sensitive to skewport target time and things like that as some
tiles near the end of the skewport will be sorted lower than near viewport soon
tiles. I would vote for keeping the bins since it makes it very clear where the
tile is coming from and having the distance only come into play when figuring
out priorities within a bin.

Later, if this is a good spot for optimization then I'm sure we can figure out a
single metric.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:342: gfx::Rect
PictureLayerTiling::ComputeSkewport(
On 2014/02/03 23:43:03, epennerAtGoogle wrote:
> This could benefit from a high-level comment describing what this is doing (eg
> This extrapolates each edge's location using it's velocity and the time
elapsed
> since last frame - except, it strictly grows and never shrinks).
> 

I added a comment in the header file.

> Also, I think this needs a clamp on how much we extrapolate (using another
> constant, unfortunately). If we are relying on finite differences, there is
> problems with that type of velocity estimate that could result in massive
> skewports.
> 
> Last, shrinking and growing viewports don't really change our priority
ordering
> within a layer. We would still just paint 'outwards'. However, a large
skewport
> might change how much memory we used compared to just sitting still on a page.
> Maybe we should keep the larger skewport as it is here (it makes the most
> sense), but if so I think we should limit the extra 'outwards' painting it
> causes eventually (to make sure it doesn't spike our memory too much).

I can buy the argument for limiting the skewport based on some constant (just
capping the extrapolation distance). I don't have a good sense of what we want
this constant to be though. Are you thinking here on the order of a few rows? Or
on the order of a few thousand pixels? I'd propose something like 2k pixels if
the choice is left up to me, which is what I've put in. Does that sound
reasonable?

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:121: const float kBackflingGuardDistancePixels =
314.0f;
On 2014/02/03 23:43:03, epennerAtGoogle wrote:
> As is, some platforms use a 'soon-only-memory-policy'. When we remove the
> 'fling-guard' and use skewport->soon in this patch, we will have effectively
no
> pre-painted tiles unless you are scrolling when using that policy.
> 
> To not regress painting behavior, we should somehow maintain the 'fling-guard'
> part of the SOON_BIN, until the binning process can be sorted out or removed.

This actually doesn't just provide the fling back, but also a border around the
visible tiles. In the long term, this would be provided by the eventually bin
from the layer (currently it's all of interest rect that falls into this
category). For now we can do the same distance check here or extend the skewport
backwards. I'm opting in for distance check.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:125: const float
kMaxPrepaintingDistancePixelsHighRes = 2000.0f;
On 2014/02/03 23:43:03, epennerAtGoogle wrote:
> This limit was effectively a velocity limit. I think if we remove this we
should
> have a limit on the viewport edge velocities, or a limit on the skewport's
> extruded size.

I've done this via skewport capping. Let me know if that works.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_prior...
File cc/resources/tile_priority.h (left):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_prior...
cc/resources/tile_priority.h:84: static float TimeForBoundsToIntersect(const
gfx::RectF& previous_bounds,
On 2014/02/03 23:43:03, epennerAtGoogle wrote:
> There is a tidbit in here that it might be nice to capture in the new solution
> somehow. The new distance calculation is not skewed in any way by different
x/y
> velocities. So if we are scrolling downwards with just a touch of movement to
> the left, we'll paint the left tiles much more eagerly than we should. The
> bigger the 'time window' is for the skewport, the worse this will get.
> 
> One approach I can think of is stretching the x/y dimensions where the
Manhattan
> distance takes place. Or getting both X/Y components from the Manhattan
> distance, and scaling those accordingly before adding them.

It's somewhat awkward to do this, I think. The problem is that it's not evident
from the skewport calculation whether the viewport will actually intersect the
tiles to the left/right when it's moving mostly down. Right now it  basically
assumes that it will (thus in a diagonal viewport move, it will result in a
larger than necessary skewport; see the unittests that I've added). 

I'm somewhat conflicted here. My gut feeling is that it's not really worth the
added complexity to figure out exactly which tiles will intersect the viewport
and which won't in a two direction scroll, since we're going back to being extra
correct but slow instead of just being roughly correct and fast. Wdyt?

[epennerAtGoogle, 2014-02-04 21:34:11]

Looking good. Only remaining concern now is the 'fling-guard' and the ordering
of those tiles relative to the skewport.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/managed_ti...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/managed_ti...
cc/resources/managed_tile_state.h:115: float distance_to_visible;
SGTM. Cramming into one priority can wait, and only if it doesn't get too messy.

My thought was, if the skewport had a size limit, that limit could be added to
all non-skewport tiles, to differentiate in-skewport tiles from other tiles...
But no need to go there yet, or maybe never if we can't remove the other sources
of complex priority decisions (eg. smoothness/resolution).

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:121: const float kBackflingGuardDistancePixels =
314.0f;
Right, it's not just fling-back, but sort of a small piece of 'interest-rect'
that gets promoted to the soon-bin.

It sounds good to just keep a distance check in here.... However, unfortunately
the simple distance metric bites us again as all these tiles have a small
distance. So we'll keep painting these tiles rather than the extruded skewport
tiles in the direction of a scroll.

If we don't augment the distance, it looks like creating another bin, or another
'in-skewport' flag would solve it for now? Or maybe we can just accept the
checker-boarding hit for soon-only-memory-policy in the short term.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:125: const float
kMaxPrepaintingDistancePixelsHighRes = 2000.0f;
SGTM.

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_prior...
File cc/resources/tile_priority.h (left):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_prior...
cc/resources/tile_priority.h:84: static float TimeForBoundsToIntersect(const
gfx::RectF& previous_bounds,
I don't think correcting for this particular issue would be slow. But it would
require fudging something to get the right ordering.

I'm okay just leaving it for now... but if so we should just verify it doesn't
become a problem in practice. There is a chance that some
'fling-direction-snapping' removes small diagonal components already, which
would also solve this for the most part.

[vmpstr, 2014-02-04 21:57:53]

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:121: const float kBackflingGuardDistancePixels =
314.0f;
On 2014/02/04 21:34:12, epennerAtGoogle wrote:
> Right, it's not just fling-back, but sort of a small piece of 'interest-rect'
> that gets promoted to the soon-bin.
> 
> It sounds good to just keep a distance check in here.... However,
unfortunately
> the simple distance metric bites us again as all these tiles have a small
> distance. So we'll keep painting these tiles rather than the extruded skewport
> tiles in the direction of a scroll.
> 

I think here we're actually saved by bin and distance calculations from the
tiling:

Skewport tiles will have TilePriority::SOON + distance, other interest rect
tiles will be TilePriority::EVENTUALLY + distance. Skewport tiles automatically
get ManagedTileBin::SOON_BIN (SOON -> SOON_BIN), and interest rect tiles closer
than backfling guard will also be promoted to ManagedTileBin::SOON_BIN
(EVENTUALLY + small distance -> SOON_BIN). In the BinComparator, the first check
is now for the TilePriority bin, which would order skewport tiles
(TilePriority::SOON) over backfling guarded tiles (TilePriority::EVENTUALLY).
It's somewhat confusing to have two distinct sets of bins both with similar
names, but hopefully we can get rid of ManagedTileBin stuff as we make progress
in the redesign.

> If we don't augment the distance, it looks like creating another bin, or
another
> 'in-skewport' flag would solve it for now? Or maybe we can just accept the
> checker-boarding hit for soon-only-memory-policy in the short term.

[epennerAtGoogle, 2014-02-04 23:48:06]

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/140513006/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:121: const float kBackflingGuardDistancePixels =
314.0f;
Oh, I see. Indeed that's confusing but indeed that sounds like it will work :)

We definitely need to get rid of parallel bin concepts, but this sounds like it
preserves the functionality we need in the interim.

[vmpstr, 2014-02-10 18:32:09]

enne, could you take another look?

[enne (OOO), 2014-02-11 00:43:55]

This looks really close.  I have a few more suggestions for unit tests, but
that's it.

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:21: int
kSkewportExtrapolatedLimitInContentPixels = 2000;
Should this be a LayerTreeSetting?

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_perftest.cc (left):

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_perftest.cc:167:
RunUpdateTilePrioritiesStationaryTest("rotation", transform);
I think I maybe said remove this, but maybe leave these in so we can see how
much faster it gets here on bots.  Then they can be removed in a followup patch?

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:272: TEST(PictureLayerTilingTest,
ComputeSkewport) {
Can you test the pixel limits too? It might make sense to make the 2000 content
pixel limit a setting somewhere so that the test doesn't have to assume it?

I think there also should be some tests about correct distances under content
scale != 1, to at least make sure that you're not multiplying a scale when you
should be dividing, etc.

Maybe also a test where you get all the tiles from the map in a layer and make
sure that if they intersect the viewport they're now, if they intersect the
skewport they're soon, and otherwise are listed as eventually? And maybe have
sane distances?

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:755:
client.set_max_tiles_for_interest_area(max_tiles_for_interest_area);
Can the fake client just set a "really large" max tiles for interest area by
default?

[vmpstr, 2014-02-11 23:45:24]

PTAL.

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:21: int
kSkewportExtrapolatedLimitInContentPixels = 2000;
On 2014/02/11 00:43:56, enne wrote:
> Should this be a LayerTreeSetting?

Done.

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_perftest.cc (left):

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_perftest.cc:167:
RunUpdateTilePrioritiesStationaryTest("rotation", transform);
On 2014/02/11 00:43:56, enne wrote:
> I think I maybe said remove this, but maybe leave these in so we can see how
> much faster it gets here on bots.  Then they can be removed in a followup
patch?

Done.

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:272: TEST(PictureLayerTilingTest,
ComputeSkewport) {
On 2014/02/11 00:43:56, enne wrote:
> Can you test the pixel limits too? It might make sense to make the 2000
content
> pixel limit a setting somewhere so that the test doesn't have to assume it?
> 
> I think there also should be some tests about correct distances under content
> scale != 1, to at least make sure that you're not multiplying a scale when you
> should be dividing, etc.
> 
> Maybe also a test where you get all the tiles from the map in a layer and make
> sure that if they intersect the viewport they're now, if they intersect the
> skewport they're soon, and otherwise are listed as eventually? And maybe have
> sane distances?

Done.

https://codereview.chromium.org/140513006/diff/400001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:755:
client.set_max_tiles_for_interest_area(max_tiles_for_interest_area);
On 2014/02/11 00:43:56, enne wrote:
> Can the fake client just set a "really large" max tiles for interest area by
> default?

Done.

[enne (OOO), 2014-02-11 23:51:49]

https://codereview.chromium.org/140513006/diff/460001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/460001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:369: // Note that if the viewport is
growing very rapidly, then
This seems unfortunate.  What about cases where the layer is zooming?

Is the problem that you're doing math on new_width vs old_width rather than
new_max_x vs. old_max_x? My understanding that the extrapolation limit was for
each of the four cardinal directions, so essentially width could go up by
2*skewport_limit.

https://codereview.chromium.org/140513006/diff/460001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/140513006/diff/460001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:430: // Tile at 5, 1 should begin
at 41x9 in content space (with borders),
Can you verify this by asking the tile for its content rect?

[vmpstr, 2014-02-12 00:27:52]

PTAL.

https://codereview.chromium.org/140513006/diff/460001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/460001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:369: // Note that if the viewport is
growing very rapidly, then
On 2014/02/11 23:51:49, enne wrote:
> This seems unfortunate.  What about cases where the layer is zooming?
> 
> Is the problem that you're doing math on new_width vs old_width rather than
> new_max_x vs. old_max_x? My understanding that the extrapolation limit was for
> each of the four cardinal directions, so essentially width could go up by
> 2*skewport_limit.

Yeah, the problem is that if I limit width by 2 * skewport_limit, then we might
just move down and expand so that the skewport will appear to be extended by
more than skewport_limit. I reworked this to be a separate calculation on each
direction. And added a test.

https://codereview.chromium.org/140513006/diff/460001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/140513006/diff/460001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:430: // Tile at 5, 1 should begin
at 41x9 in content space (with borders),
On 2014/02/11 23:51:49, enne wrote:
> Can you verify this by asking the tile for its content rect?

The distance calculation happens on tiles with no borders (since we don't care
about sampling when determining the distance to viewport). However, content_rect
includes sampling, so the check is off by 1 pixel.. However, I did put in the
check. Let me know if that's OK.

[enne (OOO), 2014-02-12 00:35:35]

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:361: int old_max_x =
last_visible_rect_in_content_space_.width() + old_min_x;
right()

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:362: int old_max_y =
last_visible_rect_in_content_space_.height() + old_min_y;
bottom()

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:371: int min_x_adjustment = std::min<int>(
I feel like this could all get simpler if you just created a viewport rect inset
by -skewport_limit and intersected the skewport by it at the very end.

Then there's no need for std::abs and multipliers, you could just adjust by
extrapolation * (new - old).

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:315: // Expand viewport.
Two more tests: one for a viewport expanded past the skewport limit in all
directions, one for a viewport that shrinks to make sure the skewport ==
viewport in that case?

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:449: EXPECT_EQ(tiling->TileAt(5,
1)->content_rect().ToString(), "40,8 10x10");
That seems reasonable.  I just wanted to test these assertions in code.

[vmpstr, 2014-02-12 18:14:17]

PTAL.

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:361: int old_max_x =
last_visible_rect_in_content_space_.width() + old_min_x;
On 2014/02/12 00:35:35, enne wrote:
> right()

Done.

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:362: int old_max_y =
last_visible_rect_in_content_space_.height() + old_min_y;
On 2014/02/12 00:35:35, enne wrote:
> bottom()

Done.

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:371: int min_x_adjustment = std::min<int>(
On 2014/02/12 00:35:35, enne wrote:
> I feel like this could all get simpler if you just created a viewport rect
inset
> by -skewport_limit and intersected the skewport by it at the very end.
> 
> Then there's no need for std::abs and multipliers, you could just adjust by
> extrapolation * (new - old).

Yay for stepping back and saying "this could be simpler" :) I changed it. PTAL.

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/140513006/diff/670001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_unittest.cc:315: // Expand viewport.
On 2014/02/12 00:35:35, enne wrote:
> Two more tests: one for a viewport expanded past the skewport limit in all
> directions, one for a viewport that shrinks to make sure the skewport ==
> viewport in that case?

Added one more for expand, the shrink case is in ComputeSkewport test.

[enne (OOO), 2014-02-12 18:23:57]

lgtm

https://codereview.chromium.org/140513006/diff/760001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/140513006/diff/760001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:382: // Clip the skewport to
|max_skewport|.
Yes, much better.  :)

[vmpstr, 2014-02-12 19:18:05]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-02-12 19:19:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/140513006/760001

[commit-bot: I haz the power, 2014-02-12 19:20:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 19:20:07]

Failed to apply patch for cc/layers/picture_layer_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layers/picture_layer_impl.cc
  Hunk #2 FAILED at 333.
  Hunk #3 succeeded at 511 (offset -1 lines).
  Hunk #4 succeeded at 735 (offset -1 lines).
  Hunk #5 succeeded at 803 (offset -1 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
cc/layers/picture_layer_impl.cc.rej

Patch:       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
a8641ce66081bb8843e5db61234439a78d5ded15..8b64b8e42cc9cb805d5e211e695727b4cfa2cf7d
100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -40,7 +40,6 @@ PictureLayerImpl::PictureLayerImpl(LayerTreeImpl* tree_impl,
int id)
     : LayerImpl(tree_impl, id),
       twin_layer_(NULL),
       pile_(PicturePileImpl::Create()),
-      last_content_scale_(0),
       is_mask_(false),
       ideal_page_scale_(0.f),
       ideal_device_scale_(0.f),
@@ -334,42 +333,31 @@ void PictureLayerImpl::UpdateTilePriorities() {
 
   UpdateLCDTextStatus(can_use_lcd_text());
 
-  gfx::Transform current_screen_space_transform = screen_space_transform();
-
-  gfx::Size viewport_size = layer_tree_impl()->DrawViewportSize();
-  gfx::Rect viewport_in_content_space;
-  gfx::Transform screen_to_layer(gfx::Transform::kSkipInitialization);
-  if (screen_space_transform().GetInverse(&screen_to_layer)) {
-    viewport_in_content_space =
-        gfx::ToEnclosingRect(MathUtil::ProjectClippedRect(
-            screen_to_layer, gfx::Rect(viewport_size)));
+  // Use visible_content_rect, unless it's empty. If it's empty, then
+  // try to inverse project the viewport into layer space and use that.
+  gfx::Rect visible_rect_in_content_space = visible_content_rect();
+  if (visible_rect_in_content_space.IsEmpty()) {
+    gfx::Transform screen_to_layer(gfx::Transform::kSkipInitialization);
+    if (screen_space_transform().GetInverse(&screen_to_layer)) {
+      gfx::Size viewport_size = layer_tree_impl()->DrawViewportSize();
+      visible_rect_in_content_space =
+          gfx::ToEnclosingRect(MathUtil::ProjectClippedRect(
+              screen_to_layer, gfx::Rect(viewport_size)));
+      visible_rect_in_content_space.Intersect(gfx::Rect(content_bounds()));
+    }
   }
 
   WhichTree tree =
       layer_tree_impl()->IsActiveTree() ? ACTIVE_TREE : PENDING_TREE;
-  size_t max_tiles_for_interest_area =
-      layer_tree_impl()->settings().max_tiles_for_interest_area;
-  tilings_->UpdateTilePriorities(
-      tree,
-      viewport_size,
-      viewport_in_content_space,
-      visible_content_rect(),
-      last_bounds_,
-      bounds(),
-      last_content_scale_,
-      contents_scale_x(),
-      last_screen_space_transform_,
-      current_screen_space_transform,
-      current_frame_time_in_seconds,
-      max_tiles_for_interest_area);
+
+  tilings_->UpdateTilePriorities(tree,
+                                 visible_rect_in_content_space,
+                                 contents_scale_x(),
+                                 current_frame_time_in_seconds);
 
   if (layer_tree_impl()->IsPendingTree())
     MarkVisibleResourcesAsRequired();
 
-  last_screen_space_transform_ = current_screen_space_transform;
-  last_bounds_ = bounds();
-  last_content_scale_ = contents_scale_x();
-
   // Tile priorities were modified.
   layer_tree_impl()->DidModifyTilePriorities();
 }
@@ -513,6 +501,20 @@ const PictureLayerTiling* PictureLayerImpl::GetTwinTiling(
   return NULL;
 }
 
+size_t PictureLayerImpl::GetMaxTilesForInterestArea() const {
+  return layer_tree_impl()->settings().max_tiles_for_interest_area;
+}
+
+float PictureLayerImpl::GetSkewportTargetTimeInSeconds() const {
+  return layer_tree_impl()->settings().skewport_target_time_in_seconds;
+}
+
+int PictureLayerImpl::GetSkewportExtrapolationLimitInContentPixels() const {
+  return layer_tree_impl()
+      ->settings()
+      .skewport_extrapolation_limit_in_content_pixels;
+}
+
 gfx::Size PictureLayerImpl::CalculateTileSize(
     const gfx::Size& content_bounds) const {
   if (is_mask_) {
@@ -723,12 +725,6 @@ void PictureLayerImpl::MarkVisibleResourcesAsRequired()
const {
       if (!*iter || !iter->IsReadyToDraw())
         continue;
 
-      // This iteration is over the visible content rect which is potentially
-      // less conservative than projecting the viewport into the layer.
-      // Ignore tiles that are know to be outside the viewport.
-      if (iter->priority(PENDING_TREE).distance_to_visible_in_pixels != 0)
-        continue;
-
       missing_region.Subtract(iter.geometry_rect());
       iter->MarkRequiredForActivation();
     }
@@ -797,12 +793,6 @@ bool PictureLayerImpl::MarkVisibleTilesAsRequired(
     if (!tile)
       continue;
 
-    // This iteration is over the visible content rect which is potentially
-    // less conservative than projecting the viewport into the layer.
-    // Ignore tiles that are know to be outside the viewport.
-    if (tile->priority(PENDING_TREE).distance_to_visible_in_pixels != 0)
-      continue;
-
     // If the missing region doesn't cover it, this tile is fully
     // covered by acceptable tiles at other scales.
     if (!missing_region.Intersects(iter.geometry_rect()))

[vmpstr, 2014-02-12 21:56:11]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-02-12 21:58:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/140513006/880001

[commit-bot: I haz the power, 2014-02-12 22:21:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 22:21:52]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[vmpstr, 2014-02-12 22:28:34]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-02-12 22:29:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/140513006/1150001

[commit-bot: I haz the power, 2014-02-13 00:14:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 00:14:52]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[vmpstr, 2014-02-13 00:23:56]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-02-13 00:27:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/140513006/1150001

[commit-bot: I haz the power, 2014-02-13 02:54:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:54:13]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[vmpstr, 2014-02-13 16:51:12]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-02-13 16:51:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/140513006/1150001

[commit-bot: I haz the power, 2014-02-13 16:55:44]

Change committed as 251064