New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 651584 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocking:
issue 644582



Sign in to add a comment

Move the screen space scale factor to root transform node

Project Member Reported by jaydasika@chromium.org, Sep 29 2016

Issue description

Screen space scale factor is device scale factor + scale component of
device transform. Both device scale factor and device transform are
currently baked into the local transform of the transform node formed by
the root layer (which is the "contents root" transform node). But this
leads to the following problem :
When we compute transform from one render surface to another, we divide
the surface contents scale at source and multiply the surface contents
scale at destination. So, if there is no scale transform between these
two surfaces, the resulting transform should not have any scale
component. But, if our source is the root node, there is no surface
contents scale at root (because we store it on "contents root" node), but
device scale factor will propagate to destination and we will wrongly end
up with a scale component in the transform between root surface and the
destination surface.

The proposed solution for this problem is to split the device scale factor + device transform such that the entire scale component goes into the root node and the remaining non-scale(translate) component goes into the contents root node. 
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6

commit 61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6
Author: jaydasika <jaydasika@chromium.org>
Date: Tue Oct 25 15:49:46 2016

cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG= 651584 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2408243002
Cr-Commit-Position: refs/heads/master@{#427366}

[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/effect_node.cc
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/property_tree.cc
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/property_tree.h
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/cc/trees/property_tree_unittest.cc
[modify] https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp

Labels: Merge-Request-55

Comment 3 by dimu@chromium.org, Oct 26 2016

Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 26 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b96a29859b70eea11eacefef82b25318d43700fc

commit b96a29859b70eea11eacefef82b25318d43700fc
Author: Jayadev Dasika <jaydasika@google.com>
Date: Wed Oct 26 22:44:53 2016

cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG= 651584 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2408243002
Cr-Commit-Position: refs/heads/master@{#427366}
(cherry picked from commit 61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6)

Review URL: https://codereview.chromium.org/2452023003 .

Cr-Commit-Position: refs/branch-heads/2883@{#319}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/effect_node.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/property_tree.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/property_tree.h
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/property_tree_unittest.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp

Status: Fixed (was: Started)
Verified that all cc_unittests pass on the M55 after merging my patch. 
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b96a29859b70eea11eacefef82b25318d43700fc

commit b96a29859b70eea11eacefef82b25318d43700fc
Author: Jayadev Dasika <jaydasika@google.com>
Date: Wed Oct 26 22:44:53 2016

cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG= 651584 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2408243002
Cr-Commit-Position: refs/heads/master@{#427366}
(cherry picked from commit 61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6)

Review URL: https://codereview.chromium.org/2452023003 .

Cr-Commit-Position: refs/branch-heads/2883@{#319}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/effect_node.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/property_tree.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/property_tree.h
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/cc/trees/property_tree_unittest.cc
[modify] https://crrev.com/b96a29859b70eea11eacefef82b25318d43700fc/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82f00f4aec461baee246b2785df4c59e2fc490ec

commit 82f00f4aec461baee246b2785df4c59e2fc490ec
Author: jaydasika <jaydasika@chromium.org>
Date: Thu Oct 27 21:08:41 2016

Revert of cc : Move screen space scale factor to root transform node (patchset #1 id:1 of https://codereview.chromium.org/2452023003/ )

Reason for revert:
https://bugs.chromium.org/p/chromium/issues/detail?id=660047

Original issue's description:
> cc : Move screen space scale factor to root transform node
>
> This CL computes the screen space scale factor which is
> combined form scale factors of device transform, device
> scale factor and page scale factor(if required), and stores
> it on the root transform node as its surface contents scale.
> This also implicitly fixes a clipping bug.
> TransformTree::ComputeTransforms(a, b) should return the
> transform between a and b without the surface contents
> scale. But, since screen space scale was baked into the
> local transform of the contents root node (before this CL),
> ComputeTransform(a, root) was having the scale baked in.
> (and this caused the bug)
>
> BUG= 651584 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
>
> Review-Url: https://codereview.chromium.org/2408243002
> Cr-Commit-Position: refs/heads/master@{#427366}
> (cherry picked from commit 61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6)
>
> Committed: https://chromium.googlesource.com/chromium/src/+/b96a29859b70eea11eacefef82b25318d43700fc

TBR=
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 651584 

Review-Url: https://codereview.chromium.org/2454203002
Cr-Commit-Position: refs/branch-heads/2883@{#344}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/effect_node.cc
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/property_tree.cc
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/property_tree.h
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/cc/trees/property_tree_unittest.cc
[modify] https://crrev.com/82f00f4aec461baee246b2785df4c59e2fc490ec/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18ea272710d605c67458b1edd162821d758fb645

commit 18ea272710d605c67458b1edd162821d758fb645
Author: jaydasika <jaydasika@chromium.org>
Date: Thu Oct 27 23:47:10 2016

Revert of cc : Move screen space scale factor to root transform node (patchset #12 id:240001 of https://codereview.chromium.org/2408243002/ )

Reason for revert:
https://bugs.chromium.org/p/chromium/issues/detail?id=660047

Original issue's description:
> cc : Move screen space scale factor to root transform node
>
> This CL computes the screen space scale factor which is
> combined form scale factors of device transform, device
> scale factor and page scale factor(if required), and stores
> it on the root transform node as its surface contents scale.
> This also implicitly fixes a clipping bug.
> TransformTree::ComputeTransforms(a, b) should return the
> transform between a and b without the surface contents
> scale. But, since screen space scale was baked into the
> local transform of the contents root node (before this CL),
> ComputeTransform(a, root) was having the scale baked in.
> (and this caused the bug)
>
> BUG= 651584 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
>
> Committed: https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6
> Cr-Commit-Position: refs/heads/master@{#427366}

TBR=weiliangc@chromium.org,ajuma@chromium.org,chrishtr@chromium.org,pdr@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 651584 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2452303003
Cr-Commit-Position: refs/heads/master@{#428221}

[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/cc/trees/effect_node.cc
[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/cc/trees/property_tree.cc
[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/cc/trees/property_tree.h
[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/cc/trees/property_tree_unittest.cc
[modify] https://crrev.com/18ea272710d605c67458b1edd162821d758fb645/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp

Comment 9 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840

Sign in to add a comment