Bug 103519 – [WK2][EFL][Qt] Pixel alignment is wrong in some cases involving a non-integral content scale

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kenneth Rohde Christiansen]

Go to google.com (make view smaller if scale == 1), search for something

mouse over the >> to show the info overlay. Now scroll up and down, you will see the overlay element shaking

[Kenneth Rohde Christiansen]

The following patch shows that the element definitely shakes:

--- a/Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp
+++ b/Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp
@@ -441,6 +441,9 @@ void TextureMapperLayer::flushCompositingStateSelf(GraphicsLayerTextureMapper* g
     m_transform.setSize(m_state.size);
     m_transform.setFlattening(!m_state.preserves3D);
     m_transform.setChildrenTransform(m_state.childrenTransform);
+
+    if (m_fixedToViewport)
+        printf("pos %f\n", m_transform.combined().m42());
 }


pos 316.438544
pos 317.167813
pos 316.897082
pos 316.626351
pos 316.355620
pos 316.626351
pos 316.626351
pos 316.355620
pos 317.084889
pos 316.814159
pos 316.543428
pos 316.272697
pos 317.001966
pos 316.731235
pos 316.460504
pos 317.189773
pos 316.648311
pos 316.377580
pos 317.106849

[Dongseong Hwang]

Chromium shake the overlay element, too.
I think layout causes this shaking.

However, Coordinated Graphics has another problem. When shaking element, the fixed layer is blurry. Chromium always renders crisp.

I think it is related to something like TiledBackingStore::mapToContents.

[Kenneth Rohde Christiansen]

