cc: Keep tilings texture size in sync

cc: Keep tilings texture size in sync

This approach keeps the viewport size as a property of the layerimpl,
doing this makes sure that the pending and active trees are in sync,
because the only way for the size to migrate from the pending to
active tree is via a push.

Since being in sync is all the matters, we avoid having to reach in from
outside of the layer and recreate tilings.

BUG=458750
Committed: https://crrev.com/757ba9eec00ccd9802ad58d841563e73f11c2074
Cr-Commit-Position: refs/heads/master@{#321851}

[hendrikw, 2015-03-19 22:31:41]

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

[hendrikw, 2015-03-19 22:31:41]

Alternative solution

[enne (OOO), 2015-03-19 22:37:00]

https://codereview.chromium.org/1024633002/diff/50001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1024633002/diff/50001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:77: layer_tree_host()->device_viewport_size());
How does this work if push properties is skipped.  Isn't that part of the
problem?

[hendrikw, 2015-03-19 22:39:04]

On 2015/03/19 22:37:00, enne wrote:
>
https://codereview.chromium.org/1024633002/diff/50001/cc/layers/picture_layer.cc
> File cc/layers/picture_layer.cc (right):
> 
>
https://codereview.chromium.org/1024633002/diff/50001/cc/layers/picture_layer...
> cc/layers/picture_layer.cc:77: layer_tree_host()->device_viewport_size());
> How does this work if push properties is skipped.  Isn't that part of the
> problem?

The problem is the pending and active trees have different sizes at time of
copy.  It
doesn't matter if the tiling texture size doesn't match the viewport (especially
if off
screen and not drawn), It only matters that they are the same.

[hendrikw, 2015-03-19 22:39:58]

On 2015/03/19 22:39:04, hendrikw wrote:
> On 2015/03/19 22:37:00, enne wrote:
> >
>
https://codereview.chromium.org/1024633002/diff/50001/cc/layers/picture_layer.cc
> > File cc/layers/picture_layer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1024633002/diff/50001/cc/layers/picture_layer...
> > cc/layers/picture_layer.cc:77: layer_tree_host()->device_viewport_size());
> > How does this work if push properties is skipped.  Isn't that part of the
> > problem?
> 
> The problem is the pending and active trees have different sizes at time of
> copy.  It
> doesn't matter if the tiling texture size doesn't match the viewport
(especially
> if off
> screen and not drawn), It only matters that they are the same.

arg - why 80 column limit on forum comments????

[vmpstr, 2015-03-19 23:14:39]

https://codereview.chromium.org/1024633002/diff/50001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1024633002/diff/50001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:508: gpu_raster_max_texture_size_ =
gpu_raster_max_texture_size;
It kind of feels like this can be a separate function or maybe even just
settings the value directly like PushProps does it. Unless there has to be a
reason this is in here?

[hendrikw, 2015-03-19 23:29:43]

Sure, why not :)

[vmpstr, 2015-03-19 23:31:51]

https://codereview.chromium.org/1024633002/diff/70001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/1024633002/diff/70001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:80: gpu_raster_max_texture_size_ =
gpu_raster_max_texture_size;
Do we need a SetNeedsPushProperties with this?

[hendrikw, 2015-03-19 23:34:33]

https://codereview.chromium.org/1024633002/diff/70001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/1024633002/diff/70001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:80: gpu_raster_max_texture_size_ =
gpu_raster_max_texture_size;
On 2015/03/19 23:31:51, vmpstr wrote:
> Do we need a SetNeedsPushProperties with this?

No, the only time we need a push with this is when we're calling it from within
the push already.

[vmpstr, 2015-03-19 23:37:44]

lgtm % enne

[vmpstr, 2015-03-19 23:38:50]

On 2015/03/19 23:37:44, vmpstr wrote:
> lgtm % enne

(can you update the title of the patch to say that it's the tile size in gpu
raster that this is keeping in sync?)

[enne (OOO), 2015-03-20 22:18:40]

https://codereview.chromium.org/1024633002/diff/70001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/1024633002/diff/70001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:80: gpu_raster_max_texture_size_ =
gpu_raster_max_texture_size;
On 2015/03/19 at 23:34:33, hendrikw wrote:
> On 2015/03/19 23:31:51, vmpstr wrote:
> > Do we need a SetNeedsPushProperties with this?
> 
> No, the only time we need a push with this is when we're calling it from
within the push already.

Sorry, so this is intentional that viewport size may be out of date on the
compositor thread for certain layers?

This patch is certainly simplest, but this is also not a forward-looking
solution for slimming paint.  https://codereview.chromium.org/1013273003 is a
lot more in line with what we'll need in the future, so it feels like we should
just do that instead, even if it's more code.

[hendrikw, 2015-03-20 22:44:13]

On 2015/03/20 22:18:40, enne wrote:
>
https://codereview.chromium.org/1024633002/diff/70001/cc/layers/picture_layer...
> File cc/layers/picture_layer_impl.h (right):
> 
>
https://codereview.chromium.org/1024633002/diff/70001/cc/layers/picture_layer...
> cc/layers/picture_layer_impl.h:80: gpu_raster_max_texture_size_ =
> gpu_raster_max_texture_size;
> On 2015/03/19 at 23:34:33, hendrikw wrote:
> > On 2015/03/19 23:31:51, vmpstr wrote:
> > > Do we need a SetNeedsPushProperties with this?
> > 
> > No, the only time we need a push with this is when we're calling it from
> within the push already.
> 
> Sorry, so this is intentional that viewport size may be out of date on the
> compositor thread for certain layers?
> 
> This patch is certainly simplest, but this is also not a forward-looking
> solution for slimming paint.  https://codereview.chromium.org/1013273003 is a
> lot more in line with what we'll need in the future, so it feels like we
should
> just do that instead, even if it's more code.

