cc: Avoid redraw for missing tile outside visible rect

cc: Avoid redraw for missing tile outside visible rect

Tiles outside of the visible rect for tile priority but
inside the draw viewport should be drawn on a best effort
basis. There is no need to redraw or block activations on
missing or incomplete tiles in this region.

Rename tile priority rect/matrix to activation rect, and
use this to control activation as well. Add new counts for
missing/incomplete tiles in the activation rect, and only
these tiles will lead to redraws.

BUG=403982
NOTRY=true

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

[boliu, 2014-08-15 21:24:06]

Factored out the code to calculate the "visible rect for tile priority in
content space" into a function, and use it in MarkVisibleResourcesAsRequired and
AppendQuads.

Existing tests are passing. Ok for a first pass review?

[aelias_OOO_until_Jul13, 2014-08-15 21:38:02]

This is looking fine to me, just needs tests.  Enne, any objections?

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

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:297: geometry_rect.Intersects(tile_rect)) {
We only iterate over the visible_content_rect in this method, so I don't get
this, won't everything intersect?

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:373: append_quads_data->num_missing_tiles++;
num missing tiles is used for UMA metrics to tell how much checkerboarding was
seen, so we can't do this optionally.

[hush (inactive), 2014-08-15 21:53:19]

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:779:
rect.Intersect(GetVisibleRectForTilePriorityInContentSpace());
I think you need to early out when intersection is empty

[boliu, 2014-08-15 21:56:27]

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:297: geometry_rect.Intersects(tile_rect)) {
On 2014/08/15 21:43:44, danakj wrote:
> We only iterate over the visible_content_rect in this method, so I don't get
> this, won't everything intersect?

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.

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:373: append_quads_data->num_missing_tiles++;
On 2014/08/15 21:43:44, danakj wrote:
> num missing tiles is used for UMA metrics to tell how much checkerboarding was
> seen, so we can't do this optionally.

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]

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:267: gfx::Rect tile_rect =
GetVisibleRectForTilePriorityInContentSpace();
scaled_tile_priority_rect?

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:297: geometry_rect.Intersects(tile_rect)) {
On 2014/08/15 21:56:26, boliu wrote:
> On 2014/08/15 21:43:44, danakj wrote:
> > We only iterate over the visible_content_rect in this method, so I don't get
> > this, won't everything intersect?
> 
> 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.

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?

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:373: append_quads_data->num_missing_tiles++;
On 2014/08/15 21:56:26, boliu wrote:
> On 2014/08/15 21:43:44, danakj wrote:
> > num missing tiles is used for UMA metrics to tell how much checkerboarding
was
> > seen, so we can't do this optionally.
> 
> 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.

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

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:779:
rect.Intersect(GetVisibleRectForTilePriorityInContentSpace());
On 2014/08/15 21:53:18, hush wrote:
> I think you need to early out when intersection is empty

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.

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

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:297: geometry_rect.Intersects(tile_rect)) {
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]

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:297: geometry_rect.Intersects(tile_rect)) {
On 2014/08/15 22:12:37, hush wrote:
> 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?

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]

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:297: geometry_rect.Intersects(tile_rect)) {
On 2014/08/15 22:09:08, danakj wrote:
> On 2014/08/15 21:56:26, boliu wrote:
> > On 2014/08/15 21:43:44, danakj wrote:
> > > We only iterate over the visible_content_rect in this method, so I don't
get
> > > this, won't everything intersect?
> > 
> > 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.
> 
> 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?

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?

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:779:
rect.Intersect(GetVisibleRectForTilePriorityInContentSpace());
On 2014/08/15 22:09:08, danakj wrote:
> On 2014/08/15 21:53:18, hush wrote:
> > I think you need to early out when intersection is empty
> 
> Why are we intersecting instead of just using the tile priority rect?

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?

> For cases
> where visible_content_rect is empty and we made something up? We early out
above
> in empty case already.

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

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:267: gfx::Rect tile_rect =
GetVisibleRectForTilePriorityInContentSpace();
"ForTilePriority" also seems not true anymore.  Is there a better name?

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

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:297: geometry_rect.Intersects(tile_rect)) {
Thanks Dana!
So I suppose the dest_scale_ in PictureLayerTiling just means geometry scale.

On 2014/08/15 22:20:45, danakj wrote:
> On 2014/08/15 22:12:37, hush wrote:
> > 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?
> 
> 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.

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

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:297: geometry_rect.Intersects(tile_rect)) {
On 2014/08/15 22:35:36, hush wrote:
> Thanks Dana!
> So I suppose the dest_scale_ in PictureLayerTiling just means geometry scale.

Yeh
 
> On 2014/08/15 22:20:45, danakj wrote:
> > On 2014/08/15 22:12:37, hush wrote:
> > > 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?
> > 
> > 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 23:07:26]

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:267: gfx::Rect tile_rect =
GetVisibleRectForTilePriorityInContentSpace();
On 2014/08/15 22:33:50, enne wrote:
> "ForTilePriority" also seems not true anymore.  Is there a better name?

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

