Fixing tile grid size used by cc:Picture to make it respect current tile configuration

Fixing tile grid size used by cc:Picture to make it respect current tile configuration.  And rolling skia DEPS to r7896

BUG=https://code.google.com/p/chromium/issues/detail?id=174965
TEST=telemetry scrolling tests with impl-side painting enabled.

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

[Justin Novosad, 2013-02-07 20:33:21]

A bunch of changes in tiling_data.cc are not strictly necessary for this fix,
they're more of a drive-by code simplification.  I could re-write that part more
conservatively if churn is a concern for merging to the android mega patch.

[danakj, 2013-02-07 20:40:01]

Why does it help to make the tile size of the recording match the size of
non-edge tiles excluding border pixels after rasterizing?

https://codereview.chromium.org/12221077/diff/1/cc/tiling_data.cc
File cc/tiling_data.cc (right):

https://codereview.chromium.org/12221077/diff/1/cc/tiling_data.cc#newcode216
cc/tiling_data.cc:216: return max_texture_size_.width() - border_texels_;
I'm not sure about the inner_tile_size concept because it obscures lines like
this. Not all tiles are sized to be inner_tile_size.

I assumed inner meant inside border texels. But it's only true on "inner" tiles,
meaning they are not on the edge of the tiling.

[Justin Novosad, 2013-02-08 16:12:01]

On 2013/02/07 20:40:01, danakj wrote:
> Why does it help to make the tile size of the recording match the size of
> non-edge tiles excluding border pixels after rasterizing?

Because it makes the lookups one-to-one in the tileGrid structure when 
everything matches-up.  This means lookup is faster because tileGrid does not
have to merge contents from neighboring tiles and it means picture playback is
faster because extraneous content is avoided.  BTW the SkTileGrid has it own
built-in border overlap provisions for anti-aliasing, so everything still
lines-up perfectly when the border texels are added to the query rectangle for
SkTileGrid lookup, as long as the page is at zoom 1:1

> 
> https://codereview.chromium.org/12221077/diff/1/cc/tiling_data.cc
> File cc/tiling_data.cc (right):
> 
> https://codereview.chromium.org/12221077/diff/1/cc/tiling_data.cc#newcode216
> cc/tiling_data.cc:216: return max_texture_size_.width() - border_texels_;
> I'm not sure about the inner_tile_size concept because it obscures lines like
> this. Not all tiles are sized to be inner_tile_size.
> 
> I assumed inner meant inside border texels. But it's only true on "inner"
tiles,
> meaning they are not on the edge of the tiling.

[Justin Novosad, 2013-02-08 16:13:43]

On 2013/02/07 20:40:01, danakj wrote:
> https://codereview.chromium.org/12221077/diff/1/cc/tiling_data.cc
> File cc/tiling_data.cc (right):
> I assumed inner meant inside border texels. But it's only true on "inner"
tiles,
> meaning they are not on the edge of the tiling.

Ok, I'll redo this part the simple way.

[Justin Novosad, 2013-02-08 17:38:55]

New Patch.

Reverted most changes to tiling_data.  Now, I'm just adding a simple new method.
 Calling it TileStride instead of InnerTileSize.  I think that more accurately
describes what that number is all about.

[Justin Novosad, 2013-02-11 16:30:29]

Ping.

[danakj, 2013-02-11 17:12:46]

https://codereview.chromium.org/12221077/diff/9002/cc/tiling_data.cc
File cc/tiling_data.cc (right):

https://codereview.chromium.org/12221077/diff/9002/cc/tiling_data.cc#newcode261
cc/tiling_data.cc:261: tile_grid_size.Enlarge(- 2 * border_texels_, - 2 *
border_texels_);
Does the TileGrid assume border pixels are on the outside of the grid squares or
on the inside of them like TilingData does?

[enne (OOO), 2013-02-11 17:45:19]

It looks like you're using the tile size of the picture pile recording here as
the tile grid size.  However, the SkPicture in the picture pile is already
naturally bounded to that tile size; we don't record over picture pile tile
edges in the same SkPicture.  In other words, an equivalent change to this CL
would be to disable tile grid entirely.

