Bug 89238 – Accumulate sub-pixel offsets through layers and transforms

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

To maintain consistent pixel snapping, we should be passing the sub-pixel offsets through RenderLayer to the paint code. We also generate transforms from elements to absolute space for repainting that should use pixel snapped offsets instead of converting LayoutUnits to floats.

[Levi Weintraub]

Created attachment 147889 [details]
Patch

[Levi Weintraub]

Created attachment 147890 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 147890 [details]
Patch

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

r- for boolean params. When do you NOT want to pixel snap when mapping coords?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:699
> +    IntRect box = IntRect(sizingBox(renderer, true));

Gah, ugly boolean params!

[Simon Fraser (smfr)]

(In reply to comment #3)
> (From update of attachment 147890 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147890&action=review
> 
> r- for boolean params. When do you NOT want to pixel snap when mapping coords?

er, when do you WANT to pixel snap?

[Levi Weintraub]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 147890 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147890&action=review
> > 
> > r- for boolean params. When do you NOT want to pixel snap when mapping coords?
> 
> er, when do you WANT to pixel snap?

We want to avoid pixel snapping for localToAbsolute to ensure that webkitConvertPoint works properly, but we want to pixel snap when translating a transform by a renderer's sub-pixel position. Otherwise, doing something as seemingly-innocuous as a rotateZ(0) can cause us to generate sub-pixel translations that'll misalign our painting.

I'll get rid of the boolean parameters.

Thanks for the review!

[Levi Weintraub]

Created attachment 147917 [details]
Patch

[Build Bot]

Comment on attachment 147917 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 147917 [details]
Patch

Attachment 147917 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12960743

New failing tests:
fast/transforms/rotated-transform-affects-scrolling-2.html
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html
fast/forms/validation-message-appearance.html

[WebKit Review Bot]

Created attachment 147924 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 147917 [details]
Patch

Attachment 147917 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12968215

New failing tests:
fast/transforms/rotated-transform-affects-scrolling-2.html
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html
fast/forms/validation-message-appearance.html

[WebKit Review Bot]

Created attachment 147932 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Levi Weintraub]

(In reply to comment #10)
> (From update of attachment 147917 [details])
> Attachment 147917 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12968215
> 
> New failing tests:
> fast/transforms/rotated-transform-affects-scrolling-2.html
> fast/repaint/transform-translate.html
> transforms/svg-vs-css.xhtml
> fast/repaint/transform-absolute-child.html
> fast/transforms/transformed-document-element.html
> fast/repaint/transform-repaint-descendants.html
> fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
> svg/transforms/svg-css-transforms.xhtml
> compositing/shadows/shadow-drawing.html
> fast/forms/validation-message-appearance.html

FYI: All these test results are expected, and the repaint tests were actually off by a pixel previously, and are now correct.

[Levi Weintraub]

Created attachment 148136 [details]
Patch

[Levi Weintraub]

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

Fixing the Mac build error.

[WebKit Review Bot]

Comment on attachment 148136 [details]
Patch

Attachment 148136 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12970794

New failing tests:
fast/transforms/rotated-transform-affects-scrolling-2.html
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html
fast/forms/validation-message-appearance.html

[WebKit Review Bot]

Created attachment 148182 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 148136 [details]
Patch

Attachment 148136 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12979296

New failing tests:
fast/transforms/rotated-transform-affects-scrolling-2.html
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
svg/css/getComputedStyle-basic.xhtml
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html
fast/forms/validation-message-appearance.html

[WebKit Review Bot]

Created attachment 148199 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Levi Weintraub]

Pinging reviewers.

[Levi Weintraub]

(In reply to comment #19)
> Pinging reviewers.

Been awhile, trying again.

[Nikolas Zimmermann]

Comment on attachment 148136 [details]
Patch

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

A first round of review. Obviously Simon should have a look at this as well!

> Source/WebCore/ChangeLog:20
> +        (WebCore::sizingBox): Adding a bool for pixel snapping.

Aha, your code doesn't do that, it adds a new function duplicating sizingBox() functionality, but pixel-snaps. See below for a possible suggestion to avoid that.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:707
> -    LayoutRect box = sizingBox(renderer);
> +    IntRect box = pixelSnappedSizingBox(renderer);

Hm, can't you just use pixelSnappedIntRect(sizingBox(renderer)) here?
From my understanding this should be equivalent, but avoids duplicating the logic of sizingBox() just to pixel-snap the values.

> Source/WebCore/rendering/RenderInline.h:138
> -    virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool fixed, bool useTransforms, TransformState&, ApplyContainerFlipOrNot = ApplyContainerFlip, bool* wasFixed = 0) const;
> +    virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool fixed, bool useTransforms, TransformState&, TransformingSubPixelOffsetBehavior adjustSubPixelOffsets = SnapOffsetsForTransforms, ApplyContainerFlipOrNot = ApplyContainerFlip, bool* wasFixed = 0) const OVERRIDE;

