cc: Force an update on tile size after viewport resize

cc: Force an update on tile size after viewport resize

GPU rasterization can cause a tile size change when the viewport changes.
Ensure that the tiling's texture sizes are correct after a viewport resize
by tracking the last viewport rect and updating the current raster source,
which will in turn update the tiling's texture size.

BUG=458750

[hendrikw, 2015-03-18 00:16:53]

hendrikw@chromium.org changed reviewers:
+ enne@chromium.org

[hendrikw, 2015-03-18 00:16:53]

PTAL -- Turns out I did need to keep the size.  I have to admit, I'm not
completely happy with this approach :(

[enne (OOO), 2015-03-18 07:32:49]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2015-03-18 07:32:50]

lgtm

[commit-bot: I haz the power, 2015-03-18 07:33:00]

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

[commit-bot: I haz the power, 2015-03-18 12:53:39]

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

[commit-bot: I haz the power, 2015-03-18 12:53:40]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[vmpstr, 2015-03-18 21:36:39]

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

https://codereview.chromium.org/1013273003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:400:
tilings_->UpdateTilingsToCurrentRasterSource(
In "typical" situations where the device viewport changes and we get like layout
or something, this function would already be called. What are the implications
of calling it for the second time?

It seems that at the very least it's would invalidate again. Is it just going to
be a performance hit? In that case, I'm not sure that taking a performance hit
in a typical situation in order to fix a rare situation is... good? 

I might just be making noise though, since I don't think UpdateTiles is a
many-times-a-frame type of an operation.

[danakj, 2015-03-18 21:46:14]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-03-18 21:46:14]

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

https://codereview.chromium.org/1013273003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:397: if (!last_viewport_size_.IsEmpty() &&
This looks like it should be below the !CanHaveTilings() early out.

