Bug 101779 – Push pixel snapping logic into TransformState

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

Instead of accumulating every offset generated in mapLocalToContainer in the TransformState, use a LayoutSize until we are at the end condition or have a transform. This allows us to snap sub-pixel offsets correctly (at the end) instead of the current solution, which incorrectly snaps as it goes up. This functionally incorrect behavior can lead to repaint artifacts, especially in Widgets that have their ultimate framerect incorrectly calculated when nested in multiple levels of sub-pixel containers. This should also improve performance since we only have to apply the transform to n+1 offsets where n is the number of transforms in the chain, whereas currently we have to apply it O(m) where m is the number of renderers in the chain, and some elements accumulate multiple offsets.

[Levi Weintraub]

Created attachment 173358 [details]
Patch

[Emil A Eklund]

Merge failures, could you try to update and upload a new patch? I'd like to see what the EWS bots think of it.

[Levi Weintraub]

Created attachment 173366 [details]
Patch

[Levi Weintraub]

Created attachment 173369 [details]
Patch

[Levi Weintraub]

(In reply to comment #4)
> Created an attachment (id=173369) [details]
> Patch

Now with 100% more applying!

[Emil A Eklund]

Comment on attachment 173366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173366&action=review

I like this change, simplifies things quite a bit and might even be a performance win.

> Source/WebCore/rendering/RenderBox.cpp:1272
> +void RenderBox::mapLocalToContainer(const RenderLayerModelObject* repaintContainer, TransformState& transformState, MapCoordinatesFlags mode, bool* wasFixed, LayoutSize accumulatedOffset) const

Why are you passing this by value? To avoid having to create a local copy?

> Source/WebCore/rendering/RenderBox.cpp:1278
> +        return transformState.move((mode & SnapOffsetForTransforms) ? roundedIntSize(accumulatedOffset) : accumulatedOffset, preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform);

This is pretty hard to follow with the nested ternary conditions. How about you make one of them an actual if statement or an inline function?

> Source/WebCore/rendering/RenderBox.cpp:-1309
> -    bool preserve3D = mode & UseTransforms && (o->style()->preserves3D() || style()->preserves3D());

Can you expand on why you don't need to special case preserve3d anymore?

[Levi Weintraub]

Comment on attachment 173366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173366&action=review

>> Source/WebCore/rendering/RenderBox.cpp:1272
>> +void RenderBox::mapLocalToContainer(const RenderLayerModelObject* repaintContainer, TransformState& transformState, MapCoordinatesFlags mode, bool* wasFixed, LayoutSize accumulatedOffset) const
> 
> Why are you passing this by value? To avoid having to create a local copy?

That's right. We could alternatively create a local variable at the start of each implementation, or make all versions of this protected and a public version that instantiates a local variable and passes by reference.

>> Source/WebCore/rendering/RenderBox.cpp:1278
>> +        return transformState.move((mode & SnapOffsetForTransforms) ? roundedIntSize(accumulatedOffset) : accumulatedOffset, preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform);
> 
> This is pretty hard to follow with the nested ternary conditions. How about you make one of them an actual if statement or an inline function?

I can't make the entire line an inline function as the virtual function is defined in RenderObject.h, but I could have the ternary resolution inlined there. I'd prefer that to a slew of if/else in these implementations.

>> Source/WebCore/rendering/RenderBox.cpp:-1309
>> -    bool preserve3D = mode & UseTransforms && (o->style()->preserves3D() || style()->preserves3D());
> 
> Can you expand on why you don't need to special case preserve3d anymore?

Look up! It just moved higher in the function.

[Early Warning System Bot]

Comment on attachment 173369 [details]
Patch

Attachment 173369 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14792209

[Emil A Eklund]

(In reply to comment #7)
> (From update of attachment 173366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173366&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:1272
> >> +void RenderBox::mapLocalToContainer(const RenderLayerModelObject* repaintContainer, TransformState& transformState, MapCoordinatesFlags mode, bool* wasFixed, LayoutSize accumulatedOffset) const
> > 
> > Why are you passing this by value? To avoid having to create a local copy?
> 
> That's right. We could alternatively create a local variable at the start of each implementation, or make all versions of this protected and a public version that instantiates a local variable and passes by reference.

Makes sense, I prefer they way you have it over having a local copy.


> >> Source/WebCore/rendering/RenderBox.cpp:1278
> >> +        return transformState.move((mode & SnapOffsetForTransforms) ? roundedIntSize(accumulatedOffset) : accumulatedOffset, preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform);
> > 
> > This is pretty hard to follow with the nested ternary conditions. How about you make one of them an actual if statement or an inline function?
> 
> I can't make the entire line an inline function as the virtual function is defined in RenderObject.h, but I could have the ternary resolution inlined there. I'd prefer that to a slew of if/else in these implementations.

That would be great if you could make it work, there is quite a bit of repeated code as it is and it isn't terribly readable.

> >> Source/WebCore/rendering/RenderBox.cpp:-1309
> >> -    bool preserve3D = mode & UseTransforms && (o->style()->preserves3D() || style()->preserves3D());
> > 
> > Can you expand on why you don't need to special case preserve3d anymore?
> 
> Look up! It just moved higher in the function.

Ah, of course. Thanks!

[Early Warning System Bot]

Comment on attachment 173369 [details]
Patch

Attachment 173369 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14787312

[Build Bot]

Comment on attachment 173369 [details]
Patch

Attachment 173369 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14786339

[Build Bot]

Comment on attachment 173369 [details]
Patch

Attachment 173369 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14792213

[EFL EWS Bot]

Comment on attachment 173369 [details]
Patch

Attachment 173369 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14787322

[kov's GTK+ EWS bot]

Comment on attachment 173369 [details]
Patch

Attachment 173369 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14777613

[Levi Weintraub]

Created attachment 173418 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 173418 [details]
Patch

Why isn't the accumulated offset part of the TransformState?

[Levi Weintraub]

(In reply to comment #16)
> (From update of attachment 173418 [details])
> Why isn't the accumulated offset part of the TransformState?

That would probably make for a cleaner change, you're right. I'll take a stab on Monday, thanks!

[Simon Fraser (smfr)]

Comment on attachment 173418 [details]
Patch

Maybe TransformState should work in LayoutUnits, and only use floats when it has to convert through transforms?

Note that there are pure-float clients of TransformState though (like GraphicsLayerCA).

[Simon Fraser (smfr)]

I'm trying to use RenderGeometryMap in more places and running into cases where mapping via renderers and mapping via RenderGeometryMap don't match. Some of this is because of the SnapOffsetForTransforms confusion (bug 101864).

I'd like to be able to use the same RenderGeometryMap to map both repaint rects and points. I'm not sure if SnapOffsetForTransforms should be different for those two operations, but if so, then we somehow need to allow RenderGeometryMap to map with SnapOffsetForTransforms on or off.

[Simon Fraser (smfr)]

One important point here is that if you touch the mapToContainer methods, you probably also need to fix up RenderGeometryMap.

[Levi Weintraub]

(In reply to comment #20)
> One important point here is that if you touch the mapToContainer methods, you probably also need to fix up RenderGeometryMap.

RenderGeometryMap indeed needed to be fixed as well. Sadly, making this change in TransformState results in a solution that's less clean, with consumers of that class needing more awareness of its state or a parallel data structure for accumulating and returning proper values.

[Levi Weintraub]

Created attachment 176578 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 176578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176578&action=review

I don't see how this works correctly with RenderGeometryMap, since I'd expect the pushMappingToContainer() methods to require similar changes to mapLocalToContainer() (ideally the code would be shared).

> LayoutTests/ChangeLog:9
> +        Update the sub-pixel-iframe-copy-on-scroll test to also test a deeply-nested iframe to ensure
> +        we're properly pixel snapping mapped coordinates.

What a about a test with interleaved transforms and subpixel pushers?

[Levi Weintraub]

Comment on attachment 176578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176578&action=review

You're right about pushMappingToContainer. RenderBox and RenderInline's implementations both needed updates.

>> LayoutTests/ChangeLog:9
>> +        we're properly pixel snapping mapped coordinates.
> 
> What a about a test with interleaved transforms and subpixel pushers?

Will do.

[Levi Weintraub]

Created attachment 177550 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 177550 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177550&action=review

A general problem I have with this code is that painting and coordinate mapping don't share a common function for pixel snapping, so it's really hard to know if they are doing the same thing, and whether RenderGeometryMap matches what they both do.

> Source/WebCore/ChangeLog:66
> +        * platform/graphics/LayoutSize.h:
> +        (LayoutSize):
> +        (WebCore::LayoutSize::expand):
> +        * rendering/RenderBox.cpp:
> +        (WebCore::RenderBox::mapLocalToContainer):
> +        (WebCore::RenderBox::pushMappingToContainer):
> +        * rendering/RenderBox.h:
> +        (RenderBox):
> +        * rendering/RenderGeometryMap.cpp:
> +        (WebCore::RenderGeometryMap::mapToContainer):
> +        * rendering/RenderInline.cpp:
> +        (WebCore::RenderInline::mapLocalToContainer):
> +        (WebCore::RenderInline::pushMappingToContainer):
> +        * rendering/RenderInline.h:
> +        (RenderInline):
> +        * rendering/RenderObject.cpp:
> +        (WebCore::RenderObject::mapLocalToContainer):
> +        * rendering/RenderObject.h:
> +        (RenderObject):
> +        (WebCore::RenderObject::adjustOffsetForSnapping):
> +        * rendering/RenderView.cpp:
> +        (WebCore::RenderView::mapLocalToContainer):
> +        * rendering/RenderView.h:
> +        (RenderView):
> +        * rendering/svg/RenderSVGForeignObject.cpp:
> +        (WebCore::RenderSVGForeignObject::mapLocalToContainer):
> +        * rendering/svg/RenderSVGForeignObject.h:
> +        (RenderSVGForeignObject):
> +        * rendering/svg/RenderSVGInline.cpp:
> +        (WebCore::RenderSVGInline::mapLocalToContainer):
> +        * rendering/svg/RenderSVGInline.h:
> +        (RenderSVGInline):
> +        * rendering/svg/RenderSVGModelObject.cpp:
> +        (WebCore::RenderSVGModelObject::mapLocalToContainer):
> +        * rendering/svg/RenderSVGModelObject.h:
> +        (RenderSVGModelObject):
> +        * rendering/svg/RenderSVGRoot.cpp:
> +        (WebCore::RenderSVGRoot::mapLocalToContainer):
> +        * rendering/svg/RenderSVGRoot.h:
> +        (RenderSVGRoot):
> +        * rendering/svg/RenderSVGText.cpp:
> +        (WebCore::RenderSVGText::mapLocalToContainer):
> +        * rendering/svg/RenderSVGText.h:
> +        (RenderSVGText):
> +        * rendering/svg/SVGRenderSupport.cpp:
> +        (WebCore::SVGRenderSupport::mapLocalToContainer):
> +        * rendering/svg/SVGRenderSupport.h:
> +        (SVGRenderSupport):

Please clean up this list. Per-function comments are recommended.

> Source/WebCore/rendering/RenderBox.cpp:1388
> -        return;
> +        return transformState.move(adjustOffsetForSnapping(mode, adjustedOffset), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform);

It's a shame that all the implementations of mapLocalToContainer() have to handle this boundary condition. I think it would be better to push this out to functions that initiate calls to the recursive mapLocalToContainer() (there should be few). Those callers will always flatten, since they are the end of the mapping chain.

> Source/WebCore/rendering/RenderBox.cpp:1422
> +        transformState.move(adjustOffsetForSnapping(mode, adjustedOffset), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform);

It's a shame to call move() when adjustOffsetForSnapping() might be zero. I think it would be better to get adjustOffsetForSnapping(), and multiply it into t, then just move once.

> Source/WebCore/rendering/RenderBox.cpp:1479
>      if (shouldUseTransformFromContainer(container)) {
>          TransformationMatrix t;
>          getTransformFromContainer(container, containerOffset, t);
> +        adjustmentForSkippedAncestor = adjustOffsetForSnapping(geometryMap.mapCoordinatesFlags(), adjustmentForSkippedAncestor);
>          t.translateRight(adjustmentForSkippedAncestor.width(), adjustmentForSkippedAncestor.height());

Why doesn't pushMappingToContainer() have all the other snapping logic that mapLocalToContainer() does, e.g. for the non-transform case?

[Levi Weintraub]

Comment on attachment 177550 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177550&action=review

>> Source/WebCore/rendering/RenderBox.cpp:1388
>> +        return transformState.move(adjustOffsetForSnapping(mode, adjustedOffset), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform);
> 
> It's a shame that all the implementations of mapLocalToContainer() have to handle this boundary condition. I think it would be better to push this out to functions that initiate calls to the recursive mapLocalToContainer() (there should be few). Those callers will always flatten, since they are the end of the mapping chain.

Okay. I didn't want to muddy the contract, but I'm happy to make that change.

>> Source/WebCore/rendering/RenderBox.cpp:1422
>> +        transformState.move(adjustOffsetForSnapping(mode, adjustedOffset), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform);
> 
> It's a shame to call move() when adjustOffsetForSnapping() might be zero. I think it would be better to get adjustOffsetForSnapping(), and multiply it into t, then just move once.

Okay.

>> Source/WebCore/rendering/RenderBox.cpp:1479
>>          t.translateRight(adjustmentForSkippedAncestor.width(), adjustmentForSkippedAncestor.height());
> 
> Why doesn't pushMappingToContainer() have all the other snapping logic that mapLocalToContainer() does, e.g. for the non-transform case?

We snap before transforms, or at the end, whichever comes first. You'll notice the snapping otherwise handled in RenderGeometryMap::mapToContainer.

[Simon Fraser (smfr)]

(In reply to comment #27)
> >> Source/WebCore/rendering/RenderBox.cpp:1479
> >>          t.translateRight(adjustmentForSkippedAncestor.width(), adjustmentForSkippedAncestor.height());
> > 
> > Why doesn't pushMappingToContainer() have all the other snapping logic that mapLocalToContainer() does, e.g. for the non-transform case?
> 
> We snap before transforms, or at the end, whichever comes first. You'll notice the snapping otherwise handled in RenderGeometryMap::mapToContainer.

Do the fast paths in RGM do the right thing?

[Levi Weintraub]

Created attachment 178306 [details]
Patch

[Early Warning System Bot]

Comment on attachment 178306 [details]
Patch

Attachment 178306 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15210327

[Early Warning System Bot]

Comment on attachment 178306 [details]
Patch

Attachment 178306 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15175892

[Levi Weintraub]

Created attachment 178312 [details]
Patch

[Build Bot]

Comment on attachment 178312 [details]
Patch

Attachment 178312 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15185594

[Levi Weintraub]

Created attachment 178596 [details]
Patch

[Levi Weintraub]

(In reply to comment #28)
> Do the fast paths in RGM do the right thing?

They do now :) And we pass all tests in Debug.

[Julien Chaffraix]

Comment on attachment 178596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178596&action=review

> Source/WebCore/rendering/RenderBox.cpp:1418
> +    bool preserve3D = mode & UseTransforms && ((o && o->style()->preserves3D()) || style()->preserves3D());

|o| cannot be 0 here as we check it above. Please remove the added NULL check.

> Source/WebCore/rendering/RenderBox.cpp:1427
> +        applyAccumulatedOffset(transformState, accumulatedOffset, mode);
> +        LayoutSize adjustedContainerOffset = adjustOffsetForSnapping(mode, containerOffset);
> +        transformState.move(adjustedContainerOffset.width(), adjustedContainerOffset.height(), TransformState::AccumulateTransform);

Shouldn't this be simplified to applyAccumulatedOffset(transformState, accumulatedOffset + containerOffset, mode) ? (not sure if that's correct as the first line uses TransformState::FlattenTransform and the last one TransformState::AccumulateTransform)

> Source/WebCore/rendering/RenderGeometryMap.cpp:51
> +    accumulatedOffset = (mapCoordinatesFlags & SnapOffsetForTransforms) ? roundedIntSize(accumulatedOffset) : accumulatedOffset;

Shouldn't this use adjustOffsetForSnapping?

> Source/WebCore/rendering/RenderGeometryMap.cpp:109
> +                applyAccumulatedOffset(transformState, accumulatedOffset, m_mapCoordinatesFlags);
> +                LayoutSize containerOffset = adjustOffsetForSnapping(m_mapCoordinatesFlags, currentStep.m_offset);
> +                transformState.move(containerOffset.width(), containerOffset.height(), accumulate);

Again could this be simplified to one applyAccumulatedOffset call?

> Source/WebCore/rendering/RenderInline.cpp:1168
> +            accumulatedOffset.expand(layoutState->m_paintOffset + accumulatedOffset);

Wouldn't that double the accumulated offset?

> Source/WebCore/rendering/RenderObject.cpp:2067
> +    accumulatedOffset = adjustOffsetForSnapping(mapCoordinatesFlags, accumulatedOffset);

Shouldn't this use adjustOffsetForSnapping?

> Source/WebCore/rendering/RenderObject.cpp:2081
> +void RenderObject::mapLocalToContainerInternal(const RenderLayerModelObject* repaintContainer, TransformState& transformState, LayoutSize& accumulatedOffset, MapCoordinatesFlags mode, bool* wasFixed) const

Not a huge fan of the name ('internal' is kinda pointless here). How about mapLocalToContainerAccumulatingOffset? (couldn't think of something better or shorter)

> Source/WebCore/rendering/RenderObject.cpp:2088
> -        return;
> +        return transformState.move(adjustOffsetForSnapping(mode, accumulatedOffset));

How can this compile? mapLocalToContainerInternal is void.

> Source/WebCore/rendering/RenderView.cpp:231
> +        transformState.move(adjustOffsetForSnapping(mode, m_frameView->scrollOffsetForFixedPosition() + accumulatedOffset));
> +        accumulatedOffset = LayoutSize();

Can't this be written:

accumulatedOffset.expand(m_frameView->scrollOffsetForFixedPosition())
applyAccumulatedOffset(transformState, accumulatedOffset, mode);

> Source/WebCore/rendering/svg/RenderSVGText.cpp:116
> -    SVGRenderSupport::mapLocalToContainer(this, repaintContainer, transformState, mode & SnapOffsetForTransforms, wasFixed);
> +    SVGRenderSupport::mapLocalToContainer(this, repaintContainer, transformState, accumulatedOffset, mode, wasFixed);

The ChangeLog should mention why it is fine to remove mode & SnapOffsetForTransforms from all the SVG classes, along with keep the default value for MapCoordinatesFlags to ApplyContainerFlip | SnapOffsetForTransforms.

[Simon Fraser (smfr)]

Comment on attachment 178596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178596&action=review

Why aren't the pixel results using mock scrollbars?

It's very hard for me to tell whether this patch is correct. I'd feel more confident if you had a bunch of tests that hit assertions with the wrong version of the patch, but no longer do so. r- for the apparently incorrect flattening.

> Source/WebCore/ChangeLog:42
> +        (WebCore::RenderObject::mapLocalToContainer): Broke mapLocalToContainer into this public function and
> +        mapLocalToContainerInternal. The internal flavors track an accumulated offset which is used to recreate the pixel
> +        snapping that's performed during paint. This public function calls the internal one, then applies any leftover
> +        accumulated offset before returning the TransformState to the caller.

mapLocalToContainer was already the "internal" method. I agree with Julian that some renaming might help.

> Source/WebCore/rendering/RenderBox.cpp:1425
> +        applyAccumulatedOffset(transformState, accumulatedOffset, mode);

applyAccumulatedOffset() is flattening here, since the default last param is 'false'. How can that be correct?

> Source/WebCore/rendering/RenderBox.cpp:1429
> -        transformState.move(containerOffset.width(), containerOffset.height(), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform);
> +        accumulatedOffset.expand(containerOffset);

So it seems like you're carrying along all of the non-transform offsets in accumulatedOffset, which kinda defeats the purpose of TransformState. This seems like a cop-out.

[Levi Weintraub]

Created attachment 179318 [details]
Patch

[Levi Weintraub]

Managed to simplify things greatly and move everything into TransformState. I'd missed a couple things that prevented me from getting this solution right before. This also *removes* SnapOffsetForTransforms, which should make everyone pretty happy :)

[Build Bot]

Comment on attachment 179318 [details]
Patch

Attachment 179318 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15315386

[Levi Weintraub]

Created attachment 179325 [details]
Patch

[Build Bot]

Comment on attachment 179325 [details]
Patch

Attachment 179325 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15313424

[Levi Weintraub]

Created attachment 179366 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 179366 [details]
Patch

I like this so much better!.

[Build Bot]

Comment on attachment 179366 [details]
Patch

Attachment 179366 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15315675

New failing tests:
fast/dom/Window/webkitConvertPoint.html
transforms/3d/point-mapping/3d-point-mapping-deep.html
accessibility/radio-button-group-members.html

[Levi Weintraub]

Created attachment 179660 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 179660 [details]
Patch for landing

Clearing flags on attachment: 179660

Committed r137847: <http://trac.webkit.org/changeset/137847>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.