cc: Changed ComputeExpansionDelta to never return a delta less than zero

cc: Changed ComputeExpansionDelta to never return a delta less than zero.

The eventually rect was returning as smaller than the visible rect, which
caused tiles to be dropped.  Returning a delta larger or equal to zero
fixes it.

BUG=407776
Committed: https://crrev.com/37974aa8d0c136e8bc1c015092540989a8b7f4b1
Cr-Commit-Position: refs/heads/master@{#294091}

[hendrikw, 2014-09-09 00:17:50]

hendrikw@chromium.org changed reviewers:
+ danakj@chromium.org, vmpstr@chromium.org

[hendrikw, 2014-09-09 00:17:50]

Please review.  It was possible for the eventually rect was becoming smaller
than the visible rect, which caused tiles to not be rendered.

The fix was to modify the function that calculates the delta to never return
anything smaller than zero.  This means that more tiles may end up drawn on
screen and won't be dropped in these situations.

I've updated one test to check for the failure case in the bug, and removed
another test that I believe no longer makes sense.

[vmpstr, 2014-09-09 00:29:48]

lgtm % danakj

[tfarina, 2014-09-09 00:32:48]

Please, do not land this before improving the CL description.

Just saying "Fix for issue 123", does not say much for a casual reader reading
the git log/commit messages.

Please, describe what you are fix and why, the motivation, etc, adding any
context relevant to the change being made here.

Thanks.

[tfarina, 2014-09-09 00:41:02]

On 2014/09/09 00:32:48, tfarina wrote:
> Please, do not land this before improving the CL description.
> 
Description was updated after (or even before) I posted this. So disregard it.

CL description lgtm.

[danakj, 2014-09-09 15:52:04]

One question: This shrinking was done to keep the number of tiles from exploding
during a pinch-zoom out. What will this change do to our tile
manager/prioritization/framerate performance during pinch?

https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:826: int result =
nit: s/result/delta/?

https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:874: // if the eventually rect
becomes smaller than the visible rect, we will start
If

https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:876: EXPECT_TRUE(out.width() >=
in.width());
out.Contains(in)? You want it to actually be including all of |in|

[vmpstr, 2014-09-09 16:08:20]

On 2014/09/09 15:52:04, danakj wrote:
> One question: This shrinking was done to keep the number of tiles from
exploding
> during a pinch-zoom out. What will this change do to our tile
> manager/prioritization/framerate performance during pinch?

Personally, I don't think it's correct to shrink the visible rect in order to
cap the tile count. It's true that we now don't really have a bound on it in one
tiling, but we could always reach that situation via multiple layers/tilings
that each respect the max count. The problem is that if we shrink the visible
rect, then the tile manager never even gets an option of rasterizing visible
content that was dropped (even if we have plenty of gpu memory to spare, or if
the content is solid). In cases where this happens, I would imagine the
performance will go down as we'll have to be managing more tiles, but I like to
this of this as a correctness fix. Just my 2c.

> 
>
https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
> File cc/resources/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
> cc/resources/picture_layer_tiling.cc:826: int result =
> nit: s/result/delta/?
> 
>
https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
> File cc/resources/picture_layer_tiling_unittest.cc (right):
> 
>
https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
> cc/resources/picture_layer_tiling_unittest.cc:874: // if the eventually rect
> becomes smaller than the visible rect, we will start
> If
> 
>
https://codereview.chromium.org/555643002/diff/60001/cc/resources/picture_lay...
> cc/resources/picture_layer_tiling_unittest.cc:876: EXPECT_TRUE(out.width() >=
> in.width());
> out.Contains(in)? You want it to actually be including all of |in|

[hendrikw, 2014-09-09 16:22:38]

Fixes for review comments

[danakj, 2014-09-09 16:52:04]

On 2014/09/09 16:08:20, vmpstr wrote:
> On 2014/09/09 15:52:04, danakj wrote:
> > One question: This shrinking was done to keep the number of tiles from
> exploding
> > during a pinch-zoom out. What will this change do to our tile
> > manager/prioritization/framerate performance during pinch?
> 
> Personally, I don't think it's correct to shrink the visible rect in order to
> cap the tile count. It's true that we now don't really have a bound on it in
one
> tiling, but we could always reach that situation via multiple layers/tilings
> that each respect the max count. The problem is that if we shrink the visible
> rect, then the tile manager never even gets an option of rasterizing visible
> content that was dropped (even if we have plenty of gpu memory to spare, or if
> the content is solid). In cases where this happens, I would imagine the
> performance will go down as we'll have to be managing more tiles, but I like
to
> this of this as a correctness fix. Just my 2c.

If we have so many visible tiles, it also means we are using an incorrect
tiling, and we should make a new tiling of a larger scale. And we'll make a
larger tiling during pinch, which will fill in those empty spots. So I'm not
totally sold that this performance hit is the right thing to do. Anyhow, the
patch changes LGTM.

[hendrikw, 2014-09-09 23:57:23]

On 2014/09/09 16:52:04, danakj wrote:
> On 2014/09/09 16:08:20, vmpstr wrote:
> > On 2014/09/09 15:52:04, danakj wrote:
> > > One question: This shrinking was done to keep the number of tiles from
> > exploding
> > > during a pinch-zoom out. What will this change do to our tile
> > > manager/prioritization/framerate performance during pinch?
> > 
> > Personally, I don't think it's correct to shrink the visible rect in order
to
> > cap the tile count. It's true that we now don't really have a bound on it in
> one
> > tiling, but we could always reach that situation via multiple layers/tilings
> > that each respect the max count. The problem is that if we shrink the
visible
> > rect, then the tile manager never even gets an option of rasterizing visible
> > content that was dropped (even if we have plenty of gpu memory to spare, or
if
> > the content is solid). In cases where this happens, I would imagine the
> > performance will go down as we'll have to be managing more tiles, but I like
> to
> > this of this as a correctness fix. Just my 2c.
> 
> If we have so many visible tiles, it also means we are using an incorrect
> tiling, and we should make a new tiling of a larger scale. And we'll make a
> larger tiling during pinch, which will fill in those empty spots. So I'm not
> totally sold that this performance hit is the right thing to do. Anyhow, the
> patch changes LGTM.

Thanks, as vmpster mentioned, the change is needed for correctness.  I
guess if this does end up being a noticeable performance issue, we'll
need to look into ways around it.

[hendrikw, 2014-09-09 23:57:28]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:07:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hendrikw@chromium.org/555643002/80001

[commit-bot: I haz the power, 2014-09-10 01:08:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 01:08:43]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hendrikw, 2014-09-10 01:44:31]

Thankfully it seems I didn't delete enough code to make the android build happy.
 Deleted an unused function.

[hendrikw, 2014-09-10 01:44:36]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-09-10 01:48:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hendrikw@chromium.org/555643002/100001

[commit-bot: I haz the power, 2014-09-10 03:29:34]

Committed patchset #6 (id:100001) as cb2232f8f49f6b9ca4becb2d1da5acc351ff8694

[commit-bot: I haz the power, 2014-09-10 03:58:46]

Patchset 6 (id:??) landed as
https://crrev.com/37974aa8d0c136e8bc1c015092540989a8b7f4b1
Cr-Commit-Position: refs/heads/master@{#294091}