REGRESSION(SUBPIXEL_LAYOUT) Composited layers can cause one pixel shifts

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

The issues was observed on hulu.com, the reduced test case is one div on a subpixel position with a an image below it also on a subpixel position. If the outer div is caused to become composited the image below it may change its pixel rounding, which causes it to shift.

Of the issues one pixel offset observed by Mac, this is the only one reliably observed on Qt as well.

Imagine element A a placed at (0,  10.6) with an image B inside placed relatively to A at (0, 4.8). The image B then has an absolute position of  (0, 15.4) which causes it to rendered at (0,15). If A becomes accelerated composited, then A will be rendered at (0,5) on the composited layer, and the composited layer will be rendered at (0,11), causing image B to end up at (0,16).

To avoid this issue, the position of accelerated layers must be floored to an integer position, and be given an internal subpixel padding of the remainder, making their content get the at the same fractional coordinates as if there were no compositing.

[Allan Sandfeld Jensen]

Created attachment 199903 [details]
test.html

Test case demonstrating rounding of fractional positions being affected by the presence or absence of compositing.

[Allan Sandfeld Jensen]

Created attachment 199904 [details]
test.html

Updated test.

[Simon Fraser (smfr)]

        var test = document.getElementById('test');
        var clas = test.getAttribute('class');
        if (clas == 'composite') {
            test.setAttribute('class', '');
        } else {
            test.setAttribute('class', 'composite');
        }

This could be document.getElementById('test').classList.toggle('composite').

[Allan Sandfeld Jensen]

Created attachment 200103 [details]
Patch

Preliminary patch. Fixes all shifts are resizes in the uploaded case, except for ones ending in exactly 0.5. I am guessing a rounding somewhere gets confused by negative vs positive rounding, but I haven't found it yet.

[Allan Sandfeld Jensen]

Created attachment 200105 [details]
subpixel-stutter.html

Updated test.

[zalan]

I've seen other places where double-snapping cause  off-by-one issues (I try to dig them out). It'd be nice to see some common pattern to fix them, instead of different type of local fixes (if possible at all). This might be a good topic for the WebKit contr meeting.

[Allan Sandfeld Jensen]

Created attachment 200309 [details]
Patch

[Build Bot]

Comment on attachment 200309 [details]
Patch

Attachment 200309 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/308160

[Build Bot]

Comment on attachment 200309 [details]
Patch

Attachment 200309 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/343112

[Allan Sandfeld Jensen]

Created attachment 200311 [details]
Patch

Build fix for pendantic clang

[Build Bot]

Comment on attachment 200311 [details]
Patch

Attachment 200311 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/290143

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (From update of attachment 200311 [details])
> Attachment 200311 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/290143

Also needs updated exported symbols it appears.

[Build Bot]

Comment on attachment 200311 [details]
Patch

Attachment 200311 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/391133

[Allan Sandfeld Jensen]

Created attachment 200888 [details]
Patch

Export missing symbol for mac

[Levi Weintraub]

Comment on attachment 200888 [details]
Patch

This LGTM, but I'll let smfr comment.

[Allan Sandfeld Jensen]

(In reply to comment #15)
> (From update of attachment 200888 [details])
> This LGTM, but I'll let smfr comment.

Simon, any comments?

[Simon Fraser (smfr)]

Comment on attachment 200888 [details]
Patch

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

I think the spirit of the patch is OK, but not the implementation.

> Source/WebCore/platform/graphics/GraphicsLayer.h:252
>      IntSize offsetFromRenderer() const { return m_offsetFromRenderer; }
>      void setOffsetFromRenderer(const IntSize&, ShouldSetNeedsDisplay = SetNeedsDisplay);
>  
> +    LayoutSize subpixelAccumulation() const { return m_subpixelAccumulation; }
> +    void setSubpixelAccumulation(const LayoutSize& subpixelAccumulation) { m_subpixelAccumulation = subpixelAccumulation; }

Unclear why we would need both offsetFromRenderer and subpixelAccumulation. Why is it ok for offsetFromRenderer to be IntSize?

> Source/WebCore/platform/graphics/GraphicsLayer.h:502
> +    // Accumulated subpixel error to be used by subpixel layout of the content.
> +    LayoutSize m_subpixelAccumulation;

I don't think GraphicsLayer should every see Layout*

> Source/WebCore/rendering/RenderLayerBacking.cpp:656
> +    // We must move the bounds by the subpixel part of delta,
> +    // so that they can be pixel snapped to actual pixels.

That comment doesn't make much sense.

> Source/WebCore/rendering/RenderLayerBacking.cpp:710
> +    m_graphicsLayer->setPosition(FloatPoint(relativePixelCompositingBounds.location() - graphicsLayerParentLocation));
> +    m_graphicsLayer->setOffsetFromRenderer(toIntSize(localPixelCompositingBounds.location()));
> +    m_graphicsLayer->setSubpixelAccumulation(toLayoutSize(delta.fraction()));

What about m_foregroundLayer, m_maskLayer and m_backgroundLayer?

Why not store subpixel accumulation here on the RLB?

[Allan Sandfeld Jensen]

(In reply to comment #17)
> (From update of attachment 200888 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200888&action=review
> 
> I think the spirit of the patch is OK, but not the implementation.
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:252
> >      IntSize offsetFromRenderer() const { return m_offsetFromRenderer; }
> >      void setOffsetFromRenderer(const IntSize&, ShouldSetNeedsDisplay = SetNeedsDisplay);
> >  
> > +    LayoutSize subpixelAccumulation() const { return m_subpixelAccumulation; }
> > +    void setSubpixelAccumulation(const LayoutSize& subpixelAccumulation) { m_subpixelAccumulation = subpixelAccumulation; }
> 
> Unclear why we would need both offsetFromRenderer and subpixelAccumulation. Why is it ok for offsetFromRenderer to be IntSize?
> 

Making offsetFromRenderer a LayoutSize would not help since it is the relative offset, where subpixelAccumulation is the fractional part of the absolute offset. SubpixelAccumulation is what you add to the relative coordinates when painting to ensure snap-to-pixel snaps as it would if they were absolute coordinates.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:710
> > +    m_graphicsLayer->setPosition(FloatPoint(relativePixelCompositingBounds.location() - graphicsLayerParentLocation));
> > +    m_graphicsLayer->setOffsetFromRenderer(toIntSize(localPixelCompositingBounds.location()));
> > +    m_graphicsLayer->setSubpixelAccumulation(toLayoutSize(delta.fraction()));
> 
> What about m_foregroundLayer, m_maskLayer and m_backgroundLayer?
> 
Yes, I overlooked them.

> Why not store subpixel accumulation here on the RLB?

That might work, I will give it a try.

[Allan Sandfeld Jensen]

Created attachment 204023 [details]
Patch

Subpixel accumulation moved to RenderLayerBacking as suggested by smfr. Improved variable naming and comments.

[Dave Hyatt]

Comment on attachment 204023 [details]
Patch

r=me

This looks great.

[Allan Sandfeld Jensen]

Committed r154009: <http://trac.webkit.org/changeset/154009>

[zalan]

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