cc: Inflate invalidation in each picture list

cc: Inflate invalidation in each picture list

We inflate invalidation and then intersect with each picture
list. This means we lose the guaranteed minimum size of the
picture, if the invalidation just crosses a picture list
boundary.

Instead, do the intersection with each picture list affected,
then inflate within the picture list's bounds to make sure
the invalidation in each picture list meets the minimum size.

Tests:
PicturePileTest.SmallInvalidateInflated
PicturePileTest.LargeInvalidateInflated
PicturePileTest.InvalidateOnTileBoundaryInflated

R=enne,nduca
BUG=237211
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199359

[enne (OOO), 2013-05-03 07:00:57]

https://codereview.chromium.org/14874004/diff/7017/cc/resources/picture_layer...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/14874004/diff/7017/cc/resources/picture_layer...
cc/resources/picture_layer_tiling.cc:55: DCHECK(!gfx::ToFlooredSize(
Oh, good thought to put a check here.

https://codereview.chromium.org/14874004/diff/7017/cc/resources/picture_pile.cc
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/14874004/diff/7017/cc/resources/picture_pile....
cc/resources/picture_pile.cc:75: for (int i = 0; i < 2; ++i) {
This is not right.  It's not just for scaling down by min contents scale.  If
you scale down by 1/4x, then 4x4 recorded pixels equal one rastered pixel. 
Let's say your invalidation includes gfx::Rect(12, 12, 1, 1).  You will need
gfx::Rect(12, 12, 4, 4) in order to correctly raster that pixel because it will
sample from all 4x4 of those recorded pixels to generate it.

See
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...
for the bit of code that requires this inflation during rasterization.

This is why it was inflated evenly in all directions by the buffer pixels, and I
think that logic needs to stay.  We do the same thing with the base picture too:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu....

[danakj, 2013-05-03 17:49:20]

https://codereview.chromium.org/14874004/diff/7017/cc/resources/picture_pile.cc
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/14874004/diff/7017/cc/resources/picture_pile....
cc/resources/picture_pile.cc:75: for (int i = 0; i < 2; ++i) {
Okay so let me see if I'm understanding correctly..

The picture list (tile) is inflated such that any texel we want to raster at any
scale will be sourcable by pixels entirely in a single picuture list (tile).

Then, if we invalidate on the edge of a picture list (tile), we'll actually be
over two picture lists due to border pixels.

Now, if we inflate the invalidation in all directions by the border pixels and
intersect with the tile, we may end up with a Picture of dimensions less than
1/scale-1, which caused the assert.

So we need each individual Picture in the PictureList to be big enough.

The other problem is that we need to inflate in all directions in order to get
the right pixels needed to raster the texel.

The tile_invalidation must be at least 1x1 px already when we get to this point.
When we finish it needs to be expanded by 2*borderpx, so it will be >=
2*borderpx+1 in each dimension if we did things right.

I think that you're saying we need to always inflate by the borderpx in all
directions, then inflate extra if required to meet our min size requirements?
I'll do that.

[danakj, 2013-05-03 18:24:23]

Updated, and added 2 more tests. PTAL

[enne (OOO), 2013-05-05 01:26:47]

https://codereview.chromium.org/14874004/diff/18001/cc/resources/picture_pile.cc
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/14874004/diff/18001/cc/resources/picture_pile...
cc/resources/picture_pile.cc:88: if (tile_invalidation.width() < buffer_pixels()
* 2 + 1)
Sorry, but no?

It's not about total width.  It's about maintaining a kernel of buffer pixels
around each invalidated pixel.

I think the right thing to do here is to intersect the invalidation against the
tile without border and then to inflate equally in all directions.

[danakj, 2013-05-06 14:40:06]

Ok, so here's our baseline for discussion. We have:

http://i.imgur.com/HmreODo.png

- A Red tile
- A Blue tile
- Each tile has border pixels which are more pale.
- The black line represents the point after which Red's border pixels overlap
pixels inside Blue (non-border pixels), and vice versa.
- White box is the invalidation.
- The paler white box are the pixels needed to raster the invalidation at
minimum contents scale.

Ok so, I think what you said is that:
- We figure out what tiles the invalidation touches, excluding border-pixels:
Red.
- We expand equally all directions, so we get the pale white rect.
- And we're done?

What I'm concerned about is the blue tile's border pixels. The invalidation is
inside the border, but we need to also need to consider it invalidated, or if we
use it to raster another part of blue at low scale, we'll get the wrong stuff.

So, I think we want to:
- Figure out what tiles intersect the invalidation /including/ their border
pixels.
- Expand the invalidation in all directions to cover small scales.

But for the blue tile, since the invalidation is inside the border, the
expansion will go outside the blue tiles border pixels. So we need to intersect
the tile again.

But now we have a region that is smaller than (border pixels*2 + 1).  And, that
may actually be okay, because we won't want to raster a blue border pixel
directly.

The problem is (and this is why I keep saying the size of the region matters,
rather than only the expansion to cover the kernel) when we do go to raster a
pixel in the blue tile, and it requires border pixels that were invalidated
here. The cc::Picture we created inside the border area was smaller than (border
px * 2 + 1), so when we try to draw all the pictures in the blue tile's picture
list, this one will scale down to < 1 px and get clipped to nothing, and cause a
DCHECK to fire.

So either the DCHECK is wrong, or we need to grow the Picture larger and I think
it's the latter.

Hope that makes my thinking clear, please tell me where I'm going off track if
you disagree. :)

https://codereview.chromium.org/14874004/diff/18001/cc/resources/picture_pile.cc
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/14874004/diff/18001/cc/resources/picture_pile...
cc/resources/picture_pile.cc:88: if (tile_invalidation.width() < buffer_pixels()
* 2 + 1)
On 2013/05/05 01:26:47, enne wrote:
> Sorry, but no?
> 
> It's not about total width.  It's about maintaining a kernel of buffer pixels
> around each invalidated pixel.
> 
> I think the right thing to do here is to intersect the invalidation against
the
> tile without border and then to inflate equally in all directions.

Hm, I am not sure if I agree, so let's make some pictures and sort this out.

[enne (OOO), 2013-05-06 15:41:11]

Your picture has convinced me that what I said in comment #5 was wrong.  You're
right that we have to care about the borders.

I think we need to take the invalidation, apply it to any tile with borders that
it intersects.  For each tile, first intersect the invalidation with each tile
*with* borders and then inflate equally in all directions.

[danakj, 2013-05-06 17:03:48]

On Mon, May 6, 2013 at 11:41 AM, <enne@chromium.org> wrote:

> Your picture has convinced me that what I said in comment #5 was wrong.
>  You're
> right that we have to care about the borders.
>
> I think we need to take the invalidation, apply it to any tile with
> borders that
> it intersects.  For each tile, first intersect the invalidation with each
> tile
> *with* borders and then inflate equally in all directions.
>

OK Done, PTAL.

The tests were checking that the Picture was large enough, not that it was
within the bounds of the tile+border, so they didn't need to change.


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

[enne (OOO), 2013-05-09 22:55:43]

https://codereview.chromium.org/14874004/diff/25001/cc/resources/picture_pile...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/14874004/diff/25001/cc/resources/picture_pile...
cc/resources/picture_pile_unittest.cc:28: const int kBasePictureSize = 3000;
Maybe make this configurable from the test so you don't have to?

https://codereview.chromium.org/14874004/diff/25001/cc/resources/picture_pile...
cc/resources/picture_pile_unittest.cc:113: int expected_inflation = ceil(1.f /
min_scale) - 1;
Can you just ask for the buffer pixels?

[danakj, 2013-05-09 23:11:53]

PTAL

https://codereview.chromium.org/14874004/diff/25001/cc/resources/picture_pile...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/14874004/diff/25001/cc/resources/picture_pile...
cc/resources/picture_pile_unittest.cc:28: const int kBasePictureSize = 3000;
On 2013/05/09 22:55:44, enne wrote:
> Maybe make this configurable from the test so you don't have to?

Hummm..!

Actually, I can just use the tiling().max_texture_size(), instead of checking
that it's equal to 3000.

https://codereview.chromium.org/14874004/diff/25001/cc/resources/picture_pile...
cc/resources/picture_pile_unittest.cc:113: int expected_inflation = ceil(1.f /
min_scale) - 1;
On 2013/05/09 22:55:44, enne wrote:
> Can you just ask for the buffer pixels?

Done.

[enne (OOO), 2013-05-09 23:15:29]

lgtm

[commit-bot: I haz the power, 2013-05-09 23:17:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14874004/33001

[commit-bot: I haz the power, 2013-05-10 00:39:38]

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

[commit-bot: I haz the power, 2013-05-10 01:45:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14874004/33001

[commit-bot: I haz the power, 2013-05-10 03:37:49]

Change committed as 199359