https://codereview.chromium.org/1013273003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:400:
tilings_->UpdateTilingsToCurrentRasterSource(
On 2015/03/18 21:36:39, vmpstr wrote:
> In "typical" situations where the device viewport changes and we get like
layout
> or something, this function would already be called. What are the implications
> of calling it for the second time?
> 
> It seems that at the very least it's would invalidate again. Is it just going
to
> be a performance hit? In that case, I'm not sure that taking a performance hit
> in a typical situation in order to fix a rare situation is... good? 
> 
> I might just be making noise though, since I don't think UpdateTiles is a
> many-times-a-frame type of an operation.

What if there's no pending tree (commit to active, so this is the active tree),
but the viewport changed. We won't DCHECK cuz there's no sync, but we'll have
tiles of the wrong size still?

For that reason I kinda wonder if this shouldn't be handled in PictureLayer
instead, by it making sure to push a new RasterSource if the viewport size
changed? Or something else on the main thread side? That would address your
concern about putting this in UpdateTiles too? I'm not sure.

[hendrikw, 2015-03-18 22:06:32]

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

https://codereview.chromium.org/1013273003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:397: if (!last_viewport_size_.IsEmpty() &&
On 2015/03/18 21:46:14, danakj wrote:
> This looks like it should be below the !CanHaveTilings() early out.

Yeah, you're right.

https://codereview.chromium.org/1013273003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:400:
tilings_->UpdateTilingsToCurrentRasterSource(
On 2015/03/18 21:46:14, danakj wrote:
> On 2015/03/18 21:36:39, vmpstr wrote:
> > In "typical" situations where the device viewport changes and we get like
> layout
> > or something, this function would already be called. What are the
implications
> > of calling it for the second time?
> > 
> > It seems that at the very least it's would invalidate again. Is it just
going
> to
> > be a performance hit? In that case, I'm not sure that taking a performance
hit
> > in a typical situation in order to fix a rare situation is... good? 
> > 
> > I might just be making noise though, since I don't think UpdateTiles is a
> > many-times-a-frame type of an operation.
> 
> What if there's no pending tree (commit to active, so this is the active
tree),
> but the viewport changed. We won't DCHECK cuz there's no sync, but we'll have
> tiles of the wrong size still?
> 
> For that reason I kinda wonder if this shouldn't be handled in PictureLayer
> instead, by it making sure to push a new RasterSource if the viewport size
> changed? Or something else on the main thread side? That would address your
> concern about putting this in UpdateTiles too? I'm not sure.

Yeah, my first change (https://codereview.chromium.org/1018463005/) would
trigger the push when we get a viewport size change, the SetNeedsPushProperties
is what normally causes the tiling texture size to be recalculated, but it
doesn't work for the layers sitting out of the viewport, earlies out before the
needs push.

Enne thinks the other approach is more work than what's needed, and too
indirect, and might possibly be optimized out (although hopefully my unit test
would catch that).

I think I prefer the other approach.

[enne (OOO), 2015-03-18 22:13:48]

The problem here is that you have a layer that is up to date.  It early outs of
an update call.  This also causes it to not push properties.  It's push
properties that eventually indirectly calls UpdateTilingsToCurrentRasterSource
which calculates the tile size.  So, if push properties is skipped and the
viewport is updated on the same frame, then the sync tree has the wrong tile
size.  And, if the sync tree is the pending tree, it will have a different tile
size than its twin and will dcheck.

Implicit Solution #1: force a push properties when the viewport changes on the
main thread size
Explicit Solution #2: make update tiles on the sync tree detect when this has
changed and recalculate the tile size

I don't like #1 and suggested #2 for the following reasons:
* it's extra work to push properties, and nothing has changed
* the only reason it works is because push properties does this work, and that
could/will move
* I think it's hella subtle to trigger PictureLayer::PushProperties to get
PictureLayerTiling's tile size to be correct
* viewport size is not related to anything on the main thread; it's all
compositor thread side
* in slimming paint, push properties will go away, and we will push a display
list for the entire frame to the compositor thread, and the PLI constructed from
that display list will have to detect that the viewport has changed, so #2 is
much more forward-compatible

> What if there's no pending tree (commit to active, so this is the active
tree), but the viewport changed. We won't DCHECK cuz there's no sync, but we'll
have tiles of the wrong size still?

We should do this on the sync tree only, in my opinion.  I think that solves
both commit-to-active and commit-to-pending paths.

> It seems that at the very least it's would invalidate again. Is it just going
to be a performance hit? In that case, I'm not sure that taking a performance
hit in a typical situation in order to fix a rare situation is... good? 

I do not have any worries here.  If PushProperties happens, then
CalculateTileSize will get run using the correct viewport.  (Maybe
PushProperties should set the last viewport as well, as it's now up to date, and
this new code will early out.)  Then this new code will run and the tile size
will be the same.  This code will catch the case where the tile size is not up
to date.  In that case, it will do the same work it would have done in
PushProperties, changing the tile size, and throwing away the tilings.

[danakj, 2015-03-18 22:20:29]

On Wed, Mar 18, 2015 at 3:13 PM, <enne@chromium.org> wrote:

> The problem here is that you have a layer that is up to date.  It early
> outs of
> an update call.  This also causes it to not push properties.  It's push
> properties that eventually indirectly calls UpdateTilingsToCurrentRasterSo
> urce
> which calculates the tile size.  So, if push properties is skipped and the
> viewport is updated on the same frame, then the sync tree has the wrong
> tile
> size.  And, if the sync tree is the pending tree, it will have a different
> tile
> size than its twin and will dcheck.
>
> Implicit Solution #1: force a push properties when the viewport changes on
> the
> main thread size
> Explicit Solution #2: make update tiles on the sync tree detect when this
> has
> changed and recalculate the tile size
>
> I don't like #1 and suggested #2 for the following reasons:
> * it's extra work to push properties, and nothing has changed
> * the only reason it works is because push properties does this work, and
> that
> could/will move
> * I think it's hella subtle to trigger PictureLayer::PushProperties to get
> PictureLayerTiling's tile size to be correct
> * viewport size is not related to anything on the main thread; it's all
> compositor thread side
> * in slimming paint, push properties will go away, and we will push a
> display
> list for the entire frame to the compositor thread, and the PLI
> constructed from
> that display list will have to detect that the viewport has changed, so #2
> is
> much more forward-compatible


These are good points. I think what's awkward about doing this in
UpdateTiles() is that it is a very general method that gets called at many
different points in time.

It would be more obvious to me if LTHI kept a "viewport changed" bool and
if true told every sync tree layer that the viewport had changed before
doing UpdateTiles, giving them a chance to explicitly change tile size if
desired (ie a UpdateTileSize type method on PLTS that either does nothing
or drops all the tiles). Do you think that's a reasonable #3 or something
like it?


>
>
>  What if there's no pending tree (commit to active, so this is the active
>>
> tree), but the viewport changed. We won't DCHECK cuz there's no sync, but
> we'll
> have tiles of the wrong size still?
>
> We should do this on the sync tree only, in my opinion.  I think that
> solves
> both commit-to-active and commit-to-pending paths.
>
>  It seems that at the very least it's would invalidate again. Is it just
>> going
>>
> to be a performance hit? In that case, I'm not sure that taking a
> performance
> hit in a typical situation in order to fix a rare situation is... good?
>
> I do not have any worries here.  If PushProperties happens, then
> CalculateTileSize will get run using the correct viewport.  (Maybe
> PushProperties should set the last viewport as well, as it's now up to
> date, and
> this new code will early out.)  Then this new code will run and the tile
> size
> will be the same.  This code will catch the case where the tile size is
> not up
> to date.  In that case, it will do the same work it would have done in
> PushProperties, changing the tile size, and throwing away the tilings.
>
> https://codereview.chromium.org/1013273003/
>

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

