cc: Reduce surface contents scale adjustments.

cc: Reduce surface contents scale adjustments.

This avoids updating the surface contents scale based on the current
transform if layer_transforms_should_scale_layer_contents is false. The
result is that render surfaces size doesn't need to be adjusted during
transform animations in the Chrome OS UI. This is a prerequisite to
caching of render surfaces and is the preferred behavior as UI raster
scale should not change during these animations.

BUG=708513
TEST=cc_unittests --gtest_filter=LayerTreeHostCommonTest.CanAdjustRasterScaleTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[reveman, 2017-04-04 06:27:58]

Description was changed from

==========
cc: Reduce surface contents scale updates.

This avoids updating the surface contents scale when the transform
is animating. The result is that render surface size is constant
on Chrome OS during UI animations, which is a prerequisite to render
surface caching for UI animations.

BUG=
==========

to

==========
cc: Reduce surface contents scale updates.

This avoids updating the surface contents scale when the transform
is animating. The result is that render surface size is constant
on Chrome OS during UI animations, which is a prerequisite to render
surface caching for UI animations.

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

[reveman, 2017-04-04 21:12:43]

reveman@chromium.org changed reviewers:
+ ajuma@chromium.org

[reveman, 2017-04-04 21:12:44]

Hi,

we need to prevent this scale factor adjustment before we can implement render
surface caching for cros UI. See
https://docs.google.com/document/d/1TKftrarbQ_lOTmhYm81__gKK0K8-FjSHEYcpH40Fr...
for more details.

This might be too aggressive and need to be limited to cros UI somehow but I
wanted to send it out for some early feedback and confirmation that the current
render surface behavior is as expected.

[ajuma, 2017-04-04 21:29:46]

Description was changed from

==========
cc: Reduce surface contents scale updates.

This avoids updating the surface contents scale when the transform
is animating. The result is that render surface size is constant
on Chrome OS during UI animations, which is a prerequisite to render
surface caching for UI animations.

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

to

==========
cc: Reduce surface contents scale updates.

This avoids updating the surface contents scale when the transform
is animating. The result is that render surface size is constant
on Chrome OS during UI animations, which is a prerequisite to render
surface caching for UI animations.

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

[ajuma, 2017-04-04 21:41:12]

Updating surface scale is really only useful when we're also updating raster
scales based on transforms, so disabling this for UI seems reasonable.

https://codereview.chromium.org/2795963002/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2795963002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:846: if (transform_node->is_currently_animating) {
Would it make sense to tie this to
LayerTreeSettings::layer_transforms_should_scale_layer_contents == false instead
of to whether we're animating?

https://codereview.chromium.org/2795963002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:847: effect_node->surface_contents_scale =
gfx::Vector2dF(1.0f, 1.0f);
Is device scale factor a better choice than 1 here? (to match the ideal contents
scale of layers)

[reveman, 2017-04-05 13:19:06]

Description was changed from

==========
cc: Reduce surface contents scale updates.

This avoids updating the surface contents scale when the transform
is animating. The result is that render surface size is constant
on Chrome OS during UI animations, which is a prerequisite to render
surface caching for UI animations.

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

to

==========
cc: Reduce surface contents scale adjustments.

This avoids updating the surface contents scale based on the current
transform if layer_transforms_should_scale_layer_contents is false. The
result is that render surfaces size doesn't need to be adjusted during
transform animations in the Chrome OS UI. This is a prerequisite to
caching of render surfaces and is the preferred behavior as UI raster
scale should not change during these animations.

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

[reveman, 2017-04-05 13:20:15]

PTAL

https://codereview.chromium.org/2795963002/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2795963002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:846: if (transform_node->is_currently_animating) {
On 2017/04/04 at 21:41:11, ajuma wrote:
> Would it make sense to tie this to
LayerTreeSettings::layer_transforms_should_scale_layer_contents == false instead
of to whether we're animating?

Makes sense. Done.

https://codereview.chromium.org/2795963002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:847: effect_node->surface_contents_scale =
gfx::Vector2dF(1.0f, 1.0f);
On 2017/04/04 at 21:41:11, ajuma wrote:
> Is device scale factor a better choice than 1 here? (to match the ideal
contents scale of layers)

Yes, it is. Done.

[ajuma, 2017-04-05 13:37:39]

Thanks, looks good. Please also add a unit test, e.g., something like a tree
with render surfaces that would ordinarily get a surface scale but don't now,
and verify the surface and layer draw properties, at least content rect (for
surfaces), visible rect (for layers), and draw transform, clip rect, and
drawable content rect (for both layers and surfaces). See
layer_tree_host_common_unittest for similar tests.

