cc: Fix CanRaster dcheck failures ONCE AND FOR ALL.

cc: Fix CanRaster dcheck failures ONCE AND FOR ALL.

When a PicturePile grows along either axis, the tiles on the edge where
it grew are dropped if any new pixels are added to their recordings.
This means that we need to invalidate them to inform the impl side
about the recording being dropped.

However, if we're going to re-record the tile (it intersects the
interest_rect), then there's no need to invalidate it as the old
recorded pixels remain valid.

Previously we made the assumption that when a PicturePile resized, the
interest rect's center would be to the left and above the old edge of
the pile. However, the interest rect can move at the same time as the
pile is resized, and it can live anywhere with respect to the recording
tiles being dropped.

So we invalidate everything inside the row/column of tiles that were
dropped that is outside of the interest rect by invalidating on all
four sides of the interest rect, but clamping the resulting invalidation
rects to within the dropped tiles row/column.

R=enne
BUG=386998
Committed: https://crrev.com/d27cb086ffcbfdc4c3f5b1d1d483b28dff5bd26c
Cr-Commit-Position: refs/heads/master@{#291762}

[danakj, 2014-08-22 23:51:36]

http://i.imgur.com/gHcGr.jpg

[enne (OOO), 2014-08-23 00:08:53]

On 2014/08/22 at 23:51:36, danakj wrote:
> http://i.imgur.com/gHcGr.jpg

http://i.imgur.com/SNkJkzR.gif

[danakj, 2014-08-23 00:35:08]

On Fri, Aug 22, 2014 at 8:08 PM, <enne@chromium.org> wrote:

> On 2014/08/22 at 23:51:36, danakj wrote:
>
>> http://i.imgur.com/gHcGr.jpg
>>
>
> http://i.imgur.com/SNkJkzR.gif


Done!

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

[danakj, 2014-08-24 16:16:55]

Ok I had dropped the "exposed rect" which was causing test failures. You can see
added back in the last patch set 6 along with corrected test expectations for
the new test cases.

PTAL

[danakj, 2014-08-25 17:04:56]

Fixed the test by using TileAt(0,0) instead of AllTilesForTesting() and then
dropping everything but the first tile in the list. The latter is not
deterministic.

[danakj, 2014-08-25 17:05:12]

Oh, wrong CL for that comment.

[enne (OOO), 2014-08-25 17:48:47]

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:235: // mmppssvvyybbeeh|h               .
If I'm reading this diagram correctly, min_toss_x is between ee and hh, e.g.
"ee|hh"?

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:250: // |===>      INVALIDATION RECT           |
This bit took me a while to understand.  Can you include the new pile edge in
this diagram too?

Also, it's not really "invalidation rect" it's like "potential invalidation
rect" or even like "old pile edge rect"? I think the word "invalidation" here is
confusing (for me) since this code is trying to generate an invalidation rect.

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:253: // |===>| INTEREST RECT   |               |
This may just be the coffee speaking, but I think I understand the "what" of
this enough to say that I don't understand the "why" of this algorithm.

Everything in the invalidation rect in this diagram is a picture pile tile that
has either been entirely or partially exposed to new content due to the bounds
change.  Why are you avoiding invalidations inside of the interest rect?

[danakj, 2014-08-25 18:03:44]

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:235: // mmppssvvyybbeeh|h               .
On 2014/08/25 17:48:47, enne wrote:
> If I'm reading this diagram correctly, min_toss_x is between ee and hh, e.g.
> "ee|hh"?

The old pile edge was in the middle of the h tile in this picture, so some new
pixels on h are exposed. The min_toss_x will be == h because that tile has new
pixels.

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:250: // |===>      INVALIDATION RECT           |
On 2014/08/25 17:48:47, enne wrote:
> This bit took me a while to understand.  Can you include the new pile edge in
> this diagram too?

Sure, this rect is no longer the entire pile, this is only the last column of
tiles on the old pile. The old pile rect's edge will be a vertical line anywhere
inside this rect. I can add something and describe that better.

> 
> Also, it's not really "invalidation rect" it's like "potential invalidation
> rect" or even like "old pile edge rect"? I think the word "invalidation" here
is
> confusing (for me) since this code is trying to generate an invalidation rect.

Hm, ok, it's the rect expanded to tile bounds of all tiles in the "to be dropped
row/column" and we are invalidating those tiles. Hence how I got there. We
invalidate everything inside it, except if we're going to re-record and keep the
tile alive. Old pile edge rect sounds okay to me.

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:253: // |===>| INTEREST RECT   |               |
On 2014/08/25 17:48:47, enne wrote:
> This may just be the coffee speaking, but I think I understand the "what" of
> this enough to say that I don't understand the "why" of this algorithm.
> 
> Everything in the invalidation rect in this diagram is a picture pile tile
that
> has either been entirely or partially exposed to new content due to the bounds
> change.  Why are you avoiding invalidations inside of the interest rect?

Outside of the interest rect we invalidate the entire tile, because we're
dropping the tile, it won't exist anymore so all pixels in the tile are bad.

If the tile intersects the intersect rect, we will re-record it below so the
tile still exists and impl can keep around recordings that use it. So for those
tiles we only invalidate new pixels. If old pixels are used by a recording tile,
and not new pixels, that recording tile doesn't need to change.

[enne (OOO), 2014-08-25 18:08:13]

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:235: // mmppssvvyybbeeh|h               .
On 2014/08/25 18:03:39, danakj wrote:
> On 2014/08/25 17:48:47, enne wrote:
> > If I'm reading this diagram correctly, min_toss_x is between ee and hh, e.g.
> > "ee|hh"?
> 
> The old pile edge was in the middle of the h tile in this picture, so some new
> pixels on h are exposed. The min_toss_x will be == h because that tile has new
> pixels.

...so you're agreeing with me?

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:253: // |===>| INTEREST RECT   |               |
On 2014/08/25 18:03:39, danakj wrote:
> On 2014/08/25 17:48:47, enne wrote:
> > This may just be the coffee speaking, but I think I understand the "what" of
> > this enough to say that I don't understand the "why" of this algorithm.
> > 
> > Everything in the invalidation rect in this diagram is a picture pile tile
> that
> > has either been entirely or partially exposed to new content due to the
bounds
> > change.  Why are you avoiding invalidations inside of the interest rect?
> 
> Outside of the interest rect we invalidate the entire tile, because we're
> dropping the tile, it won't exist anymore so all pixels in the tile are bad.
> 
> If the tile intersects the intersect rect, we will re-record it below so the
> tile still exists and impl can keep around recordings that use it. So for
those
> tiles we only invalidate new pixels. If old pixels are used by a recording
tile,
> and not new pixels, that recording tile doesn't need to change.

What do you mean by "impl can keep around recordings that use it"?

Here's what I don't understand: why do you need to special case the interest
rect? It seems like if you invalidated everything in that big "invalidation"
rect and rerecorded new tiles in the interest rect, everything would still work
ok.

[danakj, 2014-08-25 18:13:35]

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:235: // mmppssvvyybbeeh|h               .
On 2014/08/25 18:08:13, enne wrote:
> On 2014/08/25 18:03:39, danakj wrote:
> > On 2014/08/25 17:48:47, enne wrote:
> > > If I'm reading this diagram correctly, min_toss_x is between ee and hh,
e.g.
> > > "ee|hh"?
> > 
> > The old pile edge was in the middle of the h tile in this picture, so some
new
> > pixels on h are exposed. The min_toss_x will be == h because that tile has
new
> > pixels.
> 
> ...so you're agreeing with me?

Ya, but for clarify min_toss_x would be == h not between? But ya same thing.

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:253: // |===>| INTEREST RECT   |               |
On 2014/08/25 18:08:13, enne wrote:
> On 2014/08/25 18:03:39, danakj wrote:
> > On 2014/08/25 17:48:47, enne wrote:
> > > This may just be the coffee speaking, but I think I understand the "what"
of
> > > this enough to say that I don't understand the "why" of this algorithm.
> > > 
> > > Everything in the invalidation rect in this diagram is a picture pile tile
> > that
> > > has either been entirely or partially exposed to new content due to the
> bounds
> > > change.  Why are you avoiding invalidations inside of the interest rect?
> > 
> > Outside of the interest rect we invalidate the entire tile, because we're
> > dropping the tile, it won't exist anymore so all pixels in the tile are bad.
> > 
> > If the tile intersects the intersect rect, we will re-record it below so the
> > tile still exists and impl can keep around recordings that use it. So for
> those
> > tiles we only invalidate new pixels. If old pixels are used by a recording
> tile,
> > and not new pixels, that recording tile doesn't need to change.
> 
> What do you mean by "impl can keep around recordings that use it"?
> 
> Here's what I don't understand: why do you need to special case the interest
> rect? It seems like if you invalidated everything in that big "invalidation"
> rect and rerecorded new tiles in the interest rect, everything would still
work
> ok.

If a raster tile used some of the old pixels on one of those tiles, but not any
of the new pixels, invalidating the old pixels would cause us to drop the raster
tile and reraster it.

[enne (OOO), 2014-08-25 18:24:17]

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:253: // |===>| INTEREST RECT   |               |
On 2014/08/25 18:13:35, danakj wrote:
> On 2014/08/25 18:08:13, enne wrote:
> > On 2014/08/25 18:03:39, danakj wrote:
> > > On 2014/08/25 17:48:47, enne wrote:
> > > > This may just be the coffee speaking, but I think I understand the
"what"
> of
> > > > this enough to say that I don't understand the "why" of this algorithm.
> > > > 
> > > > Everything in the invalidation rect in this diagram is a picture pile
tile
> > > that
> > > > has either been entirely or partially exposed to new content due to the
> > bounds
> > > > change.  Why are you avoiding invalidations inside of the interest rect?
> > > 
> > > Outside of the interest rect we invalidate the entire tile, because we're
> > > dropping the tile, it won't exist anymore so all pixels in the tile are
bad.
> > > 
> > > If the tile intersects the intersect rect, we will re-record it below so
the
> > > tile still exists and impl can keep around recordings that use it. So for
> > those
> > > tiles we only invalidate new pixels. If old pixels are used by a recording
> > tile,
> > > and not new pixels, that recording tile doesn't need to change.
> > 
> > What do you mean by "impl can keep around recordings that use it"?
> > 
> > Here's what I don't understand: why do you need to special case the interest
> > rect? It seems like if you invalidated everything in that big "invalidation"
> > rect and rerecorded new tiles in the interest rect, everything would still
> work
> > ok.
> 
> If a raster tile used some of the old pixels on one of those tiles, but not
any
> of the new pixels, invalidating the old pixels would cause us to drop the
raster
> tile and reraster it.

Something still doesn't seem right here to me.  I don't understand why the
interest rect is relevant.  The diagram below where you start at the old pile
edge is what keeps you from rerastering existing content, since all existing
content is by definition inside of the old bounds.

[danakj, 2014-08-25 18:29:55]

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/504513002/diff/160001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:253: // |===>| INTEREST RECT   |               |
On 2014/08/25 18:24:17, enne wrote:
> On 2014/08/25 18:13:35, danakj wrote:
> > On 2014/08/25 18:08:13, enne wrote:
> > > On 2014/08/25 18:03:39, danakj wrote:
> > > > On 2014/08/25 17:48:47, enne wrote:
> > > > > This may just be the coffee speaking, but I think I understand the
> "what"
> > of
> > > > > this enough to say that I don't understand the "why" of this
algorithm.
> > > > > 
> > > > > Everything in the invalidation rect in this diagram is a picture pile
> tile
> > > > that
> > > > > has either been entirely or partially exposed to new content due to
the
> > > bounds
> > > > > change.  Why are you avoiding invalidations inside of the interest
rect?
> > > > 
> > > > Outside of the interest rect we invalidate the entire tile, because
we're
> > > > dropping the tile, it won't exist anymore so all pixels in the tile are
> bad.
> > > > 
> > > > If the tile intersects the intersect rect, we will re-record it below so
> the
> > > > tile still exists and impl can keep around recordings that use it. So
for
> > > those
> > > > tiles we only invalidate new pixels. If old pixels are used by a
recording
> > > tile,
> > > > and not new pixels, that recording tile doesn't need to change.
> > > 
> > > What do you mean by "impl can keep around recordings that use it"?
> > > 
> > > Here's what I don't understand: why do you need to special case the
interest
> > > rect? It seems like if you invalidated everything in that big
"invalidation"
> > > rect and rerecorded new tiles in the interest rect, everything would still
> > work
> > > ok.
> > 
> > If a raster tile used some of the old pixels on one of those tiles, but not
> any
> > of the new pixels, invalidating the old pixels would cause us to drop the
> raster
> > tile and reraster it.
> 
> Something still doesn't seem right here to me.  I don't understand why the
> interest rect is relevant.  The diagram below where you start at the old pile
> edge is what keeps you from rerastering existing content, since all existing
> content is by definition inside of the old bounds.

There are 2 possible outcomes for each tile on the edge of the old pile here:

1. We drop the tile and it no longer exists
- This is because the interest rect doesn't overlap.
- In this case we need to drop Tiles on the impl side that would try to use
these recording tiles since they are now destroyed.

2. We drop the tile, but we re-record it below
- This is because the interest rect touches the recording tile
- In this case the pixels in the tile and inside the old pile bounds are still
valid and are not changed (modulo external invalidation).
- Since the old pixels are still valid, we don't want to add invalidation for
them as that would cause unneeded reraster.
- So we only invalidate the new pixels outside the old pile bounds.

Case 2 is handled below and is talked about by that diagram. This part is
dealing with case 1. Either a tile intersects the interest rect or it does not.
Maybe this diagram is poor, because the interest rect will always be either
entirely overlapping the "INVALIDATION RECT TBR" horizontally, or it won't
overlap at all. It can't be partially horizontally overlapping the tiles in the
old-last-column, since the rect is expanded to tile bounds.

Does that help?

[enne (OOO), 2014-08-25 19:00:43]

AHA.  Thank you, that makes a lot more sense.  I think it would be more clear to
me if case 1 was split into 1a (interest rect entirely overlaps "INVALIDATION
RECT TBR", so partial invalidation where there's new content) and 1b (interest
rect doesn't overlap, so full invalidation).  But I guess the problem is that
you have to do this check per recording tile, more or less, so I guess this is
an attempt to avoid regions?

To validate my understanding, take an example where you have non-intersecting
interest rect and "INVALIDATION RECT TBR" where interest rect is entirely left
but adjacent to "INVALIDATION RECT TBR", then the "left" invalidation doesn't do
anything, but the "right" invalidation does the full width of "INVALIDATION RECT
TBR".  Is that true?

[danakj, 2014-08-25 19:05:12]

On Mon, Aug 25, 2014 at 3:00 PM, <enne@chromium.org> wrote:

> AHA.  Thank you, that makes a lot more sense.  I think it would be more
> clear to
> me if case 1 was split into 1a (interest rect entirely overlaps
> "INVALIDATION
> RECT TBR", so partial invalidation where there's new content) and 1b
> (interest
> rect doesn't overlap, so full invalidation).  But I guess the problem is
> that
> you have to do this check per recording tile, more or less, so I guess
> this is
> an attempt to avoid regions?
>
> To validate my understanding, take an example where you have
> non-intersecting
> interest rect and "INVALIDATION RECT TBR" where interest rect is entirely
> left
> but adjacent to "INVALIDATION RECT TBR", then the "left" invalidation
> doesn't do
> anything, but the "right" invalidation does the full width of
> "INVALIDATION RECT
> TBR".  Is that true?
>

Yes exactly.

Most of the time some of these rects will be empty. But it's "simpler code"
to avoid a lot of if statements, and it avoids regions.

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

[danakj, 2014-08-25 19:58:23]

I've updated the description and pictures, PTAL

[enne (OOO), 2014-08-25 20:06:16]

lgtm

Thanks for taking the time to explain this to me and to make awesome diagrams.

https://codereview.chromium.org/504513002/diff/220001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/504513002/diff/220001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:271: // is a single tile wide, and the interst rect
has been expanded to the
typo

[danakj, 2014-08-25 20:06:49]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-08-25 20:07:42]

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

[commit-bot: I haz the power, 2014-08-25 21:21:57]

Committed patchset #9 (240001) as 3c429b26bd04bb2a69694fa8c8e30553afc664d6

[commit-bot: I haz the power, 2014-09-10 02:37:36]

Patchset 9 (id:??) landed as
https://crrev.com/d27cb086ffcbfdc4c3f5b1d1d483b28dff5bd26c
Cr-Commit-Position: refs/heads/master@{#291762}