All these bools should go away IMHO. And I'd rather do this now before bloating mapLocalToContainer any further with new extra arguments.
(Though not your fault, but we should clean this up rather today than tomorrow :-)

enum MapLocalToContainerMode {
    IsFixed = 1 << 0,
    UseTransforms = 1 << 1,
    ApplyContainerFlip = 1 << 2,
    SnapOffsetForTransforms 1 << 3;
}
typedef unsigned MapLocalToContainerFlags;
virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, TransformState&, MapLocalToContainerFlags = SnapOffsetForTransforms | ApplyContainerFlip, bool* wasFixed = 0) const OVERRIDE;

I didn't investigate in-detail if that would make some call-sites worse to read, but it should improve readability in general.

> Source/WebCore/rendering/RenderLayer.cpp:549
> -        box->style()->applyTransform(*m_transform, box->borderBoxRect().size(), RenderStyle::IncludeTransformOrigin);
> +        box->style()->applyTransform(*m_transform, box->pixelSnappedBorderBoxRect().size(), RenderStyle::IncludeTransformOrigin);

I'd like to fully understand the effects of this change, please correct me if I got anything wrong:

This change is relevant for computing the transform-origin induced translation (+ the argument is also passed to the TransformOperations::apply function).
You're now using pixel-snapped values here. This is correct as you're aligning the layer at pixel boundaries, but respect any fractional translation while computing the layer transform (in RenderLayer::paint).

So for borderBoxRect.size = 100.5 x 100.5, and a transform-origin of 50% 50%, this now results in a m_transform containing: translate(50, 50) * specifiedTransforms * translate(-50, -50).
In RenderLayer::paint, you're computing the layer transform passed to the GraphicsContext via: m_transform * translate(100, 100), and store subPixelAccumulation="0.5, 0.5".
Then In RenderLayer::paintLayerContents, you're adding the subPixelAccumulation to the paintOffset, so you're effectively aligning the layer on pixel boundaries, but compensate fractional sizes by translating the context before painting.

Am I reading your code correctly?

[Levi Weintraub]

Comment on attachment 148136 [details]
Patch

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

>> Source/WebCore/ChangeLog:20
>> +        (WebCore::sizingBox): Adding a bool for pixel snapping.
> 
> Aha, your code doesn't do that, it adds a new function duplicating sizingBox() functionality, but pixel-snaps. See below for a possible suggestion to avoid that.

You caught me!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:707
>> +    IntRect box = pixelSnappedSizingBox(renderer);
> 
> Hm, can't you just use pixelSnappedIntRect(sizingBox(renderer)) here?
> From my understanding this should be equivalent, but avoids duplicating the logic of sizingBox() just to pixel-snap the values.

The issue is that sizingBox can return RenderBox::borderBoxRect, which while maintaining the sub-pixel precision of the width and height, zeros out the location, resulting in incorrect pixel snapping.

>> Source/WebCore/rendering/RenderInline.h:138
>> +    virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool fixed, bool useTransforms, TransformState&, TransformingSubPixelOffsetBehavior adjustSubPixelOffsets = SnapOffsetsForTransforms, ApplyContainerFlipOrNot = ApplyContainerFlip, bool* wasFixed = 0) const OVERRIDE;
> 
> All these bools should go away IMHO. And I'd rather do this now before bloating mapLocalToContainer any further with new extra arguments.
> (Though not your fault, but we should clean this up rather today than tomorrow :-)
> 
> enum MapLocalToContainerMode {
>     IsFixed = 1 << 0,
>     UseTransforms = 1 << 1,
>     ApplyContainerFlip = 1 << 2,
>     SnapOffsetForTransforms 1 << 3;
> }
> typedef unsigned MapLocalToContainerFlags;
> virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, TransformState&, MapLocalToContainerFlags = SnapOffsetForTransforms | ApplyContainerFlip, bool* wasFixed = 0) const OVERRIDE;
> 
> I didn't investigate in-detail if that would make some call-sites worse to read, but it should improve readability in general.

I'm happy to give this a try. The bools are bad and getting worse.

