Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13582006:
Prioritize tiles near the viewport in out-of-viewport layers (Closed)
|
Created:
7 years, 8 months ago by Xianzhu Modified:
7 years, 8 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrioritize tiles near the viewport in out-of-viewport layers
Not prioritizing tiles near the viewport in out-of-viewport layers causes
frequent invalidation and redraw of the tiles when the page is scrolled.
Modified ExpandRectEquallyToAreaBoundedBy() to expand starting_rect first
and check if further expansion is needed for the intersection of
starting_rect and bounding_rect.
BUG=223697
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193436
Patch Set 1 #
Total comments: 2
Patch Set 2 : Avoid aggressive expansion in ExpandRectEquallyToAreaBoundedBy so that it works for out-of-viewport… #Patch Set 3 : Keep original behavior #
Total comments: 9
Patch Set 4 : Fix nits #
Total comments: 2
Patch Set 5 : Add GT in the new test #Patch Set 6 : Rebased and fixed presubmit warnings #Patch Set 7 : Use ToString() #
Messages
Total messages: 27 (0 generated)
https://codereview.chromium.org/13582006/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/13582006/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:382: prioritized_rect = ExpandRectBySizeBoundedBy( NAK: Using the viewport rect (or something at most a constant size away from the viewport rect) gives you a quadratic number of tiles relative to the page scale, which is not desirable.
https://codereview.chromium.org/13582006/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/13582006/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:546: if (rect.IsEmpty()) I think this patch should really just remove this conditional.
On 2013/04/03 23:41:49, enne wrote: > https://codereview.chromium.org/13582006/diff/1/cc/resources/picture_layer_ti... > File cc/resources/picture_layer_tiling.cc (right): > > https://codereview.chromium.org/13582006/diff/1/cc/resources/picture_layer_ti... > cc/resources/picture_layer_tiling.cc:546: if (rect.IsEmpty()) > I think this patch should really just remove this conditional. That's incorrect. Please see the second paragraph (excluding the title) in the Description.
> That's incorrect. Please see the second paragraph (excluding the title) in the > Description. and crbug.com/223697 #26.
Removing that conditional is essentially a revert of the patch you said was problematic. Is it the case that reverting that patch does not actually fix the problem entirely?
On 2013/04/03 23:53:18, enne wrote: > Removing that conditional is essentially a revert of the patch you said was > problematic. Is it the case that reverting that patch does not actually fix the > problem entirely? Though it solve the problem, but it brings another issue that almost all tiles in out-of-viewport layers are prioritized even if the layer is very far from the viewport. BTW about the residue issue, reverting the patch just hide the issue. There seems some issues about opaqueness and occlusion caused the residue. We should never show residue even if the tiles are not properly prioritized, right?
In the new patch set, the aggressive behavior of ExpandRectEquallyToAreaBoundedBy() (which sometimes expand to certain side too much, and always return non-empty rect for out-of-viewport layers) is removed.
Uploaded a new patch set that keeps the original aggressive expansion algorithm.
Patch Set 3 is equivalent to Patch Set 2 in the out-of-bounds case; and is equivalent to the original code in the non-out-of-bounds case.
https://codereview.chromium.org/13582006/diff/9001/cc/resources/picture_layer... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/13582006/diff/9001/cc/resources/picture_layer... cc/resources/picture_layer_tiling.cc:536: rect = starting_rect; I like this idea. https://codereview.chromium.org/13582006/diff/9001/cc/resources/picture_layer... cc/resources/picture_layer_tiling.cc:563: for (int event_index = out_of_bounds ? 3 : 0; I don't understand this. Why 3? You want to only grow one edge? Why not just leave this alone? What happens then? https://codereview.chromium.org/13582006/diff/9001/cc/resources/picture_layer... cc/resources/picture_layer_tiling.cc:617: return IntersectRects(gfx::Rect(origin_x, origin_y, width, height), I don't understand this either. If this ends up shrinking the starting_rect, then intersecting it is going to make an empty rect again which we were trying to avoid.
I must admit that Change Set 3 is tricky to keep the original behavior and
support the out-of-viewport case. I think you'll understand the intention here
if you read Patch Set 2. As stated in my previous comment, Patch Set 3 and Patch
Set 2 are equivalent about out-of-bounds layers.
Actually I still more like Patch Set 2 which is clearer and avoids aggressive
expansion. It doesn't always try to expand to target_area. Instead it returns
the rect that the layer rect intersects with the "maximum interest rect" which
covers target_area centered at the viewport:
1. Calculate the expansion delta (not clamped to distance) which would expand
starting_rect to target_area in the next step;
2. Expand the intersected rect by the delta ("maximum interest rect");
3. Intersect the result with bounding_rect.
That is, for the example in crbug.com/226419 #26:
viewport=(0,0, 101x101)
layer=(100,100 1000x1000)
target_area=160000
the Patch Set 2 code returns (100,100 151x151) instead of (100,100 400x400)
which I think is expanded to the right and bottom unnecessarily too much.
https://codereview.chromium.org/13582006/diff/9001/cc/resources/picture_layer...
File cc/resources/picture_layer_tiling.cc (right):
https://codereview.chromium.org/13582006/diff/9001/cc/resources/picture_layer...
cc/resources/picture_layer_tiling.cc:563: for (int event_index = out_of_bounds ?
3 : 0;
On 2013/04/04 21:37:48, danakj wrote:
> I don't understand this. Why 3? You want to only grow one edge? Why not just
> leave this alone? What happens then?
Together with the next change at line 589/591, the code will try the largest
distance for all of the 4 edges.
https://codereview.chromium.org/13582006/diff/9001/cc/resources/picture_layer...
cc/resources/picture_layer_tiling.cc:617: return
IntersectRects(gfx::Rect(origin_x, origin_y, width, height),
On 2013/04/04 21:37:48, danakj wrote:
> I don't understand this either. If this ends up shrinking the starting_rect,
> then intersecting it is going to make an empty rect again which we were trying
> to avoid.
This is what I expected:
- For a layer that is near to the viewport, we prioritize the tiles that are
covered by the "maximum interest rect" (the rect whose area is target_area
centered at the viewport).
- For a layer that is far from the viewport, we return empty rect, to avoid too
many unnecessary prioritized tiles.
On 2013/04/04 21:55:44, Xianzhu wrote:
> 1. Calculate the expansion delta (not clamped to distance) which would expand
> starting_rect to target_area in the next step;
> 2. Expand the intersected rect by the delta ("maximum interest rect");
Correction:
2. Expand starting_rect by delta (resulting "maximum interest rect");
If you agree the Patch Set 2 method, I can further simplify it, because "maximum
interest rect" is the same among all layers for the same viewport. We can reduce
the number of sqrt() calls from number of layers to 1.
> 3. Intersect the result with bounding_rect.
> If you agree the Patch Set 2 method, I can further simplify it, because "maximum > interest rect" is the same among all layers for the same viewport. We can reduce > the number of sqrt() calls from number of layers to 1. > Sorry, please ignore the above paragraph (only). It is incorrect because the viewport rect is in layer content space.
Removed Patch Set 3 which I don't like. Still request review for Patch Set 2. Please let me know if you want me to explain it more.
PTAL. Modified according to today's discussion.
LGTM with some test nits https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... File cc/resources/picture_layer_tiling_unittest.cc (right): https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:264: EXPECT_RECT_EQ(bounds, out); nit: please leave these as they were. ToString() gives a nicer debug output when the rects differ. https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:356: int64 target_area = 10 * 10; nit: instead of changing this to be so small, can you instead move the |in| rect to be far enough from the |bounds| rect so that the expanded |in| rect doesn't intersect bounds? https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:364: gfx::Rect bounds(0, 0, 10, 10); Can you have this bounds be large enough that the resulting out rect actually covers the |target_area|? https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:368: EXPECT_RECT_EQ(bounds, out); nit: compare with EXPECT_EQ and .ToString()
https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... File cc/resources/picture_layer_tiling_unittest.cc (right): https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:264: EXPECT_RECT_EQ(bounds, out); On 2013/04/10 05:33:23, danakj wrote: > nit: please leave these as they were. ToString() gives a nicer debug output when > the rects differ. EXPECT_RECT_EQ is provided to compare gfx::Rects. If it doesn't provide nicer debug output than ToString(), we should fix it, otherwise EXPECT_RECT_EQ should also not be used in other places, so it'd useless and we should remove it. https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:356: int64 target_area = 10 * 10; On 2013/04/10 05:33:23, danakj wrote: > nit: instead of changing this to be so small, can you instead move the |in| rect > to be far enough from the |bounds| rect so that the expanded |in| rect doesn't > intersect bounds? Done. https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:364: gfx::Rect bounds(0, 0, 10, 10); On 2013/04/10 05:33:23, danakj wrote: > Can you have this bounds be large enough that the resulting out rect actually > covers the |target_area|? Done. Now test both cases.
https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... File cc/resources/picture_layer_tiling_unittest.cc (right): https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:264: EXPECT_RECT_EQ(bounds, out); On 2013/04/10 16:19:55, Xianzhu wrote: > On 2013/04/10 05:33:23, danakj wrote: > > nit: please leave these as they were. ToString() gives a nicer debug output > when > > the rects differ. > > EXPECT_RECT_EQ is provided to compare gfx::Rects. If it doesn't provide nicer > debug output than ToString(), we should fix it, otherwise EXPECT_RECT_EQ should > also not be used in other places, so it'd useless and we should remove it. I agree, I've been using .ToString() in new code. But going back and replacing other uses is more work than its worth really.
https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... File cc/resources/picture_layer_tiling_unittest.cc (right): https://chromiumcodereview.appspot.com/13582006/diff/19001/cc/resources/pictu... cc/resources/picture_layer_tiling_unittest.cc:264: EXPECT_RECT_EQ(bounds, out); On 2013/04/10 16:30:59, danakj wrote: > On 2013/04/10 16:19:55, Xianzhu wrote: > > On 2013/04/10 05:33:23, danakj wrote: > > > nit: please leave these as they were. ToString() gives a nicer debug output > > when > > > the rects differ. > > > > EXPECT_RECT_EQ is provided to compare gfx::Rects. If it doesn't provide nicer > > debug output than ToString(), we should fix it, otherwise EXPECT_RECT_EQ > should > > also not be used in other places, so it'd useless and we should remove it. > > I agree, I've been using .ToString() in new code. But going back and replacing > other uses is more work than its worth really. I'd like to bring this topic to the graphics-dev group for decision.
https://codereview.chromium.org/13582006/diff/28001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/13582006/diff/28001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:379: EXPECT_LE(out.width() * out.height(), target_area); can you give a lower bound on the out rect's area as well so we know it's covering the target_area? In this case, with two edges snapped to the bounding rect, I think this should pass? EXPECT_GT(out.width() * out.height(), target_area - out.width() - out.height());
About rect equality I will change definition of EXPECT_RECT_EQ to use ToString() in another CL. https://codereview.chromium.org/13582006/diff/28001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/13582006/diff/28001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:379: EXPECT_LE(out.width() * out.height(), target_area); On 2013/04/10 16:46:24, danakj wrote: > can you give a lower bound on the out rect's area as well so we know it's > covering the target_area? > > In this case, with two edges snapped to the bounding rect, I think this should > pass? > EXPECT_GT(out.width() * out.height(), > target_area - out.width() - out.height()); Done.
LGTM
On Wed, Apr 10, 2013 at 10:08 AM, <wangxianzhu@chromium.org> wrote: > About rect equality I will change definition of EXPECT_RECT_EQ to use > ToString() > in another CL. Okay, thanks. > > > > https://codereview.chromium.**org/13582006/diff/28001/cc/** > resources/picture_layer_**tiling_unittest.cc<https://codereview.chromium.org/13582006/diff/28001/cc/resources/picture_layer_tiling_unittest.cc> > File cc/resources/picture_layer_**tiling_unittest.cc (right): > > https://codereview.chromium.**org/13582006/diff/28001/cc/** > resources/picture_layer_**tiling_unittest.cc#newcode379<https://codereview.chromium.org/13582006/diff/28001/cc/resources/picture_layer_tiling_unittest.cc#newcode379> > cc/resources/picture_layer_**tiling_unittest.cc:379: EXPECT_LE(out.width() > * out.height(), target_area); > On 2013/04/10 16:46:24, danakj wrote: > >> can you give a lower bound on the out rect's area as well so we know >> > it's > >> covering the target_area? >> > > In this case, with two edges snapped to the bounding rect, I think >> > this should > >> pass? >> EXPECT_GT(out.width() * out.height(), >> target_area - out.width() - out.height()); >> > > Done. > > https://codereview.chromium.**org/13582006/<https://codereview.chromium.org/1... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/13582006/28003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/13582006/54001
Message was sent while issue was closed.
Change committed as 193436 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