[hendrikw, 2015-03-18 22:22:32]

> We should do this on the sync tree only, in my opinion.  I think that solves
> both commit-to-active and commit-to-pending paths.

Unfortunately the tests do not work with IsSyncTree(), only works with
IsPendingTree().

[danakj, 2015-03-18 22:23:41]

On Wed, Mar 18, 2015 at 3:22 PM, <hendrikw@chromium.org> wrote:

> We should do this on the sync tree only, in my opinion.  I think that
>> solves
>> both commit-to-active and commit-to-pending paths.
>>
>
> Unfortunately the tests do not work with IsSyncTree(), only works with
> IsPendingTree().
>

Why?

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

[hendrikw, 2015-03-18 22:27:32]

On 2015/03/18 22:23:41, danakj wrote:
> On Wed, Mar 18, 2015 at 3:22 PM, <mailto:hendrikw@chromium.org> wrote:
> 
> > We should do this on the sync tree only, in my opinion.  I think that
> >> solves
> >> both commit-to-active and commit-to-pending paths.
> >>
> >
> > Unfortunately the tests do not work with IsSyncTree(), only works with
> > IsPendingTree().
> >
> 
> Why?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

browser_test - WebViewDPITest.Shim_TestAutosizeHeight
[4344:8744:0318/152038:FATAL:picture_layer_tiling.cc(334)] Check failed:
null_recycled_twin == client_->GetRecycledTwinTiling(this) (00000000 vs.
332445E0)

In this case we must be ActiveTree + SyncTree (I admit I have little to no idea
what SyncTree is)

[danakj, 2015-03-18 22:29:28]

On Wed, Mar 18, 2015 at 3:27 PM, <hendrikw@chromium.org> wrote:

> On 2015/03/18 22:23:41, danakj wrote:
>
>> On Wed, Mar 18, 2015 at 3:22 PM, <mailto:hendrikw@chromium.org> wrote:
>>
>
>  > We should do this on the sync tree only, in my opinion.  I think that
>> >> solves
>> >> both commit-to-active and commit-to-pending paths.
>> >>
>> >
>> > Unfortunately the tests do not work with IsSyncTree(), only works with
>> > IsPendingTree().
>> >
>>
>
>  Why?
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> browser_test - WebViewDPITest.Shim_TestAutosizeHeight
> [4344:8744:0318/152038:FATAL:picture_layer_tiling.cc(334)] Check failed:
> null_recycled_twin == client_->GetRecycledTwinTiling(this) (00000000 vs.
> 332445E0)
>
> In this case we must be ActiveTree + SyncTree (I admit I have little to no
> idea
> what SyncTree is)
>

SyncTree is the tree main thread is committing to. In commit-to-active (ui
compositor) it is the active tree, we don't have a pending one ever.

In your test you have a recycled twin, which is weird. If you're commiting
to active there is never a pending or recycle tree.

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

[enne (OOO), 2015-03-18 22:30:04]

> These are good points. I think what's awkward about doing this in
> UpdateTiles() is that it is a very general method that gets called at many
> different points in time.

I disagree with this assessment.  UpdateTiles gets called after
UpdateDrawProperties only.  UpdateDrawProperties gets called right after commit.
 It gets called at other times too, sure, but this code will only end up running
after a commit.  It's in the flavor of the smelly DoPostCommitWhizbangsIfNeeded
function, so I can get why it's not that appealing either.

> It would be more obvious to me if LTHI kept a "viewport changed" bool and
> if true told every sync tree layer that the viewport had changed before
> doing UpdateTiles, giving them a chance to explicitly change tile size if
> desired (ie a UpdateTileSize type method on PLTS that either does nothing
> or drops all the tiles). Do you think that's a reasonable #3 or something
> like it?

Ehhhhhhhhh, bleh on an "every sync tree layer" function.

How about proposal #4.  Do something like UpdateCanUseLCDTextAfterCommit, i.e.
add a non-virtual PictureLayerImpl::DidUpdateViewportAfterCommit call and call
it in the same place in LayerTreeImpl?  This also requires changing
update_lcd_text to be first_update_after_commit in the UpdateDrawProperties
signature, I guess.

[vmpstr, 2015-03-18 22:32:35]

On 2015/03/18 22:13:48, enne wrote:
> The problem here is that you have a layer that is up to date.  It early outs
of

I think that's where I somewhat disagree. We don't have a layer that is up to
date. All of the inputs that determine the layer properties would normally be up
to date yes, but the device viewport is one of these properties that is used in
gpu raster to determine the tile size. And it has changed. I think another
alternative is to use a different signal as a tile size input (something that
would be propagated via push props), but that's probably not just a patch to fix
a dcheck. 


> an update call.  This also causes it to not push properties.  It's push
> properties that eventually indirectly calls UpdateTilingsToCurrentRasterSource
> which calculates the tile size.  So, if push properties is skipped and the
> viewport is updated on the same frame, then the sync tree has the wrong tile
> size.  And, if the sync tree is the pending tree, it will have a different
tile
> size than its twin and will dcheck.
> 
> Implicit Solution #1: force a push properties when the viewport changes on the
> main thread size
> Explicit Solution #2: make update tiles on the sync tree detect when this has
> changed and recalculate the tile size
> 
> I don't like #1 and suggested #2 for the following reasons:
> * it's extra work to push properties, and nothing has changed
> * the only reason it works is because push properties does this work, and that
> could/will move
> * I think it's hella subtle to trigger PictureLayer::PushProperties to get
> PictureLayerTiling's tile size to be correct
> * viewport size is not related to anything on the main thread; it's all
> compositor thread side
> * in slimming paint, push properties will go away, and we will push a display
> list for the entire frame to the compositor thread, and the PLI constructed
from
> that display list will have to detect that the viewport has changed, so #2 is
> much more forward-compatible
> 
> > What if there's no pending tree (commit to active, so this is the active
> tree), but the viewport changed. We won't DCHECK cuz there's no sync, but
we'll
> have tiles of the wrong size still?
> 
> We should do this on the sync tree only, in my opinion.  I think that solves
> both commit-to-active and commit-to-pending paths.
> 
> > It seems that at the very least it's would invalidate again. Is it just
going
> to be a performance hit? In that case, I'm not sure that taking a performance
> hit in a typical situation in order to fix a rare situation is... good? 
> 
> I do not have any worries here.  If PushProperties happens, then
> CalculateTileSize will get run using the correct viewport.  (Maybe
> PushProperties should set the last viewport as well, as it's now up to date,
and
> this new code will early out.)  Then this new code will run and the tile size
> will be the same.  This code will catch the case where the tile size is not up
> to date.  In that case, it will do the same work it would have done in
> PushProperties, changing the tile size, and throwing away the tilings.

