Bug 84306 - [Qt][WK2] Fixed layers are shaking when zoom level is not 1.0 due to a rounding error.
Summary: [Qt][WK2] Fixed layers are shaking when zoom level is not 1.0 due to a roundi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-18 17:47 PDT by Yael
Modified: 2012-04-30 18:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch. (7.01 KB, patch)
2012-04-26 12:04 PDT, Yael
noam: review-
Details | Formatted Diff | Diff
Patch. (5.64 KB, patch)
2012-04-26 14:05 PDT, Yael
noam: review-
Details | Formatted Diff | Diff
Patch. (11.09 KB, patch)
2012-04-27 09:33 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (11.54 KB, patch)
2012-04-29 05:42 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (11.91 KB, patch)
2012-04-30 16:49 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2012-04-18 17:47:00 PDT
SSIA
Comment 1 Yael 2012-04-18 17:48:28 PDT
A patch is coming soon.
Comment 2 Yael 2012-04-26 07:54:03 PDT
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.
Comment 3 Yael 2012-04-26 09:20:31 PDT
(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.
Comment 4 Yael 2012-04-26 12:04:30 PDT
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.
Comment 5 Noam Rosenthal 2012-04-26 12:11:39 PDT
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 :)
Comment 6 Kenneth Rohde Christiansen 2012-04-26 12:19:28 PDT
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.
Comment 7 Yael 2012-04-26 12:43:30 PDT
(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.
Comment 8 Noam Rosenthal 2012-04-26 12:48:09 PDT
(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.
Comment 9 Noam Rosenthal 2012-04-26 12:48:28 PDT
Comment on attachment 139036 [details]
Patch.

See comments from Kenneth.
Comment 10 Yael 2012-04-26 12:53:47 PDT
(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.
Comment 11 Noam Rosenthal 2012-04-26 12:59:09 PDT
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.
Comment 12 Yael 2012-04-26 14:02:13 PDT
(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.
Comment 13 Yael 2012-04-26 14:05:18 PDT
Created attachment 139067 [details]
Patch.

Address Kenneth's comments.
Comment 14 Noam Rosenthal 2012-04-27 06:00:13 PDT
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.
Comment 15 Noam Rosenthal 2012-04-27 06:34:58 PDT
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()));
Comment 16 Jocelyn Turcotte 2012-04-27 08:18:00 PDT
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).
Comment 17 Noam Rosenthal 2012-04-27 09:10:12 PDT
(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
Comment 18 Yael 2012-04-27 09:31:56 PDT
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.
Comment 19 Yael 2012-04-27 09:33:26 PDT
Created attachment 139219 [details]
Patch.
Comment 20 Yael 2012-04-27 09:37:15 PDT
With the latest version of the patch, everything is calculated and passed together. The chances of things getting out of sync are very slim.
Comment 21 Jocelyn Turcotte 2012-04-27 09:49:56 PDT
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.
Comment 22 Yael 2012-04-28 06:36:17 PDT
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?
Comment 23 Yael 2012-04-29 05:42:33 PDT
Created attachment 139400 [details]
Patch.

Update patch after Friday's changes.
Comment 24 Noam Rosenthal 2012-04-30 15:17:16 PDT
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
Comment 25 Yael 2012-04-30 16:49:08 PDT
Created attachment 139546 [details]
Patch.

Address comment #24.
Comment 26 WebKit Review Bot 2012-04-30 18:09:52 PDT
Comment on attachment 139546 [details]
Patch.

Clearing flags on attachment: 139546

Committed r115696: <http://trac.webkit.org/changeset/115696>
Comment 27 WebKit Review Bot 2012-04-30 18:10:00 PDT
All reviewed patches have been landed.  Closing bug.