Bug 32738 - Speed-up SVG Masking
Summary: Speed-up SVG Masking
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-18 13:19 PST by Dirk Schulze
Modified: 2010-02-10 16:24 PST (History)
4 users (show)

See Also:


Attachments
Speed-up of SVG Mask (3.25 KB, patch)
2009-12-18 13:27 PST, Dirk Schulze
darin: review+
Details | Formatted Diff | Diff
Speed-up of SVG Mask (7.73 KB, patch)
2009-12-19 12:20 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Speed-up of SVG Mask (7.87 KB, patch)
2009-12-20 02:10 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Speed-up of SVG Mask (7.83 KB, patch)
2009-12-20 13:19 PST, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff
Speed-up of SVG Mask (108.24 KB, patch)
2009-12-21 08:55 PST, Dirk Schulze
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2009-12-18 13:19:55 PST
SVG Masking needs to be faster.
Comment 1 Dirk Schulze 2009-12-18 13:27:39 PST
Created attachment 45178 [details]
Speed-up of SVG Mask

This makes SVG Masking faster. The next step is to just manipulate visible areas.
Comment 2 WebKit Review Bot 2009-12-18 13:30:32 PST
style-queue ran check-webkit-style on attachment 45178 [details] without any errors.
Comment 3 Darin Adler 2009-12-18 16:31:56 PST
Comment on attachment 45178 [details]
Speed-up of SVG Mask

Looks good. r=me

I assume there some existing test that checks the correct behavior after the change?
Comment 4 Dirk Schulze 2009-12-19 10:02:26 PST
(In reply to comment #3)
> (From update of attachment 45178 [details])
> Looks good. r=me
> 
> I assume there some existing test that checks the correct behavior after the
> change?

Thank you for the review. There is still more optimization possible and I would like to make it all in one small patch.
Comment 5 Dirk Schulze 2009-12-19 12:20:58 PST
Created attachment 45244 [details]
Speed-up of SVG Mask

Make pixelmanipulation just for the visible part. Don't create a extra ImageBuffer or pixelarray.
Comment 6 WebKit Review Bot 2009-12-19 12:22:31 PST
style-queue ran check-webkit-style on attachment 45244 [details] without any errors.
Comment 7 Nikolas Zimmermann 2009-12-19 13:43:50 PST
Comment on attachment 45244 [details]
Speed-up of SVG Mask

LGTM, r=me.
Comment 8 WebKit Commit Bot 2009-12-19 13:58:59 PST
Comment on attachment 45244 [details]
Speed-up of SVG Mask

Clearing flags on attachment: 45244

Committed r52395: <http://trac.webkit.org/changeset/52395>
Comment 9 WebKit Commit Bot 2009-12-19 13:59:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Eric Seidel (no email) 2009-12-19 16:04:08 PST
To answer Darin's question, yes there are masking tests which should test if we break masking.
Comment 11 Eric Seidel (no email) 2009-12-19 23:21:31 PST
I think this broke the world:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52399%20(8505)/results.html

probably some ASSERT.
Comment 12 Eric Seidel (no email) 2009-12-19 23:26:36 PST
Yes, I've confirmed that http://trac.webkit.org/changeset/52395 is causing the 15 layout test failures in Debug mode.  Sadly the commit-queue currently only uses Release mode (see bug 28450).  I'll roll out this change for now.
Comment 13 Eric Seidel (no email) 2009-12-19 23:48:54 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/svg/SVGMaskElement.cpp
	M	WebCore/svg/SVGMaskElement.h
	M	WebCore/svg/graphics/SVGResourceMasker.cpp
	M	WebCore/svg/graphics/SVGResourceMasker.h
Committed r52403
	M	WebCore/ChangeLog
	M	WebCore/svg/SVGMaskElement.h
	M	WebCore/svg/SVGMaskElement.cpp
	M	WebCore/svg/graphics/SVGResourceMasker.cpp
	M	WebCore/svg/graphics/SVGResourceMasker.h
r52403 = 4e9a236afa577a5dad6ae3be03b3dd184d53b1f7 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
Comment 14 Dirk Schulze 2009-12-19 23:53:14 PST
Thanks Eric. It realy hits asserts. I'm not sure if some of them are valid. I take a look at it.
Comment 15 Dirk Schulze 2009-12-20 02:10:22 PST
Created attachment 45263 [details]
Speed-up of SVG Mask

We have the same ASSERT's in putImageData on Gtk. And they didn't fire with this patch. I added checks to be sure, that the manipulation area is in the bounds of the imageBuffer. The code skips pixel manipulation, if the wanted area has a size of 0 in one dimension.
Comment 16 WebKit Review Bot 2009-12-20 02:12:38 PST
style-queue ran check-webkit-style on attachment 45263 [details] without any errors.
Comment 17 Dirk Schulze 2009-12-20 13:19:55 PST
Created attachment 45293 [details]
Speed-up of SVG Mask

Use maskImage->size() directly instead of the rounded maskRect. This makes sure, that the area for pixel manipilation is never bigger than the image size.
Comment 18 WebKit Review Bot 2009-12-20 13:22:45 PST
style-queue ran check-webkit-style on attachment 45293 [details] without any errors.
Comment 19 Nikolas Zimmermann 2009-12-20 13:22:50 PST
Comment on attachment 45293 [details]
Speed-up of SVG Mask

Looks good to me, after discussion on IRC. r=me. Let's watch buildbots this time :-)
Comment 20 Nikolas Zimmermann 2009-12-20 13:22:59 PST
Comment on attachment 45293 [details]
Speed-up of SVG Mask

Looks good to me, after discussion on IRC. r=me. Let's watch buildbots this time :-)
Comment 21 Dirk Schulze 2009-12-21 08:55:14 PST
Created attachment 45333 [details]
Speed-up of SVG Mask

Moved the pixel manipulation to SVGMaskElement. Now we make the pixel manipulation once and not on every call of applyMask. The intermediate ImageBuffer is also limited to the viewable area.
Comment 22 WebKit Review Bot 2009-12-21 09:02:05 PST
style-queue ran check-webkit-style on attachment 45333 [details] without any errors.
Comment 23 Darin Adler 2009-12-21 10:45:07 PST
Comment on attachment 45333 [details]
Speed-up of SVG Mask

> +    // calculate the smallest rect for the mask ImageBuffer.

Should capitalize this as well as having a period at the end to use the "sentence style".

> +    for (Node* n = firstChild(); n; n = n->nextSibling()) {

We normally prefer short words over single letters for variable names like this one.

> +        if (!n->isSVGElement() || !static_cast<SVGElement*>(n)->isStyled() || !n->renderer())
> +            continue;
> +        // FIXME: repaintRectInLocalCoordinates() is not correctly implemented.
> +        // The current implementation makes it possible to use it here.
> +        // We need more detailed boundingBoxes.
> +        // See bug: https://bugs.webkit.org/show_bug.cgi?id=32815

I don't understand the phrase "makes it possible to use it here" in this comment.

To me it's completely non obvious that we should union all "non-styled" SVG elements' repaint rects here. I think this needs a comment to make clear why this is the correct algorithm. I know this code was just moved, but I still think the clarity improvement would be simple.

> +    if (maskContentUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {

Here, I think the most useful type of comment would be a brief one that note that the following is an optimization and explains why the optimization can only be applied when the mask content units have this type.

r=me
Comment 24 Dirk Schulze 2009-12-21 12:55:58 PST
landed in r52449.
Comment 25 Simon Fraser (smfr) 2010-02-10 16:24:44 PST
And caused a regression: bug 34185.