Bug 80714 - [Qt] [WK2] Shouldn't use item for clipping rect calculation in paint node.
Summary: [Qt] [WK2] Shouldn't use item for clipping rect calculation in paint node.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 76661
  Show dependency treegraph
 
Reported: 2012-03-09 11:46 PST by Viatcheslav Ostapenko
Modified: 2012-03-09 20:50 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.61 KB, patch)
2012-03-09 11:51 PST, Viatcheslav Ostapenko
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Updated patch (5.78 KB, patch)
2012-03-09 14:29 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Updated patch (5.51 KB, patch)
2012-03-09 14:43 PST, Viatcheslav Ostapenko
noam: review+
noam: commit-queue-
Details | Formatted Diff | Diff
Patch for commit. (5.49 KB, patch)
2012-03-09 15:12 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2012-03-09 11:46:53 PST
This will not work with threaded rendering or require some synchronization and clipping nodes have all necessary information.
Comment 1 Viatcheslav Ostapenko 2012-03-09 11:51:45 PST
Created attachment 131067 [details]
Patch
Comment 2 Noam Rosenthal 2012-03-09 13:27:09 PST
Comment on attachment 131067 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Replace item based clipping rect calculation with clipping nodes based.

Explain that this is needed for threaded rendering, where you don't have access to the items.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:121
> +    static void adjustBoundingRect(QRectF& rect, const QPointF& point)

rename uniteRectWithPoint

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:131
> +        if (rect.top() > point.y())
> +            rect.setTop(point.y());
> +        else if (rect.bottom() < point.y())
> +            rect.setBottom(point.y());
> +
> +        if (rect.left() > point.x())
> +            rect.setLeft(point.x());
> +        else if (rect.right() < point.x())
> +            rect.setRight(point.x());

rect |= QRectF(point, QSizeF(1, 1));

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:137
> +        QRectF retRect(0, 0, -1, -1);

