Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581843002:
Implement merging non-composited paint property nodes in the PACompositor. (Closed)
|
Created:
4 years ago by chrishtr Modified:
3 years, 11 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, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement merging non-composited paint property nodes in the PACompositor.
Currently, the PaintArtifactCompositor naively puts every PaintChunk
into its own composited layer. This patch improves on that by merging
a PaintChunk into the same layer as the PaintChunk which precedes it,
if the new PaintChunk has a compatible PropertyTreeState (no directly
composited property tree nodes between the state of the new PaintChunk
and the old one).
This algorithm is spelled out in a design doc, see crbug.com/668342.
A large amount of this patch is additional tests and testing machinery
to support the change in behavior.
BUG=668342
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/7239114dd9ee5270ff6ac3e066113b34814f777d
Cr-Commit-Position: refs/heads/master@{#440775}
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : none #Patch Set 4 : none #Patch Set 5 : none #
Total comments: 48
Patch Set 6 : none #Patch Set 7 : none #Patch Set 8 : ' #Patch Set 9 : none #Patch Set 10 : none #Patch Set 11 : none #
Total comments: 15
Patch Set 12 : none #Patch Set 13 : none #Patch Set 14 : none #Patch Set 15 : none #Patch Set 16 : none #
Total comments: 8
Patch Set 17 : none #Patch Set 18 : none #Patch Set 19 : none #Patch Set 20 : none #Patch Set 21 : none #
Total comments: 4
Patch Set 22 : none #Patch Set 23 : none #
Total comments: 6
Patch Set 24 : none #
Total comments: 1
Patch Set 25 : none #Patch Set 26 : none #Messages
Total messages: 94 (65 generated)
Description was changed from ========== none none none BUG= ========== to ========== none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@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...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by chrishtr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Implement merging non-composited paint property nodes in the PACompositor. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
chrishtr@chromium.org changed reviewers: + pdr@chromium.org, trchen@chromium.org, wkorman@chromium.org
The CQ bit was checked by chrishtr@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...
The CQ bit was checked by chrishtr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Implement merging non-composited paint property nodes in the PACompositor. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own Layer. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own Layer. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. A large amount of this patch is additional tests and testing machinery to support the change in behavior. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
lgtm https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:319: extra line https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:417: recordPendingLayer(paintArtifact, pendingLayer, ccCombinedBounds); Possible to have ccCombinedBounds be an empty rect here, if so should we early-out/skip or otherwise avoid creating a cc::Layer at all? https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:441: std::max(0, rect.width()), Edge cases we're guarding against with max() here and in next line could create an empty rect. If that happens, should we skip some of the below? https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:897: // TODO(chrishtr): implement Mentioned to pdr@ already, we should consider how to split out the logic growing in PAC into separate classes for maintainability. When we add overlap implementation could be a good early case. Could do similarly to how we moved paint logic out of LayoutObject, perhaps, with a stack-alloc'd delegate class, or just moving static methods to separate class. Could make unit testing easier. Think about it! https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:921: pendingLayers.append(PendingLayer(paintChunk)); Does this move, or copy, the new PendingLayer instance? Move preferred, thought worth checking. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:946: Vector<PendingLayer> pendingLayers; Wondered whether there is optimization value to not allocating the initial vector until/unless we know there will be something added to it. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:86: PendingLayer(const PaintChunk& firstPaintChunk) OK to put the impl for these methods in the .cpp file just for future maintenance ease? Or do we do it this way for structs, which would make me sad. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:133: const PendingLayer& x, Remove 'x', or use more meaningful name? https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:154: FRIEND_TEST_ALL_PREFIXES(PaintArtifactCompositorTestWithPropertyTrees, I thought there was a way to have one line do this for all methods in a test class, but I can't find it, so I may be imagining things.
https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:337: gfx::Transform translation; I don't understand the paired begin/end display items concept in this function. Is it supporting non-composited effects? Which tests cover this? https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:905: for (const PaintChunk& paintChunk : paintChunks) { What do you think about the performance of this function? I think it will be at least O(n^2) in the case of n independently transformed layers (sort of the "famous.co" case). https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:907: for (Vector<PendingLayer>::reverse_iterator candidatePendingLayer = Could this candidate layer loop could be skipped if the paint chunk has immediate direct compositing reasons that prevent it from being merged with other layers? https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:29: class GeometryMapper; Can you writeup a high-level description of the overall approach in a README.md file and reference it here? https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:109: void collectPendingLayers(const Vector<PaintChunk>& paintChunks, Nit: Just return Vector<PendingLayer>: Vector<PendingLayer> collectPendingLayers(const Vector<PaintChunk>&) const; Can you add a short comment here as well? Something about how this is the secret formula for grouping paint chunks into pending layers. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:1003: Can you add a test like PaintPropertyTreeBuilderTest.CSSClipFixedPositionDescendantNonShared which has clip escaping? https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:82: InnermostNode innermostNode() const; Please add a comment describing this and the iterator below. It looks like innermostNode is sort of the current "lowest common ancestor" in the property tree, and you're using that to define an iteration order for the PropertyTreeStateIterator to traverse up the tree--is that correct? https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeStateTest.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeStateTest.cpp:32: state.setTransform(transform.get()); Are you creating the PropertyTreeState and then calling setTransform/setClip/setEffect to avoid the DCHECKs about hasOneRef? If so, lets just remove those DCHECKs or add a test-only helper to create PropertyTreeStates.
https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:259: static void recordPairedBeginDisplayItems( Can you add a todo(chrishtr) comment about the grouping special sauce here? The algorithm that groups chunks with the same property tree state. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:337: gfx::Transform translation; On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > I don't understand the paired begin/end display items concept in this function. Is it supporting non-composited effects? Which tests cover this? After discussion, I now get this. It's just the code that turns non-composited property tree nodes into display items. It uses a push and pop algorithm with the property tree state iteration pattern to ensure the nesting is correct. I think we're still missing tests though. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:905: for (const PaintChunk& paintChunk : paintChunks) { On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > What do you think about the performance of this function? I think it will be at least O(n^2) in the case of n independently transformed layers (sort of the "famous.co" case). We discussed this offline and other folks were aware of this. Can you please add a comment here about optimizing this in the future? I agree 100% about moving forward with this simple approach and optimizing later. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:96: class PLATFORM_EXPORT PropertyTreeStateIterator { Can you add a comment or file a bug about the invariant asserts we talked about? Basically, assert here (or in the PropertyTreeState ctor) that: a) the clip's transform is equal to or lower than the current transform b) the effect's clip is equal to or lower than the current clip c) the effect's transform is equal to or lower than the current transform I think the iteration algorithm is clever and clean, but it's still a lot to page into a brain. Heavily asserting these core invariants will save future refactorers from frighteningly tricky bugs.
https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:319: On 2016/12/20 at 00:28:56, wkorman wrote: > extra line Fixed. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:337: gfx::Transform translation; On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > I don't understand the paired begin/end display items concept in this function. Is it supporting non-composited effects? Which tests cover this? Explained in person. The tests MergeSimpleChunks, MergeClip, MergeTransform and MergeNested explicitly exercise this code and test that it's done correctly. There are also tests of PropertyTreeStateIterator and the PropertyTreeState::innermostNode() method, which is another critical piece of the algorithm. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:417: recordPendingLayer(paintArtifact, pendingLayer, ccCombinedBounds); On 2016/12/20 at 00:28:56, wkorman wrote: > Possible to have ccCombinedBounds be an empty rect here, if so should we early-out/skip or otherwise avoid creating a cc::Layer at all? It could, but that seems a premature optimization at this point in the implementation cycle. I prefer to keep this simple for now. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:441: std::max(0, rect.width()), On 2016/12/20 at 00:28:56, wkorman wrote: > Edge cases we're guarding against with max() here and in next line could create an empty rect. If that happens, should we skip some of the below? Optimized. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:905: for (const PaintChunk& paintChunk : paintChunks) { On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > What do you think about the performance of this function? I think it will be at least O(n^2) in the case of n independently transformed layers (sort of the "famous.co" case). Added a comment about n^3 etc. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:921: pendingLayers.append(PendingLayer(paintChunk)); On 2016/12/20 at 00:28:56, wkorman wrote: > Does this move, or copy, the new PendingLayer instance? Move preferred, thought worth checking. Copies. I ran into some complications when attempting the move constructor, prefer to do that in another patch. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:946: Vector<PendingLayer> pendingLayers; On 2016/12/20 at 00:28:56, wkorman wrote: > Wondered whether there is optimization value to not allocating the initial vector until/unless we know there will be something added to it. Done. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:86: PendingLayer(const PaintChunk& firstPaintChunk) On 2016/12/20 at 00:28:56, wkorman wrote: > OK to put the impl for these methods in the .cpp file just for future maintenance ease? Or do we do it this way for structs, which would make me sad. Done. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:109: void collectPendingLayers(const Vector<PaintChunk>& paintChunks, On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > Nit: Just return Vector<PendingLayer>: > Vector<PendingLayer> collectPendingLayers(const Vector<PaintChunk>&) const; Returning a vector will cause a vector copy operation, no? I'd rather not pay that penalty. > Can you add a short comment here as well? Something about how this is the secret formula for grouping paint chunks into pending layers. Done. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:133: const PendingLayer& x, On 2016/12/20 at 00:28:56, wkorman wrote: > Remove 'x', or use more meaningful name? Done. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:154: FRIEND_TEST_ALL_PREFIXES(PaintArtifactCompositorTestWithPropertyTrees, On 2016/12/20 at 00:28:56, wkorman wrote: > I thought there was a way to have one line do this for all methods in a test class, but I can't find it, so I may be imagining things. I don't know of one either. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:82: InnermostNode innermostNode() const; On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > Please add a comment describing this and the iterator below. > > It looks like innermostNode is sort of the current "lowest common ancestor" in the property tree, and you're using that to define an iteration order for the PropertyTreeStateIterator to traverse up the tree--is that correct? Done. The innermostNode is the first of the transform, clip or effect to be applied. There is always a total order on them. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeStateTest.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeStateTest.cpp:32: state.setTransform(transform.get()); On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > Are you creating the PropertyTreeState and then calling setTransform/setClip/setEffect to avoid the DCHECKs about hasOneRef? If so, lets just remove those DCHECKs or add a test-only helper to create PropertyTreeStates. No good reason, except that in an earlier iteration of the test I made use of it. Fixed.
https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:259: static void recordPairedBeginDisplayItems( On 2016/12/20 at 22:50:58, pdr (OOO Dec 23 to Jan 2) wrote: > Can you add a todo(chrishtr) comment about the grouping special sauce here? The algorithm that groups chunks with the same property tree state. Done. Added in recordPendingLayer. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:337: gfx::Transform translation; On 2016/12/20 at 22:50:58, pdr (OOO Dec 23 to Jan 2) wrote: > On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > > I don't understand the paired begin/end display items concept in this function. Is it supporting non-composited effects? Which tests cover this? > > After discussion, I now get this. It's just the code that turns non-composited property tree nodes into display items. It uses a push and pop algorithm with the property tree state iteration pattern to ensure the nesting is correct. > > I think we're still missing tests though. Agreed offline no extra test needed. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:1003: On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > Can you add a test like PaintPropertyTreeBuilderTest.CSSClipFixedPositionDescendantNonShared which has clip escaping? Per offline discussion, not needed. I did add a comment to PictureMatchers.h https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:96: class PLATFORM_EXPORT PropertyTreeStateIterator { On 2016/12/20 at 22:50:58, pdr (OOO Dec 23 to Jan 2) wrote: > Can you add a comment or file a bug about the invariant asserts we talked about? Basically, assert here (or in the PropertyTreeState ctor) that: > a) the clip's transform is equal to or lower than the current transform > b) the effect's clip is equal to or lower than the current clip > c) the effect's transform is equal to or lower than the current transform > > I think the iteration algorithm is clever and clean, but it's still a lot to page into a brain. Heavily asserting these core invariants will save future refactorers from frighteningly tricky bugs. These asserts are a good idea, will add them in another patch.
https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:9: bool PropertyTreeState::hasDirectCompositingReasons() const { This feels a weird place to me. IMO the concept of direct compositing reason doesn't apply to chunks. A chunk gets a reason not to be squashed into a previous chunk because one of its property node cross an edge that has direct compositing reason. For example: <div>A</div> <div style="transform:translateZ(0);"> <div style="transform:translate(0,0);">B</div> </div> Chunk B points to no nodes that have a direct compositing reason, but chunk B still needs to be composited separately because of the 3D parent. For another example: <div style="transform:translateZ(0);"> <div style="opacity:0.5">A</div> <div style="opacity:0.5">B</div> </div> A and B both points to a directly composited transform node, but there's no reason why they need to be composited separately. I think the algorithm should roughly look like this: bool shouldBeBackedSeparately(const TransformNode* a, const TransformNode* b) { if (depth(a) > depth(b)) swap(a, b); for(;depth(b) > depth(a); b = b->parent) { if (b->hasDirectCompositingReason()) return true; } for (;a != b; a = a->parent, b = b->parent) if (a->hasDirectCompositingReason() || b->hasDirectCompositingReason()) return true; return false; } https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:22: PropertyTreeState::InnermostNode PropertyTreeState::innermostNode() const { Likewise, an imaginary combined tree of mixed types of property nodes is a concept introduced by layerization implementation. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:24: m_effect->localTransformSpace() != m_transform) FYI there is one corner case where the transform space of the clip may be deeper than transform of the chunk. <div style="position:absolute; clip:rect(1,2,3,4);"> <div style="position:fixed;"></div> </div> https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:82: InnermostNode innermostNode() const; On 2016/12/20 23:36:32, chrishtr wrote: > On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > > Please add a comment describing this and the iterator below. > > > > It looks like innermostNode is sort of the current "lowest common ancestor" in > the property tree, and you're using that to define an iteration order for the > PropertyTreeStateIterator to traverse up the tree--is that correct? > > Done. > > The innermostNode is the first of the transform, clip or effect to be applied. > There is always a total order on them. The canonical order is: chunk.transform.screen_matrix chunk.clip.combined_screen_region chunk.effect chunk.effect.outputClip.combined_screen_region chunk.effect.parent chunk.effect.parent.outputClip.combined_screen_region chunk.effect.parent.parent ... The innermostNode implementation seems to already include some reordering to improve edge bleeding. That's one reason I feel this belong to PAC because it involves compositing decision.
https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:9: bool PropertyTreeState::hasDirectCompositingReasons() const { On 2016/12/20 at 23:49:36, trchen wrote: > This feels a weird place to me. IMO the concept of direct compositing reason doesn't apply to chunks. I don't have it on chunks, I have it on nodes. Also see below. > A chunk gets a reason not to be squashed into a previous chunk because one of its property node cross an edge that has direct compositing reason. > > For example: > <div>A</div> > <div style="transform:translateZ(0);"> > <div style="transform:translate(0,0);">B</div> > </div> > > Chunk B points to no nodes that have a direct compositing reason, but chunk B still needs to be composited separately because of the 3D parent. > > For another example: > <div style="transform:translateZ(0);"> > <div style="opacity:0.5">A</div> > <div style="opacity:0.5">B</div> > </div> > > A and B both points to a directly composited transform node, but there's no reason why they need to be composited separately. > > I think the algorithm should roughly look like this: > bool shouldBeBackedSeparately(const TransformNode* a, const TransformNode* b) { > if (depth(a) > depth(b)) > swap(a, b); > for(;depth(b) > depth(a); b = b->parent) { > if (b->hasDirectCompositingReason()) > return true; > } > for (;a != b; a = a->parent, b = b->parent) > if (a->hasDirectCompositingReason() || b->hasDirectCompositingReason()) > return true; > return false; > } This is the algorithm I have implemented, generalized to multiple property trees. The method here is a convenience method to check against the innermostNode(). https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:22: PropertyTreeState::InnermostNode PropertyTreeState::innermostNode() const { On 2016/12/20 at 23:49:36, trchen wrote: > Likewise, an imaginary combined tree of mixed types of property nodes is a concept introduced by layerization implementation. See above. I've added it on PropertyTreeState for convenience. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:24: m_effect->localTransformSpace() != m_transform) On 2016/12/20 at 23:49:36, trchen wrote: > FYI there is one corner case where the transform space of the clip may be deeper than transform of the chunk. > <div style="position:absolute; clip:rect(1,2,3,4);"> > <div style="position:fixed;"></div> > </div> In this case, the position: fixed chunk would point to the root transform node and the clip node for the clip: rect(...) clip. The position: absolute chunk would point to the frame scrolling transform node and the root clip. Seems ok to me.. https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:82: InnermostNode innermostNode() const; On 2016/12/20 at 23:49:37, trchen wrote: > On 2016/12/20 23:36:32, chrishtr wrote: > > On 2016/12/20 at 06:46:30, pdr (OOO Dec 23 to Jan 2) wrote: > > > Please add a comment describing this and the iterator below. > > > > > > It looks like innermostNode is sort of the current "lowest common ancestor" in > > the property tree, and you're using that to define an iteration order for the > > PropertyTreeStateIterator to traverse up the tree--is that correct? > > > > Done. > > > > The innermostNode is the first of the transform, clip or effect to be applied. > > There is always a total order on them. > > The canonical order is: > chunk.transform.screen_matrix > chunk.clip.combined_screen_region > chunk.effect > chunk.effect.outputClip.combined_screen_region > chunk.effect.parent > chunk.effect.parent.outputClip.combined_screen_region > chunk.effect.parent.parent > ... > > The innermostNode implementation seems to already include some reordering to improve edge bleeding. That's one reason I feel this belong to PAC because it involves compositing decision. I'm not sure what your point is here. Is there a bug in my algorithm?
https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp (right): https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:9: bool PropertyTreeState::hasDirectCompositingReasons() const { On 2016/12/20 23:58:10, chrishtr wrote: > On 2016/12/20 at 23:49:36, trchen wrote: > > This feels a weird place to me. IMO the concept of direct compositing reason > doesn't apply to chunks. > > I don't have it on chunks, I have it on nodes. Also see below. > > > A chunk gets a reason not to be squashed into a previous chunk because one of > its property node cross an edge that has direct compositing reason. > > > > For example: > > <div>A</div> > > <div style="transform:translateZ(0);"> > > <div style="transform:translate(0,0);">B</div> > > </div> > > > > Chunk B points to no nodes that have a direct compositing reason, but chunk B > still needs to be composited separately because of the 3D parent. > > > > For another example: > > <div style="transform:translateZ(0);"> > > <div style="opacity:0.5">A</div> > > <div style="opacity:0.5">B</div> > > </div> > > > > A and B both points to a directly composited transform node, but there's no > reason why they need to be composited separately. > > > > I think the algorithm should roughly look like this: > > bool shouldBeBackedSeparately(const TransformNode* a, const TransformNode* b) > { > > if (depth(a) > depth(b)) > > swap(a, b); > > for(;depth(b) > depth(a); b = b->parent) { > > if (b->hasDirectCompositingReason()) > > return true; > > } > > for (;a != b; a = a->parent, b = b->parent) > > if (a->hasDirectCompositingReason() || b->hasDirectCompositingReason()) > > return true; > > return false; > > } > > This is the algorithm I have implemented, generalized to multiple property > trees. > The method here is a convenience method to check against the innermostNode(). Ah, I see. It all makes sense now. However I feel this introduces a lot confusion. These concepts apply to the multi-prop-tree node, where both the key and the value is a property state. Why is it less convenient to do it as a private impl class of PAC? Like this: namespace { class MultiPropTreeNode : public PropertyTreeState { public: bool hasDirectCompositingReasons() const; InnermostNode innermostNode() const; }; } https://codereview.chromium.org/2581843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:24: m_effect->localTransformSpace() != m_transform) On 2016/12/20 23:58:10, chrishtr wrote: > On 2016/12/20 at 23:49:36, trchen wrote: > > FYI there is one corner case where the transform space of the clip may be > deeper than transform of the chunk. > > <div style="position:absolute; clip:rect(1,2,3,4);"> > > <div style="position:fixed;"></div> > > </div> > > In this case, the position: fixed chunk would point to the root transform node > and the clip node for the clip: rect(...) clip. The position: absolute chunk > would point to > the frame scrolling transform node and the root clip. Seems ok to me.. It seemed to me you want to guarantee that if state->innermostNode() == Transform, that implies state->transform()->isDescendantOf(state->clip()->localTransformSpace()). If you didn't want that guarantee, it is probably okay. Another thing I worried about is the inferred total order of nodes could be inconsistent. Consider the following tree & chunks: T0 <- T1 <- T2 C0 <- C1(localSpace=T2) ChunkA(T1, C1) ChunkB(T2, C1) The inferred total order from Chunk A would be: Root <- C1 <- T1 While the inferred order from Chunk B would be: Root <- T1 <- T2 <- C1 Fortunately today we never generate such tree because of CssClipFixedPosition nodes, but I feel an extra DCHECK would be handy...
Updated to address all comments. I have revised the algorithm to apply clips and effects significantly. See "algorithm step 4" in goo.gl/6xP8Oe. A bunch more tests to exercise the cases Tien-Ren brought up have been added.
The CQ bit was checked by chrishtr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:273: struct RectAndClipState { Nit: remove this (not used). https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:303: nit: newline. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:307: static FloatRect infinteFloatRect = FloatRect(LayoutRect::infiniteIntRect()); This won't actually be equal to an infinite float rect. What is this code for? https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:331: for (Vector<PropertyTreeState>::const_reverse_iterator pairedState = Please add a comments here describing this algorithm at a high level, and how clips are pushed around and effects are transformed with a 2d offset. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:355: // PropertyTreeState of the first found clip, Nit: reflow this comment. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:378: // TODO(chrishtr): specify origin of the filter. Non-rhetorical question: what will the origin be equal to? https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp (right): https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:32: PropertyTreeState::InnermostNode PropertyTreeState::innermostNode() const { Please document this. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/PictureMatchers.cpp (right): https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/PictureMatchers.cpp:7: #include "base/debug/stack_trace.h" Nit: remove
https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:273: struct RectAndClipState { On 2016/12/22 at 19:46:26, pdr (OOO Dec 23 to Jan 2) wrote: > Nit: remove this (not used). Removed. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:303: On 2016/12/22 at 19:46:26, pdr (OOO Dec 23 to Jan 2) wrote: > nit: newline. Done. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:307: static FloatRect infinteFloatRect = FloatRect(LayoutRect::infiniteIntRect()); On 2016/12/22 at 19:46:26, pdr (OOO Dec 23 to Jan 2) wrote: > This won't actually be equal to an infinite float rect. What is this code for? The early-out is to avoid effects that do nothing. But I can remove that. Otherwise, I have to supply some input to the clip machinery in localToVisualRectInAncestorSpace. Note that PaintLayerClipper does the same thing.. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:331: for (Vector<PropertyTreeState>::const_reverse_iterator pairedState = On 2016/12/22 at 19:46:26, pdr (OOO Dec 23 to Jan 2) wrote: > Please add a comments here describing this algorithm at a high level, and how clips are pushed around and effects are transformed with a 2d offset. I added a comment in the .h file for the method which calls, this, that refers readers to the designdoc. Good enough, or add more comments? https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:355: // PropertyTreeState of the first found clip, On 2016/12/22 at 19:46:26, pdr (OOO Dec 23 to Jan 2) wrote: > Nit: reflow this comment. Done. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:378: // TODO(chrishtr): specify origin of the filter. On 2016/12/22 at 19:46:26, pdr (OOO Dec 23 to Jan 2) wrote: > Non-rhetorical question: what will the origin be equal to? Filters can have origins specified by the offsets from the element of the filter. SPv1 supports it, but it's not yet implemented on effect nodes. https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp (right): https://codereview.chromium.org/2581843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:32: PropertyTreeState::InnermostNode PropertyTreeState::innermostNode() const { On 2016/12/22 at 19:46:26, pdr (OOO Dec 23 to Jan 2) wrote: > Please document this. Done.
The CQ bit was checked by chrishtr@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...
The CQ bit was checked by chrishtr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:464: // transforms into single clips in the same way that PaintLayerClipper does. Sharing is required for effect node, otherwise grouping won't work correctly. For transform and clip I feel more like just emit a drawing display item that does { canvas->setMatrix(combinedTransform); canvas->clipRegion(combinedClip, SkClipOp::kReplace); } Less hassle than sharing & collapsing the pairs. I don't feel strongly about either way though. https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:530: gfx::Rect ccCombinedBounds(enclosingIntRect(pendingLayer.bounds)); In PendingLayer::add it seemed to me pendingLayer.bounds is in screen coordinate? Did we convert it to local rect somewhere? https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:1022: bool PaintArtifactCompositor::overlaps( nits: I think it should be named "possiblyOverlap" with a comment saying that false positive may be returned but no false negative. https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:1042: knownToBeOpaque = knownToBeOpaque && paintChunk.knownToBeOpaque; This looks wrong. A layer is opaque as long as there is one chunk that's opaque and covers the layer rect. Also even if every chunks are opaque, the layer is not if the union'd region is not a rect.
LGTM
https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:464: // transforms into single clips in the same way that PaintLayerClipper does. On 2016/12/22 at 22:21:24, trchen wrote: > Sharing is required for effect node, otherwise grouping won't work correctly. Sure, but it's not always possible. > > For transform and clip I feel more like just emit a drawing display item that does > { canvas->setMatrix(combinedTransform); canvas->clipRegion(combinedClip, SkClipOp::kReplace); } > Less hassle than sharing & collapsing the pairs. I don't feel strongly about either way though. That would be ideal, but it's not easy to do performantly. https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:530: gfx::Rect ccCombinedBounds(enclosingIntRect(pendingLayer.bounds)); On 2016/12/22 at 22:21:24, trchen wrote: > In PendingLayer::add it seemed to me pendingLayer.bounds is in screen coordinate? Did we convert it to local rect somewhere? It's in the space of the transform node of the first paint chunk. I see there is a bug in that I'm not mapping subsequent chunks in the same PaintLayer to that space, will fix. https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:1022: bool PaintArtifactCompositor::overlaps( On 2016/12/22 at 22:21:24, trchen wrote: > nits: I think it should be named "possiblyOverlap" with a comment saying that false positive may be returned but no false negative. Done. https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:1042: knownToBeOpaque = knownToBeOpaque && paintChunk.knownToBeOpaque; On 2016/12/22 at 22:21:24, trchen wrote: > This looks wrong. A layer is opaque as long as there is one chunk that's opaque and covers the layer rect. Also even if every chunks are opaque, the layer is not if the union'd region is not a rect. Good point, will fix.
On 2016/12/22 22:28:46, chrishtr wrote: > https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp > (right): > > https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:464: > // transforms into single clips in the same way that PaintLayerClipper does. > On 2016/12/22 at 22:21:24, trchen wrote: > > Sharing is required for effect node, otherwise grouping won't work correctly. > > Sure, but it's not always possible. Why not? Probably will need to composite effect nodes for indirect (descendant) reasons. No need to implement it in this CL, but I believe it is always possible to generate a layer tree that draws equivalently. > > For transform and clip I feel more like just emit a drawing display item that > does > > { canvas->setMatrix(combinedTransform); canvas->clipRegion(combinedClip, > SkClipOp::kReplace); } > > Less hassle than sharing & collapsing the pairs. I don't feel strongly about > either way though. > > That would be ideal, but it's not easy to do performantly. Why isn't it? At least for transform, it is trivial to do O(1) per chunk. CTM = flatten(layer.to_screen)^-1 * flatten(chunk.to_screen) For clip it will need some cache... But don't we already have it for PaintLayerClipper?
Description was changed from ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. A large amount of this patch is additional tests and testing machinery to support the change in behavior. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited layer. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. A large amount of this patch is additional tests and testing machinery to support the change in behavior. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2016/12/22 at 22:48:37, trchen wrote: > On 2016/12/22 22:28:46, chrishtr wrote: > > https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... > > File > > third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp > > (right): > > > > https://codereview.chromium.org/2581843002/diff/300001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:464: > > // transforms into single clips in the same way that PaintLayerClipper does. > > On 2016/12/22 at 22:21:24, trchen wrote: > > > Sharing is required for effect node, otherwise grouping won't work correctly. > > > > Sure, but it's not always possible. > > Why not? Probably will need to composite effect nodes for indirect (descendant) reasons. No need to implement it in this CL, but I believe it is always possible to generate a layer tree that draws equivalently. Because there might be intervening separately composited layers. It's just a hard problem, though I agree with your goal. I thin we are on the same page. We've discussed this in the past in various design docs. > > > > For transform and clip I feel more like just emit a drawing display item that > > does > > > { canvas->setMatrix(combinedTransform); canvas->clipRegion(combinedClip, > > SkClipOp::kReplace); } > > > Less hassle than sharing & collapsing the pairs. I don't feel strongly about > > either way though. > > > > That would be ideal, but it's not easy to do performantly. > > Why isn't it? At least for transform, it is trivial to do O(1) per chunk. CTM = flatten(layer.to_screen)^-1 * flatten(chunk.to_screen) > For clip it will need some cache... But don't we already have it for PaintLayerClipper? Oh I see, emit a drawing display item instead. That could be done, but the performance will be basically the same I think. In any case not in this CL..
The CQ bit was checked by chrishtr@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...
Yep neither issue should be blocking this CL. Let's submit & iterate! lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by chrishtr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by chrishtr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by chrishtr@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...
In a follow-on patch I will add unittests for the three bugs mentioned in this comment set. https://codereview.chromium.org/2581843002/diff/400001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2581843002/diff/400001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:701: crbug.com/668342 virtual/spv2/svg/custom/object-sizing-explicit-height.xhtml [ NeedsRebaseline ] This test already had a different baseline for virtual/spv2/. https://codereview.chromium.org/2581843002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:329: hasClip = false; In an earlier patch set, I neglected to reset this to false. https://codereview.chromium.org/2581843002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:335: matrix.applyTransformOrigin(pairedState->transform()->origin()); In an earlier patchset I neglected to apply the transform origin. https://codereview.chromium.org/2581843002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:1043: knownToBeOpaque(firstPaintChunk.knownToBeOpaque), In an earlier patchset I got the business logic wrong for knownToBeOpaque wrong on the first paint chunk.
The CQ bit was checked by chrishtr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by chrishtr@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/2581843002/diff/440001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 (right): https://codereview.chromium.org/2581843002/diff/440001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:541: crbug.com/668342 fast/clip/010.html [ Failure ] Fails due to presence of paint offset transforms for position: fixed elements. https://codereview.chromium.org/2570423003 will fix this. https://codereview.chromium.org/2581843002/diff/440001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1251: Bug(none) svg/custom/image-with-transform-clip-filter.svg [ Failure ] Assert fail in paint. https://codereview.chromium.org/2581843002/diff/440001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1340: Bug(none) svg/custom/viewport-clippath-invalidation.html [ Failure ] Assert fail in paint. https://codereview.chromium.org/2581843002/diff/440001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:2139: Bug(none) css3/blending/mix-blend-mode-with-filters.html [ Failure ] Fails because we don't expand the bounds of the composited layer to include blur effects. https://codereview.chromium.org/2581843002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2581843002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:391: ccList.CreateAndAppendPairedBeginItem<cc::CompositingDisplayItem>( Switched these in order. Old one was wrong. https://codereview.chromium.org/2581843002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:1024: if (newChunk.properties.backfaceHidden != pendingLayerFirstChunk.properties.backfaceHidden) Added this code to not merge with backface visibility disagreements.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by chrishtr@chromium.org
https://codereview.chromium.org/2581843002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/PictureMatchers.cpp (right): https://codereview.chromium.org/2581843002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/PictureMatchers.cpp:57: if (layerAlpha < 255) { Made a small tweak to to the code here so the tests pass with filter & compositing saveLayer calls reversed.
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org, pdr@chromium.org, trchen@chromium.org Link to the patchset: https://codereview.chromium.org/2581843002/#ps460001 (title: "none")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, trchen@chromium.org, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2581843002/#ps480001 (title: "none")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, trchen@chromium.org, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2581843002/#ps500001 (title: "none")
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": 500001, "attempt_start_ts": 1482859033165130,
"parent_rev": "2787afd8784e23bcfce1310ba07d0c305f26586c", "commit_rev":
"81b94fa35a49be6ed010c44e8c8a4d2a522cccb3"}
Message was sent while issue was closed.
Description was changed from ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited layer. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. A large amount of this patch is additional tests and testing machinery to support the change in behavior. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited layer. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. A large amount of this patch is additional tests and testing machinery to support the change in behavior. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2581843002 ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited layer. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. A large amount of this patch is additional tests and testing machinery to support the change in behavior. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2581843002 ========== to ========== Implement merging non-composited paint property nodes in the PACompositor. Currently, the PaintArtifactCompositor naively puts every PaintChunk into its own composited layer. This patch improves on that by merging a PaintChunk into the same layer as the PaintChunk which precedes it, if the new PaintChunk has a compatible PropertyTreeState (no directly composited property tree nodes between the state of the new PaintChunk and the old one). This algorithm is spelled out in a design doc, see crbug.com/668342. A large amount of this patch is additional tests and testing machinery to support the change in behavior. BUG=668342 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/7239114dd9ee5270ff6ac3e066113b34814f777d Cr-Commit-Position: refs/heads/master@{#440775} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/7239114dd9ee5270ff6ac3e066113b34814f777d Cr-Commit-Position: refs/heads/master@{#440775} |
