cc : Don't draw animating layers with singular screen space transform

cc : Don't draw animating layers with singular screen space transform

This is done by not adding the layer to the render surface layer list
(rsll) on the active tree. The layer will still be part of rsll on
the pending tree to ensure that it gets rastered.

BUG=517170
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2838033002
Cr-Commit-Position: refs/heads/master@{#468348}
Committed: https://chromium.googlesource.com/chromium/src/+/af0dcd17a4c16c8de78e2021d505cf1dbf212a95

[jaydasika, 2017-04-24 23:55:38]

Description was changed from

==========
cc : Don't draw animating layers with singular screen space transform

This is done by not adding the layer to the render surface layer list
(rsll) on the active tree. The layer will still be part of rsll on
the pending tree to ensure that it gets rastered.

BUG=517170
==========

to

==========
cc : Don't draw animating layers with singular screen space transform

This is done by not adding the layer to the render surface layer list
(rsll) on the active tree. The layer will still be part of rsll on
the pending tree to ensure that it gets rastered.

BUG=517170
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jaydasika, 2017-04-24 23:57:56]

jaydasika@chromium.org changed reviewers:
+ vmpstr@chromium.org, weiliangc@chromium.org

[jaydasika, 2017-04-24 23:57:57]

PTAL

[jaydasika, 2017-04-24 23:59:11]

The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 23:59:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 00:55:45]

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

[commit-bot: I haz the power, 2017-04-25 00:55:46]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-04-25 01:04:40]

I'm not sure what I think about this.  In general, I would assume that such a
layer would not be required for activation, so may end up unrastered and on the
active tree.  I think such a layer should be rastered no matter what tree it's
on?

Do we need to decouple the set of things to raster and the set of things to
draw?

[weiliangc, 2017-04-25 14:59:38]

LGTM

[weiliangc, 2017-04-25 15:54:58]

On 2017/04/25 at 01:04:40, enne wrote:
> I'm not sure what I think about this.  In general, I would assume that such a
layer would not be required for activation, so may end up unrastered and on the
active tree.  I think such a layer should be rastered no matter what tree it's
on?
> 
> Do we need to decouple the set of things to raster and the set of things to
draw?


It seems like the Skip layer logic is used at both deciding what layers to
update draw properties, and what layers to include in RSLL, maybe the difference
should be in the two use cases rather than pending tree v.s. active tree?


Though if we don't skip these layers in trying to update draw properties, we
would still get empty visible rect and not raster? Or do we treat the layers as
fully visible w/ singular transform?

[jaydasika, 2017-04-26 21:24:28]

On 2017/04/25 15:54:58, weiliangc wrote:
> On 2017/04/25 at 01:04:40, enne wrote:
> > I'm not sure what I think about this.  In general, I would assume that such
a
> layer would not be required for activation, so may end up unrastered and on
the
> active tree.  I think such a layer should be rastered no matter what tree it's
> on?
> > 
> > Do we need to decouple the set of things to raster and the set of things to
> draw?
> 
> 
> It seems like the Skip layer logic is used at both deciding what layers to
> update draw properties, and what layers to include in RSLL, maybe the
difference
> should be in the two use cases rather than pending tree v.s. active tree?
> 
> 
> Though if we don't skip these layers in trying to update draw properties, we
> would still get empty visible rect and not raster? Or do we treat the layers
as
> fully visible w/ singular transform?

if we don't skip the layer and it has a singular transform, we treat it as fully
visible. But I don't think that's enough to make sure we raster it ? Right now,
I think it has to be in rsll for it to get rastered. But, I agree that pending
vs active tree distinction doesn't make sense. Maybe we should introduce a way
to raster things that are not in rsll. Then we can not add these layers to rsll
and still raster them ?

[vmpstr, 2017-04-26 22:37:28]

