Handle pixel snapping under scale transform in GeometryMapper |
|||||
Issue descriptionThis is basically a GeometryMapper version of bug 622232 which was fixed with the following code in LayoutBox::mapToVisualRectInAncestorSpace(). if (hasLayer() && layer()->transform()) { // Use enclosingIntRect because we cannot properly compute pixel snapping for painted elements within // the transform since we don't know the desired subpixel accumulation at this point, and the transform may // include a scale. rect = LayoutRect(layer()->transform()->mapRect(enclosingIntRect(rect))); } We need an equivalent (or a better?) way to handle the issue. Tried to add enclosingIntRect() but it also unexpectedly affected svg. Perhaps pixel-snapping should be another effect paint property?
,
Oct 10 2016
chrishtr@ what's the more desired operation than the following in LayoutBox::
mapToVisualrectInAncestorSpace()?
if (hasLayer() && layer()->transform()) {
// Use enclosingIntRect because we cannot properly compute pixel snapping
// for painted elements within the transform since we don't know the desired
// subpixel accumulation at this point, and the transform may include a
// scale.
rect = LayoutRect(layer()->transform()->mapRect(enclosingIntRect(rect)));
}
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBox.cpp?rcl=0&l=2327)
It seems that we can do more accurate operation in GeometryMapper because we know the subpixel accumulation.
,
Oct 10 2016
When computing paint offset across transforms, the following code executes: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp?q=paintpropertytreebuilder.cpp&sq=package:chromium&dr&l=235 Therefore in a case like this: <div id=parent style="transform: ..."> <div id=child> </div> </div> The information necessary to map from the child space and across the transform to screen coordinates should be encoded exactly in these values: childBox.objectPaintProperties().paintOffset parentBox.objectPaintProperties().paintOffsetTranslation() parentBox.objectPaintProperties().transform() put together. As for pixel snapping, pixel snapping of the child occurs in its local paint offset space, so we should be able to map to screen coordinates with this operation: pixelSnappedChildBox = pixelSnappedIntRect(LayoutRect(childBox.frameRect()).moveBy(childBox.objectPaintProperties(). localBorderBoxProperties()->paintOffset)) pixelSnappedScreenCoordinates = geometryMapper.mapToVisualRectInDestinationSpace(pixelSnappedChildBox, childBox.objectPaintProperties().contentsProperties(), layoutView.objectPaintProperties().contentsProperties()) Does this work? The results will indeed be different from SPv1 because SPv1 is a bit lossy in this operation.
,
Oct 10 2016
,
Oct 10 2016
This is simpler than I expected. I though we needed to apply pixel snapping at intermediate transform nodes from a descendant to an ancestor. What kinds of objects should pixel snapping apply to? Where pixel snapping should occur for SVG objects? (at SVGRoot?)
,
Oct 11 2016
Your question about what is pixel snapped reminds me that we can't really assume
pixel snapping is happening. As a result, I recommend just using enclosingIntRect rather than pixelSnappedIntRect of
the visual overflow of an element before mapping to screen space.
BTW here is one more example I just thought through to help convince myself things are ok:
<div id=parent style="transform: ...: left: 0.1px">
<div id=child style="transform: ...; position: relative; left: 0.2px">
<div id=grandchild></div>
</div>
</div>
In this case I think we'll end up with these properties:
parent.objectPaintProperties():
paintOffsetTransform = identity matrix // Rounding off 0.1 yields 0
paintOffset = (0.1px, 0px)
parent.paintOffset for children = (0.1px, 0px) // residual after subtracting off paintOffsetTransform
child.objectPaintProperties():
paintOffsetTransform = identity matrix // Rounding off 0.1 yields 0
paintOffset = (0.3px, 0px) // 0.1 + 0.2
child.paintOffset for children = (0.3px, 0px) // residual after subtracting off paintOffsetTransform
grandchild.objectPaintProperties():
paintOffset = (0.3px, 0px)
So as we hoped, the sum of the subpixel accumulations come through both transforms.
,
Oct 11 2016
Second comment is incorrect. It should say "Rounding off 0.3 yields 0".
,
Oct 21 2016
About pixel snapping of svg, for example: <svg style="position: absolute; top: 0.6px; left: 0.3px" width="100" height="100"> <rect width="100" height="100" fill="green" /> </svg> Based on the painting result, the rect's visual rect is (0,1 100x100). Should we just bake the rounded paint offset into svgLocalToBorderBoxTransform and discard the fractional part because we don't need to propagate the fractional part to svg descendants? Should we propagate the fractional part to foreign objects?
,
Oct 21 2016
pdr says yes to the first, and no to the second. We looked at PaintPropertyTreeBuilder.cpp and it seems it might not be doing the first correctly.
,
Oct 21 2016
Yeah, I agree that doing the pixel snapping in a transform is a good idea. I thought we already were for the svg root, but it looks like I was wrong. For foreign object, I am not sure yet. Can we just match the current behavior for now?
,
Oct 21 2016
(When I spoke with Chris, I was wrong to say "no" to the second option.)
,
Oct 21 2016
http://output.jsbin.com/sezatil (based on pdr@'s http://jsbin.com/yupavef) shows the current behavior of pixel-snapping of svg-root, svg foreign object and html descendants of svg foreign object: 1. svg x=0.3px; foreign x=0.4px; div x=0.5px result: absolute-x(svg) = 0; absolute-x(foreign) = 0; absolute-x(div) = 1 2. svg x=0.3px; foreign x=0.5px; div x=0.5px result: absolute-x(svg) = 0; absolute-x(foreign) = 1; absolute-x(div) = 2 3. svg x=0.3px; foreign x=0.4px; div x=0.3px result: absolute-x(svg) = 0; absolute-x(foreign) = 0; absolute-x(div) = 0 Based on the above results, we don't propagate subpixel accumulation to descendants of svgroot and foreign objects. Instead we snap svgroot to whole pixel and discard the offset fraction, and do the same thing at foreign object.
,
Oct 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd commit 8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd Author: wangxianzhu <wangxianzhu@chromium.org> Date: Wed Oct 26 21:50:43 2016 [SPInvalidation] Handle pixel-snapping of paint invalidation rects In SlimmingPaintInvalidation mode we can snap a paint invalidation rect to whole pixels more accurately by applying paint offset to the local paint invalidation rect and expanding the rect to whole pixels before mapping it to backing, as discussed in https://bugs.chromium.org/p/chromium/issues/detail?id=648769#c6. This causes a new issue that the paint invalidation rect may cover extra pixels causing incremental invalidation not applicable. To avoid the issue, now force full paint invalidation in the case. Also confirmed that our current handling of SVG pixel snapping is already correct. In PaintPropertyTreeBuilder, we bake the current paint offset into transformToPixelSnappedBorderBox() of SVGRoot which correctly handles pixel snapping. For SVG foreign objects, pixel snapping of geometries is done during layout. BUG= 648769 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2449953005 Cr-Commit-Position: refs/heads/master@{#427827} [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=SlimmingPaintInvalidation [add] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=SlimmingPaintInvalidation/paint/invalidation/border-radius-repaint-2-expected.txt [add] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=SlimmingPaintInvalidation/paint/invalidation/flexbox/repaint-expected.txt [add] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=SlimmingPaintInvalidation/paint/invalidation/flexbox/repaint-rtl-column-expected.txt [add] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=SlimmingPaintInvalidation/paint/invalidation/line-flow-with-floats-4-expected.txt [add] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=SlimmingPaintInvalidation/paint/invalidation/line-flow-with-floats-5-expected.txt [add] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=SlimmingPaintInvalidation/paint/invalidation/repaint-during-scroll-with-zoom-expected.txt [add] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=SlimmingPaintInvalidation/paint/invalidation/window-resize-percent-html-expected.txt [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/core/layout/LayoutObject.cpp [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/core/layout/LayoutObject.h [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/core/paint/PaintInvalidator.cpp [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/core/paint/PaintInvalidator.h [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp [modify] https://crrev.com/8f9c94471ef61f9bc6ddd7ddb3b485c3b76b30cd/third_party/WebKit/Source/platform/testing/RuntimeEnabledFeaturesTestHelpers.h
,
Oct 26 2016
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by wangxianzhu@chromium.org
, Sep 22 2016