Bug 32375 – [Qt] QtWebkit drawing path uses too complicated clipping

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

[Noam Rosenthal]

This makes CSS animations not run smoothly and painting be slower than necessary.

[Benjamin Poulain]

Noam provided me a test case.

We clip to the exact shape of the transformed item. This cause the clipping region to be composed of hundreds of rectangles, and make the painting very slow.

[Kenneth Rohde Christiansen]

So this is on our side.

WebCore normally has an upper limit of regions (currently 25) after than the bounding rect of all regions is used. Maybe we should consider something similar.

[Noam Rosenthal]

Note that some of the other ports don't even implement it:

see GraphicsContextWin.cpp, GraphicsContextPlatformPrivate::clip

[Noam Rosenthal]

Also see what we're doing in qwebframe.cpp: QWebFramePrivate::renderPrivate.
We seem to go through a list of rects in a region, which some debugging shows that they can add up to quite a lot, and render the layer with a clip to that rect. This is extremely wasteful.

An optimization could simplify the clip region to its bounding rect when the number of rects in the region is too high (>4?). So far playing around with it seems to improve performance, but it needs more testing and "absolute" benchmarking.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> Also see what we're doing in qwebframe.cpp: QWebFramePrivate::renderPrivate.
> We seem to go through a list of rects in a region, which some debugging shows
> that they can add up to quite a lot, and render the layer with a clip to that
> rect. This is extremely wasteful.
> 
> An optimization could simplify the clip region to its bounding rect when the
> number of rects in the region is too high (>4?). So far playing around with it
> seems to improve performance, but it needs more testing and "absolute"
> benchmarking.

You need to update your source, renderPrivate doesnt exist anymore :-) WebCore internally takes the bounding rect when there are more than 25 rects.

[Noam Rosenthal]

Right, my newbie mistake...
from what I see, the names changed, but QWebFramePrivate::renderRelativeCoords still does the same (iterates over the clip region and renders the FrameView with a different clip rect). Maybe WebCore optimizes later, but there are still several calls to Frame-render which might mean extra raster operations.

[Noam Rosenthal]

Created attachment 45499 [details]
Use window as a control rect for the inverse-path instead of the original clipPath

valgrind shows that one of the major problems is not in actually applying the clip, but rather in requesting for QPainter::clipPath() over and over again, which is an expensive operation if the clip is already complex (Qt tries to translate the clip to a region etc.).

Most of the requests for QPainter::clipPath() come from trying to clip-out a rounded rectangle, which requires a control rect in order to inverse the clip. Currently GraphicsContextQt tries to use the current clipping path's bounding rect as the control rect, but that's an expensive operation that's not actually necessary! using the window as the control rect should suffice, as we're later intersecting the resulting path with the current clip-path anyway.

That's what the proposed patch tries to do, but it requires another set of eyes.

[Noam Rosenthal]

Testing the new patch, it does create some rendering issues with shadows (where
the clip-path is bigger than the window).
However, fixing QPainter::clipPath() to be less expensive (see
http://bugreports.qt.nokia.com/browse/QTBUG-7033) might be an alternate
solution.

[Andreas Kling]

Could you attach the test case to this bug? Thanks :)

[Noam Rosenthal]

Please have a look at my CSS3 example:
http://qt.gitorious.org/qt-labs/scxml/blobs/raw/statechartz/statechartz/demo.html

Valgrind claims that a lot of time is spent in complex clipping: it probably has to do with rounded rectangles, but I'm not sure.

[Simon Hausmann]

Removing dependency to QtWebKit 4.7 API as this change doesn't affect the API

[Ariya Hidayat]

Created attachment 48580 [details]
use clipRegion instead of clipPath

[Kenneth Rohde Christiansen]

Comment on attachment 48580 [details]
use clipRegion instead of clipPath

Great stuff

[WebKit Commit Bot]

Comment on attachment 48580 [details]
use clipRegion instead of clipPath

Clearing flags on attachment: 48580

Committed r54705: <http://trac.webkit.org/changeset/54705>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Can somebody checks if the bug is entirely fixed?