The texture size is not important here and is probably not important for
slimming paint either.  Using the viewport size for texture size is just an
optimization for GPU rasterization.

What is important is keeping the trees sync'd. Forcing an extra updatetilings on
one of the trees, to me, is not a good answer for that.

Maybe there is something that slimming paint needs, but unless we're already
aware of a reproducible problem, I really don't support the other approach.  I
would suggest that when a problem arises with slimming paint, we re-evaluate it
and work in a non-hack solution that fits with it (this change is simple enough
that doing it this way will not cost us anything to change later).

This approach only affects gpu rasterization, is self contained, ensures the
trees are in sync for the tiling texture size, doesn't have any extra overhead.

[enne (OOO), 2015-03-20 22:58:07]

It's weird to not keep that value up to date, but I guess if it's ok that the
value is stale then it's just confusing but not too problematic.

I don't know what you mean about an extra UpdateTilings.  I don't think that's
how I would describe the other patch.

Can you clarify what you mean about reproducible?  In slimming paint,
PictureLayer will not exist and PictureLayerImpl will still exist.  It will
either need to detect or be informed when the viewport changes to update its
tile size even when the display list hasn't changed for the frame.  That's what
the other patch does.

[hendrikw, 2015-03-20 23:28:32]

On 2015/03/20 22:58:07, enne wrote:
> It's weird to not keep that value up to date, but I guess if it's ok that the
> value is stale then it's just confusing but not too problematic.
> 
> I don't know what you mean about an extra UpdateTilings.  I don't think that's
> how I would describe the other patch.

Yeah, you're right, other approach calls UpdateTilingsToCurrentRasterSource, and
while it does extra work we normally wouldn't need to do (in most of cases,
layout has triggered the code already), it would only recreate the tiles in the
rare case where when we weren't triggered on the other path.  The LCD change
causes tilings to be created twice, I confused this with that.

> Can you clarify what you mean about reproducible?  In slimming paint,
> PictureLayer will not exist and PictureLayerImpl will still exist.  It will
> either need to detect or be informed when the viewport changes to update its
> tile size even when the display list hasn't changed for the frame.  That's
what
> the other patch does.

When something changes with the layer (e.g. blink layout), the tile texture
sizes will update accordingly.  If there's nothing to draw, there's no need to
re-render the tiles with a different tiling texture size.  If the layer is
instead transformed, we would continue to use the old tiles, nothing needs to be
re-rendered (just recomposited).

Maybe I'm missing something here?

[enne (OOO), 2015-03-20 23:34:33]

> > Can you clarify what you mean about reproducible?  In slimming paint,
> > PictureLayer will not exist and PictureLayerImpl will still exist.  It will
> > either need to detect or be informed when the viewport changes to update its
> > tile size even when the display list hasn't changed for the frame.  That's
what
> > the other patch does.
> 
> When something changes with the layer (e.g. blink layout), the tile texture
sizes will update accordingly.  If there's nothing to draw, there's no need to
re-render the tiles with a different tiling texture size.  If the layer is
instead transformed, we would continue to use the old tiles, nothing needs to be
re-rendered (just recomposited).

In the future, the main-thread side layer will not exist.  Any logic there will
need to be moved elsewhere.

[hendrikw, 2015-03-20 23:57:32]

On 2015/03/20 23:34:33, enne wrote:
> > > Can you clarify what you mean about reproducible?  In slimming paint,
> > > PictureLayer will not exist and PictureLayerImpl will still exist.  It
will
> > > either need to detect or be informed when the viewport changes to update
its
> > > tile size even when the display list hasn't changed for the frame.  That's
> what
> > > the other patch does.
> > 
> > When something changes with the layer (e.g. blink layout), the tile texture
> sizes will update accordingly.  If there's nothing to draw, there's no need to
> re-render the tiles with a different tiling texture size.  If the layer is
> instead transformed, we would continue to use the old tiles, nothing needs to
be
> re-rendered (just recomposited).
> 
> In the future, the main-thread side layer will not exist.  Any logic there
will
> need to be moved elsewhere.

Right, where ever the UpdateRasterSource call goes,
set_gpu_raster_max_texture_size would need to follow.

[enne (OOO), 2015-03-23 17:39:38]

On 2015/03/20 at 23:57:32, hendrikw wrote:

> > In the future, the main-thread side layer will not exist.  Any logic there
will
> > need to be moved elsewhere.
> 
> Right, where ever the UpdateRasterSource call goes,
set_gpu_raster_max_texture_size would need to follow.

No, that's exactly the problem we have today.  There's a no-op and
UpdateRasterSource doesn't get called.  That will continue to be the case in the
future.  We'll have a display list for the page.  We'll turn that into layers. 
We'll discover we already have a layer and that layer has the same subset of the
display list.  Some code, somewhere, on the compositor thread needs to check the
viewport and make new tiles.  https://codereview.chromium.org/1013273003 will do
that.

[enne (OOO), 2015-03-23 17:54:54]

lgtm.  Will assign future bugs to you when this breaks with slimming paint.

[hendrikw, 2015-03-23 18:20:06]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-03-23 18:20:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1024633002/70001

[commit-bot: I haz the power, 2015-03-23 21:13:22]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-03-23 21:13:58]

Patchset 5 (id:??) landed as
https://crrev.com/757ba9eec00ccd9802ad58d841563e73f11c2074
Cr-Commit-Position: refs/heads/master@{#321851}