I was under the impression that the tile grid was an playback optimization and
not a recording one, and so that you'd want to pick something close to the tile
size used for playback and not for recording.  (And, maybe the fact that we are
using different tile sizes for playback means that the tile grid optimization
isn't a good fit?)

[Justin Novosad, 2013-02-12 17:58:10]

On 2013/02/11 17:45:19, enne wrote:
> It looks like you're using the tile size of the picture pile recording here as
> the tile grid size.  However, the SkPicture in the picture pile is already
> naturally bounded to that tile size; we don't record over picture pile tile
> edges in the same SkPicture.  In other words, an equivalent change to this CL
> would be to disable tile grid entirely.
> 
> I was under the impression that the tile grid was an playback optimization and
> not a recording one, and so that you'd want to pick something close to the
tile
> size used for playback and not for recording.  (And, maybe the fact that we
are
> using different tile sizes for playback means that the tile grid optimization
> isn't a good fit?)

It is a playback optimization, but it needs to be configured at record time.
This change simply tunes the configuration of the tile grid for the case where
playback occurs at the same scale as the recording (which is a frequent case).

[Justin Novosad, 2013-02-12 18:00:01]

On 2013/02/11 17:12:46, danakj wrote:
> https://codereview.chromium.org/12221077/diff/9002/cc/tiling_data.cc
> File cc/tiling_data.cc (right):
> 
>
https://codereview.chromium.org/12221077/diff/9002/cc/tiling_data.cc#newcode261
> cc/tiling_data.cc:261: tile_grid_size.Enlarge(- 2 * border_texels_, - 2 *
> border_texels_);
> Does the TileGrid assume border pixels are on the outside of the grid squares
or
> on the inside of them like TilingData does?

It assumes that border pixels are on the outside of grid squares

[danakj, 2013-02-12 18:00:33]

On Tue, Feb 12, 2013 at 9:58 AM, <junov@chromium.org> wrote:

> On 2013/02/11 17:45:19, enne wrote:
>
>> It looks like you're using the tile size of the picture pile recording
>> here as
>> the tile grid size.  However, the SkPicture in the picture pile is already
>> naturally bounded to that tile size; we don't record over picture pile
>> tile
>> edges in the same SkPicture.  In other words, an equivalent change to
>> this CL
>> would be to disable tile grid entirely.
>>
>
>  I was under the impression that the tile grid was an playback
>> optimization and
>> not a recording one, and so that you'd want to pick something close to the
>>
> tile
>
>> size used for playback and not for recording.  (And, maybe the fact that
>> we
>>
> are
>
>> using different tile sizes for playback means that the tile grid
>> optimization
>> isn't a good fit?)
>>
>
> It is a playback optimization, but it needs to be configured at record
> time.
> This change simply tunes the configuration of the tile grid for the case
> where
> playback occurs at the same scale as the recording (which is a frequent
> case).
>

I'm still unclear how TileGrid works internally. Does the TileGrid assume
border pixels are on the outside of the grid
squares or on the inside of them like TilingData does? It'd be nice if it
worked the same way as cc/ - so if you gave it 256 it would use that many
pixels for each tile with 1px overlap for border pixels.


>
>
https://codereview.chromium.**org/12221077/<https://codereview.chromium.org/1...
>

[enne (OOO), 2013-02-12 19:07:37]

I'm still confused by what this patch is doing.

https://codereview.chromium.org/12221077/diff/9002/cc/picture_pile.cc
File cc/picture_pile.cc (right):

https://codereview.chromium.org/12221077/diff/9002/cc/picture_pile.cc#newcode94
cc/picture_pile.cc:94: (*pic)->Record(painter, stats, tiling_.TileStride());
What ballpark tile size are you expecting this to be?

[Justin Novosad, 2013-02-13 15:33:33]

Ok, the effect of this patch really isn't that complicated. Let's just start
from the beginning...

SkTileGrid is a data structure used to accelerate the search for the set of draw
commands in an SkPicture that intersect a given 'query' rectangle.  When the CC
draws tile content, it sets up an SkCanvas that uses the tile texture as a
backing store. A transform is applied to that SkCanvas so that it acts as a
windows into the SkPicture that contains the entire layer's content. The bounds
of this 'window' in the SkPicture's frame of reference constitutes the query
rectangle that is used by the SkTileGrid for isolating the draw commands that
intersect it.

Now here is how SkTileGrid works. The tile grid creates an array of tile
buckets.  When recording a draw command to an SkPicture with a tile grid
attached to it, a reference to the recorded command is stored in the buckets
that correspond to the tiles that intersect the bounding box of the draw
command. At playback time, queries are executed by computing which tiles of the
tile grid intersect the query rectangle, and the contents of the tile buckets
are merged to form a conservative query result (may contain extraneous
commands).

The objective of this change is to increase the likelihood that query rectangles
will align with the tile grid's tiles, which will result in a) no tile bucket
merging and b) no extraneous commands. 
When queries are not aligned with the tileGrid, it is no big deal: we still
benefit from an accelerated search, it is just not as fast as in the ideal case.
The extraneous commands still get filtered out of the stream, but at a later
stage.

This CL makes the structure of the tileGrid match the layout of compositor tiles
that are played back at scale 1:1, which is a very common case.

In SkTileGrid terminology, 'tile size' is the stride between buckets, such that
tileSizeX*numTilesX = tileGridWidth. This is where there was a disconnect with
the CC notion of tileSize, which corresponds to the size of the image buffer
that is necessary for covering the inside of the tile plus a border region.

[enne (OOO), 2013-02-13 16:40:44]

I feel like we're talking past each other here.  I'm not trying to be obtuse. 
Thanks for the explanation of tile grid; that was what I had supposed was going
on.

However, I've been trying to say that if that's what you're doing, you are using
the wrong tile size.  Not wrong in the sense of off by one border pixel, but
wrong in the sense of off by a factor of ~12.

[Tom Hudson, 2013-02-13 16:49:17]

On 2013/02/13 16:40:44, enne wrote:
> I feel like we're talking past each other here.  I'm not trying to be obtuse. 
> Thanks for the explanation of tile grid; that was what I had supposed was
going
> on.
> 
> However, I've been trying to say that if that's what you're doing, you are
using
> the wrong tile size.  Not wrong in the sense of off by one border pixel, but
> wrong in the sense of off by a factor of ~12.

This is one of those parts of CC that was utterly confusing to me reading the
code until somebody in MTV explained to me. There are two different tiling
systems in use, their names aren't very distinct, and there doesn't seem to be
either detailed comments or a high-level overview to explain that to the reader.
I wouldn't be surprised if Justin is as confused as I was/remain.

[enne (OOO), 2013-02-13 16:59:24]

For what it's worth, picture pile tiling didn't exist three weeks ago.  Once the
dust has settled a little bit more, I'll try to find some time to update
http://www.chromium.org/developers/design-documents/impl-side-painting to flesh
out more details about picture piles.

underconstruction.gif

[Justin Novosad, 2013-02-13 21:38:15]

On 2013/02/13 16:40:44, enne wrote:
> I feel like we're talking past each other here.  I'm not trying to be obtuse. 

Yeah, I feel like there is something I am not getting, but I have no idea what
it is. 

> Thanks for the explanation of tile grid; that was what I had supposed was
going
> on.
> 
> However, I've been trying to say that if that's what you're doing, you are
using
> the wrong tile size.  Not wrong in the sense of off by one border pixel, but
> wrong in the sense of off by a factor of ~12.

Ok, I don't understand that.  That statement disagrees with what I am observing
in the debugger. I verified that with this CL, SkTileGrid queries use the fast
path when rendering at 1:1 scale, and there is a measurable improvement to in
tile rasterization performance.

[Justin Novosad, 2013-02-14 19:24:47]

On 2013/02/12 19:07:37, enne wrote:
https://codereview.chromium.org/12221077/diff/9002/cc/picture_pile.cc#newcode94
> cc/picture_pile.cc:94: (*pic)->Record(painter, stats, tiling_.TileStride());
> What ballpark tile size are you expecting this to be?

254, 510 on Nexus 10

[enne (OOO), 2013-02-14 20:33:17]

On 2013/02/14 19:24:47, junov wrote:
> On 2013/02/12 19:07:37, enne wrote:
>
https://codereview.chromium.org/12221077/diff/9002/cc/picture_pile.cc#newcode94
> > cc/picture_pile.cc:94: (*pic)->Record(painter, stats, tiling_.TileStride());
> > What ballpark tile size are you expecting this to be?
> 
> 254, 510 on Nexus 10

I have no idea how you get that value.  With your patch, printing out
tiling_.TileStride() is 2970, 2970.  Tile size for the picture pile is set to
3000 in PicturePileBase.

[Justin Novosad, 2013-02-14 21:03:26]

Ok, so something changed since I originally started writing this patch (checkout
is now almost 2 weeks old).

Basically I am trying to determine what will be the size, in the SkPicture's
coordinate space of a single compositor tile when drawing at scale 1:1.  Any
suggestions?

[Justin Novosad, 2013-02-14 21:20:04]

I finally figured-out what y'all have been trying to explain to me for days
(thanks Tom).  I am looking for the size of playback tile (CC tiles) and I was
looking at the size of recording tiles (picture tiles).  Geez.  The existence of
these two different tilings was the bit I was missing.

[enne (OOO), 2013-02-14 22:36:17]

On 2013/02/14 21:03:26, junov wrote:
> Basically I am trying to determine what will be the size, in the SkPicture's
> coordinate space of a single compositor tile when drawing at scale 1:1.  Any
> suggestions?

From PictureLayer, you can ask layerTreeImpl()->settings().defaultTileSize.  You
could set this on the picture pile or something for it to use when recording.

Dana also suggested that you divide this tile size by the device scale (which
you can get from LayerImpl), since that's the ratio from the SkPicture's
coordinate space to the tile's content space at 1:1.

[Justin Novosad, 2013-02-15 16:49:04]

FWIW: the main reason I was confused was that I did not realize that the
TilingData object owned by the picture pile was different from the TilingData
object owned by the PictureLayer.  Anyways, what you just suggested seem fine to
me.  Do you have any objection to tile_grid_size being a creation argument for
PicturePile, or would you rather there be a hard-coded default val and a setter
method?

[enne (OOO), 2013-02-15 17:15:46]

A ctor arg for PicturePile sounds good to me.  :)

(Sorry for all the confusion!)

[Justin Novosad, 2013-02-15 19:40:59]

Patch.

[enne (OOO), 2013-02-15 20:39:50]

Also, I am curious why this helps so much.  Given that the first tile doesn't
have border texels on its external edge, you're going to be off-by-one pixel. 
So, it seems like you're still going to end up needing to merge four tile grid
tiles to raster one tile.

https://codereview.chromium.org/12221077/diff/25001/cc/picture_layer.cc
File cc/picture_layer.cc (right):

https://codereview.chromium.org/12221077/diff/25001/cc/picture_layer.cc#newco...
cc/picture_layer.cc:53: // The -2 is to account for border pixels, +0.5 is for
rounding
This math doesn't look right.  The border texels are not affected by scale, and
everything is rounded up, not rounded to the nearest int.  In other words,
ceil(width / scale) - 2.

[Justin Novosad, 2013-02-15 21:07:44]

On 2013/02/15 20:39:50, enne wrote:
> Also, I am curious why this helps so much.  Given that the first tile doesn't
> have border texels on its external edge, you're going to be off-by-one pixel. 

That's no what I am seeing. The first tile is querying the tile grid with the
range -1 to 255.

> File cc/picture_layer.cc (right):
> 
>
https://codereview.chromium.org/12221077/diff/25001/cc/picture_layer.cc#newco...
> cc/picture_layer.cc:53: // The -2 is to account for border pixels, +0.5 is for
> rounding
> This math doesn't look right.  The border texels are not affected by scale, 

My understanding is that with a device scale of 2, a 512x512 tile would receive
content of a 256x256 region in the SkPicture's frame of reference. This would
mean that the border pixel in device space corresponds to a margin of 1/2 a
pixel in SkPicture (recording) space.

Am I wrong about that? Are you saying that there would be a border of 2 device
pixels in that case?

> and
> everything is rounded up, not rounded to the nearest int.  In other words,
> ceil(width / scale) - 2.

Ah ok, will ceil to match the compositor's arithmetic.

[enne (OOO), 2013-02-15 21:16:07]

Oh, you're quite right.  The border pixels are scaled.  Sorry about that.  :)

[Justin Novosad, 2013-02-15 22:00:03]

Looks like something has changed in that past few days. I just took another look
at the query rects being sent to SkTileGrid, and like you said: no border texels
on the external edge.  I am going to have to make some skia-side changes to
accomodate this.

[nduca, 2013-02-19 19:25:30]

Lets punt this out of m25, and do the right thing here. We look solid on 25.

[Justin Novosad, 2013-02-19 19:31:24]

I read your mind 4 hours in the future:
https://code.google.com/p/chromium/issues/detail?id=174965

[Justin Novosad, 2013-02-20 22:27:05]

Patch 7 applies previous review feedback and integrates with skia-side changes
that are not yet landed in skia.

[Justin Novosad, 2013-02-20 23:04:02]

The gpu/aura try fails are red herrings. Looks like the "Try more bots" optin at
the top of this page launches new try jobs without the -s options that were
specified in the original try.  Re-sending jobs from the command line...

[enne (OOO), 2013-02-21 01:38:48]

Would it be possible to add histograms to see how effective this choice of
tiling is, e.g. counting how many tile grid tiles a given cc::Tile rasterization
touches?

[Justin Novosad, 2013-02-21 15:54:03]

That is trickier than it sounds because SkTileGrid is not published in the skia
public API, and it should remain that way.  SkTileGridPicture, which is public,
is just a factory class.

I can add TRACE_EVENT instrumentation though, but it will only work in custom
builds that enable the skia trace event shim.

[enne (OOO), 2013-02-21 18:23:13]

That's unfortunate that histograms for this are tricky.  That'd be really
interesting info to have.  Trace events seem less useful, especially if you have
to turn on some flag in Skia to make them happen.

https://codereview.chromium.org/12221077/diff/45006/cc/picture_layer.cc
File cc/picture_layer.cc (right):

https://codereview.chromium.org/12221077/diff/45006/cc/picture_layer.cc#newco...
cc/picture_layer.cc:56: int border_pixels = static_cast<int>(ceil(1.0f /
scale));
I agree that the border pixels are scaled as well, but I don't think the ceil is
applied to each separately.  Like, a 254 pixel tile with 1 border texel at a
device scale of 2 has a 128 css pixel stride per tile (i.e. (254 + 2 * 1)/2),
but this math would round 254 to 127 and border_pixels to 1, so it seems like
you'd have a stride of 129.

How important is this to match exactly for the 1:1 case? What sorts of gains are
you seeing when the default tile size is 512x512?

Is there any way to test that what you're intending to do here (i.e. match
rastering tile size at 1:1) is actually happening? I could lgtⅿ this, but
without tests it's hard to be convinced that this is doing the right thing.

