Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2379753003:
[SPInvalidation] Fix paint offset issue of descendants of composited svg root (Closed)
Description[SPInvalidation] Fix paint offset issue of descendants of composited svg root
We should only exclude container offset if it's included in the
mapping result. Svg root records paint offset in itself, creates
svgLocalToBorderBoxTransform and clears paint offset for descendants
so the result doesn't include the svg root's paint offset.
This fixes svg/custom/svg-root-with-opacity.html in
slimmingPaintInvalidation mode.
BUG=646176
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/3d949beb350914d3bbdd8b2df3da71d549a1f674
Cr-Commit-Position: refs/heads/master@{#422025}
Patch Set 1 #Patch Set 2 : Update comment #
Total comments: 6
Patch Set 3 : paintOffsetFromState #
Total comments: 2
Patch Set 4 : update tests #
Messages
Total messages: 31 (18 generated)
Description was changed from ========== [SPInvalidation] Fix paint offset issue of descendants of composited svg root We should only exclude container offset if it's included in the mapping result. Svg root records paint offset in itself, creates svgLocalToBorderBoxTransform and clears paint offset for descendants so the result doesn't include the svg root's paint offset. This fixes svg/custom/svg-root-with-opacity.html in slimmingPaintInvalidation mode. BUG=646176 ========== to ========== [SPInvalidation] Fix paint offset issue of descendants of composited svg root We should only exclude container offset if it's included in the mapping result. Svg root records paint offset in itself, creates svgLocalToBorderBoxTransform and clears paint offset for descendants so the result doesn't include the svg root's paint offset. This fixes svg/custom/svg-root-with-opacity.html in slimmingPaintInvalidation mode. BUG=646176 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 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: This issue passed the CQ dry run.
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:82: // Exclude container's paint offset from the result to get it in container's local coordinates, If possible I'd like to avoid having to do this kind of code. It's best if we can rely on the property tree being exactly right so we don't need special cases in all the users of the tree. Can we refactor how the property tree is organized so this special case is not needed? For example, maybe zero out the paint offset for foreign object?
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:82: // Exclude container's paint offset from the result to get it in container's local coordinates, On 2016/09/29 21:00:54, pdr. wrote: > If possible I'd like to avoid having to do this kind of code. It's best if we > can rely on the property tree being exactly right so we don't need special cases > in all the users of the tree. Can we refactor how the property tree is organized > so this special case is not needed? For example, maybe zero out the paint offset > for foreign object? We do zero out the paint offset in PaintPropertyTreeBuilderContext, but that'd be incorrect to zero out it in the container's localBorderBoxProperties. We need the special case because of the difference between the state of the border box space and the contents space needed by paint invalidation. Tried to make it clearer by adding an out parameter paintOffsetFromState for ObjectPaintProperties::getContentsPropertyTreeState() to indicate the paint offset that is not baked into the property tree state yet.
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/2379753003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2379753003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:130: point.moveBy(-paintOffsetFromContainerTreeState); This was a bug but doesn't matter much because the result of the function is just for detecting change of location. (This logic will be replaced by paint offset change detection soon.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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_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 wangxianzhu@chromium.org to run a CQ dry run
Updated tests.
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/2379753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:82: // Exclude container's paint offset from the result to get it in container's local coordinates, On 2016/09/29 at 22:18:06, Xianzhu wrote: > On 2016/09/29 21:00:54, pdr. wrote: > > If possible I'd like to avoid having to do this kind of code. It's best if we > > can rely on the property tree being exactly right so we don't need special cases > > in all the users of the tree. Can we refactor how the property tree is organized > > so this special case is not needed? For example, maybe zero out the paint offset > > for foreign object? > > We do zero out the paint offset in PaintPropertyTreeBuilderContext, but that'd be incorrect to zero out it in the container's localBorderBoxProperties. We need the special case because of the difference between the state of the border box space and the contents space needed by paint invalidation. > > Tried to make it clearer by adding an out parameter paintOffsetFromState for ObjectPaintProperties::getContentsPropertyTreeState() to indicate the paint offset that is not baked into the property tree state yet. Your latest patch does make this much clearer but I think it has exposed that we made a mistake in the updateSvgLocalToBorderBoxTransform patch. For html elements, we bake the paint offset into a transform node in updatePaintOffsetTranslation. Could we do that for foreign object too, instead of using svgLocalToBorderBoxTransform? Is there an issue in the ordering of the paint offset and transform nodes? Can you think of a way to fix this bug by modifying the property trees without changing PaintInvalidator.cpp? https://codereview.chromium.org/2379753003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.cpp (right): https://codereview.chromium.org/2379753003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.cpp:14: // No paint offset from the state because svgLocalToBorderTransform nit: svgLocalToBorderBoxTransform
https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:82: // Exclude container's paint offset from the result to get it in container's local coordinates, On 2016/09/29 23:31:54, pdr. wrote: > On 2016/09/29 at 22:18:06, Xianzhu wrote: > > On 2016/09/29 21:00:54, pdr. wrote: > > > If possible I'd like to avoid having to do this kind of code. It's best if > we > > > can rely on the property tree being exactly right so we don't need special > cases > > > in all the users of the tree. Can we refactor how the property tree is > organized > > > so this special case is not needed? For example, maybe zero out the paint > offset > > > for foreign object? > > > > We do zero out the paint offset in PaintPropertyTreeBuilderContext, but that'd > be incorrect to zero out it in the container's localBorderBoxProperties. We need > the special case because of the difference between the state of the border box > space and the contents space needed by paint invalidation. > > > > Tried to make it clearer by adding an out parameter paintOffsetFromState for > ObjectPaintProperties::getContentsPropertyTreeState() to indicate the paint > offset that is not baked into the property tree state yet. > > Your latest patch does make this much clearer but I think it has exposed that we > made a mistake in the updateSvgLocalToBorderBoxTransform patch. > > For html elements, we bake the paint offset into a transform node in > updatePaintOffsetTranslation. Could we do that for foreign object too, instead > of using svgLocalToBorderBoxTransform? Is there an issue in the ordering of the > paint offset and transform nodes? > > Can you think of a way to fix this bug by modifying the property trees without > changing PaintInvalidator.cpp? I think the root cause of the concept "paintOffsetFromState" in getContentsPropertyTreeState() is the difference between the state of localBorderBoxProperties (for the object itself, which records the paint offset) and contentsPropertyTreeState. SvgLocalToBorderBoxTransform is not the root cause, but happens to be the thing affected by the above difference: the transform doesn't apply to the object itself, but for children, and it resets paint offset (required), so if it's the target state of geometry mapping, the result doesn't include paint offset. Current the paint property tree state is not accurate because it doesn't represent paint offset. The latest patch added paint offset parameter to remedy that. We could avoid the special treaty of paint offset by always emit paint offsets as transform nodes, so that we can always get an accurate paint property tree state of the container for its contents, but this would cause too many transform nodes, and also breaks subpixel accumulation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:82: // Exclude container's paint offset from the result to get it in container's local coordinates, On 2016/09/30 at 00:20:24, Xianzhu wrote: > On 2016/09/29 23:31:54, pdr. wrote: > > On 2016/09/29 at 22:18:06, Xianzhu wrote: > > > On 2016/09/29 21:00:54, pdr. wrote: > > > > If possible I'd like to avoid having to do this kind of code. It's best if > > we > > > > can rely on the property tree being exactly right so we don't need special > > cases > > > > in all the users of the tree. Can we refactor how the property tree is > > organized > > > > so this special case is not needed? For example, maybe zero out the paint > > offset > > > > for foreign object? > > > > > > We do zero out the paint offset in PaintPropertyTreeBuilderContext, but that'd > > be incorrect to zero out it in the container's localBorderBoxProperties. We need > > the special case because of the difference between the state of the border box > > space and the contents space needed by paint invalidation. > > > > > > Tried to make it clearer by adding an out parameter paintOffsetFromState for > > ObjectPaintProperties::getContentsPropertyTreeState() to indicate the paint > > offset that is not baked into the property tree state yet. > > > > Your latest patch does make this much clearer but I think it has exposed that we > > made a mistake in the updateSvgLocalToBorderBoxTransform patch. > > > > For html elements, we bake the paint offset into a transform node in > > updatePaintOffsetTranslation. Could we do that for foreign object too, instead > > of using svgLocalToBorderBoxTransform? Is there an issue in the ordering of the > > paint offset and transform nodes? > > > > Can you think of a way to fix this bug by modifying the property trees without > > changing PaintInvalidator.cpp? > > I think the root cause of the concept "paintOffsetFromState" in getContentsPropertyTreeState() is the difference between the state of localBorderBoxProperties (for the object itself, which records the paint offset) and contentsPropertyTreeState. > > SvgLocalToBorderBoxTransform is not the root cause, but happens to be the thing affected by the above difference: the transform doesn't apply to the object itself, but for children, and it resets paint offset (required), so if it's the target state of geometry mapping, the result doesn't include paint offset. > > Current the paint property tree state is not accurate because it doesn't represent paint offset. The latest patch added paint offset parameter to remedy that. We could avoid the special treaty of paint offset by always emit paint offsets as transform nodes, so that we can always get an accurate paint property tree state of the container for its contents, but this would cause too many transform nodes, and also breaks subpixel accumulation. Aha! I think I understand it now. In other words, an SVGRoot's LocalBorderBoxProperties has a paint offset that is different from the paint offset of its contents (children of the root). What do you think of a final design for ObjectPaintProperties like this: struct BoxProperties { LayoutPoint paintOffset; PaintPropertyTreeState propertyTreeState; } const BoxProperties* borderBoxProperties() const { return m_localBorderBoxProperties.get(); } BoxProperties contentsBoxProperties() { return basically what your latest patch does for getContentsPropertyTreeState } If that design looks good to you, I'd like to land your latest patch as-is. After your patch lands, I will rebase my refactoring (replace GeometryPropertyTreeState with just PropertyTreeState) on top of your patch so we'll have this design.
LGTM
https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2379753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:82: // Exclude container's paint offset from the result to get it in container's local coordinates, On 2016/09/30 04:06:00, pdr. wrote: > On 2016/09/30 at 00:20:24, Xianzhu wrote: > > On 2016/09/29 23:31:54, pdr. wrote: > > > On 2016/09/29 at 22:18:06, Xianzhu wrote: > > > > On 2016/09/29 21:00:54, pdr. wrote: > > > > > If possible I'd like to avoid having to do this kind of code. It's best > if > > > we > > > > > can rely on the property tree being exactly right so we don't need > special > > > cases > > > > > in all the users of the tree. Can we refactor how the property tree is > > > organized > > > > > so this special case is not needed? For example, maybe zero out the > paint > > > offset > > > > > for foreign object? > > > > > > > > We do zero out the paint offset in PaintPropertyTreeBuilderContext, but > that'd > > > be incorrect to zero out it in the container's localBorderBoxProperties. We > need > > > the special case because of the difference between the state of the border > box > > > space and the contents space needed by paint invalidation. > > > > > > > > Tried to make it clearer by adding an out parameter paintOffsetFromState > for > > > ObjectPaintProperties::getContentsPropertyTreeState() to indicate the paint > > > offset that is not baked into the property tree state yet. > > > > > > Your latest patch does make this much clearer but I think it has exposed > that we > > > made a mistake in the updateSvgLocalToBorderBoxTransform patch. > > > > > > For html elements, we bake the paint offset into a transform node in > > > updatePaintOffsetTranslation. Could we do that for foreign object too, > instead > > > of using svgLocalToBorderBoxTransform? Is there an issue in the ordering of > the > > > paint offset and transform nodes? > > > > > > Can you think of a way to fix this bug by modifying the property trees > without > > > changing PaintInvalidator.cpp? > > > > I think the root cause of the concept "paintOffsetFromState" in > getContentsPropertyTreeState() is the difference between the state of > localBorderBoxProperties (for the object itself, which records the paint offset) > and contentsPropertyTreeState. > > > > SvgLocalToBorderBoxTransform is not the root cause, but happens to be the > thing affected by the above difference: the transform doesn't apply to the > object itself, but for children, and it resets paint offset (required), so if > it's the target state of geometry mapping, the result doesn't include paint > offset. > > > > Current the paint property tree state is not accurate because it doesn't > represent paint offset. The latest patch added paint offset parameter to remedy > that. We could avoid the special treaty of paint offset by always emit paint > offsets as transform nodes, so that we can always get an accurate paint property > tree state of the container for its contents, but this would cause too many > transform nodes, and also breaks subpixel accumulation. > > Aha! I think I understand it now. In other words, an SVGRoot's > LocalBorderBoxProperties has a paint offset that is different from the paint > offset of its contents (children of the root). > > What do you think of a final design for ObjectPaintProperties like this: > struct BoxProperties { > LayoutPoint paintOffset; > PaintPropertyTreeState propertyTreeState; > } > const BoxProperties* borderBoxProperties() const { return > m_localBorderBoxProperties.get(); } > BoxProperties contentsBoxProperties() { return basically what your latest patch > does for getContentsPropertyTreeState } > > If that design looks good to you, I'd like to land your latest patch as-is. > After your patch lands, I will rebase my refactoring (replace > GeometryPropertyTreeState with just PropertyTreeState) on top of your patch so > we'll have this design. The design looks great! Thanks!
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [SPInvalidation] Fix paint offset issue of descendants of composited svg root We should only exclude container offset if it's included in the mapping result. Svg root records paint offset in itself, creates svgLocalToBorderBoxTransform and clears paint offset for descendants so the result doesn't include the svg root's paint offset. This fixes svg/custom/svg-root-with-opacity.html in slimmingPaintInvalidation mode. BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Fix paint offset issue of descendants of composited svg root We should only exclude container offset if it's included in the mapping result. Svg root records paint offset in itself, creates svgLocalToBorderBoxTransform and clears paint offset for descendants so the result doesn't include the svg root's paint offset. This fixes svg/custom/svg-root-with-opacity.html in slimmingPaintInvalidation mode. BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/3d949beb350914d3bbdd8b2df3da71d549a1f674 Cr-Commit-Position: refs/heads/master@{#422025} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3d949beb350914d3bbdd8b2df3da71d549a1f674 Cr-Commit-Position: refs/heads/master@{#422025} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