[reveman, 2017-04-05 18:44:53]

Description was changed from

==========
cc: Reduce surface contents scale adjustments.

This avoids updating the surface contents scale based on the current
transform if layer_transforms_should_scale_layer_contents is false. The
result is that render surfaces size doesn't need to be adjusted during
transform animations in the Chrome OS UI. This is a prerequisite to
caching of render surfaces and is the preferred behavior as UI raster
scale should not change during these animations.

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

to

==========
cc: Reduce surface contents scale adjustments.

This avoids updating the surface contents scale based on the current
transform if layer_transforms_should_scale_layer_contents is false. The
result is that render surfaces size doesn't need to be adjusted during
transform animations in the Chrome OS UI. This is a prerequisite to
caching of render surfaces and is the preferred behavior as UI raster
scale should not change during these animations.

BUG=708513
TEST=cc_unittests
--gtest_filter=LayerTreeHostCommonTest.CanAdjustRasterScaleTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[reveman, 2017-04-05 18:45:43]

On 2017/04/05 at 13:37:39, ajuma wrote:
> Thanks, looks good. Please also add a unit test, e.g., something like a tree
with render surfaces that would ordinarily get a surface scale but don't now,
and verify the surface and layer draw properties, at least content rect (for
surfaces), visible rect (for layers), and draw transform, clip rect, and
drawable content rect (for both layers and surfaces). See
layer_tree_host_common_unittest for similar tests.

Done. Please take a look and let me know if there's something more I should
check in the test.

[reveman, 2017-04-05 19:21:02]

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

[commit-bot: I haz the power, 2017-04-05 19:21:33]

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-05 20:07:20]

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

[commit-bot: I haz the power, 2017-04-05 20:07:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ajuma, 2017-04-05 21:36:16]

On 2017/04/05 18:45:43, reveman wrote:
> On 2017/04/05 at 13:37:39, ajuma wrote:
> > Thanks, looks good. Please also add a unit test, e.g., something like a tree
> with render surfaces that would ordinarily get a surface scale but don't now,
> and verify the surface and layer draw properties, at least content rect (for
> surfaces), visible rect (for layers), and draw transform, clip rect, and
> drawable content rect (for both layers and surfaces). See
> layer_tree_host_common_unittest for similar tests.
> 
> Done. Please take a look and let me know if there's something more I should
> check in the test.

Test looks good. There are other tests failing though, I wonder if they're
unintentionally running with can_adjust_raster_scales == false?

[reveman, 2017-04-05 23:25:51]

On 2017/04/05 at 21:36:16, ajuma wrote:
> On 2017/04/05 18:45:43, reveman wrote:
> > On 2017/04/05 at 13:37:39, ajuma wrote:
> > > Thanks, looks good. Please also add a unit test, e.g., something like a
tree
> > with render surfaces that would ordinarily get a surface scale but don't
now,
> > and verify the surface and layer draw properties, at least content rect (for
> > surfaces), visible rect (for layers), and draw transform, clip rect, and
> > drawable content rect (for both layers and surfaces). See
> > layer_tree_host_common_unittest for similar tests.
> > 
> > Done. Please take a look and let me know if there's something more I should
> > check in the test.
> 
> Test looks good. There are other tests failing though, I wonder if they're
unintentionally running with can_adjust_raster_scales == false?

Yes, layer_transforms_should_scale_layer_contents is false by default. Updated
the patch to set it to true for the tests that assumed that it wouldn't affect
raster scale. Ptal.

[reveman, 2017-04-05 23:25:57]

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

[commit-bot: I haz the power, 2017-04-05 23:26:34]

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-06 01:06:12]

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

[commit-bot: I haz the power, 2017-04-06 01:06:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2017-04-06 02:31:19]

reveman@chromium.org changed reviewers:
+ sky@chromium.org

[reveman, 2017-04-06 02:31:19]

+sky for ui/snapshot

A result of this change is that aura snapshots will now return a result that
matches the real size of the window and not something that has been scaled by
the "UI scale". That might not be what we want. Not sure what the unit test
meant by "physical pixels" as we used to get a result that was not physical
pixels but scaled. Shouldn't be hard to add some scaling that keep the size as
it appears on screen if that's what we want?

[reveman, 2017-04-06 04:00:23]

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