[Justin Novosad, 2013-02-21 19:01:10]

The importance of 1:1 match give a 0-2% perf win on total picture playback cost
using 256x256 tiles, measured using skia's bench_pictures. I haven't benchmarked
512x512, but I would expect the win to be slightly lower since the skia draw
call overhead per pixels blitted ratio would be lower.

I will fix the arithmetic, and I will validate the tile matching in the debugger
using device scales != 1

[enne (OOO), 2013-02-21 19:07:57]

Is there any way to write a unit test for this so that it doesn't break in the
future if somebody changes the way tiling works for rasterization?

[Justin Novosad, 2013-02-21 19:31:28]

On 2013/02/21 19:07:57, enne wrote:
> Is there any way to write a unit test for this so that it doesn't break in the
> future if somebody changes the way tiling works for rasterization.

Yes, by playing the picture into a mock canvas, we can test whether the mock
canvas receives extraneous content.  I wrote a test in skia that does exactly
that. That test s based on a tile access pattern that currently represents the
CC's access pattern, but may not in the future if the two diverge.  Adding a
similar test in CC is a very good idea.

[Justin Novosad, 2013-02-25 20:53:29]

New Patch. Added tests.

Turns out I was wrong to do anything about the device scale factor, since it is
cancelled-out in recording space by PictureLayerImpl::raster_source_scale_.

[enne (OOO), 2013-02-25 23:59:15]

https://codereview.chromium.org/12221077/diff/44014/cc/picture_layer_impl_uni...
File cc/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/12221077/diff/44014/cc/picture_layer_impl_uni...
cc/picture_layer_impl_unittest.cc:267: EXPECT_EQ(2, mock_canvas.rects_.size());
This is great.  Thanks for the tests.

https://codereview.chromium.org/12221077/diff/44014/cc/picture_pile_base.cc
File cc/picture_pile_base.cc (right):

https://codereview.chromium.org/12221077/diff/44014/cc/picture_pile_base.cc#n...
cc/picture_pile_base.cc:15: const int kDefaultTileGridInterval = 254;
This should never get used, right? If possible, I'd prefer to not have this
constant, since it might make somebody think that this value is real and gets
used.

https://codereview.chromium.org/12221077/diff/44014/cc/picture_pile_base.h
File cc/picture_pile_base.h (right):

https://codereview.chromium.org/12221077/diff/44014/cc/picture_pile_base.h#ne...
cc/picture_pile_base.h:44: void ApplyLayerTreeSettings(LayerTreeHostImpl*
host_impl);  // for testing
Could you just directly pass in the values you need here rather than exposing
LTH(I) to PicturePile?

[Justin Novosad, 2013-02-26 16:08:15]

Patch.

[enne (OOO), 2013-02-26 18:12:25]

lgtm

https://codereview.chromium.org/12221077/diff/37011/cc/picture_pile_base.h
File cc/picture_pile_base.h (right):

https://codereview.chromium.org/12221077/diff/37011/cc/picture_pile_base.h#ne...
cc/picture_pile_base.h:41: void ApplyLayerTreeSettings(const LayerTreeSettings&
settings);
Sorry, I should have been more clear.  I don't think PicturePile should know
anything about LayerTreeSettings either.  I'd really like
s/ApplyLayerTreeSettings/SetTileGridSize/, and just calling that function along
with SetMinContentsScale in PictureLayer::setLayerTreeHost.