(In reply to comment #2)
> Chromium shake the overlay element, too.
> I think layout causes this shaking.
> 
> However, Coordinated Graphics has another problem. When shaking element, the fixed layer is blurry. Chromium always renders crisp.
> 
> I think it is related to something like TiledBackingStore::mapToContents.

Yeah, it seems to map things into non discrete positions making it blurry.

[Dongseong Hwang]

(In reply to comment #3)
> (In reply to comment #2)
> > Chromium shake the overlay element, too.
> > I think layout causes this shaking.
> > 
> > However, Coordinated Graphics has another problem. When shaking element, the fixed layer is blurry. Chromium always renders crisp.
> > 
> > I think it is related to something like TiledBackingStore::mapToContents.
> 
> Yeah, it seems to map things into non discrete positions making it blurry.

Do you think what is the scope of this bug?

We need to separate two problems.
1. Shaking layout
2. Blurry fixed layer.

#1 may require to fix layout routine. It seems to be hard.
We can fix #2 by ourselves.

[Kenneth Rohde Christiansen]

> Do you think what is the scope of this bug?
> 
> We need to separate two problems.
> 1. Shaking layout
> 2. Blurry fixed layer.
> 
> #1 may require to fix layout routine. It seems to be hard.
> We can fix #2 by ourselves.

We could do that, basically we need to fix both

[Mikhail Pozdnyakov]

Created attachment 177490 [details]
patch

Fix for shaking. Blurring is a separate issue and should be addressed separately.

[Rafael Brandao]

(In reply to comment #6)
> Created an attachment (id=177490) [details]
> patch
> 
> Fix for shaking. Blurring is a separate issue and should be addressed separately.

This has been changed to fix the blurry tiles when it was scrolled. If that was wrong, I think we should just rollout the patch for bug 103079 and reopen that bug.

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=177490) [details] [details]
> > patch
> > 
> > Fix for shaking. Blurring is a separate issue and should be addressed separately.
> 
> This has been changed to fix the blurry tiles when it was scrolled. If that was wrong, I think we should just rollout the patch for bug 103079 and reopen that bug.

This is not all of that patch. But doing this does mean that the transform used for events and painting AC is slightly different that the one for positioning the content.

[Mikhail Pozdnyakov]

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=177490) [details] [details]
> > patch
> > 
> > Fix for shaking. Blurring is a separate issue and should be addressed separately.
> 
> This has been changed to fix the blurry tiles when it was scrolled. If that was wrong, I think we should just rollout the patch for bug 103079 and reopen that bug.

Patch has a lot of good things which I would keep, we just need to find good solution to fix blurring issue, including fixed elements

[Mikhail Pozdnyakov]

Comment on attachment 177490 [details]
patch

removing the flags yet, as proper fix should also solve blurring issue

[Kenneth Rohde Christiansen]

Created attachment 177745 [details]
Patch

[Kenneth Rohde Christiansen]

Created attachment 178066 [details]
Patch (work in progress)

This patch shows crisp pixel aligned fixed positioned elements, but with the following test case it can be seen that it sometimes moves one pixel (shaking)

[Kenneth Rohde Christiansen]

Created attachment 178067 [details]
Testcase

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> Created an attachment (id=178066) [details]
> Patch (work in progress)
> 
> This patch shows crisp pixel aligned fixed positioned elements, but with the following test case it can be seen that it sometimes moves one pixel (shaking)

This relates to our visibleContentsRect which in WebCore doesn't 100% represent where we are (as it is roundedIntRect). We can only fix this properly if we move to scale in WebCore.

[Yael]

(In reply to comment #14)
> (In reply to comment #12)
> > Created an attachment (id=178066) [details] [details]
> > Patch (work in progress)
> > 
> > This patch shows crisp pixel aligned fixed positioned elements, but with the following test case it can be seen that it sometimes moves one pixel (shaking)
> 
> This relates to our visibleContentsRect which in WebCore doesn't 100% represent where we are (as it is roundedIntRect). We can only fix this properly if we move to scale in WebCore.

Does Qt have the same problem?

[Kenneth Rohde Christiansen]

> Does Qt have the same problem?

Yes. Your adjustment code hides the shaking somehow (but results in some misalignment and blurriness), so turning it off and doing debugging makes it easier to see the issue. I didn't dig into fixing this adjustment code yet.

How big the issue is depends on the exact scale. At some scales it is barely notifiable.

[Kenneth Rohde Christiansen]

Created attachment 178123 [details]
Patch (work in progress)

Fixes the temporarily position adjustment to be aligned to device units (comparing using the float version of visibleContentsRect).
Fixed that the layers can be blurry then the page reaches the end of the page.

one-off shaking still there.

[Kenneth Rohde Christiansen]

(In reply to comment #17)
> Created an attachment (id=178123) [details]
> Patch (work in progress)
> 
> Fixes the temporarily position adjustment to be aligned to device units (comparing using the float version of visibleContentsRect).
> Fixed that the layers can be blurry then the page reaches the end of the page.
> 
> one-off shaking still there.

With this patch you can go to google.com, search for "hest"
If you use the default EFL window size, the page will be slightly scaled down.

Now hover the mouse over one of the >> signs for one element

Then the fixed position element pops up and if you put the mouse over it and scroll up and down, it stays perfectly and is very crisp :-)

That is quite an improvement on what we had before, even if we have the one-off issue at certain scales.

[Dongseong Hwang]

Comment on attachment 178123 [details]
Patch (work in progress)

View in context: https://bugs.webkit.org/attachment.cgi?id=178123&action=review
great work!
However, we will use pageScaleFactor and deviceScaleFactor in WebCore::Page and move GraphicsLayerCA's snapping algorithm to RenderLayerBacking or RenderLayerCompositor in the future.

> Source/WebKit2/UIProcess/PageViewportController.cpp:97
> +    // Unfortunately it doesn't seem to be enough, so just always allow one pixel more.

I think it is because CoordinatedGraphicsLayer::computePixelAlignment replaces a layer, so the layer shifts off by 1 pixel.
I think we can not reproduce this bug before this patch.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:108
> +    computePixelAlignment(adjustedPosition, adjustedSize, adjustedAnchorPoint, pixelAlignmentOffset);

It is better to computePixel during flushing.
computing here causes redundant computing over and over again. for example, setPosition after setSize.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:863
> +    FloatRect alignedBounds = roundedIntRect(scaledBounds);

You're right. I think mac has bug also.
I think we should use something like pixelSnappedIntRect instead of roundedIntRece and enclosingIntRect to preserve size (size change causes TiledBackingStore's work!)
http://trac.webkit.org/wiki/LayoutUnit

[Kenneth Rohde Christiansen]

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:108
> > +    computePixelAlignment(adjustedPosition, adjustedSize, adjustedAnchorPoint, pixelAlignmentOffset);
> 
> It is better to computePixel during flushing.
> computing here causes redundant computing over and over again. for example, setPosition after setSize.

Possible yes, but I didn't look into making this performant yet :-)

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:863
> > +    FloatRect alignedBounds = roundedIntRect(scaledBounds);
> 
> You're right. I think mac has bug also.
> I think we should use something like pixelSnappedIntRect instead of roundedIntRece and enclosingIntRect to preserve size (size change causes TiledBackingStore's work!)
> http://trac.webkit.org/wiki/LayoutUnit

I am not sure, because LayoutUnits store the value (float) in an int. So if you apply scale to a rect and then remove it again, then you don't necessarily end up with the same rect, at least not if the values are not int aligned first. Also then we would need to enable sub pixel layouts which we cannot yet due to some blocker bugs (cairo fonts for one)

[Kenneth Rohde Christiansen]

Created attachment 178215 [details]
Patch

[Early Warning System Bot]

Comment on attachment 178215 [details]
Patch

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

[Kenneth Rohde Christiansen]

Created attachment 178220 [details]
Patch

[Noam Rosenthal]

Comment on attachment 178220 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:98
> +    if (!isIntegral(m_effectiveScale)) {

if (modf(m_effectiveScale))

[Kenneth Rohde Christiansen]

Created attachment 178223 [details]
Patch

[Noam Rosenthal]

Comment on attachment 178223 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:599
> +    // Sets the values.
> +    computePixelAlignment(m_adjustedPosition, m_adjustedSize, m_adjustedAnchorPoint, m_pixelAlignmentOffset);

Let's rename it then to adjustGeometryForPixelAlignment

[Kenneth Rohde Christiansen]

> > +    // Sets the values.
> > +    computePixelAlignment(m_adjustedPosition, m_adjustedSize, m_adjustedAnchorPoint, m_pixelAlignmentOffset);
> 
> Let's rename it then to adjustGeometryForPixelAlignment

You find that more clear? it modifies its arguments and I am just passing them directly here. The method itself doesn't adjust anything, plus I like using the same naming as in the mac code

[Kenneth Rohde Christiansen]

(In reply to comment #27)
> > > +    // Sets the values.
> > > +    computePixelAlignment(m_adjustedPosition, m_adjustedSize, m_adjustedAnchorPoint, m_pixelAlignmentOffset);
> > 
> > Let's rename it then to adjustGeometryForPixelAlignment
> 
> You find that more clear? it modifies its arguments and I am just passing them directly here. The method itself doesn't adjust anything, plus I like using the same naming as in the mac code

Can be renamed later, if we decide to do so.

[Kenneth Rohde Christiansen]

Landed in 136976