Bug 32738

Created attachment 45178 [details] Speed-up of SVG Mask This makes SVG Masking faster. The next step is to just manipulate visible areas.

style-queue ran check-webkit-style on attachment 45178 [details] without any errors.

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?

(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.

Created attachment 45244 [details] Speed-up of SVG Mask Make pixelmanipulation just for the visible part. Don't create a extra ImageBuffer or pixelarray.

style-queue ran check-webkit-style on attachment 45244 [details] without any errors.

Comment on attachment 45244 [details] Speed-up of SVG Mask LGTM, r=me.

Comment on attachment 45244 [details] Speed-up of SVG Mask Clearing flags on attachment: 45244 Committed r52395: <http://trac.webkit.org/changeset/52395>

All reviewed patches have been landed. Closing bug.

To answer Darin's question, yes there are masking tests which should test if we break masking.

I think this broke the world: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52399%20(8505)/results.html probably some ASSERT.

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.

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

Thanks Eric. It realy hits asserts. I'm not sure if some of them are valid. I take a look at it.

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.

style-queue ran check-webkit-style on attachment 45263 [details] without any errors.

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.

style-queue ran check-webkit-style on attachment 45293 [details] without any errors.

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 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 :-)

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.

style-queue ran check-webkit-style on attachment 45333 [details] without any errors.

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

landed in r52449.

And caused a regression: bug 34185.