Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2211113002:
cc: Compute draw transforms correctly when non root surfaces disabled (Closed)
Descriptioncc: Compute draw transforms correctly when non root surfaces disabled
If non root surfaces are disabled, we do not clean the target ids of
transform nodes. It does not make sense to treat transforms from the
node to its target as draw transforms in this case.
This CL on one hand makes DrawTransform combine the transforms between
node to its true render target, on the other hand forces the property
tree to return screen space transforms as draw transforms if non root
surfaces are disabled.
BUG=624120
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/371b0a33409ef84a0d50276839bdb3fe35ac20ee
Cr-Commit-Position: refs/heads/master@{#410090}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Caller of DrawTransform does not need to consider non root sufaces conditions. #Patch Set 3 : ToTarget is called with a valid effect id. #
Total comments: 8
Patch Set 4 : Resolve comments #
Messages
Total messages: 28 (16 generated)
Description was changed from ========== cc: Compute draw transforms correctly when non root surfaces disabled If non root surfaces are disabled, we do not clean the target ids of transform nodes. It does not make sense to treat transforms from the node to its target as draw transforms in this case. This CL on one hand makes DrawTransform combine the transforms between node to its true render target, on the other hand forces the property tree to return screen space transforms as draw transforms if non root surfaces are disabled. BUG=624120 ========== to ========== cc: Compute draw transforms correctly when non root surfaces disabled If non root surfaces are disabled, we do not clean the target ids of transform nodes. It does not make sense to treat transforms from the node to its target as draw transforms in this case. This CL on one hand makes DrawTransform combine the transforms between node to its true render target, on the other hand forces the property tree to return screen space transforms as draw transforms if non root surfaces are disabled. BUG=624120 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sunxd@chromium.org changed reviewers: + ajuma@chromium.org, jaydasika@chromium.org, vollick@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2211113002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/2211113002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:1447: if (property_trees->non_root_surfaces_enabled) { Do we still need this branch if DrawTransform now does the right thing when non-root surfaces are disabled?
https://codereview.chromium.org/2211113002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/2211113002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:1447: if (property_trees->non_root_surfaces_enabled) { On 2016/08/04 19:58:30, ajuma (slow till Aug. 8) wrote: > Do we still need this branch if DrawTransform now does the right thing when > non-root surfaces are disabled? I think we can get rid of it.
Thanks, lgtm
On 2016/08/04 21:27:23, sunxd wrote: > https://codereview.chromium.org/2211113002/diff/1/cc/trees/draw_property_util... > File cc/trees/draw_property_utils.cc (right): > > https://codereview.chromium.org/2211113002/diff/1/cc/trees/draw_property_util... > cc/trees/draw_property_utils.cc:1447: if > (property_trees->non_root_surfaces_enabled) { > On 2016/08/04 19:58:30, ajuma (slow till Aug. 8) wrote: > > Do we still need this branch if DrawTransform now does the right thing when > > non-root surfaces are disabled? > > I think we can get rid of it. I'm trying to eliminate the usage of kInvalidNodeId when computing ToTarget, by using layer's render target effect tree id instead. So please ignore patch set 2.
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.... cc/trees/property_tree.cc:1804: if (node && node->id != transform_tree.kRootNodeId) Is |node| ever null here? Can we just use |transform_node_id|? https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.... cc/trees/property_tree.cc:1807: .ToTarget(transform_node_id, Hmm. Do we need to use ToTarget here? What about using ToParent instead? If any ancestor has a non-scale-or-translation transform, the code below should already ensure that we fail to compute animation scales, so I think we only need to handle the case of the node itself having a non-scale-or-translation. https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.... cc/trees/property_tree.cc:1809: ->render_target_effect_tree_index()) If we really do need this, it'd be better to pass it in rather than adding a dependency on LayerImpl. https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.... cc/trees/property_tree.cc:1878: transform_tree.ToTarget( I think we might be able to use ToScreen here (since the scale components of ToTarget and ToScreen should be the same).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.... cc/trees/property_tree.cc:1804: if (node && node->id != transform_tree.kRootNodeId) On 2016/08/04 22:55:58, ajuma (slow till Aug. 8) wrote: > Is |node| ever null here? Can we just use |transform_node_id|? Done. https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.... cc/trees/property_tree.cc:1807: .ToTarget(transform_node_id, On 2016/08/04 22:55:58, ajuma (slow till Aug. 8) wrote: > Hmm. Do we need to use ToTarget here? What about using ToParent instead? If any > ancestor has a non-scale-or-translation transform, the code below should already > ensure that we fail to compute animation scales, so I think we only need to > handle the case of the node itself having a non-scale-or-translation. Done. https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.... cc/trees/property_tree.cc:1809: ->render_target_effect_tree_index()) On 2016/08/04 22:55:58, ajuma (slow till Aug. 8) wrote: > If we really do need this, it'd be better to pass it in rather than adding a > dependency on LayerImpl. Done. https://codereview.chromium.org/2211113002/diff/40001/cc/trees/property_tree.... cc/trees/property_tree.cc:1878: transform_tree.ToTarget( On 2016/08/04 22:55:58, ajuma (slow till Aug. 8) wrote: > I think we might be able to use ToScreen here (since the scale components of > ToTarget and ToScreen should be the same). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! lgtm
The CQ bit was checked by sunxd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== cc: Compute draw transforms correctly when non root surfaces disabled If non root surfaces are disabled, we do not clean the target ids of transform nodes. It does not make sense to treat transforms from the node to its target as draw transforms in this case. This CL on one hand makes DrawTransform combine the transforms between node to its true render target, on the other hand forces the property tree to return screen space transforms as draw transforms if non root surfaces are disabled. BUG=624120 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Compute draw transforms correctly when non root surfaces disabled If non root surfaces are disabled, we do not clean the target ids of transform nodes. It does not make sense to treat transforms from the node to its target as draw transforms in this case. This CL on one hand makes DrawTransform combine the transforms between node to its true render target, on the other hand forces the property tree to return screen space transforms as draw transforms if non root surfaces are disabled. BUG=624120 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/371b0a33409ef84a0d50276839bdb3fe35ac20ee Cr-Commit-Position: refs/heads/master@{#410090} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/371b0a33409ef84a0d50276839bdb3fe35ac20ee Cr-Commit-Position: refs/heads/master@{#410090} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