[commit-bot: I haz the power, 2017-04-06 04:00:39]

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-06 05:33:59]

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

[commit-bot: I haz the power, 2017-04-06 05:34:00]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-04-06 13:27:05]

Thanks, cc/ lgtm

[sky, 2017-04-06 15:52:08]

On 2017/04/06 02:31:19, reveman wrote:
> +sky for ui/snapshot
> 
> A result of this change is that aura snapshots will now return a result that
> matches the real size of the window and not something that has been scaled by
> the "UI scale". That might not be what we want. Not sure what the unit test
> meant by "physical pixels" as we used to get a result that was not physical
> pixels but scaled. Shouldn't be hard to add some scaling that keep the size as
> it appears on screen if that's what we want?

I would assume the test is meant to mimic a different device scale factor.
Screen grabs should grab the actual pixels, otherwise it's hard for users to
report what they are actually seeing.

[reveman, 2017-04-06 21:36:34]

On 2017/04/06 at 15:52:08, sky wrote:
> I would assume the test is meant to mimic a different device scale factor.
Screen grabs should grab the actual pixels, otherwise it's hard for users to
report what they are actually seeing.

I limited this change to render surfaces without copy requests for now. That
makes the transform still effect the snapshots size. If we want to change that
and instead scale the result as a post processing step then we can do that as a
follow up.

ajuma@, does cc/ still look ok?

[reveman, 2017-04-06 21:36:40]

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

[commit-bot: I haz the power, 2017-04-06 21:37:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-04-06 21:48:01]

Great! I think that means you no longer need me as a reviewer.

On Thu, Apr 6, 2017 at 2:36 PM, <reveman@chromium.org> wrote:

> On 2017/04/06 at 15:52:08, sky wrote:
> > I would assume the test is meant to mimic a different device scale
> factor.
> Screen grabs should grab the actual pixels, otherwise it's hard for users
> to
> report what they are actually seeing.
>
> I limited this change to render surfaces without copy requests for now.
> That
> makes the transform still effect the snapshots size. If we want to change
> that
> and instead scale the result as a post processing step then we can do that
> as a
> follow up.
>
> ajuma@, does cc/ still look ok?
>
> https://codereview.chromium.org/2795963002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajuma, 2017-04-06 21:52:36]

On 2017/04/06 21:36:34, reveman wrote:
> On 2017/04/06 at 15:52:08, sky wrote:
> > I would assume the test is meant to mimic a different device scale factor.
> Screen grabs should grab the actual pixels, otherwise it's hard for users to
> report what they are actually seeing.
> 
> I limited this change to render surfaces without copy requests for now. That
> makes the transform still effect the snapshots size. If we want to change that
> and instead scale the result as a post processing step then we can do that as
a
> follow up.
> 
> ajuma@, does cc/ still look ok?

Still lgtm

[reveman, 2017-04-06 21:58:01]

On 2017/04/06 at 21:48:01, sky wrote:
> Great! I think that means you no longer need me as a reviewer.

Correct.

[reveman, 2017-04-06 21:58:06]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2017-04-06 21:58:08]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2017-04-06 21:59:00]

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-06 23:39:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491515888468070,
"parent_rev": "9bd4d39997fab20c32cd0c7a75c45dbcf3f00642", "commit_rev":
"b2b99a3b809f5ecbad2c3c9106dda9835e39a247"}

[commit-bot: I haz the power, 2017-04-06 23:40:11]

Description was changed from

==========
cc: Reduce surface contents scale adjustments.

This avoids updating the surface contents scale based on the current
transform if layer_transforms_should_scale_layer_contents is false. The
result is that render surfaces size doesn't need to be adjusted during
transform animations in the Chrome OS UI. This is a prerequisite to
caching of render surfaces and is the preferred behavior as UI raster
scale should not change during these animations.

BUG=708513
TEST=cc_unittests
--gtest_filter=LayerTreeHostCommonTest.CanAdjustRasterScaleTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reduce surface contents scale adjustments.

This avoids updating the surface contents scale based on the current
transform if layer_transforms_should_scale_layer_contents is false. The
result is that render surfaces size doesn't need to be adjusted during
transform animations in the Chrome OS UI. This is a prerequisite to
caching of render surfaces and is the preferred behavior as UI raster
scale should not change during these animations.

BUG=708513
TEST=cc_unittests
--gtest_filter=LayerTreeHostCommonTest.CanAdjustRasterScaleTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-04-06 23:40:12]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b2b99a3b809f5ecbad2c3c9106dd...