cc: Find a better prioritized rect when the viewport rect is huge.

cc: Find a better prioritized rect when the viewport rect is huge.

When a part of the layer is visible, then it has a visible_content_rect which
is the best place to expand the prioritized rect from.

If no part of the layer is visible, we use the viewport rect in content space.
However, when the layer clips the viewport, this rect becomes enormous.
Currently the ExpandRectEquallyToAreaBoundedBy function "expands" the viewport
to cover the target area, but if the viewport is huge, it shrinks it instead.
When it shrinks to a rect far away from the layer, the layer is considered too
far from the viewport for prioritization.

Instead, we should only grow the viewport in content space, then intersect with
the tiling's content bounds (as before) to find an appropriate starting rect.

Tests:
PictureLayerTilingIteratorTest.TilesExistGiantViewport
PictureLayerTilingIteratorTest.TilesExistOutsideViewport
PictureLayerTilingIteratorTest.TilesExistLargeViewportAndLayerWithSmallVisibleArea
PictureLayerTilingIteratorTest.TilesExistLargeViewportAndLayerWithLargeVisibleArea

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

[enne (OOO), 2013-04-16 20:16:20]

Not to be rude, but this direction feels a lot like another complex band-aid on
an already complicated function.

I'd love to see something more simple here, e.g. "expand the viewport rect
equally in all directions, intersect with content rect", which would fix this
bug.  The "your interest rect is too big" could get fixed differently, possibly
at the picture layer tiling set level.

[enne (OOO), 2013-04-16 21:34:06]

I am going to punt this patch to wangxianzhu and let you two own this
functionality.  If he ⅼgtms this, then that's fine by me.

I do not want to be responsible in the short term for this system's complexity
or for continually patching it up as it keeps breaking.

[Xianzhu, 2013-04-16 22:42:52]

I think if we only need to resolve the shrunk starting_rect issue, adding the
'delta > 0' condition is enough, given that viewport_in_content_space is valid
(though enormous).

Besides enormous viewport_in_content_space, there is still chance that it's
invalid if
- screen_space_transform is not inversible;
- overflow during calculation.

What if we always use visible_content_rect as the starting_rect?

https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer...
File cc/resources/picture_layer_tiling.cc (left):

https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer...
cc/resources/picture_layer_tiling.cc:582: }
I think we can still keep this shortcut by adding 'delta >= 0 &&' to just
exclude the shrinking case.

[danakj, 2013-04-16 22:45:06]

On Tue, Apr 16, 2013 at 6:42 PM, <wangxianzhu@chromium.org> wrote:

> I think if we only need to resolve the shrunk starting_rect issue, adding
> the
> 'delta > 0' condition is enough, given that viewport_in_content_space is
> valid
> (though enormous).
>
> Besides enormous viewport_in_content_space, there is still chance that it's
> invalid if
> - screen_space_transform is not inversible;
> - overflow during calculation.
>
> What if we always use visible_content_rect as the starting_rect?
>

If you're not in the viewport, it is (0, 0, 0x0). So that won't include
layers outside the viewport. Also layers that are fully clipped.

[Xianzhu, 2013-04-16 22:57:16]

On 2013/04/16 22:45:06, danakj wrote:
> If you're not in the viewport, it is (0, 0, 0x0). So that won't include
> layers outside the viewport. Also layers that are fully clipped.

I see.

I feel the same as enne@. Using visible_content_rect seems just a workaround of
the viewport_in_content_space issue (bug 231783) and I expect a better solution
for it. For now, preventing starting_rect from shrinking in the first round
looks simple and enough. Does it fix bug 231521?

[Xianzhu, 2013-04-17 00:06:36]

As the issue breaks chrome-os beta and we need a simple patch that can be
ensured safe to be merged into beta, I suggest to consider
https://codereview.chromium.org/13926008 first, then we can discuss the best
solution to the huge viewport rect issue.

I reopened https://codereview.chromium.org/13926008 for review.

[reveman, 2013-04-17 00:48:49]

