Bug 84306 – [Qt][WK2] Fixed layers are shaking when zoom level is not 1.0 due to a rounding error.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yael]

SSIA

[Yael]

A patch is coming soon.

[Yael]

There are 2 issues here:
1. When we zoom, the width and height of visibleContentsRect are not constant, and can change if we cross the integer boundaries.
2. The parent transform that is calculated in TextureMapperLayer::computeTransformsRecursive has a rounding error.
I have a patch that needs some cleanup. I will upload it once it is ready.

[Yael]

(In reply to comment #2)
> There are 2 issues here:
> 1. When we zoom, the width and height of visibleContentsRect are not constant, and can change if we cross the integer boundaries.
I meant to say when we scroll after we zoom.

[Yael]

Created attachment 139036 [details]
Patch.

When zooming, we need to be careful about how to convert the visible rect from float to int. Using toAlignedRect can produce inconsistent width and height when we are scrolling. This patch carefully modifies each piece of the visible rect, to avoid such rounding errors. In addition, the TransformMatrix we use for painting, needs to be adjusted for the same rounding error.

[Noam Rosenthal]

Comment on attachment 139036 [details]
Patch.

why does visibleContentsRect need to have a consistent with and height? You only apply the rounding adjustment to x and y anyway.
Seems like you're solving two problems in this patch, one of which is not actually a problem :)

[Kenneth Rohde Christiansen]

Comment on attachment 139036 [details]
Patch.

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

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:108
> +void WebLayerTreeRenderer::paintToCurrentGLContext(const TransformationMatrix& transformationMatrix, float opacity, const FloatRect& clipRect, TextureMapper::PaintFlags PaintFlags)
>  {
> +    TransformationMatrix matrix = transformationMatrix;

Why not keep matrix and name the new one newMatrix or so, that would follow the style elsewehre

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:551
> +    // We cannot use toAlignedRect() because it produces inconsistent width and height.

QRectF::toAlignedRect() to be more precise

// We avoid using QRectF::toAlignedRect() as it produces incon...

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:556
> +    result.setWidth(floor(result.width()));
> +    result.setHeight(floor(result.height()));
> +    result.moveLeft(floor(result.x()));
> +    result.moveTop(floor(result.y()));
> +    return result;

So now it is basically a QRect? What is the advantage of returning a QRectF which is basically a QRect and then later (look below) converting it to a IntRect?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:601
> +    webPageProxy->drawingArea()->setVisibleContentsRect(IntRect(IntPoint(), IntSize(viewportSize.width(), viewportSize.height())), 1, FloatPoint());

Maybe it would be nicer to create an IntRect and call setWidth etc... or maybe there is a better ctor.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:788
> +    const QRectF visibleRect(visibleContentsRect());

What is the point in converting this to QRectF and then later converting it to IntRect, I assume it is already a FloatRect

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:127
> +    QRectF visibleContentsRect() const;

Ah, it is not.

[Yael]

(In reply to comment #5)
> (From update of attachment 139036 [details])
> why does visibleContentsRect need to have a consistent with and height? You only apply the rounding adjustment to x and y anyway.
> Seems like you're solving two problems in this patch, one of which is not actually a problem :)

Consider the css to be 
#myelement { position: fixed; right: 5px; } 
This element will be jumping back and forth as the width changes.

[Noam Rosenthal]

(In reply to comment #7)
> Consider the css to be 
> #myelement { position: fixed; right: 5px; } 
> This element will be jumping back and forth as the width changes.

Ah, right, since we're using the setFixedVisibleContentsRect thing.

[Noam Rosenthal]

Comment on attachment 139036 [details]
Patch.

See comments from Kenneth.

[Yael]

(In reply to comment #6)
> (From update of attachment 139036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139036&action=review
> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:108
> > +void WebLayerTreeRenderer::paintToCurrentGLContext(const TransformationMatrix& transformationMatrix, float opacity, const FloatRect& clipRect, TextureMapper::PaintFlags PaintFlags)
> >  {
> > +    TransformationMatrix matrix = transformationMatrix;
> 
> Why not keep matrix and name the new one newMatrix or so, that would follow the style elsewehre
> 
ok
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:551
> > +    // We cannot use toAlignedRect() because it produces inconsistent width and height.
> 
> QRectF::toAlignedRect() to be more precise
> 
> // We avoid using QRectF::toAlignedRect() as it produces incon...
> 
ok
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:556
> > +    result.setWidth(floor(result.width()));
> > +    result.setHeight(floor(result.height()));
> > +    result.moveLeft(floor(result.x()));
> > +    result.moveTop(floor(result.y()));
> > +    return result;
> 
> So now it is basically a QRect? What is the advantage of returning a QRectF which is basically a QRect and then later (look below) converting it to a IntRect?
> 
I think you are right, I'll give it a try.
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:601
> > +    webPageProxy->drawingArea()->setVisibleContentsRect(IntRect(IntPoint(), IntSize(viewportSize.width(), viewportSize.height())), 1, FloatPoint());
> 
> Maybe it would be nicer to create an IntRect and call setWidth etc... or maybe there is a better ctor.
> 
(0, 0, viewportSize.width(), viewportSize.height()) 

> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:788
> > +    const QRectF visibleRect(visibleContentsRect());
> 
> What is the point in converting this to QRectF and then later converting it to IntRect, I assume it is already a FloatRect
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:127
> > +    QRectF visibleContentsRect() const;
> 
> Ah, it is not.

[Noam Rosenthal]

Comment on attachment 139036 [details]
Patch.

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:796
> +    if (layerTreeHostProxy->layerTreeRenderer())
> +        layerTreeHostProxy->layerTreeRenderer()->setRoundingAdjustment(FloatSize(accurateVisibleRect.x() - visibleRect.x() * scale, accurateVisibleRect.y() - visibleRect.y() * scale));

Are you sure this should happen in commitScale? Seems like it would cause a small flicker since the rounding adjustment is only needed a bit later, after the committed scale has been actually rendered in the web process.

[Yael]

(In reply to comment #11)
> (From update of attachment 139036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139036&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:796
> > +    if (layerTreeHostProxy->layerTreeRenderer())
> > +        layerTreeHostProxy->layerTreeRenderer()->setRoundingAdjustment(FloatSize(accurateVisibleRect.x() - visibleRect.x() * scale, accurateVisibleRect.y() - visibleRect.y() * scale));
> 
> Are you sure this should happen in commitScale? Seems like it would cause a small flicker since the rounding adjustment is only needed a bit later, after the committed scale has been actually rendered in the web process.

I did not notice flicker when committing the zoom. We do have to do this after zooming, or else it bits the purpose of aligning the visible content rect all the time.

[Yael]

Created attachment 139067 [details]
Patch.

Address Kenneth's comments.

[Noam Rosenthal]

Comment on attachment 139067 [details]
Patch.

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

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:111
> +    TransformationMatrix newMatrix = matrix;
> +    newMatrix.setM41(newMatrix.m41() + m_roundingAdjustment.width());
> +    newMatrix.setM42(newMatrix.m42() + m_roundingAdjustment.height());
> +

just use translate(m_roundingAdjustment)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:793
> +        layerTreeHostProxy->layerTreeRenderer()->setRoundingAdjustment(FloatSize(accurateVisibleRect.x() - visibleRect.x() * scale, accurateVisibleRect.y() - visibleRect.y() * scale));

This will create a flicker in certain situations.
LayerTreeRenderer updates the rounding adjustment here, but only updates the scroll position in didChangeScrollPosition. Those two values need to go in sync, e.g. by sending the rounding adjustment together with the trajectory, or something to that effect.

[Noam Rosenthal]

Comment on attachment 139067 [details]
Patch.

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

Thread safety issues with this patch - calling WebLayerTreeRenderer functions from the main thread and renderer thread.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:553
> +    QRect result(floor(mappedRect.x()), floor(mappedRect.y()), floor(mappedRect.width()), floor(mappedRect.height()));
> +    return result;

return QRect(floor(mappedRect.x()), floor(mappedRect.y()), floor(mappedRect.width()), floor(mappedRect.height()));

[Jocelyn Turcotte]

This feels like a brittle solution, I don't understand why
- We still send a rounded visible rect
- We calculate the error of the rounding and send it to the same destination in parallel
- Then we readjust the inaccurate visible rect with the accuracy adjustment.

Why not send a proper visible rect in the first place that uses floats?
Else I think we should just send a separate complete accurate visible rect instead of an adjustment of the error of the first one. This code is bound to break the moment that one of those two values is touched without making sure that they are synchronized (and we have no auto-test that covers this).

[Noam Rosenthal]

(In reply to comment #16)
> This feels like a brittle solution, I don't understand why
> - We still send a rounded visible rect
> - We calculate the error of the rounding and send it to the same destination in parallel
> - Then we readjust the inaccurate visible rect with the accuracy adjustment.
> 
> Why not send a proper visible rect in the first place that uses floats?
> Else I think we should just send a separate complete accurate visible rect instead of an adjustment of the error of the first one. This code is bound to break the moment that one of those two values is touched without making sure that they are synchronized (and we have no auto-test that covers this).

I have to agree with Jocelyn on this. There must be a more robust solution

[Yael]

The simple answer to your concern is that we have to make absolutely _everything_ float. That includes backing store, visibleContentRect in webcore, etc.
This type of change will impact all ports and I highly doubt that it will be accepted.

[Yael]

Created attachment 139219 [details]
Patch.

[Yael]

With the latest version of the patch, everything is calculated and passed together. The chances of things getting out of sync are very slim.

[Jocelyn Turcotte]

Comment on attachment 139219 [details]
Patch.

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:787
> +    QRectF accurateVisibleRect(q->boundingRect());
> +    accurateVisibleRect.translate(contentPos());
> +    drawingArea->setVisibleContentsRect(visibleRect, scale, trajectoryVector, FloatSize((accurateVisibleRect.x() - visibleRect.x() * scale) / scale, (accurateVisibleRect.y() - visibleRect.y() * scale) / scale));

How about sending a FloatRect visible rect directly to WebLayerTreeRenderer, and continue sending the rounded one to DrawingArea?
Plus the scale isn't needed in WebLayerTreeRenderer, m_contentsScale is unused.

[Yael]

Setting the adjustment in updatePaintNode is causing problems.

visibleContentsRect is not being updated during pinch zoom, so I cannot use it to calculate the adjustment during pinch zoom without hacking something. I need to either know that we are in the process of pinching, or do a lot of re-calculations in updatePaintNode.

Setting the adjustment in _q_contentViewportChanged does not require such hacks, because the adjustment does not change during pinch zoom.

Noam, Jocelyn, is it really important that we do it in updatePaintNode, and hack around the pinch issue?

[Yael]

Created attachment 139400 [details]
Patch.

Update patch after Friday's changes.

[Noam Rosenthal]

Comment on attachment 139400 [details]
Patch.

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

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:95
> +    virtual void setVisibleContentsRect(const WebCore::IntRect& visibleContentsRect, float scale, const WebCore::FloatPoint& trajectoryVector, const WebCore::FloatPoint& = WebCore::FloatPoint()) { }

you can name the parameter here

> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:83
> +    virtual void setVisibleContentsRect(const WebCore::IntRect& visibleContentsRect, float scale, const WebCore::FloatPoint& trajectory, const WebCore::FloatPoint& = WebCore::FloatPoint());

Use parameter name

> Source/WebKit2/UIProcess/LayerTreeHostProxy.cpp:132
> +    dispatchUpdate(bind(&WebLayerTreeRenderer::setVisibleContentsRect, m_renderer.get(), rect, scale, accurateVisibleContentPosition));

Content -> Contents

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:58
> +    void setVisibleContentsRect(const WebCore::IntRect&, float scale, const WebCore::FloatPoint& trajectory, const WebCore::FloatPoint& accurateVisibleContentPosition);

Content -> Contents

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:113
> +    newMatrix.translate(m_accurateVisibleContentPosition.x() / m_contentsScale - m_visibleContentsRect.x(), m_accurateVisibleContentPosition.y() / m_contentsScale - m_visibleContentsRect.y());

Add comment

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:163
> +void WebLayerTreeRenderer::setVisibleContentsRect(const IntRect& rect, float scale, const WebCore::FloatPoint& accurateVisibleContentPosition)

Content -> Contents

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.h:105
> +    WebCore::FloatPoint m_accurateVisibleContentPosition;

Content -> Contents

[Yael]

Created attachment 139546 [details]
Patch.

Address comment #24.

[WebKit Review Bot]

Comment on attachment 139546 [details]
Patch.

Clearing flags on attachment: 139546

Committed r115696: <http://trac.webkit.org/changeset/115696>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.