New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 648769 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 646176



Sign in to add a comment

Handle pixel snapping under scale transform in GeometryMapper

Project Member Reported by wangxianzhu@chromium.org, Sep 20 2016

Issue description

This 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?
 
This affects the following tests with slimmingPaintInvalidation enabled (run-webkit-tests --additional-driver-flag=--enable-blink-feature=slimmingPaintInvalidation <tests>):

paint/invalidation/invalidation-with-scale-transform.html
paint/invalidation/transform-inline-layered-child.html
paint/invalidation/invalidation-with-scale-transform.html

I'm not sure if the following failures are also caused by this bug:
paint/invalidation/svg/animated-path-inside-transformed-html.xhtml
paint/invalidation/svg/js-late-gradient-and-object-creation.svg
paint/invalidation/svg/js-late-pattern-and-object-creation.svg
paint/invalidation/svg/tabgroup.svg
paint/invalidation/svg/window.svg

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.
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.
Cc: wangxianzhu@chromium.org
Cc: pdr@chromium.org
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?)
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.


Cc: chrishtr@chromium.org
Owner: wangxianzhu@chromium.org
Second comment is incorrect. It should say "Rounding off 0.3 yields 0".
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? 
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.

Comment 10 by pdr@chromium.org, 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?

Comment 11 by pdr@chromium.org, Oct 21 2016

(When I spoke with Chris, I was wrong to say "no" to the second option.)
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.


Project Member

Comment 13 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment