Don't pass subpixel offsets through non-translation transforms (issue 2847873002 by wangxianzhu@chromium.org)

1 view
Skip to first unread message

wangx...@chromium.org

unread,
Apr 27, 2017, 6:53:56 PM4/27/17
to wangx...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, blink-rev...@chromium.org
Reviewers:
CL: https://codereview.chromium.org/2847873002/


https://codereview.chromium.org/2847873002/diff/20001/third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt
File
third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt
(right):

https://codereview.chromium.org/2847873002/diff/20001/third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt#newcode16
third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt:16:
"rect": [23, 8, 41, 40],
*This is disable-spinvalidation*.

The invalidation rect is incorrect or inconsistent with where we paint
the child. The child is painted at (8,8 40x40) and (8.3,8 40x40) before
and after repaintTest, respectively. The local offset (0.4, 0) is
rounded off when snapping to "pixels" in the child's space during
painting.

The invalidation rect is from the slow path
(MapToVisualRectInAncestorSpace()) in PaintInvalidationState.

PaintInvalidator MapToVisualRectInAncestorSpace
LocalVisualRect (0,0 1x1) (0,0 1x1)
+local offset (0.4,0 1x1) (0.4,0 1x1)
EnclosingIntRect (0,0 2x1)
Transformed (0,0 80x40) (~15,0 40x40)
EnclosingBoundingBox (~15,0 41x40)
+container offset (8.3,8 80x40) (~23,8 41x40)

The difference comes from the EnclosingIntRect step. Should we call
EnclosingIntRect() before applying transform in
MapToVisualRectInAncestorSpace? It was changed to EnclosingBoundingRect
after transform in https://codereview.chromium.org/2727093002.

Description:
Don't pass subpixel offsets through non-translation transforms

Non-translation transforms will change direction and/or scale, etc.
of offsets thus making subpixel accumulation through the transform
meaningless.

This CL addresses the issue in PaintLayerPainter and
PaintPropertyTreeBuilder. We still need to address the issue in
CompositedLayerMapping (crbug.com/716163).

BUG=710665
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2


Affected files (+433, -11 lines):
A third_party/WebKit/LayoutTests/paint/invalidation/subpixel-offset-scaled-transform.html
A third_party/WebKit/LayoutTests/paint/invalidation/subpixel-offset-scaled-transform-expected.png
A third_party/WebKit/LayoutTests/paint/invalidation/subpixel-offset-scaled-transform-expected.txt
A third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.png
A third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt
M third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp


wangx...@chromium.org

unread,
Apr 27, 2017, 7:06:58 PM4/27/17
to chri...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, blink-rev...@chromium.org

chri...@chromium.org

unread,
Apr 27, 2017, 7:12:40 PM4/27/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
Looks good, but please add a test that exercises the composited transform path.
I think you'll need to adjust corresponding code in
CompositedLayerMapping/PaintLayer
to get the same effect.

https://codereview.chromium.org/2847873002/

wangx...@chromium.org

unread,
Apr 27, 2017, 8:14:59 PM4/27/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
On 2017/04/27 23:12:39, chrishtr wrote:
> Looks good, but please add a test that exercises the composited transform
path.
> I think you'll need to adjust corresponding code in
> CompositedLayerMapping/PaintLayer
> to get the same effect.

chri...@chromium.org

unread,
Apr 27, 2017, 10:23:50 PM4/27/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
On 2017/04/28 at 00:14:59, wangxianzhu wrote:
> On 2017/04/27 23:12:39, chrishtr wrote:
> > Looks good, but please add a test that exercises the composited transform
path.
> > I think you'll need to adjust corresponding code in
> > CompositedLayerMapping/PaintLayer
> > to get the same effect.
>
> I would like to add tests for the composited issue and address it in another
patch. I filed crbug.com/716163 for it.

As long as it happens in the same Chrome release, ok.

chri...@chromium.org

unread,
Apr 27, 2017, 10:23:53 PM4/27/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

https://codereview.chromium.org/2847873002/diff/20001/third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt
File
third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt
(right):

https://codereview.chromium.org/2847873002/diff/20001/third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt#newcode16
third_party/WebKit/LayoutTests/virtual/disable-spinvalidation/paint/invalidation/subpixel-offset-scaled-transform-expected.txt:16:
"rect": [23, 8, 41, 40],
On 2017/04/27 at 22:53:55, Xianzhu wrote:
> *This is disable-spinvalidation*.
>
> The invalidation rect is incorrect or inconsistent with where we paint
the child. The child is painted at (8,8 40x40) and (8.3,8 40x40) before
and after repaintTest, respectively. The local offset (0.4, 0) is
rounded off when snapping to "pixels" in the child's space during
painting.
>
> The invalidation rect is from the slow path
(MapToVisualRectInAncestorSpace()) in PaintInvalidationState.
>
> PaintInvalidator MapToVisualRectInAncestorSpace
> LocalVisualRect (0,0 1x1) (0,0 1x1)
> +local offset (0.4,0 1x1) (0.4,0 1x1)
> EnclosingIntRect (0,0 2x1)
> Transformed (0,0 80x40) (~15,0 40x40)
> EnclosingBoundingBox (~15,0 41x40)
> +container offset (8.3,8 80x40) (~23,8 41x40)
>
> The difference comes from the EnclosingIntRect step. Should we call
EnclosingIntRect() before applying transform in
MapToVisualRectInAncestorSpace? It was changed to EnclosingBoundingRect
after transform in https://codereview.chromium.org/2727093002.

I think in general it has to be that way, at least for
non-spinvalidation, because the paint
offset is not known, so invalidations for 2D transforms that are
"identity or translation" would
incur under-invalidations.

https://codereview.chromium.org/2847873002/

wangx...@chromium.org

unread,
Apr 28, 2017, 3:35:28 PM4/28/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

https://codereview.chromium.org/2847873002/diff/40001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2847873002/diff/40001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1248
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1248:
FloatQuad(transform_state.LastPlanarQuad().EnclosingBoundingBox()));
I'm not sure if this is correct:
1. Is it OK to flatten before applying the transform?
2. What if preserve3D and the painter snaps to pixels?

https://codereview.chromium.org/2847873002/

chri...@chromium.org

unread,
Apr 28, 2017, 3:52:04 PM4/28/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
On 2017/04/28 at 19:35:28, wangxianzhu wrote:
>
https://codereview.chromium.org/2847873002/diff/40001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
>
>
https://codereview.chromium.org/2847873002/diff/40001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1248
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:1248:
FloatQuad(transform_state.LastPlanarQuad().EnclosingBoundingBox()));
> I'm not sure if this is correct:
> 1. Is it OK to flatten before applying the transform?

Only if preserve3D is false.


> 2. What if preserve3D and the painter snaps to pixels?

If preserve3D is true and it matters (i.e. there is a 3D transform), then this
is not an SPv1 use-case. This code is just for SPv1 right?


https://codereview.chromium.org/2847873002/

wangx...@chromium.org

unread,
Apr 28, 2017, 4:42:13 PM4/28/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
For the purpose of visual rect calculation for paint invalidation, this code is
just for SPv1 (including SPInvalidation).

For non-paint-invalidation purposes, the function is also used on SPv2, called
from LayoutObject::SelectionRectInViewCoordinates(),
LayoutObject::AbsoluteVisualRect() etc. Whether the rect covers snapped pixels
might be insiginificant.

Is preserve3D always false for SPv1?

https://codereview.chromium.org/2847873002/

chri...@chromium.org

unread,
Apr 28, 2017, 4:49:29 PM4/28/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
It matters, but for SPv2 and SPInvalidation we have an accurate paint offset
pre-computed, so I think we should be able to
compute the exact answer if needed.


>
> Is preserve3D always false for SPv1?

No, but 3D transforms are irrelevant to SPv1 paint invalidation, because they
never cross paint invalidation containers.

https://codereview.chromium.org/2847873002/

wangx...@chromium.org

unread,
Apr 28, 2017, 6:28:34 PM4/28/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
Does the CL look good to you?
Do you mean we should switch the coordinate mapping functions to using
GeometryMapper/paint-properties in the future? I wonder if we have cases that
the functions are needed before PrePaint.

Anyway this CL seems to not cause any regressions about this.

chri...@chromium.org

unread,
Apr 28, 2017, 7:25:26 PM4/28/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
No, I just mean that we have a means of making it accurate in cases involving 3D
if necessary.

chri...@chromium.org

unread,
Apr 28, 2017, 7:26:44 PM4/28/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1241
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1241: // Use
EnclosingBoundingBox because we cannot properly compute pixel
Why does this have to be first now? Don't see it yet.

https://codereview.chromium.org/2847873002/

wangx...@chromium.org

unread,
May 1, 2017, 2:13:43 PM5/1/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1241
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1241: // Use
EnclosingBoundingBox because we cannot properly compute pixel
On 2017/04/28 23:26:44, chrishtr wrote:
> Why does this have to be first now? Don't see it yet.

This is to fix the MapToVisualRectInAncestorSpace() problem described in
#4. I thought you agreed the solution, no?

https://codereview.chromium.org/2847873002/

chri...@chromium.org

unread,
May 1, 2017, 4:22:28 PM5/1/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1204
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1204: // 6.
Transform flattening.
Not an issue you created, but could you make this change?

s/Transform flattening/Transform+flattening/


https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1241
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1241: // Use
EnclosingBoundingBox because we cannot properly compute pixel
On 2017/05/01 at 18:13:42, Xianzhu wrote:
> On 2017/04/28 23:26:44, chrishtr wrote:
> > Why does this have to be first now? Don't see it yet.
>
> This is to fix the MapToVisualRectInAncestorSpace() problem described
in #4. I thought you agreed the solution, no?

Oh I see. Yes I did make this change in 2727093002; fixing it to be back
the way it was in non-preserve3d
makes sense.

https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1244
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1244: // include a
scale.
Please add a comment saying that pixel-snapping like this only makes
sense
for non-preserve3D, and also that it's applicable only for SPv1
paint invalidation use cases, which have no preserve3D.

https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1246
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1246:
transform_state.Flatten();
if (!preserve3D && !RuntimeEnabledFeatures::slimmingPaintV2Enabled())

https://codereview.chromium.org/2847873002/

wangx...@chromium.org

unread,
May 1, 2017, 6:03:32 PM5/1/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1204
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1204: // 6.
Transform flattening.
On 2017/05/01 20:22:28, chrishtr wrote:
> Not an issue you created, but could you make this change?
>
> s/Transform flattening/Transform+flattening/

The steps 1 to 4 are actually generate the transform to be applied in
step 6, so I changed the steps to one step "Generate transform" and
sub-steps under the step.

Also moved step 5 to step 1 because it actually a preparation before
applying transform.
On 2017/05/01 20:22:28, chrishtr wrote:
> Please add a comment saying that pixel-snapping like this only makes
sense
> for non-preserve3D, and also that it's applicable only for SPv1
> paint invalidation use cases, which have no preserve3D.

Done.


https://codereview.chromium.org/2847873002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1246
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1246:
transform_state.Flatten();
On 2017/05/01 20:22:28, chrishtr wrote:
> if (!preserve3D && !RuntimeEnabledFeatures::slimmingPaintV2Enabled())

wangx...@chromium.org

unread,
May 1, 2017, 6:04:04 PM5/1/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

chri...@chromium.org

unread,
May 1, 2017, 6:13:43 PM5/1/17
to wangx...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
lgtm




https://codereview.chromium.org/2847873002/diff/100001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2847873002/diff/100001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1200
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1200: // 2. Generate
transformation matrix by combining:
"combining, in this order"

https://codereview.chromium.org/2847873002/

wangx...@chromium.org

unread,
May 1, 2017, 6:59:25 PM5/1/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

https://codereview.chromium.org/2847873002/diff/100001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2847873002/diff/100001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1200
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1200: // 2. Generate
transformation matrix by combining:
On 2017/05/01 22:13:42, chrishtr wrote:
> "combining, in this order"

Done.

https://codereview.chromium.org/2847873002/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 1, 2017, 7:03:38 PM5/1/17
to wangx...@chromium.org, chri...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

commit-bot@chromium.org via codereview.chromium.org

unread,
May 1, 2017, 8:53:13 PM5/1/17
to wangx...@chromium.org, chri...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com

wangx...@chromium.org

unread,
May 3, 2017, 11:41:42 AM5/3/17
to chri...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com
A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2859483004/ by wangx...@chromium.org.

The reason for reverting is: BUG=717882.

https://codereview.chromium.org/2847873002/
Reply all
Reply to author
Forward
0 new messages