Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2625133003:
Handle geometry effects of filters in GeometryMapper (Closed)
|
Created:
3 years, 11 months ago by Xianzhu Modified:
3 years, 10 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-paint_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle geometry effects of filters in GeometryMapper
Some filters affects visual rects. Previously this is done by forcing
slow old path for descendant of filters.
BUG=637313
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2625133003
Cr-Commit-Position: refs/heads/master@{#449214}
Committed: https://chromium.googlesource.com/chromium/src/+/598043a9d645cec72159c5dac7f0b21ac29ca470
Patch Set 1 #Patch Set 2 : - #Patch Set 3 : - #Patch Set 4 : - #Patch Set 5 : Rebaseline tests #
Total comments: 15
Patch Set 6 : - #Patch Set 7 : - #Patch Set 8 : Fix a typo in function name (hasFilterThatMovesPixels) #
Total comments: 5
Patch Set 9 : - #Messages
Total messages: 54 (32 generated)
Description was changed from ========== Handle geometry effects of filters in GeometryMapper Some filters affects visual rects. Previously this is done by forcing slow old path for descendant of filters. BUG=637313 ========== to ========== Handle geometry effects of filters in GeometryMapper Some filters affects visual rects. Previously this is done by forcing slow old path for descendant of filters. BUG=637313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@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...)
The CQ bit was checked by wangxianzhu@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 wangxianzhu@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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 wangxianzhu@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_...)
The CQ bit was checked by wangxianzhu@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...
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, trchen@chromium.org
https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filter-invalidation-with-composited-container-change-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filter-invalidation-with-composited-container-change-expected.txt:26: "rect": [0, 0, 200, 200], This change is because we no longer map the rect with the effect on the paint invalidation container which is unnecessary because the final effect is applied after the layer is rasterized. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filter-repaint-accelerated-on-accelerated-filter-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filter-repaint-accelerated-on-accelerated-filter-expected.txt:24: "rect": [0, 0, 200, 200], Ditto. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt:11: "rect": [28, 20, 300, 300], This is because now we use cc side effect mapRect, which sometimes is different from the blink side one. There seems no regressions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrishtr@chromium.org changed reviewers: + ajuma@chromium.org
This CL: https://codereview.chromium.org/2423483003 does the same thing in cc but by using a clip node that expands the rect. The expansion is computed in the same way as your CL is, by calling mapRect on the filter. We should use the same method in both cc and Blink.
On 2017/01/28 18:58:42, chrishtr wrote: > This CL: > > https://codereview.chromium.org/2423483003 > > does the same thing in cc but by using a clip node that expands the rect. The > expansion > is computed in the same way as your CL is, by calling mapRect on the filter. We > should > use the same method in both cc and Blink. I'm not sure if I understand it correctly. Do you mean we should create ClipPaintPropertyNode that expands a rect for an EffectPaintPropertyNode containing filters? Why is that necessary given that the EffectPaintPropertyNode can expands a visual rect? There seems no common things between a normal CilpPaintPropertyNode having a fixed clip rect and a ClipPaintPropertyNode that would expands a rect by calling mapRect method of filters.
On 2017/01/29 06:38:06, Xianzhu wrote: > On 2017/01/28 18:58:42, chrishtr wrote: > > This CL: > > > > https://codereview.chromium.org/2423483003 > > > > does the same thing in cc but by using a clip node that expands the rect. The > > expansion > > is computed in the same way as your CL is, by calling mapRect on the filter. > We > > should > > use the same method in both cc and Blink. > > I'm not sure if I understand it correctly. Do you mean we should create > ClipPaintPropertyNode that expands a rect for an EffectPaintPropertyNode > containing filters? Why is that necessary given that the EffectPaintPropertyNode > can expands a visual rect? There seems no common things between a normal > CilpPaintPropertyNode having a fixed clip rect and a ClipPaintPropertyNode that > would expands a rect by calling mapRect method of filters. We used this approach in cc to simplify caching and visible rect computation. With this approach, each clip node has a well-defined combined clip (the clip found by accumulating all ancestor clips and doing any necessary expansion along the way) that we can cache, we can compute the combined clip for a node knowing only its parent's combined clip and its own clip, and in particular we don't need to simultaneously walk the clip and effect trees to do this.
On 2017/01/30 18:21:39, ajuma wrote: > On 2017/01/29 06:38:06, Xianzhu wrote: > > On 2017/01/28 18:58:42, chrishtr wrote: > > > This CL: > > > > > > https://codereview.chromium.org/2423483003 > > > > > > does the same thing in cc but by using a clip node that expands the rect. > The > > > expansion > > > is computed in the same way as your CL is, by calling mapRect on the filter. > > We > > > should > > > use the same method in both cc and Blink. > > > > I'm not sure if I understand it correctly. Do you mean we should create > > ClipPaintPropertyNode that expands a rect for an EffectPaintPropertyNode > > containing filters? Why is that necessary given that the > EffectPaintPropertyNode > > can expands a visual rect? There seems no common things between a normal > > CilpPaintPropertyNode having a fixed clip rect and a ClipPaintPropertyNode > that > > would expands a rect by calling mapRect method of filters. > > We used this approach in cc to simplify caching and visible rect computation. > With this approach, each clip node has a well-defined combined clip (the clip > found by accumulating all ancestor clips and doing any necessary expansion along > the way) that we can cache, we can compute the combined clip for a node knowing > only its parent's combined clip and its own clip, and in particular we don't > need to simultaneously walk the clip and effect trees to do this. This seems not applicable to blink::GeometryMapper. Our case is to map a descendant visual rect in local space to an ancestor space. During the mapping, a filters expands the source visual rect instead of expanding combined ancestor clips.
On 2017/01/30 at 18:30:30, wangxianzhu wrote: > On 2017/01/30 18:21:39, ajuma wrote: > > On 2017/01/29 06:38:06, Xianzhu wrote: > > > On 2017/01/28 18:58:42, chrishtr wrote: > > > > This CL: > > > > > > > > https://codereview.chromium.org/2423483003 > > > > > > > > does the same thing in cc but by using a clip node that expands the rect. > > The > > > > expansion > > > > is computed in the same way as your CL is, by calling mapRect on the filter. > > > We > > > > should > > > > use the same method in both cc and Blink. > > > > > > I'm not sure if I understand it correctly. Do you mean we should create > > > ClipPaintPropertyNode that expands a rect for an EffectPaintPropertyNode > > > containing filters? Why is that necessary given that the > > EffectPaintPropertyNode > > > can expands a visual rect? There seems no common things between a normal > > > CilpPaintPropertyNode having a fixed clip rect and a ClipPaintPropertyNode > > that > > > would expands a rect by calling mapRect method of filters. > > > > We used this approach in cc to simplify caching and visible rect computation. > > With this approach, each clip node has a well-defined combined clip (the clip > > found by accumulating all ancestor clips and doing any necessary expansion along > > the way) that we can cache, we can compute the combined clip for a node knowing > > only its parent's combined clip and its own clip, and in particular we don't > > need to simultaneously walk the clip and effect trees to do this. > > This seems not applicable to blink::GeometryMapper. Our case is to map a descendant visual rect in local space to an ancestor space. During the mapping, a filters expands the source visual rect instead of expanding combined ancestor clips. I think it does apply, unless there is a complication I am missing. Currently,GeometryMapper does cached combined clips and combined transforms, and the combined clip for a node depends only in itself and the combined clip for its parent. If you implemented an "expansion" clip like Ali did in cc, it seems it would solve this case and preserve caching. Right now, slowLocalToAncestorVisualRectWithEffects is implemented to always iterate over each effect between the source and dest, which would be slower.
On 2017/01/31 21:16:58, chrishtr wrote: > On 2017/01/30 at 18:30:30, wangxianzhu wrote: > > On 2017/01/30 18:21:39, ajuma wrote: > > > On 2017/01/29 06:38:06, Xianzhu wrote: > > > > On 2017/01/28 18:58:42, chrishtr wrote: > > > > > This CL: > > > > > > > > > > https://codereview.chromium.org/2423483003 > > > > > > > > > > does the same thing in cc but by using a clip node that expands the > rect. > > > The > > > > > expansion > > > > > is computed in the same way as your CL is, by calling mapRect on the > filter. > > > > We > > > > > should > > > > > use the same method in both cc and Blink. > > > > > > > > I'm not sure if I understand it correctly. Do you mean we should create > > > > ClipPaintPropertyNode that expands a rect for an EffectPaintPropertyNode > > > > containing filters? Why is that necessary given that the > > > EffectPaintPropertyNode > > > > can expands a visual rect? There seems no common things between a normal > > > > CilpPaintPropertyNode having a fixed clip rect and a ClipPaintPropertyNode > > > that > > > > would expands a rect by calling mapRect method of filters. > > > > > > We used this approach in cc to simplify caching and visible rect > computation. > > > With this approach, each clip node has a well-defined combined clip (the > clip > > > found by accumulating all ancestor clips and doing any necessary expansion > along > > > the way) that we can cache, we can compute the combined clip for a node > knowing > > > only its parent's combined clip and its own clip, and in particular we don't > > > need to simultaneously walk the clip and effect trees to do this. > > > > This seems not applicable to blink::GeometryMapper. Our case is to map a > descendant visual rect in local space to an ancestor space. During the mapping, > a filters expands the source visual rect instead of expanding combined ancestor > clips. > > I think it does apply, unless there is a complication I am missing. > > Currently,GeometryMapper does cached combined clips and combined transforms, and > the combined > clip for a node depends only in itself and the combined clip for its parent. > If you implemented an "expansion" clip like Ali did in cc, it seems it would > solve > this case and preserve caching. Right now, > slowLocalToAncestorVisualRectWithEffects > is implemented to always iterate over each effect between the source and dest, > which > would be slower. Example: <reflection> <editable> </reflection> Would the combined clip be infinite? How should we map a caret rect using the combined clip?
On 2017/01/31 at 23:30:43, wangxianzhu wrote: > On 2017/01/31 21:16:58, chrishtr wrote: > > On 2017/01/30 at 18:30:30, wangxianzhu wrote: > > > On 2017/01/30 18:21:39, ajuma wrote: > > > > On 2017/01/29 06:38:06, Xianzhu wrote: > > > > > On 2017/01/28 18:58:42, chrishtr wrote: > > > > > > This CL: > > > > > > > > > > > > https://codereview.chromium.org/2423483003 > > > > > > > > > > > > does the same thing in cc but by using a clip node that expands the > > rect. > > > > The > > > > > > expansion > > > > > > is computed in the same way as your CL is, by calling mapRect on the > > filter. > > > > > We > > > > > > should > > > > > > use the same method in both cc and Blink. > > > > > > > > > > I'm not sure if I understand it correctly. Do you mean we should create > > > > > ClipPaintPropertyNode that expands a rect for an EffectPaintPropertyNode > > > > > containing filters? Why is that necessary given that the > > > > EffectPaintPropertyNode > > > > > can expands a visual rect? There seems no common things between a normal > > > > > CilpPaintPropertyNode having a fixed clip rect and a ClipPaintPropertyNode > > > > that > > > > > would expands a rect by calling mapRect method of filters. > > > > > > > > We used this approach in cc to simplify caching and visible rect > > computation. > > > > With this approach, each clip node has a well-defined combined clip (the > > clip > > > > found by accumulating all ancestor clips and doing any necessary expansion > > along > > > > the way) that we can cache, we can compute the combined clip for a node > > knowing > > > > only its parent's combined clip and its own clip, and in particular we don't > > > > need to simultaneously walk the clip and effect trees to do this. > > > > > > This seems not applicable to blink::GeometryMapper. Our case is to map a > > descendant visual rect in local space to an ancestor space. During the mapping, > > a filters expands the source visual rect instead of expanding combined ancestor > > clips. > > > > I think it does apply, unless there is a complication I am missing. > > > > Currently,GeometryMapper does cached combined clips and combined transforms, and > > the combined > > clip for a node depends only in itself and the combined clip for its parent. > > If you implemented an "expansion" clip like Ali did in cc, it seems it would > > solve > > this case and preserve caching. Right now, > > slowLocalToAncestorVisualRectWithEffects > > is implemented to always iterate over each effect between the source and dest, > > which > > would be slower. > > Example: > <reflection> > <editable> > </reflection> > > Would the combined clip be infinite? How should we map a caret rect using the combined clip? Yeah I see your point. It could be used for combined clip I think, but that doesn't help to compute transformed rects, which can't be simplified into a single matrix I guess.
https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt:11: "rect": [28, 20, 300, 300], On 2017/01/26 at 23:17:45, Xianzhu wrote: > This is because now we use cc side effect mapRect, which sometimes is different from the blink side one. There seems no regressions. Do you know what is different about the two implementations of mapRect? Maybe there is a bug in there. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-2-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-2-expected.txt:11: "rect": [28, 20, 300, 300], What about here and below? Also because of cc vs blink difference? https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:45: FloatRect mapRect(const FloatRect&) const; Document what this maps from and to. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:92: FloatRect mapRect(const FloatRect&) const; Nit: name the argument inputRect. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:186: FloatPoint m_offset; Why not m_paintOffset?
https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt:11: "rect": [28, 20, 300, 300], On 2017/01/26 at 23:17:45, Xianzhu wrote: > This is because now we use cc side effect mapRect, which sometimes is different from the blink side one. There seems no regressions. Do you know what is different about the two implementations of mapRect? Maybe there is a bug in there. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-2-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-2-expected.txt:11: "rect": [28, 20, 300, 300], What about here and below? Also because of cc vs blink difference? https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:45: FloatRect mapRect(const FloatRect&) const; Document what this maps from and to. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:92: FloatRect mapRect(const FloatRect&) const; Nit: name the argument inputRect. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:186: FloatPoint m_offset; Why not m_paintOffset?
https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt:11: "rect": [28, 20, 300, 300], On 2017/02/01 02:59:12, chrishtr wrote: > On 2017/01/26 at 23:17:45, Xianzhu wrote: > > This is because now we use cc side effect mapRect, which sometimes is > different from the blink side one. There seems no regressions. > > Do you know what is different about the two implementations of mapRect? Maybe > there is a bug in there. <div class="before box"></div> <svg width="0" height="0"> <filter id="composite" x="-0.5" y="-0.5" width="2" height="2"> <feFlood x="100" y="100" width="200" height="200" flood-color="red"/> <feComposite operator="in" in="SourceGraphic"/> </filter> </svg> The difference seems for feComposite: blink (blink::ReferenceFilterOperation): feFlood: 100,100 200x200 feComposite: 100,100 200x200 result is 100,100 200x200 skia (SkImageFilter): feFlood: 100,100 200x200 feComposite: 0,0 200x200 result is union of the above: 0, 0, 300, 300 Skia seems to use a simple method that - just uses input as feComposite bounds; - and unions the bounds which definitely covers any feComposite combinations but the rect may be larger than optimal. The blink result seems also not optimal. If I understand it correctly, the optimal result should be (100,100 100x100) which is the intersection of the two inputs of feComposite. Perhaps the suboptimility is acceptable? https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-2-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-2-expected.txt:11: "rect": [28, 20, 300, 300], On 2017/02/01 02:59:12, chrishtr wrote: > What about here and below? Also because of cc vs blink difference? Yes, it's the same as effect-reference-repaint-composite-1.html. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:45: FloatRect mapRect(const FloatRect&) const; On 2017/02/01 02:59:12, chrishtr wrote: > Document what this maps from and to. Done. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:92: FloatRect mapRect(const FloatRect&) const; On 2017/02/01 02:59:12, chrishtr wrote: > Nit: name the argument inputRect. Done. https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:186: FloatPoint m_offset; On 2017/02/01 02:59:12, chrishtr wrote: > Why not m_paintOffset? No specific reason :) Modified to paintOffset to keep consistency with other modules.
chrishtr@chromium.org changed reviewers: + senorblanco@chromium.org
I asked for a quick review of the example from comment 34 from senorblanco. Other than that, looks good.
The CQ bit was checked by wangxianzhu@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/02/03 01:49:55, chrishtr wrote: > I asked for a quick review of the example from comment 34 from senorblanco. > Other than that, > looks good. sernorblanco@ what do you think about the the difference between blink and cc about reference filter mapRect? #34 contains an example.
https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt:11: "rect": [28, 20, 300, 300], On 2017/02/03 01:38:16, Xianzhu wrote: > On 2017/02/01 02:59:12, chrishtr wrote: > > On 2017/01/26 at 23:17:45, Xianzhu wrote: > > > This is because now we use cc side effect mapRect, which sometimes is > > different from the blink side one. There seems no regressions. > > > > Do you know what is different about the two implementations of mapRect? Maybe > > there is a bug in there. > > <div class="before box"></div> > <svg width="0" height="0"> > <filter id="composite" x="-0.5" y="-0.5" width="2" height="2"> > <feFlood x="100" y="100" width="200" height="200" flood-color="red"/> > <feComposite operator="in" in="SourceGraphic"/> > </filter> > </svg> > > The difference seems for feComposite: > > blink (blink::ReferenceFilterOperation): > feFlood: 100,100 200x200 > feComposite: 100,100 200x200 > result is 100,100 200x200 > > skia (SkImageFilter): > feFlood: 100,100 200x200 > feComposite: 0,0 200x200 > result is union of the above: 0, 0, 300, 300 > > Skia seems to use a simple method that > - just uses input as feComposite bounds; > - and unions the bounds which definitely covers any feComposite combinations > but the rect may be larger than optimal. > > The blink result seems also not optimal. > > If I understand it correctly, the optimal result should be (100,100 100x100) > which is the intersection of the two inputs of feComposite. > > Perhaps the suboptimility is acceptable? This optimization was landed by the Swiffy folks in Blink a long while back for an actual use case, back when they used SVG rather than Canvas. It's something I've been meaning to move to Skia for some time, but haven't had a chance. It would be unfortunate to lose it, but I don't think it would be crippling. Unfortunately they didn't land a perf test at the time either. https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/filter-invalidation-with-composited-container-change-expected.txt (right): https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/filter-invalidation-with-composited-container-change-expected.txt:26: "rect": [0, 0, 200, 200], This doesn't seem correct. Shouldn't the entire div be invalidated, including the padding induced by the filter? Or does the paint invalidation on the document above subsume this one? https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/filter-repaint-accelerated-on-accelerated-filter-expected.txt (right): https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/filter-repaint-accelerated-on-accelerated-filter-expected.txt:24: "rect": [0, 0, 200, 200], Same here. https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h (right): https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:45: // Returns a rect covering the pixels that can be affected by pixels in Nit: s/pixels/destination pixels/? ie., something to indicate that this a "forward" mapping.
https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt (right): https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt:11: "rect": [28, 20, 300, 300], On 2017/02/07 18:26:41, Stephen White wrote: > On 2017/02/03 01:38:16, Xianzhu wrote: > > On 2017/02/01 02:59:12, chrishtr wrote: > > > On 2017/01/26 at 23:17:45, Xianzhu wrote: > > > > This is because now we use cc side effect mapRect, which sometimes is > > > different from the blink side one. There seems no regressions. > > > > > > Do you know what is different about the two implementations of mapRect? > Maybe > > > there is a bug in there. > > > > <div class="before box"></div> > > <svg width="0" height="0"> > > <filter id="composite" x="-0.5" y="-0.5" width="2" height="2"> > > <feFlood x="100" y="100" width="200" height="200" flood-color="red"/> > > <feComposite operator="in" in="SourceGraphic"/> > > </filter> > > </svg> > > > > The difference seems for feComposite: > > > > blink (blink::ReferenceFilterOperation): > > feFlood: 100,100 200x200 > > feComposite: 100,100 200x200 > > result is 100,100 200x200 > > > > skia (SkImageFilter): > > feFlood: 100,100 200x200 > > feComposite: 0,0 200x200 > > result is union of the above: 0, 0, 300, 300 > > > > Skia seems to use a simple method that > > - just uses input as feComposite bounds; > > - and unions the bounds which definitely covers any feComposite combinations > > but the rect may be larger than optimal. > > > > The blink result seems also not optimal. > > > > If I understand it correctly, the optimal result should be (100,100 100x100) > > which is the intersection of the two inputs of feComposite. > > > > Perhaps the suboptimility is acceptable? > > This optimization was landed by the Swiffy folks in Blink a long while back for > an actual use case, back when they used SVG rather than Canvas. It's something > I've been meaning to move to Skia for some time, but haven't had a chance. It > would be unfortunate to lose it, but I don't think it would be crippling. > Unfortunately they didn't land a perf test at the time either. Thanks Stephen for the explanation. I would like to avoid the regression in production by keeping PaintInvalidator as-is and limiting the change in GeometryMapper only. There seems no big advantage for PaintInvalidator to use GeometryMapper for filters, while spv2 may benefit from GeometryMapper with filter support. Chris, does this sound good to you? https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/filter-invalidation-with-composited-container-change-expected.txt (right): https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/filter-invalidation-with-composited-container-change-expected.txt:26: "rect": [0, 0, 200, 200], On 2017/02/07 18:26:41, Stephen White wrote: > This doesn't seem correct. Shouldn't the entire div be invalidated, including > the padding induced by the filter? Or does the paint invalidation on the > document above subsume this one? If the object with filter is composited, the graphics layer bounds (8,8 200x200) doesn't cover the filter effect, and any paint invalidation rect bigger than the layer bounds is treated as for the whole layer. The filter effect will be applied by cc upon the whole layer. The one on the document is for the old visual rect of the object before it became composited. The old visual rect covered filter effect which was not composited. https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h (right): https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:45: // Returns a rect covering the pixels that can be affected by pixels in On 2017/02/07 18:26:41, Stephen White wrote: > Nit: s/pixels/destination pixels/? ie., something to indicate that this a > "forward" mapping. Done.
On 2017/02/07 at 21:23:51, wangxianzhu wrote: > https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt (right): > > https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt:11: "rect": [28, 20, 300, 300], > On 2017/02/07 18:26:41, Stephen White wrote: > > On 2017/02/03 01:38:16, Xianzhu wrote: > > > On 2017/02/01 02:59:12, chrishtr wrote: > > > > On 2017/01/26 at 23:17:45, Xianzhu wrote: > > > > > This is because now we use cc side effect mapRect, which sometimes is > > > > different from the blink side one. There seems no regressions. > > > > > > > > Do you know what is different about the two implementations of mapRect? > > Maybe > > > > there is a bug in there. > > > > > > <div class="before box"></div> > > > <svg width="0" height="0"> > > > <filter id="composite" x="-0.5" y="-0.5" width="2" height="2"> > > > <feFlood x="100" y="100" width="200" height="200" flood-color="red"/> > > > <feComposite operator="in" in="SourceGraphic"/> > > > </filter> > > > </svg> > > > > > > The difference seems for feComposite: > > > > > > blink (blink::ReferenceFilterOperation): > > > feFlood: 100,100 200x200 > > > feComposite: 100,100 200x200 > > > result is 100,100 200x200 > > > > > > skia (SkImageFilter): > > > feFlood: 100,100 200x200 > > > feComposite: 0,0 200x200 > > > result is union of the above: 0, 0, 300, 300 > > > > > > Skia seems to use a simple method that > > > - just uses input as feComposite bounds; > > > - and unions the bounds which definitely covers any feComposite combinations > > > but the rect may be larger than optimal. > > > > > > The blink result seems also not optimal. > > > > > > If I understand it correctly, the optimal result should be (100,100 100x100) > > > which is the intersection of the two inputs of feComposite. > > > > > > Perhaps the suboptimility is acceptable? > > > > This optimization was landed by the Swiffy folks in Blink a long while back for > > an actual use case, back when they used SVG rather than Canvas. It's something > > I've been meaning to move to Skia for some time, but haven't had a chance. It > > would be unfortunate to lose it, but I don't think it would be crippling. > > Unfortunately they didn't land a perf test at the time either. > > Thanks Stephen for the explanation. > > I would like to avoid the regression in production by keeping PaintInvalidator as-is and limiting the change in GeometryMapper only. There seems no big advantage for PaintInvalidator to use GeometryMapper for filters, while spv2 may benefit from GeometryMapper with filter support. > > Chris, does this sound good to you? Do you mean continuing to keep the slow path that does not use GeometryMapper in such cases? Stephen, how hard would it be to just fix Skia now? > > https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/paint/invalidation/filter-invalidation-with-composited-container-change-expected.txt (right): > > https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/paint/invalidation/filter-invalidation-with-composited-container-change-expected.txt:26: "rect": [0, 0, 200, 200], > On 2017/02/07 18:26:41, Stephen White wrote: > > This doesn't seem correct. Shouldn't the entire div be invalidated, including > > the padding induced by the filter? Or does the paint invalidation on the > > document above subsume this one? > > If the object with filter is composited, the graphics layer bounds (8,8 200x200) doesn't cover the filter effect, and any paint invalidation rect bigger than the layer bounds is treated as for the whole layer. The filter effect will be applied by cc upon the whole layer. > > The one on the document is for the old visual rect of the object before it became composited. The old visual rect covered filter effect which was not composited. > > https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h (right): > > https://codereview.chromium.org/2625133003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:45: // Returns a rect covering the pixels that can be affected by pixels in > On 2017/02/07 18:26:41, Stephen White wrote: > > Nit: s/pixels/destination pixels/? ie., something to indicate that this a > > "forward" mapping. > > Done.
On 2017/02/08 01:43:36, chrishtr wrote: > On 2017/02/07 at 21:23:51, wangxianzhu wrote: > > > https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... > > File > third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt > (right): > > > > > https://codereview.chromium.org/2625133003/diff/80001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1-expected.txt:11: > "rect": [28, 20, 300, 300], > > On 2017/02/07 18:26:41, Stephen White wrote: > > > On 2017/02/03 01:38:16, Xianzhu wrote: > > > > On 2017/02/01 02:59:12, chrishtr wrote: > > > > > On 2017/01/26 at 23:17:45, Xianzhu wrote: > > > > > > This is because now we use cc side effect mapRect, which sometimes is > > > > > different from the blink side one. There seems no regressions. > > > > > > > > > > Do you know what is different about the two implementations of mapRect? > > > Maybe > > > > > there is a bug in there. > > > > > > > > <div class="before box"></div> > > > > <svg width="0" height="0"> > > > > <filter id="composite" x="-0.5" y="-0.5" width="2" height="2"> > > > > <feFlood x="100" y="100" width="200" height="200" flood-color="red"/> > > > > <feComposite operator="in" in="SourceGraphic"/> > > > > </filter> > > > > </svg> > > > > > > > > The difference seems for feComposite: > > > > > > > > blink (blink::ReferenceFilterOperation): > > > > feFlood: 100,100 200x200 > > > > feComposite: 100,100 200x200 > > > > result is 100,100 200x200 > > > > > > > > skia (SkImageFilter): > > > > feFlood: 100,100 200x200 > > > > feComposite: 0,0 200x200 > > > > result is union of the above: 0, 0, 300, 300 > > > > > > > > Skia seems to use a simple method that > > > > - just uses input as feComposite bounds; > > > > - and unions the bounds which definitely covers any feComposite > combinations > > > > but the rect may be larger than optimal. > > > > > > > > The blink result seems also not optimal. > > > > > > > > If I understand it correctly, the optimal result should be (100,100 > 100x100) > > > > which is the intersection of the two inputs of feComposite. > > > > > > > > Perhaps the suboptimility is acceptable? > > > > > > This optimization was landed by the Swiffy folks in Blink a long while back > for > > > an actual use case, back when they used SVG rather than Canvas. It's > something > > > I've been meaning to move to Skia for some time, but haven't had a chance. > It > > > would be unfortunate to lose it, but I don't think it would be crippling. > > > Unfortunately they didn't land a perf test at the time either. > > > > Thanks Stephen for the explanation. > > > > I would like to avoid the regression in production by keeping PaintInvalidator > as-is and limiting the change in GeometryMapper only. There seems no big > advantage for PaintInvalidator to use GeometryMapper for filters, while spv2 may > benefit from GeometryMapper with filter support. > > > > Chris, does this sound good to you? > > Do you mean continuing to keep the slow path that does not use GeometryMapper in > such cases? Yes. However PaintArtifactCompositor can use the new code. > Stephen, how hard would it be to just fix Skia now? > > >
The new plan is ok as a next step, but let's get the fix in Skia soon so we can fix it. And a TODO..
lgtm
On 2017/02/08 16:46:59, chrishtr wrote: > The new plan is ok as a next step, but let's get the fix in Skia soon so we can > fix it. > And a TODO.. Done.
The CQ bit was checked by wangxianzhu@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/2625133003/#ps160001 (title: "-")
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": 160001, "attempt_start_ts": 1486602810372760,
"parent_rev": "de2a69791692ee94fa066dec6bcaf2f2075072c1", "commit_rev":
"598043a9d645cec72159c5dac7f0b21ac29ca470"}
Message was sent while issue was closed.
Description was changed from ========== Handle geometry effects of filters in GeometryMapper Some filters affects visual rects. Previously this is done by forcing slow old path for descendant of filters. BUG=637313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Handle geometry effects of filters in GeometryMapper Some filters affects visual rects. Previously this is done by forcing slow old path for descendant of filters. BUG=637313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2625133003 Cr-Commit-Position: refs/heads/master@{#449214} Committed: https://chromium.googlesource.com/chromium/src/+/598043a9d645cec72159c5dac7f0... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/598043a9d645cec72159c5dac7f0... |