https://codereview.chromium.org/475233002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:779:
rect.Intersect(GetVisibleRectForTilePriorityInContentSpace());
On 2014/08/15 22:33:42, boliu wrote:
> On 2014/08/15 22:09:08, danakj wrote:
> > On 2014/08/15 21:53:18, hush wrote:
> > > I think you need to early out when intersection is empty
> > 
> > Why are we intersecting instead of just using the tile priority rect?
> 
> 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.

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.

> 
> Umm...thoughts?
> 
> > For cases
> > where visible_content_rect is empty and we made something up? We early out
> above
> > in empty case already.
>

[boliu, 2014-08-16 00:05:33]

Renamed new code to "activation rect". Will rename existing code in follow up.

Added separate counts for missing and incomplete tiles inside activation rect.

Added intersect activation rect with visible_content_rect() in
UpdateTilePriorities

[hush (inactive), 2014-08-16 00:21:52]

On 2014/08/16 00:05:33, boliu wrote:
> Renamed new code to "activation rect". Will rename existing code in follow up.
> 
> Added separate counts for missing and incomplete tiles inside activation rect.
> 
> Added intersect activation rect with visible_content_rect() in
> UpdateTilePriorities

lgtm

[boliu, 2014-08-18 13:44:17]

Add tests

[boliu, 2014-08-18 18:42:22]

This is ready for a more thorough review.

Chose "activation rect" as the name. Only used the new name in new code to keep
this smaller.

[hush (inactive), 2014-08-18 20:48:16]

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

https://codereview.chromium.org/475233002/diff/140001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1516:
tile_version.SetSolidColorForTesting(SK_ColorRED);
are you going to test if the color of these tiles is indeed red after the active
layer is drawn?

https://codereview.chromium.org/475233002/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/475233002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2089: bool tile_missing = false;
should this be true?
tile_missing_outside_activation_rect is true implies tile_missing is true,
right?

https://codereview.chromium.org/475233002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2092: bool
had_incomplete_tile_outside_activation_rect = false;
above 2 lines means there are incomplete tiles inside the activation rect. How
about setting both to false, so that the existence of incomplete tiles does not
interfere with the test results? Because you are trying to test
PrepareToDrawSucceedsWithMissingTileOutsideActivationRect here.

[boliu, 2014-08-18 21:32:57]

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

https://codereview.chromium.org/475233002/diff/140001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1516:
tile_version.SetSolidColorForTesting(SK_ColorRED);
On 2014/08/18 20:48:16, hush wrote:
> are you going to test if the color of these tiles is indeed red after the
active
> layer is drawn?

I think checking the quads are red isn't really the point of this test.

https://codereview.chromium.org/475233002/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/475233002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2089: bool tile_missing = false;
On 2014/08/18 20:48:16, hush wrote:
> should this be true?
> tile_missing_outside_activation_rect is true implies tile_missing is true,
> right?

Not as written. But I can see why this is confusing.

Renamed the vars without suffix as "_inside_activation_rect".

https://codereview.chromium.org/475233002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2092: bool
had_incomplete_tile_outside_activation_rect = false;
On 2014/08/18 20:48:16, hush wrote:
> above 2 lines means there are incomplete tiles inside the activation rect. How
> about setting both to false, so that the existence of incomplete tiles does
not
> interfere with the test results? Because you are trying to test
> PrepareToDrawSucceedsWithMissingTileOutsideActivationRect here.

Done. Good catch.

[boliu, 2014-08-19 16:07:59]

ping?

[enne (OOO), 2014-08-19 17:55:18]

Why does AppendQuadsData need a separate set of variables for activation rect?

[danakj, 2014-08-19 17:58:46]

