Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885233002:
cc : Create effect nodes for non axis aligned clips (Closed)
Descriptioncc : Create effect nodes for non axis aligned clips
We currently create effect node (and render surface) if there
is a clip and the transform to the previous render target is not axis
aligned. But, when we stop deciding whether to create a render surface
during property tree building, we won't know if the transform to the
previous render surface is axis aligned. With this CL, we create
effect nodes if thre is a non axis aligned transform between clips.
Transform to previous render surface might be axis aligned
even if there is some ancestor between the clips that has a non axis
aligned transform. This implies, we might create an unnecessary effect
node in that case.
BUG=723036
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2885233002
Cr-Commit-Position: refs/heads/master@{#474161}
Committed: https://chromium.googlesource.com/chromium/src/+/fbc951ded1697aef6fb274375740d89e66953d18
Patch Set 1 #Patch Set 2 : comment #
Total comments: 4
Patch Set 3 : rebase #Patch Set 4 : comments #Patch Set 5 : rebase #Patch Set 6 : rebase #Messages
Total messages: 27 (13 generated)
Description was changed from ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. Instead, with this CL, we check if the transform to screen is axis aligned. It is true that transform to previous render surface might be axis aligned even if the transform to screen is not. This implies, we might create an unnecessary effect node in that case. BUG=723036 ========== to ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. Instead, with this CL, we check if the transform to screen is axis aligned. It is true that transform to previous render surface might be axis aligned even if the transform to screen is not. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. Instead, with this CL, we check if the transform to screen is axis aligned. It is true that transform to previous render surface might be axis aligned even if the transform to screen is not. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. With this CL, we create effect nodes for all clips that have an ancestor with non axis aligned transform. Transform to previous render surface might be axis aligned even if there is some ancestor that has a non axis aligned transform. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. With this CL, we create effect nodes for all clips that have an ancestor with non axis aligned transform. Transform to previous render surface might be axis aligned even if there is some ancestor that has a non axis aligned transform. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. With this CL, we create effect nodes for all clips that have an ancestor with non axis aligned transform. Transform to previous render surface might be axis aligned even if there is some ancestor that has a non axis aligned transform. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
jaydasika@chromium.org changed reviewers: + chrishtr@chromium.org, enne@chromium.org
Since non axis aligned transforms are rare(?), I think this is fine. Let me know what you think about this.
Would it work to only allocate an effect node if the current effect node is in a non-axis-aligned space w.r.t a clip? That would avoid multiple effects in the presence of multiple clips that are together in a non-axis-aligned space.
Description was changed from ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. With this CL, we create effect nodes for all clips that have an ancestor with non axis aligned transform. Transform to previous render surface might be axis aligned even if there is some ancestor that has a non axis aligned transform. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. With this CL, we create effect nodes if thre is a non axis aligned transform between clips. Transform to previous render surface might be axis aligned even if there is some ancestor between the clips that has a non axis aligned transform. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/05/16 22:08:16, chrishtr wrote: > Would it work to only allocate an effect node if the current effect node is in > a non-axis-aligned space w.r.t a clip? That would avoid multiple effects in the > presence of multiple clips that are together in a non-axis-aligned space. Done
Also, needs a test. https://codereview.chromium.org/2885233002/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2885233002/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:899: !Transform(layer).Preserves2dAxisAlignment(); What about transforms that don't have effects?
Will add a test https://codereview.chromium.org/2885233002/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2885233002/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:899: !Transform(layer).Preserves2dAxisAlignment(); On 2017/05/16 23:36:42, chrishtr wrote: > What about transforms that don't have effects? The recursion variable not_axis_aligned_since_last_clip is updated for every layer, not just for effects. So, if there is a non-axis aligned transform with no effect, data_for_children->not_axis_aligned_since_last_clip should be true for descendants. Am I missing something ?
https://codereview.chromium.org/2885233002/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2885233002/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:899: !Transform(layer).Preserves2dAxisAlignment(); On 2017/05/16 at 23:46:28, jaydasika wrote: > On 2017/05/16 23:36:42, chrishtr wrote: > > What about transforms that don't have effects? > > The recursion variable not_axis_aligned_since_last_clip is updated for every layer, not just for effects. So, if there is a non-axis aligned transform with no effect, data_for_children->not_axis_aligned_since_last_clip should be true for descendants. Am I missing something ? Oh I see. AddEffectNodeIfNeeded is called unconditionally. It seems it would be cleaner to update for children outside of this method, since it is not specific to effect nodes.
Added a test https://codereview.chromium.org/2885233002/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2885233002/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:899: !Transform(layer).Preserves2dAxisAlignment(); On 2017/05/17 00:44:49, chrishtr wrote: > On 2017/05/16 at 23:46:28, jaydasika wrote: > > On 2017/05/16 23:36:42, chrishtr wrote: > > > What about transforms that don't have effects? > > > > The recursion variable not_axis_aligned_since_last_clip is updated for every > layer, not just for effects. So, if there is a non-axis aligned transform with > no effect, data_for_children->not_axis_aligned_since_last_clip should be true > for descendants. Am I missing something ? > > Oh I see. AddEffectNodeIfNeeded is called unconditionally. It seems it > would be cleaner to update for children outside of this method, since it > is not specific to effect nodes. The reason I put it here is to avoid duplicating the code. Since we create an effect node for this, this code has to exist here also. I added it to property tree builder too to update the children.
jaydasika@chromium.org changed reviewers: + weiliangc@chromium.org
enne / weiliangc : Since chrishtr@ is OOO, can one of you takeover this review ?
I think this looks reasonable to me and fits in with the rest of the effect node building code. If chrishtr has more feedback after he returns from vacation, we can land some follow up patches. lgtm
The CQ bit was checked by jaydasika@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/2885233002/#ps80001 (title: "rebase")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The failing tests should be fixed with https://codereview.chromium.org/2900153002/
The CQ bit was checked by jaydasika@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/2885233002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495586923734750,
"parent_rev": "e449acf2a251e5130b41c2e69ab1bc7b6a534b48", "commit_rev":
"fbc951ded1697aef6fb274375740d89e66953d18"}
Message was sent while issue was closed.
Description was changed from ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. With this CL, we create effect nodes if thre is a non axis aligned transform between clips. Transform to previous render surface might be axis aligned even if there is some ancestor between the clips that has a non axis aligned transform. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Create effect nodes for non axis aligned clips We currently create effect node (and render surface) if there is a clip and the transform to the previous render target is not axis aligned. But, when we stop deciding whether to create a render surface during property tree building, we won't know if the transform to the previous render surface is axis aligned. With this CL, we create effect nodes if thre is a non axis aligned transform between clips. Transform to previous render surface might be axis aligned even if there is some ancestor between the clips that has a non axis aligned transform. This implies, we might create an unnecessary effect node in that case. BUG=723036 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2885233002 Cr-Commit-Position: refs/heads/master@{#474161} Committed: https://chromium.googlesource.com/chromium/src/+/fbc951ded1697aef6fb274375740... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/fbc951ded1697aef6fb274375740... |