On 2013/04/17 00:06:36, Xianzhu wrote:
> As the issue breaks chrome-os beta and we need a simple patch that can be
> ensured safe to be merged into beta, I suggest to consider
> https://codereview.chromium.org/13926008 first, then we can discuss the best
> solution to the huge viewport rect issue.
> 
> I reopened https://codereview.chromium.org/13926008 for review.

chromeos beta doesn't need a fix. next chromeos beta release will have impl side
painting turned off.

[danakj, 2013-04-17 01:13:22]

On 2013/04/16 22:57:16, Xianzhu wrote:
> On 2013/04/16 22:45:06, danakj wrote:
> > If you're not in the viewport, it is (0, 0, 0x0). So that won't include
> > layers outside the viewport. Also layers that are fully clipped.
> 
> I see.
> 
> I feel the same as enne@. Using visible_content_rect seems just a workaround
of
> the viewport_in_content_space issue (bug 231783) and I expect a better
solution
> for it. For now, preventing starting_rect from shrinking in the first round
> looks simple and enough. Does it fix bug 231521?

It is not needed for poster circle, but during discussion I realized a test was
missing to demonstrate why it is important.

I have added 2 more tests that each fail without all of the changes made in this
CL.

[danakj, 2013-04-17 01:13:33]

https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer...
File cc/resources/picture_layer_tiling.cc (left):

https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer...
cc/resources/picture_layer_tiling.cc:582: }
On 2013/04/16 22:42:52, Xianzhu wrote:
> I think we can still keep this shortcut by adding 'delta >= 0 &&' to just
> exclude the shrinking case.

PictureLayerTilingIteratorTest.TilesExistLargeViewportAndLayerWithLargeVisibleArea
will fail with this.

[Xianzhu, 2013-04-17 01:35:59]

On 2013/04/17 01:13:33, danakj wrote:
>
https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer...
> File cc/resources/picture_layer_tiling.cc (left):
> 
>
https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer...
> cc/resources/picture_layer_tiling.cc:582: }
> On 2013/04/16 22:42:52, Xianzhu wrote:
> > I think we can still keep this shortcut by adding 'delta >= 0 &&' to just
> > exclude the shrinking case.
> 
>
PictureLayerTilingIteratorTest.TilesExistLargeViewportAndLayerWithLargeVisibleArea
> will fail with this.

Thanks for the new CL and new test cases.

For TilesExistLargeViewportAndLayerWithLargeVisibleArea:

starting_rect: 0,0 10000x10000
bounding_rect: 0,0 10000x10000
target_area: 100x100
first round delta: -450
expanded_starting_rect: 0,0 10000x10000

So it should not meet the 'if (delta >= 0 && rect == expanded_starting_rect)'
condition, so it seeems not to be failed by the condition. What did my miss?

[danakj, 2013-04-17 01:38:04]

On Tue, Apr 16, 2013 at 9:35 PM, <wangxianzhu@chromium.org> wrote:

> On 2013/04/17 01:13:33, danakj wrote:
>
> https://codereview.chromium.**org/13895005/diff/8001/cc/**
>
resources/picture_layer_**tiling.cc<https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer_tiling.cc>
>
>> File cc/resources/picture_layer_**tiling.cc (left):
>>
>
>
> https://codereview.chromium.**org/13895005/diff/8001/cc/**
>
resources/picture_layer_**tiling.cc#oldcode582<https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer_tiling.cc#oldcode582>
>
>> cc/resources/picture_layer_**tiling.cc:582: }
>> On 2013/04/16 22:42:52, Xianzhu wrote:
>> > I think we can still keep this shortcut by adding 'delta >= 0 &&' to
>> just
>> > exclude the shrinking case.
>>
>
>
> PictureLayerTilingIteratorTest**.**TilesExistLargeViewportAndLaye**
> rWithLargeVisibleArea
>
>> will fail with this.
>>
>
> Thanks for the new CL and new test cases.
>
> For TilesExistLargeViewportAndLaye**rWithLargeVisibleArea:
>
> starting_rect: 0,0 10000x10000
> bounding_rect: 0,0 10000x10000
> target_area: 100x100
> first round delta: -450
> expanded_starting_rect: 0,0 10000x10000
>
> So it should not meet the 'if (delta >= 0 && rect ==
> expanded_starting_rect)'
> condition, so it seeems not to be failed by the condition. What did my
> miss?
>