retRect -> resultRect

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:139
> +        while (clip) {

Would be nicer as a for loop
for (const QSGClipNode* clip = clipList(); clip; clip = clip->clipList)

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:140
> +            QMatrix4x4 m;

m -> matrix or clipMatrix

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:142
> +                m *= *clip->matrix();

Why multiply instead of assign?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:152
> +                if (geometry->vertexCount() > 1) {

Shouldn't it be 2? I don't see the point in a single-line clip.
Also, this requires a comment.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:153
> +                    currentClip.setTopLeft(m.map(QPoint(geometryPoints[0].x, geometryPoints[0].y)));

If you use my unite suggestion, you don't need this and can simply start from 0.
Comment 3 Viatcheslav Ostapenko 2012-03-09 14:05:06 PST
(In reply to comment #2)
> (From update of attachment 131067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131067&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Replace item based clipping rect calculation with clipping nodes based.
> 
> Explain that this is needed for threaded rendering, where you don't have access to the items.
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:121
> > +    static void adjustBoundingRect(QRectF& rect, const QPointF& point)
> 
> rename uniteRectWithPoint
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:131
> > +        if (rect.top() > point.y())
> > +            rect.setTop(point.y());
> > +        else if (rect.bottom() < point.y())
> > +            rect.setBottom(point.y());
> > +
> > +        if (rect.left() > point.x())
> > +            rect.setLeft(point.x());
> > +        else if (rect.right() < point.x())
> > +            rect.setRight(point.x());
> 
> rect |= QRectF(point, QSizeF(1, 1));

This will give rect bigger by 1 on right and bottom sides.
QSize(0, 0) will not work becase QRectF operator| has check for null rect and will return original rect.

> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:137
> > +        QRectF retRect(0, 0, -1, -1);
> 
> retRect -> resultRect
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:139
> > +        while (clip) {
> 
> Would be nicer as a for loop
> for (const QSGClipNode* clip = clipList(); clip; clip = clip->clipList)
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:140
> > +            QMatrix4x4 m;
> 
> m -> matrix or clipMatrix
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:142
> > +                m *= *clip->matrix();

Remaining artifacts ;)

> Why multiply instead of assign?
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:152
> > +                if (geometry->vertexCount() > 1) {
> 
> Shouldn't it be 2? I don't see the point in a single-line clip.
> Also, this requires a comment.

Yes, 2 make more sense.

> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:153
> > +                    currentClip.setTopLeft(m.map(QPoint(geometryPoints[0].x, geometryPoints[0].y)));
> 
> If you use my unite suggestion, you don't need this and can simply start from 0.
Comment 4 Noam Rosenthal 2012-03-09 14:24:24 PST
> QSize(0, 0) will not work becase QRectF operator| has check for null rect and will return original rect.
How about:
Use a QPolygonF
Add the points to the polygon
Use polygon->boundingRect
Comment 5 Viatcheslav Ostapenko 2012-03-09 14:29:28 PST
Created attachment 131111 [details]
Updated patch
Comment 6 Viatcheslav Ostapenko 2012-03-09 14:32:37 PST
(In reply to comment #4)
> > QSize(0, 0) will not work becase QRectF operator| has check for null rect and will return original rect.
> How about:
> Use a QPolygonF
> Add the points to the polygon
> Use polygon->boundingRect

Didn't know that such thing exists.
Was looking for it some time ago and there was QPolygon only.
I'll try.
Comment 7 Noam Rosenthal 2012-03-09 14:36:21 PST
Comment on attachment 131111 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:159
> +                const QSGGeometry::Point2D* geometryPoints = geometry->vertexDataAsPoint2D();
> +
> +                // Clip region should be at least triangle to make valid clip.
> +                if (geometry->vertexCount() > 2) {
> +                    currentClip.setTopLeft(clipMatrix.map(QPoint(geometryPoints[0].x, geometryPoints[0].y)));
> +
> +                    for (int i = 1; i < geometry->vertexCount(); i++)
> +                        uniteRectWithPoint(currentClip, clipMatrix.map(QPoint(geometryPoints[i].x, geometryPoints[i].y)));
> +                }

Better to add the points to a QPolygonF and then call boundingRect.
Otherwise patch is good.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:162
> +            if (!currentClip.isEmpty()) {

if (currentClip.isEmpty())
    continue;

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:163
> +                if (resultRect.isValid())

Comment.
Comment 8 Viatcheslav Ostapenko 2012-03-09 14:43:44 PST
Created attachment 131115 [details]
Updated patch
Comment 9 Noam Rosenthal 2012-03-09 14:52:03 PST
Comment on attachment 131115 [details]
Updated patch

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

This is good. Let's fix the nitpicks and commit.

> Source/WebKit2/ChangeLog:11
> +        Replace item based clipping rect calculation with clipping nodes based.
> +        Part of threaded rendering implementation.
> +        In Qt5 threaded rendering paint nodes live on paint thread and items on 
> +        main thread. Paint nodes should not have any direct access to items.

Some rewording if you don't mind :)
Replace item based clip-rect calculation with clipping-nodes based calculation.
This is required for threaded rendering, since we don't have access to the QSGItems from the render thread.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:25
> +#include <QPolygonF>

This belongs at the end, before the QtQuick includes.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:126
> +        QRectF resultRect(0, 0, -1, -1);

Comment: start with an invalid rect.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:142
> +                if (geometry->vertexCount() > 2) {

<nitpick>
if (geometry->vertexCount() < 3)
    continue;

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:151
> +            if (!currentClip.isEmpty()) {

if (currentClip.isEmpty())
    continue;
Comment 10 Viatcheslav Ostapenko 2012-03-09 15:12:06 PST
Created attachment 131122 [details]
Patch for commit.
Comment 11 WebKit Review Bot 2012-03-09 20:50:35 PST
Comment on attachment 131122 [details]
Patch for commit.

Clearing flags on attachment: 131122

Committed r110368: <http://trac.webkit.org/changeset/110368>
Comment 12 WebKit Review Bot 2012-03-09 20:50:41 PST
All reviewed patches have been landed.  Closing bug.