On 2017/04/26 21:24:28, jaydasika wrote:
> On 2017/04/25 15:54:58, weiliangc wrote:
> > On 2017/04/25 at 01:04:40, enne wrote:
> > > I'm not sure what I think about this.  In general, I would assume that
such
> a
> > layer would not be required for activation, so may end up unrastered and on
> the
> > active tree.  I think such a layer should be rastered no matter what tree
it's
> > on?
> > > 
> > > Do we need to decouple the set of things to raster and the set of things
to
> > draw?
> > 
> > 
> > It seems like the Skip layer logic is used at both deciding what layers to
> > update draw properties, and what layers to include in RSLL, maybe the
> difference
> > should be in the two use cases rather than pending tree v.s. active tree?
> > 
> > 
> > Though if we don't skip these layers in trying to update draw properties, we
> > would still get empty visible rect and not raster? Or do we treat the layers
> as
> > fully visible w/ singular transform?
> 
> if we don't skip the layer and it has a singular transform, we treat it as
fully
> visible. But I don't think that's enough to make sure we raster it ? Right
now,
> I think it has to be in rsll for it to get rastered. But, I agree that pending
> vs active tree distinction doesn't make sense. Maybe we should introduce a way
> to raster things that are not in rsll. Then we can not add these layers to
rsll
> and still raster them ?

Tiles that come from layer that are not in rsll are skipped for the purposes of
rasterization and they are among the first to be evicted if we hit memory
pressure.

FWIW, it should be pretty straight forward to say "not in rsll but raster it
anyway". The code for controlling this (IIRC) is PLI::HasValidTilePriorities. If
that's false, then we don't raster.

We can do something like
HasValidTilePriorities:
  IsActiveOrPending && (in_rsll || rasterize_even_if_not_in_rsll)

[jaydasika, 2017-04-28 00:25:16]

Added a new bool Layer::raster_even_if_not_in_rsll_. So, now animating layers
with singular transform don't go into rsll but still be rastered.

[vmpstr, 2017-04-28 16:26:24]

lgtm % weiliangc

[jaydasika, 2017-04-28 17:24:14]

The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 17:25:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 18:46:08]

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

[commit-bot: I haz the power, 2017-04-28 18:46:09]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2017-05-01 15:17:18]

LGTM

[jaydasika, 2017-05-01 16:41:20]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2017-05-01 16:41:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 17:37:20]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493656880166570,
"parent_rev": "0d89774170b1e7f1bdf62f355c64edb654c1010e", "commit_rev":
"af0dcd17a4c16c8de78e2021d505cf1dbf212a95"}

[commit-bot: I haz the power, 2017-05-01 17:37:29]

Description was changed from

==========
cc : Don't draw animating layers with singular screen space transform

This is done by not adding the layer to the render surface layer list
(rsll) on the active tree. The layer will still be part of rsll on
the pending tree to ensure that it gets rastered.

BUG=517170
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc : Don't draw animating layers with singular screen space transform

This is done by not adding the layer to the render surface layer list
(rsll) on the active tree. The layer will still be part of rsll on
the pending tree to ensure that it gets rastered.

BUG=517170
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2838033002
Cr-Commit-Position: refs/heads/master@{#468348}
Committed:
https://chromium.googlesource.com/chromium/src/+/af0dcd17a4c16c8de78e2021d505...
==========

[commit-bot: I haz the power, 2017-05-01 17:37:30]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/af0dcd17a4c16c8de78e2021d505...

[jaydasika, 2017-05-02 20:57:58]

Re-looking at this CL, I realized this has an un-intentional side-effect of
removing layers with singular transform and copy requests from RSLL. They should
be part of RSLL, right ?

[enne (OOO), 2017-05-02 21:09:19]

On 2017/05/02 at 20:57:58, jaydasika wrote:
> Re-looking at this CL, I realized this has an un-intentional side-effect of
removing layers with singular transform and copy requests from RSLL. They should
be part of RSLL, right ?

Yeah, it should be possible to copy request a layer with such a transform.