If the rect == expanded_starting_rect, but the expanded_starting_rect is
larger than the target_area, the rect needs to be shrunken.


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

[Xianzhu, 2013-04-17 01:43:19]

On 2013/04/17 01:38:04, danakj wrote:
> On Tue, Apr 16, 2013 at 9:35 PM, <mailto:wangxianzhu@chromium.org> wrote:
> 
> > On 2013/04/17 01:13:33, danakj wrote:
> >
> > https://codereview.chromium.**org/13895005/diff/8001/cc/**
> >
>
resources/picture_layer_**tiling.cc<https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer_tiling.cc>
> >
> >> File cc/resources/picture_layer_**tiling.cc (left):
> >>
> >
> >
> > https://codereview.chromium.**org/13895005/diff/8001/cc/**
> >
>
resources/picture_layer_**tiling.cc#oldcode582<https://codereview.chromium.org/13895005/diff/8001/cc/resources/picture_layer_tiling.cc#oldcode582>
> >
> >> cc/resources/picture_layer_**tiling.cc:582: }
> >> On 2013/04/16 22:42:52, Xianzhu wrote:
> >> > I think we can still keep this shortcut by adding 'delta >= 0 &&' to
> >> just
> >> > exclude the shrinking case.
> >>
> >
> >
> > PictureLayerTilingIteratorTest**.**TilesExistLargeViewportAndLaye**
> > rWithLargeVisibleArea
> >
> >> will fail with this.
> >>
> >
> > Thanks for the new CL and new test cases.
> >
> > For TilesExistLargeViewportAndLaye**rWithLargeVisibleArea:
> >
> > starting_rect: 0,0 10000x10000
> > bounding_rect: 0,0 10000x10000
> > target_area: 100x100
> > first round delta: -450
> > expanded_starting_rect: 0,0 10000x10000
> >
> > So it should not meet the 'if (delta >= 0 && rect ==
> > expanded_starting_rect)'
> > condition, so it seeems not to be failed by the condition. What did my
> > miss?
> >
> 
> If the rect == expanded_starting_rect, but the expanded_starting_rect is
> larger than the target_area, the rect needs to be shrunken.
>

Is this possible when delta >= 0?

The CL lgtm except this question :)

> 
https://codereview.chromium.**org/13895005/%3Chttps://codereview.chromium.org...>
> >

[danakj, 2013-04-17 01:46:02]

On Tue, Apr 16, 2013 at 9:43 PM, <wangxianzhu@chromium.org> wrote:

> If the rect == expanded_starting_rect, but the expanded_starting_rect is
>> larger than the target_area, the rect needs to be shrunken.
>>
>
>
> Is this possible when delta >= 0?
>
> The CL lgtm except this question :)


Oh, I didn't have the delta >= 0 in at the time, you're absolutely right.
Will add back. Thanks.

[Xianzhu, 2013-04-17 01:53:03]

@enne, would you approve? I am not an owner yet.

I think passing both the viewport_in_layer_rect and visible_layer_rect is
reasonable to resolve the issue. I realized this after reading the added test
cases. The main logic is like what you said but we do need the extra logic to
handle the special cases.

[danakj, 2013-04-17 01:58:41]

On Tue, Apr 16, 2013 at 9:53 PM, <wangxianzhu@chromium.org> wrote:

> @enne, would you approve? I am not an owner yet.
>

I'm an owner so your LGTM is enough. I think enne doesn't want to review
this function ever again, heh. :/


> I think passing both the viewport_in_layer_rect and visible_layer_rect is
> reasonable to resolve the issue. I realized this after reading the added
> test
> cases. The main logic is like what you said but we do need the extra logic
> to
> handle the special cases.
>

This whole system needs a rethink rather than chasing fires forever, but I
guess I'm still willing to chase fires so far.


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

[danakj, 2013-04-17 15:43:39]

Committed patchset #3 manually as r194596 (presubmit successful).