[danakj, 2015-03-18 22:34:04]

On Wed, Mar 18, 2015 at 3:30 PM, <enne@chromium.org> wrote:

> These are good points. I think what's awkward about doing this in
>> UpdateTiles() is that it is a very general method that gets called at many
>> different points in time.
>>
>
> I disagree with this assessment.  UpdateTiles gets called after
> UpdateDrawProperties only.  UpdateDrawProperties gets called right after
> commit.
>  It gets called at other times too, sure, but this code will only end up
> running
> after a commit.  It's in the flavor of the smelly
> DoPostCommitWhizbangsIfNeeded
> function, so I can get why it's not that appealing either.
>
>  It would be more obvious to me if LTHI kept a "viewport changed" bool and
>> if true told every sync tree layer that the viewport had changed before
>> doing UpdateTiles, giving them a chance to explicitly change tile size if
>> desired (ie a UpdateTileSize type method on PLTS that either does nothing
>> or drops all the tiles). Do you think that's a reasonable #3 or something
>> like it?
>>
>
> Ehhhhhhhhh, bleh on an "every sync tree layer" function.
>
> How about proposal #4.  Do something like UpdateCanUseLCDTextAfterCommit,
> i.e.
> add a non-virtual PictureLayerImpl::DidUpdateViewportAfterCommit call and
> call
> it in the same place in LayerTreeImpl?  This also requires changing
> update_lcd_text to be first_update_after_commit in the UpdateDrawProperties
> signature, I guess.
>

Yes I like that even better than #3.

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

[hendrikw, 2015-03-19 17:38:18]

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

[hendrikw, 2015-03-19 17:38:18]

I've tried the LCD approach.

I hit a snag:

There was already a 'did viewport change' mechanism.  

It felt dirty to add a second one.

It works, but weird that I'm checking an active tree var for a pending tree
update.

[enne (OOO), 2015-03-19 17:55:24]

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

https://codereview.chromium.org/1013273003/diff/80001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:575: DCHECK(layer_tree_impl()->IsPendingTree());
IsSyncTree.  Please make sure you have a commit to active test for this.

https://codereview.chromium.org/1013273003/diff/80001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1013273003/diff/80001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:678: bool layersNeedNotificationOfViewportChange =
layers_need_notification_etc

https://codereview.chromium.org/1013273003/diff/80001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:679:
layer_tree_host_impl_->active_tree()->ViewportSizeInvalid() &&
This will not work.  If you commit to the active tree, then there will not be a
pending tree, and ViewportSizeInvalid will not be true.  Then you will not
update the tile size until the next time a layer has a push properties.

https://codereview.chromium.org/1013273003/diff/80001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:680: layer_tree_host_impl_->use_gpu_rasterization();
This is leaking implementation details of PictureLayerImpl, somewhat.  I'd just
always call it and let the PictureLayerImpl function early out.

[danakj, 2015-03-19 18:43:38]

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

https://codereview.chromium.org/1013273003/diff/80001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:575: DCHECK(layer_tree_impl()->IsPendingTree());
On 2015/03/19 17:55:24, enne wrote:
> IsSyncTree.  Please make sure you have a commit to active test for this.

(Any single thread LayerTreeTest would)