Bug 32375 - [Qt] QtWebkit drawing path uses too complicated clipping
Summary: [Qt] QtWebkit drawing path uses too complicated clipping
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Major
Assignee: Benjamin Poulain
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-12-10 05:12 PST by Noam Rosenthal
Modified: 2010-02-12 01:59 PST (History)
7 users (show)

See Also:


Attachments
Use window as a control rect for the inverse-path instead of the original clipPath (1010 bytes, patch)
2009-12-25 13:41 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
use clipRegion instead of clipPath (2.03 KB, patch)
2010-02-11 10:28 PST, Ariya Hidayat
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2009-12-10 05:12:47 PST
This makes CSS animations not run smoothly and painting be slower than necessary.
Comment 1 Benjamin Poulain 2009-12-11 05:40:16 PST
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.
Comment 2 Kenneth Rohde Christiansen 2009-12-14 06:36:01 PST
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.
Comment 3 Noam Rosenthal 2009-12-14 13:22:58 PST
Note that some of the other ports don't even implement it:

see GraphicsContextWin.cpp, GraphicsContextPlatformPrivate::clip
Comment 4 Noam Rosenthal 2009-12-17 00:17:38 PST
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.
Comment 5 Kenneth Rohde Christiansen 2009-12-17 04:36:38 PST
(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.
Comment 6 Noam Rosenthal 2009-12-17 08:00:30 PST
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.
Comment 7 Noam Rosenthal 2009-12-25 13:41:27 PST
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.
Comment 8 Noam Rosenthal 2009-12-25 16:42:35 PST
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.
Comment 9 Andreas Kling 2010-01-11 09:59:06 PST
Could you attach the test case to this bug? Thanks :)
Comment 10 Noam Rosenthal 2010-01-11 11:33:37 PST
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.
Comment 11 Simon Hausmann 2010-02-09 05:34:52 PST
Removing dependency to QtWebKit 4.7 API as this change doesn't affect the API
Comment 12 Ariya Hidayat 2010-02-11 10:28:43 PST
Created attachment 48580 [details]
use clipRegion instead of clipPath
Comment 13 Kenneth Rohde Christiansen 2010-02-11 12:42:15 PST
Comment on attachment 48580 [details]
use clipRegion instead of clipPath

Great stuff
Comment 14 WebKit Commit Bot 2010-02-11 23:36:50 PST
Comment on attachment 48580 [details]
use clipRegion instead of clipPath

Clearing flags on attachment: 48580

Committed r54705: <http://trac.webkit.org/changeset/54705>
Comment 15 WebKit Commit Bot 2010-02-11 23:36:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Benjamin Poulain 2010-02-12 01:59:55 PST
Can somebody checks if the bug is entirely fixed?