>> Source/WebCore/rendering/RenderLayer.cpp:549
>> +        box->style()->applyTransform(*m_transform, box->pixelSnappedBorderBoxRect().size(), RenderStyle::IncludeTransformOrigin);
> 
> I'd like to fully understand the effects of this change, please correct me if I got anything wrong:
> 
> This change is relevant for computing the transform-origin induced translation (+ the argument is also passed to the TransformOperations::apply function).
> You're now using pixel-snapped values here. This is correct as you're aligning the layer at pixel boundaries, but respect any fractional translation while computing the layer transform (in RenderLayer::paint).
> 
> So for borderBoxRect.size = 100.5 x 100.5, and a transform-origin of 50% 50%, this now results in a m_transform containing: translate(50, 50) * specifiedTransforms * translate(-50, -50).
> In RenderLayer::paint, you're computing the layer transform passed to the GraphicsContext via: m_transform * translate(100, 100), and store subPixelAccumulation="0.5, 0.5".
> Then In RenderLayer::paintLayerContents, you're adding the subPixelAccumulation to the paintOffset, so you're effectively aligning the layer on pixel boundaries, but compensate fractional sizes by translating the context before painting.
> 
> Am I reading your code correctly?

Currently, we carry a sub-pixel value down through paint offsets that influences the way things are pixel snapped. This is broken with Layers. We avoid translating the transform by sub-pixel values (this would be wrong for a relatively positioned object that just happens to be an a sub-pixel offset, for example), but the current code also drops the sub-pixel remainder on the floor, which causes elements in a layer to round differently than otherwise identical elements that aren't.

As for your example, you're making the mistake of considering a fractional size without a location, which you can't quite get away with with sub-pixel layout enabled.

[Levi Weintraub]

Created attachment 155600 [details]
Patch.

Patch.

[Levi Weintraub]

Created attachment 155618 [details]
Patch

[Build Bot]

Comment on attachment 155618 [details]
Patch

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

[Levi Weintraub]

Created attachment 155629 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155629 [details]
Patch

Attachment 155629 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13401543

New failing tests:
fast/transforms/rotated-transform-affects-scrolling-2.html
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
fast/sub-pixel/layout-boxes-with-zoom.html
media/audio-repaint.html
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
media/video-zoom-controls.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html
fast/multicol/vertical-lr/break-properties.html
fast/forms/validation-message-appearance.html
fast/multicol/vertical-rl/break-properties.html
fast/repaint/repaint-across-writing-mode-boundary.html
fast/block/float/shrink-to-avoid-float-complexity.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html
fast/multicol/break-properties.html

[WebKit Review Bot]

Created attachment 155641 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Levi Weintraub]

Created attachment 155909 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155909 [details]
Patch

Attachment 155909 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13421237

New failing tests:
fast/transforms/rotated-transform-affects-scrolling-2.html
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
media/audio-repaint.html
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html
fast/forms/validation-message-appearance.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html
fast/overflow/overflow-update-transform.html

[WebKit Review Bot]

Created attachment 155925 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Levi Weintraub]

Created attachment 157611 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157611 [details]
Patch

Attachment 157611 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13473189

New failing tests:
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
media/audio-repaint.html
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html

[WebKit Review Bot]

Created attachment 157620 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 157611 [details]
Patch

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

[Levi Weintraub]

Created attachment 157746 [details]
Patch

[Levi Weintraub]

Created attachment 158038 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158038 [details]
Patch

Attachment 158038 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13493356

New failing tests:
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
media/audio-repaint.html
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html

[WebKit Review Bot]

Created attachment 158165 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Levi Weintraub]

Smfr, any chance you can take a look at this?

[Eric Seidel (no email)]

Comment on attachment 158038 [details]
Patch

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

This change looks fine.

> Source/WebCore/rendering/RenderLayer.h:719
> +    void paintLayerContents(RenderLayer* rootLayer, GraphicsContext*, const LayoutRect& paintDirtyRect, const LayoutSize& subPixelAccumulation,

You mentioned you wanted to ASSERT these were never over 1,1.

> Source/WebCore/rendering/RenderObject.h:692
> +    FloatQuad localToContainerQuad(const FloatQuad&, RenderBoxModelObject* repaintContainer, bool snapOffsetForTransforms = true, bool fixed = false, bool* wasFixed = 0) const;
> +    FloatPoint localToContainerPoint(const FloatPoint&, RenderBoxModelObject* repaintContainer, bool snapOffsetForTransforms = true, bool fixed = false, bool* wasFixed = 0) const;

Not another bool!

[Levi Weintraub]

Created attachment 158674 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 158674 [details]
Patch for landing

Attachment 158674 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13504793

New failing tests:
fast/repaint/transform-translate.html
transforms/svg-vs-css.xhtml
fast/repaint/transform-absolute-child.html
media/audio-repaint.html
fast/transforms/transformed-document-element.html
fast/repaint/transform-repaint-descendants.html
fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html
svg/transforms/svg-css-transforms.xhtml
compositing/shadows/shadow-drawing.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html

[WebKit Review Bot]

Created attachment 158687 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Levi Weintraub]

Committed r125794: <http://trac.webkit.org/changeset/125794>

[Levi Weintraub]

*** Bug 91233 has been marked as a duplicate of this bug. ***