On Tue, Aug 19, 2014 at 1:55 PM, <enne@chromium.org> wrote:

> Why does AppendQuadsData need a separate set of variables for activation
> rect?
>

I asked for that because conditionally setting the missing quads variable
made it very unclear if we were correctly tracking missing quads for our
UMA stats (which we supposedly were still on windows because the condition
would always be true but it's not obvious from the code). Adding a couple
ints/bools to that singleton struct seemed way more clear to me. WDYT?


>
> https://codereview.chromium.org/475233002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-08-19 18:04:48]

On 2014/08/19 at 17:58:46, danakj wrote:
> On Tue, Aug 19, 2014 at 1:55 PM, <enne@chromium.org> wrote:
> 
> > Why does AppendQuadsData need a separate set of variables for activation
> > rect?
> 
> I asked for that because conditionally setting the missing quads variable
> made it very unclear if we were correctly tracking missing quads for our
> UMA stats (which we supposedly were still on windows because the condition
> would always be true but it's not obvious from the code). Adding a couple
> ints/bools to that singleton struct seemed way more clear to me. WDYT?

I would rather us just not track these sort of stats for WebView (or when one of
these external viewports is specified).  I don't think tracking them for WebView
when external draw contraints are set meets the purpose of what we were trying
to figure out with those stats.  Personally, I'd rather just have one
conditional to say "don't send these stats" rather than adding a whole bunch of
complication around tracking different numbers.  How's that sound?

[boliu, 2014-08-19 18:11:02]

On 2014/08/19 18:04:48, enne wrote:
> On 2014/08/19 at 17:58:46, danakj wrote:
> > On Tue, Aug 19, 2014 at 1:55 PM, <mailto:enne@chromium.org> wrote:
> > 
> > > Why does AppendQuadsData need a separate set of variables for activation
> > > rect?
> > 
> > I asked for that because conditionally setting the missing quads variable
> > made it very unclear if we were correctly tracking missing quads for our
> > UMA stats (which we supposedly were still on windows because the condition
> > would always be true but it's not obvious from the code). Adding a couple
> > ints/bools to that singleton struct seemed way more clear to me. WDYT?
> 
> I would rather us just not track these sort of stats for WebView (or when one
of
> these external viewports is specified).  I don't think tracking them for
WebView
> when external draw contraints are set meets the purpose of what we were trying
> to figure out with those stats.  Personally, I'd rather just have one
> conditional to say "don't send these stats" rather than adding a whole bunch
of
> complication around tracking different numbers.  How's that sound?

Webview does not upload any UMA :(, but let's assume it does.

Not tracking these just means skipping the UMA_HISTOGRAM_COUNTS_* code? Would
that make overall stats look better just because webview is not tracking them?

[enne (OOO), 2014-08-19 18:20:41]

On 2014/08/19 at 18:11:02, boliu wrote:

> Webview does not upload any UMA :(, but let's assume it does.
>
> Not tracking these just means skipping the UMA_HISTOGRAM_COUNTS_* code? Would
that make overall stats look better just because webview is not tracking them?

Yeah.  I was picturing just skipping that code.  It would make the overall stats
better, but I think it might be unfair to lump these stats in.  A non-WebView
page that can jump around randomly would be pushed through the activation code
and wouldn't have missing tiles.  It would only get missing tiles for
animations/scrolling/pinch zoom, which has some locality to it.

I don't feel super strongly about this if you and danakj want to track it this
way.  It just seems like it'd be simpler to not track so many things.  :)

[boliu, 2014-08-19 18:39:14]

On 2014/08/19 18:20:41, enne wrote:
> On 2014/08/19 at 18:11:02, boliu wrote:
> 
> > Webview does not upload any UMA :(, but let's assume it does.
> >
> > Not tracking these just means skipping the UMA_HISTOGRAM_COUNTS_* code?
Would
> that make overall stats look better just because webview is not tracking them?
> 
> Yeah.  I was picturing just skipping that code.  It would make the overall
stats
> better, but I think it might be unfair to lump these stats in.  A non-WebView
> page that can jump around randomly would be pushed through the activation code
> and wouldn't have missing tiles.  It would only get missing tiles for
> animations/scrolling/pinch zoom, which has some locality to it.

Oh I see. Lumping webview with chrome would make overall look worse, because
missing/incomplete tiles may be expected in webview in the cases you listed.

I think for UMA, the cleanest thing would be switch to tracking activation rect,
which makes sense in both webview and chrome. But there's a logic jump that this
doesn't change the UMA for chrome, which goes back to Dana's original complaint
that it looks like UMA is modified.

My vote goes to switching to tracking activation rect everywhere. Thoughts?

> 
> I don't feel super strongly about this if you and danakj want to track it this
> way.  It just seems like it'd be simpler to not track so many things.  :)

[danakj, 2014-08-19 19:16:03]

On Tue, Aug 19, 2014 at 2:39 PM, <boliu@chromium.org> wrote:

> On 2014/08/19 18:20:41, enne wrote:
>
>> On 2014/08/19 at 18:11:02, boliu wrote:
>>
>
>  > Webview does not upload any UMA :(, but let's assume it does.
>> >
>> > Not tracking these just means skipping the UMA_HISTOGRAM_COUNTS_* code?
>>
> Would
>
>> that make overall stats look better just because webview is not tracking
>> them?
>>
>
>  Yeah.  I was picturing just skipping that code.  It would make the overall
>>
> stats
>
>> better, but I think it might be unfair to lump these stats in.  A
>> non-WebView
>> page that can jump around randomly would be pushed through the activation
>> code
>> and wouldn't have missing tiles.  It would only get missing tiles for
>> animations/scrolling/pinch zoom, which has some locality to it.
>>
>
> Oh I see. Lumping webview with chrome would make overall look worse,
> because
> missing/incomplete tiles may be expected in webview in the cases you
> listed.
>
> I think for UMA, the cleanest thing would be switch to tracking activation
> rect,
> which makes sense in both webview and chrome. But there's a logic jump
> that this
> doesn't change the UMA for chrome, which goes back to Dana's original
> complaint
> that it looks like UMA is modified.
>
> My vote goes to switching to tracking activation rect everywhere. Thoughts?


What guarantee is there in the code that the activation rect is everything
that you can see elsewhere? What if we pinch zoom out, does the activation
rect change? But the tree has already been activated. Maybe it's the name
but this seems unclear to me, can you help me feel like this will be
reliable?


>
>
>
>  I don't feel super strongly about this if you and danakj want to track it
>> this
>> way.  It just seems like it'd be simpler to not track so many things.  :)
>>
>
>
>
> https://codereview.chromium.org/475233002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-19 19:29:39]

Ok enne@ talked to me about this. I was under the impression you wanted to only
++num_missing_tiles when the tile is in the activation rect for some functional
reason, but if we can just not add conditionals around that /and/ not add new
members to AppendQuadsData and just not UMA_THING then I'm super happy.

[boliu, 2014-08-19 20:32:49]

On 2014/08/19 19:29:39, danakj wrote:
> Ok enne@ talked to me about this. I was under the impression you wanted to
only
> ++num_missing_tiles when the tile is in the activation rect for some
functional
> reason,

There are functional reasons. I only want to redraw (set
frame->contains_incomplete_tile) if the missing/incomplete tiles are inside the
activation rect.

> but if we can just not add conditionals around that /and/ not add new
> members to AppendQuadsData and just not UMA_THING then I'm super happy.

[enne (OOO), 2014-08-19 20:44:34]

On 2014/08/19 at 20:32:49, boliu wrote:
> On 2014/08/19 19:29:39, danakj wrote:
> > Ok enne@ talked to me about this. I was under the impression you wanted to
only
> > ++num_missing_tiles when the tile is in the activation rect for some
functional
> > reason,
> 
> There are functional reasons. I only want to redraw (set
frame->contains_incomplete_tile) if the missing/incomplete tiles are inside the
activation rect.

Maybe I don't understand, then.  If you're appending missing/incomplete quads to
the frame at all, don't you need to redraw? Alternatively, if you're appending
missing/incomplete quads and you don't, are you appending quads that don't end
up on screen?

[boliu, 2014-08-19 20:55:17]

On 2014/08/19 20:44:34, enne wrote:
> On 2014/08/19 at 20:32:49, boliu wrote:
> > On 2014/08/19 19:29:39, danakj wrote:
> > > Ok enne@ talked to me about this. I was under the impression you wanted to
> only
> > > ++num_missing_tiles when the tile is in the activation rect for some
> functional
> > > reason,
> > 
> > There are functional reasons. I only want to redraw (set
> frame->contains_incomplete_tile) if the missing/incomplete tiles are inside
the
> activation rect.
> 
> Maybe I don't understand, then.  If you're appending missing/incomplete quads
to
> the frame at all, don't you need to redraw? Alternatively, if you're appending
> missing/incomplete quads and you don't, are you appending quads that don't end
> up on screen?

The second one, we are appending quads that don't end up on screen.

More detailed reason: Android L added the "RenderThread", which is a lot like
when the chrome compositor went from single threaded to using the compositor
thread. There can be render thread animations (like how compositor thread can do
its own scrolling). One nasty part of these animations is the render thread
animations can make more content visible, and there is no time to go to cc to
get another frame filling in with the new content.

So Alex and I figured just have cc draw everything (the draw viewport), but make
sure tile priority is still correct (added the tile priority rect/matrix). So if
the render thread animation is simple scrolling, we have enough tiles available
to cover it until we go back to cc and get a new frame.

[boliu, 2014-08-19 22:50:46]

On 2014/08/19 20:55:17, boliu wrote:
> On 2014/08/19 20:44:34, enne wrote:
> > Maybe I don't understand, then.  If you're appending missing/incomplete
quads
> to
> > the frame at all, don't you need to redraw? Alternatively, if you're
appending
> > missing/incomplete quads and you don't, are you appending quads that don't
end
> > up on screen?
> 
> The second one, we are appending quads that don't end up on screen.
> 
> More detailed reason: Android L added the "RenderThread", which is a lot like
> when the chrome compositor went from single threaded to using the compositor
> thread. There can be render thread animations (like how compositor thread can
do
> its own scrolling). One nasty part of these animations is the render thread
> animations can make more content visible, and there is no time to go to cc to
> get another frame filling in with the new content.
> 
> So Alex and I figured just have cc draw everything (the draw viewport), but
make
> sure tile priority is still correct (added the tile priority rect/matrix). So
if
> the render thread animation is simple scrolling, we have enough tiles
available
> to cover it until we go back to cc and get a new frame.

I guess another analogy is how chrome records 8k (I think?) pixels around the
viewport to enable impl-side painting/scrolling. Webview is "recording" as many
tiles as gpu memory allows into a cc delegated frame, to enable
render-thread-side scrolls.

Hope that's clear

So how should this move forward with the UMA thing? I still vote for switching
to tracking activation rect tiles in both chrome and webview.

[enne (OOO), 2014-08-19 22:59:55]

Thanks for all the explanation.  I apologize for my misunderstanding.

I think I prefer what I think you are suggesting.  In other words, only mark
missing/incomplete tiles inside of the activation rect in AppendQuadsData. 
Don't add additional members to AppendQuadsData.  Maybe also leave a comment
here as to how missing quads might be appended but not counted?

danakj mentioned to me that the name "activation_rect" doesn't make much sense
for a rect that you're using on the active tree.  It may not have even been the
rect you used for activation, since it could change from frame to frame for the
same active tree.  I'm not sure what to call it.  "prioritization_viewport_rect"
maybe?

[boliu, 2014-08-19 23:22:36]

On 2014/08/19 22:59:55, enne wrote:
> Thanks for all the explanation.  I apologize for my misunderstanding.
> 

No problems :D

> I think I prefer what I think you are suggesting.  In other words, only mark
> missing/incomplete tiles inside of the activation rect in AppendQuadsData. 
> Don't add additional members to AppendQuadsData.  Maybe also leave a comment
> here as to how missing quads might be appended but not counted?

Yep, that's my proposal.

> 
> danakj mentioned to me that the name "activation_rect" doesn't make much sense
> for a rect that you're using on the active tree.  It may not have even been
the
> rect you used for activation, since it could change from frame to frame for
the
> same active tree.  I'm not sure what to call it. 
"prioritization_viewport_rect"
> maybe?

Name..

The name hush introduced is "external_viewport_for_tile_prioritization". Can
drop the "external" part in some places. So how about
"viewport_for_tile_prioritization". Will also need a comment that it now also
controls the activation rect.

[enne (OOO), 2014-08-19 23:32:29]

On 2014/08/19 at 23:22:36, boliu wrote:

> Name..
> 
> The name hush introduced is "external_viewport_for_tile_prioritization". Can
drop the "external" part in some places. So how about
"viewport_for_tile_prioritization". Will also need a comment that it now also
controls the activation rect.

viewport_for_tile_prioritization sounds good to me.  I think a comment in the
mark as required function about why this viewport is being used and what that
means for WebView vs. non-WebView might be enough to explain that detail.

[boliu, 2014-08-20 03:00:06]

PTAL

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

https://codereview.chromium.org/475233002/diff/200001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:274: bool
viewport_for_tile_priority_contains_visible_rect =
This is always true for chrome, so I thought it's a nice optimization (even in
webview). Not sure if it's worth it over the additional complexity though. I'm
not overly attached.

[danakj, 2014-08-20 14:27:48]

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

https://codereview.chromium.org/475233002/diff/200001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:304:
!viewport_for_tile_priority_contains_visible_rect &&
Shouldn't this be

viewport_for_tile_prio_contains_vis_rect ||
geometry_rect.Intersects(scaled_viewport_for_tile_priority) ?

If the visible content rect is inside the the priority rect we always want to do
this ++, right?

[boliu, 2014-08-20 15:26:15]

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

https://codereview.chromium.org/475233002/diff/200001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:304:
!viewport_for_tile_priority_contains_visible_rect &&
On 2014/08/20 14:27:48, danakj wrote:
> Shouldn't this be
> 
> viewport_for_tile_prio_contains_vis_rect ||
> geometry_rect.Intersects(scaled_viewport_for_tile_priority) ?
> 
> If the visible content rect is inside the the priority rect we always want to
do
> this ++, right?

Yep. Renamed the var to "skip_viewport_for_tile_priority_check" to make it a
little clearer.

[danakj, 2014-08-20 15:31:12]

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

https://codereview.chromium.org/475233002/diff/220001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:273: // Optmization to avoid Contains checks. No
need for checks for
s/Contains/Intersects/?

[boliu, 2014-08-20 15:38:05]

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

https://codereview.chromium.org/475233002/diff/220001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:273: // Optmization to avoid Contains checks. No
need for checks for
On 2014/08/20 15:31:12, danakj wrote:
> s/Contains/Intersects/?

Done.

And fixed spelling in other comments too..

[enne (OOO), 2014-08-20 17:33:21]

lgtm % danakj

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

https://codereview.chromium.org/475233002/diff/240001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:275: bool skip_viewport_for_tile_priority_check
=
I don't know that it's worth this boolean and a complicated conditional to save
a few comparisons of integers on the stack.

[boliu, 2014-08-20 17:38:03]

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

https://codereview.chromium.org/475233002/diff/240001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:275: bool skip_viewport_for_tile_priority_check
=
On 2014/08/20 17:33:21, enne wrote:
> I don't know that it's worth this boolean and a complicated conditional to
save
> a few comparisons of integers on the stack.

It's always true for chrome. So I guess it also makes the logic jump that this
is no-op for chrome a bit smaller.

I'm not overly attached. I can remove it if Dana is feels the same?

[danakj, 2014-08-20 19:14:52]

LGTM

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

https://codereview.chromium.org/475233002/diff/240001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:275: bool skip_viewport_for_tile_priority_check
=
On 2014/08/20 17:38:03, boliu wrote:
> On 2014/08/20 17:33:21, enne wrote:
> > I don't know that it's worth this boolean and a complicated conditional to
> save
> > a few comparisons of integers on the stack.
> 
> It's always true for chrome. So I guess it also makes the logic jump that this
> is no-op for chrome a bit smaller.
> 
> I'm not overly attached. I can remove it if Dana is feels the same?

I don't feel strongly. We'll have a branch either way.

[boliu, 2014-08-20 19:22:27]

Removed skip_viewport_for_tile_priority_check.

Thanks all for the review. cq-ing now :)

[boliu, 2014-08-20 19:22:48]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-08-20 19:24:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/475233002/280001

[commit-bot: I haz the power, 2014-08-20 21:08:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-20 21:12:36]

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

[commit-bot: I haz the power, 2014-08-20 21:12:37]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[boliu, 2014-08-20 22:43:39]

win_chromium_rel_swarming is known flake (crbug.com/253092)
linux_gpu has a bot issue (trooper notified)
Plus everything was green on PS13

NOTRY!

[boliu, 2014-08-20 22:44:23]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-08-20 22:48:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/475233002/280001

[commit-bot: I haz the power, 2014-08-20 22:51:23]

Committed patchset #15 (280001) as 290937