Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714673002:
[SPv2] Implement effect compositing for indirect reasons (Closed)
|
Created:
3 years, 10 months ago by trchen Modified:
3 years, 9 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, fmalita+watch_chromium.org, jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[SPv2] Implement effect compositing for indirect reasons
The idea is to make PaintArtifactCompositor::collectPendingLayers a recursive
algorithm to keep track of the scope of effects in the chunk list. We make a
recursive call whenever an effect is entered, and return to caller whenever
an effect exits. Pending layers generated in a recursion are only allowed to
merge into previous layers generated in the same recursion, as if effects
always have a compositing reason. So far this becomes a very conservative
algorithm that would zealously composite effects for correctness.
When a recursion returned to its caller, the caller would examine the pending
layers generated by the callee and potentially peform two transformation:
1. If only one layer is generated, and the layer is compatible to the effect's
input space, i.e. no compositing reasons in between two spaces, and the effect
itself doesn't have a direct compositing reason, then "decomposite" the effect.
2. If the above transformation is done, then the "decomposited" pending layer
can be seen as a regular paint chunk that paints into the current effect. Thus
may be merged into a previous layer as if it is just a chunk.
BUG=683425
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2714673002
Cr-Commit-Position: refs/heads/master@{#457620}
Committed: https://chromium.googlesource.com/chromium/src/+/d07c15ef0a9b942de053a5eb8fba1547972d6f23
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix one crash bug & update tests & added unit tests #
Total comments: 22
Patch Set 3 : address review comments #
Total comments: 12
Patch Set 4 : address feedbacks up to #16 #Patch Set 5 : rebase & rebaselined #
Total comments: 1
Patch Set 6 : mark tests 589265 #
Depends on Patchset: Messages
Total messages: 32 (15 generated)
Description was changed from ========== [SPv2] Implement effect compositing for indirect reasons This is a WIP, not ready for full review yet. The idea is to make PaintArtifactCompositor::collectPendingLayers a recursive algorithm to keep track of the scope of effects in the chunk list. We make a recursive call whenever an effect is entered, and return to caller whenever an effect exits. Pending layers generated in a recursion are only allowed to merge into previous layers generated in the same recursion, as if effects always have a compositing reason. So far this becomes a very conservative algorithm that would zealously composite effects for correctness. When a recursion returned to its caller, the caller would examine the pending layers generated by the callee and potentially peform two transformation: 1. If only one layer is generated, and the layer is compatible to the effect's input space, i.e. no compositing reasons in between two spaces, and the effect itself doesn't have a direct compositing reason, then "decomposite" the effect. 2. If the above transformation is done, then the "decomposited" pending layer can be seen as a regular paint chunk that paints into the current effect. Thus may be merged into a previous layer as if it is just a chunk. BUG=683425 ========== to ========== [SPv2] Implement effect compositing for indirect reasons This is a WIP, not ready for full review yet. The idea is to make PaintArtifactCompositor::collectPendingLayers a recursive algorithm to keep track of the scope of effects in the chunk list. We make a recursive call whenever an effect is entered, and return to caller whenever an effect exits. Pending layers generated in a recursion are only allowed to merge into previous layers generated in the same recursion, as if effects always have a compositing reason. So far this becomes a very conservative algorithm that would zealously composite effects for correctness. When a recursion returned to its caller, the caller would examine the pending layers generated by the callee and potentially peform two transformation: 1. If only one layer is generated, and the layer is compatible to the effect's input space, i.e. no compositing reasons in between two spaces, and the effect itself doesn't have a direct compositing reason, then "decomposite" the effect. 2. If the above transformation is done, then the "decomposited" pending layer can be seen as a regular paint chunk that paints into the current effect. Thus may be merged into a previous layer as if it is just a chunk. BUG=683425 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
trchen@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
Here is a quick preview for the effect grouping fix. My local test run showed another ~300 layout tests passing. (Many of them are sub-pixel failures due to layerization differences. A few are due to grouping correctness.) Exciting eh? :D
Great work Tien-Ren! https://codereview.chromium.org/2714673002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2714673002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:716: // Look at the effect node of the next chunk. There are 3 possible cases: Do you have an intuition about the frequency of each of these cases? https://codereview.chromium.org/2714673002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:753: !canUpcastTo(subgroupLayer.propertyTreeState, Can you add a note about the performance of this? I think you're taking the right approach by getting the code working first and optimizing later (maybe later patches), just want to note it in a comment so we don't forget.
I just realized one weird thing. Although the grouping of composited effects will be correct, PaintArtifactCompositor::recordPendingLayer() doesn't group non-composited effects yet. I shall double check if this CL actually worked as intended... https://codereview.chromium.org/2714673002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2714673002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:716: // Look at the effect node of the next chunk. There are 3 possible cases: On 2017/02/23 06:05:18, pdr. wrote: > Do you have an intuition about the frequency of each of these cases? Case A and B always happen in pair, and should be as frequent as the ratio between effect nodes / total paint chunks. For most pages case C should be the majority. Now you mentioned it, I think I should lift case C to the top, according to the style guide (shorter branch goes first). https://codereview.chromium.org/2714673002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:753: !canUpcastTo(subgroupLayer.propertyTreeState, On 2017/02/23 06:05:18, pdr. wrote: > Can you add a note about the performance of this? I think you're taking the > right approach by getting the code working first and optimizing later (maybe > later patches), just want to note it in a comment so we don't forget. Acknowledged.
Description was changed from ========== [SPv2] Implement effect compositing for indirect reasons This is a WIP, not ready for full review yet. The idea is to make PaintArtifactCompositor::collectPendingLayers a recursive algorithm to keep track of the scope of effects in the chunk list. We make a recursive call whenever an effect is entered, and return to caller whenever an effect exits. Pending layers generated in a recursion are only allowed to merge into previous layers generated in the same recursion, as if effects always have a compositing reason. So far this becomes a very conservative algorithm that would zealously composite effects for correctness. When a recursion returned to its caller, the caller would examine the pending layers generated by the callee and potentially peform two transformation: 1. If only one layer is generated, and the layer is compatible to the effect's input space, i.e. no compositing reasons in between two spaces, and the effect itself doesn't have a direct compositing reason, then "decomposite" the effect. 2. If the above transformation is done, then the "decomposited" pending layer can be seen as a regular paint chunk that paints into the current effect. Thus may be merged into a previous layer as if it is just a chunk. BUG=683425 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPv2] Implement effect compositing for indirect reasons The idea is to make PaintArtifactCompositor::collectPendingLayers a recursive algorithm to keep track of the scope of effects in the chunk list. We make a recursive call whenever an effect is entered, and return to caller whenever an effect exits. Pending layers generated in a recursion are only allowed to merge into previous layers generated in the same recursion, as if effects always have a compositing reason. So far this becomes a very conservative algorithm that would zealously composite effects for correctness. When a recursion returned to its caller, the caller would examine the pending layers generated by the callee and potentially peform two transformation: 1. If only one layer is generated, and the layer is compatible to the effect's input space, i.e. no compositing reasons in between two spaces, and the effect itself doesn't have a direct compositing reason, then "decomposite" the effect. 2. If the above transformation is done, then the "decomposited" pending layer can be seen as a regular paint chunk that paints into the current effect. Thus may be merged into a previous layer as if it is just a chunk. BUG=683425 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by trchen@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...
Ready for full review. :) A few unit tests probe into PendingLayer internals, but those probes no longer works because I made canMergeInto and mightOverlap static functions. I think it is okay because I feel tests don't need to probe into such implementation internals. Please let me know if you feel strongly about it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some comments. As we discussed offline, not sure if this is the simplest approach, still thinking it through. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:588: .localToAncestorVisualRect( This is a change (used to be localToAncestorRect). Is it unittested? https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:603: // knownToBeOpaque = knownToBeOpaque && enclosingRect(mappedRegion) == Are you trying to detect exact integer bounds here? https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:628: if (!isNonCompositingAncestorOf(home.transform(), Won't the check on line 633 handle all of these? https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:724: // composited as well. Explain why. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:761: if (!candidateLayer.isForeign && I think it would be clearer to move these conditions back into a canMergeInto method similar to what existed before this CL. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h (right): https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:99: void upcast(const PropertyTreeState&, GeometryMapper&); Document these methods. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:149: static void layerizeGroup(const PaintArtifact&, Document this method. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp (right): https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:608: RefPtr<EffectPaintPropertyNode> effect1 = EffectPaintPropertyNode::create( Why can't you use createOpacityOnlyEffect and add an argument to specify compositing reasons?
https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:588: .localToAncestorVisualRect( On 2017/02/28 22:50:35, chrishtr wrote: > This is a change (used to be localToAncestorRect). Is it unittested? Good point. I shall add a test. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:603: // knownToBeOpaque = knownToBeOpaque && enclosingRect(mappedRegion) == On 2017/02/28 22:50:33, chrishtr wrote: > Are you trying to detect exact integer bounds here? Sorry my comment was confusing. By enclosedRect I meant a rect that is guaranteed to be a subset of the transformed region, for the purpose of determining opaque rect. For rectilinear transforms it is trivial. For transforms that outputs a quad there is no unique best answer (and I think using empty rect is fine in this case). https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:628: if (!isNonCompositingAncestorOf(home.transform(), On 2017/02/28 22:50:35, chrishtr wrote: > Won't the check on line 633 handle all of these? Unfortunately not. Sometimes clips applied to a chunk can have deeper transform node than the chunk. One notable example is CSS clip with fixed-pos child: <div style="overflow:scroll: position:relative;"> <div style="position:absolute; clip:rect(1px,2px,3px,4px);"> <div style="position:fixed;">Foo</div> </div> </div> And worse, we could have transform space inversion: <div style="overflow:scroll: position:relative;"> <div style="position:absolute; clip:rect(1px,2px,3px,4px);"> <div style="position:fixed; overflow:hidden;">Ouch</div> </div> </div> Another example would be CSS mask (which creates an implicit clip) with scroll escaping: <div style="overflow:scroll"> <div style="mask:url('foo')"> <div style="position:absolute">foo</div> </div> </div> https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:724: // composited as well. On 2017/02/28 22:50:33, chrishtr wrote: > Explain why. Acknowledged. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:761: if (!candidateLayer.isForeign && On 2017/02/28 22:50:35, chrishtr wrote: > I think it would be clearer to move these conditions back into a canMergeInto > method > similar to what existed before this CL. How about making it PendingLayer::canMerge(const PendingLayer& guest)? https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h (right): https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:99: void upcast(const PropertyTreeState&, GeometryMapper&); On 2017/02/28 22:50:35, chrishtr wrote: > Document these methods. Acknowledged. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:149: static void layerizeGroup(const PaintArtifact&, On 2017/02/28 22:50:35, chrishtr wrote: > Document this method. Acknowledged. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp (right): https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:608: RefPtr<EffectPaintPropertyNode> effect1 = EffectPaintPropertyNode::create( On 2017/02/28 22:50:36, chrishtr wrote: > Why can't you use createOpacityOnlyEffect and add an argument to specify > compositing > reasons? Actually I'm thinking to abuse designated initializer to implement optional keyword arguments. Pattern like this: class EffectPaintPropertyNode { public: struct Params { PassRefPtr<EffectPaintPropertyNode> parent = nullptr; float opacity = 1.0; SkBlendMode blendMode = kSrcOver; CompositorFilterOperation filter; CompositingReasons compositingReasons = CompositingReasonNone; ... }; PassRefPtr<EffectPaintPropertyNode> create(const Params&); }; void func() { RefPtr<EffectPaintPropertyNode> newNode = EffectPaintPropertyNode::create({.parent = EffectPaintPropertyNode::root(), .opacity = 0.5, .compositingReasons = CompositingReasonAll}); } The style guide doesn't say whether it's approved or not, but I don't see any obvious foot gun.
https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:588: .localToAncestorVisualRect( On 2017/03/01 01:26:11, trchen wrote: > On 2017/02/28 22:50:35, chrishtr wrote: > > This is a change (used to be localToAncestorRect). Is it unittested? > > Good point. I shall add a test. Done. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:603: // knownToBeOpaque = knownToBeOpaque && enclosingRect(mappedRegion) == Comments clarified. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:724: // composited as well. On 2017/03/01 01:26:11, trchen wrote: > On 2017/02/28 22:50:33, chrishtr wrote: > > Explain why. > > Acknowledged. Done. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:761: if (!candidateLayer.isForeign && On 2017/03/01 01:26:11, trchen wrote: > On 2017/02/28 22:50:35, chrishtr wrote: > > I think it would be clearer to move these conditions back into a canMergeInto > > method > > similar to what existed before this CL. > > How about making it PendingLayer::canMerge(const PendingLayer& guest)? Done. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h (right): https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:99: void upcast(const PropertyTreeState&, GeometryMapper&); On 2017/03/01 01:26:11, trchen wrote: > On 2017/02/28 22:50:35, chrishtr wrote: > > Document these methods. > > Acknowledged. Done. https://codereview.chromium.org/2714673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:149: static void layerizeGroup(const PaintArtifact&, On 2017/03/01 01:26:11, trchen wrote: > On 2017/02/28 22:50:35, chrishtr wrote: > > Document this method. > > Acknowledged. Done.
After much debate with myself, I think we should proceed with this patch. It's a good approach. Thanks for your patience. More comments here and to come. https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:641: static bool canUpcastTo(const PropertyTreeState& guest, Document this method. https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:660: static const EffectPaintPropertyNode* strictAncestorOf( strictChildOfAlongPath ? https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:743: // Now the chunk iterator stepped over the subgroup we just saw. What about a 3d composited child? https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:750: if (pendingLayers.size() != firstLayerInSubgroup + 1) Please factor these conditions into a helper method.
Could you also add a unittest with a case like that described in https://codereview.chromium.org/2729613002/ ? https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp (left): https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:1553: TEST_F(PaintArtifactCompositorTestWithPropertyTrees, MightOverlap) { Why did you delete the tests for mightOverlap? https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:1588: TEST_F(PaintArtifactCompositorTestWithPropertyTrees, PendingLayer) { The PendingLayer tests seem still relevant, in particular whether it sets the opaque etc. bits correctly.
Pretty much done! Still need to rebaseline FlagExpectations/enable-slimming-paint-v2, will upload another one soon. > Could you also add a unittest with a case like that described in > https://codereview.chromium.org/2729613002/ ? Done. https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:641: static bool canUpcastTo(const PropertyTreeState& guest, On 2017/03/09 01:27:00, chrishtr wrote: > Document this method. Done. https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:660: static const EffectPaintPropertyNode* strictAncestorOf( On 2017/03/09 01:27:00, chrishtr wrote: > strictChildOfAlongPath ? Done. https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:743: // Now the chunk iterator stepped over the subgroup we just saw. On 2017/03/09 01:27:00, chrishtr wrote: > What about a 3d composited child? Hmmm that's a good point. 3D transformed children themselves are fine, but nested 3D context may cause a problem. For example: <div style="transform-style:preserve-3d;"> <div style="transform:*;">A</div> <div style="transform:*;"> <div style="transform-style:preserve-3d;"> <div style="transform:*;">B</div> <div style="transform:*;">C</div> <div style="transform:*;">D</div> </div> </div> <div style="transform:*;">E</div> </div>> The semantics of above is that B,C,D are 3D-sorted, and A,BCD,E are 3D-sorted, where BCD is the flattened result of the former. BCD needs to have a render surface[1] to represent it. There is a TODO in PropertyTreeBuilder to generate effect node when such case is detected, but not implemented yet. [1] Which I think is unnecessary. An effect node without render surface should be good enough. All we need is some improvement in DirectRenderer::DrawRenderPass(). https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:750: if (pendingLayers.size() != firstLayerInSubgroup + 1) On 2017/03/09 01:27:00, chrishtr wrote: > Please factor these conditions into a helper method. Done. https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp (left): https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:1553: TEST_F(PaintArtifactCompositorTestWithPropertyTrees, MightOverlap) { On 2017/03/09 01:41:01, chrishtr wrote: > Why did you delete the tests for mightOverlap? Was thinking to hide more internal details. Reverted. https://codereview.chromium.org/2714673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:1588: TEST_F(PaintArtifactCompositorTestWithPropertyTrees, PendingLayer) { On 2017/03/09 01:41:01, chrishtr wrote: > The PendingLayer tests seem still relevant, in particular whether it sets the > opaque etc. bits correctly. That makes sense. I'll resurrect these tests. Though I'll have to disable them because the old PendingLayer::add() didn't take non-composited clip into consideration, and the newly added PendingLayer::merge() has a TODO in there.
The CQ bit was checked by trchen@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/2714673002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 (right): https://codereview.chromium.org/2714673002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1320: Bug(none) animations/animated-filter-svg-element.html [ Failure ] What is the cause of this failure and the ones below? Were they failing before? Please also file a bug associated with them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/16 00:36:39, chrishtr wrote: > https://codereview.chromium.org/2714673002/diff/80001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 > (right): > > https://codereview.chromium.org/2714673002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1320: > Bug(none) animations/animated-filter-svg-element.html [ Failure ] > What is the cause of this failure and the ones below? Were they failing > before? > > Please also file a bug associated with them. I confirmed all of them are rounding differences, likely due to some subtle differences in layerization. There is a generic crbug for this class of failures: https://crbug.com/589265 Basically all it says is that some rounding differences are expected, while some may be real bugs. We'll investigate when layerization algorithm starts to stabilize.
On 2017/03/16 at 19:11:46, trchen wrote: > On 2017/03/16 00:36:39, chrishtr wrote: > > https://codereview.chromium.org/2714673002/diff/80001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 > > (right): > > > > https://codereview.chromium.org/2714673002/diff/80001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1320: > > Bug(none) animations/animated-filter-svg-element.html [ Failure ] > > What is the cause of this failure and the ones below? Were they failing > > before? > > > > Please also file a bug associated with them. > > I confirmed all of them are rounding differences, likely due to some subtle differences in layerization. There is a generic crbug for this class of failures: https://crbug.com/589265 > Basically all it says is that some rounding differences are expected, while some may be real bugs. We'll investigate when layerization algorithm starts to stabilize. OK, please change Bug(none) to crbug.com/589265 for these then. Thanks for checking.
lgtm Looks good modulo that last change.
On 2017/03/16 20:00:16, chrishtr wrote: > OK, please change Bug(none) to crbug.com/589265 for these then. Thanks for > checking. Done.
The CQ bit was checked by trchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/2714673002/#ps100001 (title: "mark tests 589265")
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": 1489700044336420,
"parent_rev": "f0e2fb482636b0c1aed01f2d672ec5619bff9eb9", "commit_rev":
"d07c15ef0a9b942de053a5eb8fba1547972d6f23"}
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Implement effect compositing for indirect reasons The idea is to make PaintArtifactCompositor::collectPendingLayers a recursive algorithm to keep track of the scope of effects in the chunk list. We make a recursive call whenever an effect is entered, and return to caller whenever an effect exits. Pending layers generated in a recursion are only allowed to merge into previous layers generated in the same recursion, as if effects always have a compositing reason. So far this becomes a very conservative algorithm that would zealously composite effects for correctness. When a recursion returned to its caller, the caller would examine the pending layers generated by the callee and potentially peform two transformation: 1. If only one layer is generated, and the layer is compatible to the effect's input space, i.e. no compositing reasons in between two spaces, and the effect itself doesn't have a direct compositing reason, then "decomposite" the effect. 2. If the above transformation is done, then the "decomposited" pending layer can be seen as a regular paint chunk that paints into the current effect. Thus may be merged into a previous layer as if it is just a chunk. BUG=683425 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPv2] Implement effect compositing for indirect reasons The idea is to make PaintArtifactCompositor::collectPendingLayers a recursive algorithm to keep track of the scope of effects in the chunk list. We make a recursive call whenever an effect is entered, and return to caller whenever an effect exits. Pending layers generated in a recursion are only allowed to merge into previous layers generated in the same recursion, as if effects always have a compositing reason. So far this becomes a very conservative algorithm that would zealously composite effects for correctness. When a recursion returned to its caller, the caller would examine the pending layers generated by the callee and potentially peform two transformation: 1. If only one layer is generated, and the layer is compatible to the effect's input space, i.e. no compositing reasons in between two spaces, and the effect itself doesn't have a direct compositing reason, then "decomposite" the effect. 2. If the above transformation is done, then the "decomposited" pending layer can be seen as a regular paint chunk that paints into the current effect. Thus may be merged into a previous layer as if it is just a chunk. BUG=683425 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2714673002 Cr-Commit-Position: refs/heads/master@{#457620} Committed: https://chromium.googlesource.com/chromium/src/+/d07c15ef0a9b942de053a5eb8fba... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d07c15ef0a9b942de053a5eb8fba... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
