407463 - SVG painting significantly slower with a empty filter than with no filter at all

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Mike Schroepfer]

Attachment: SVG File with empty filter body

See attachments for two sample SVG files derived from Jresig's ecma-cloud svg file.  

The first has no filter defined with id="Shadow".  The second has the following filter: 

<filter id="Shadow" filterUnits="userSpaceOnUse">
</filter>

If I load both on a recent nightly on a MacMini the page with an empty filter element beachballs like crazy - try re-sizing the window and watch it lock up.  The same file with no filter element is slow but usable.

Shark profiles attached - it looks like the filter path is going through an entirely different, an much less optimized, drawing path.   I don't think this particular use case is that interesting but it sets a pretty bad minimun performance penalty for using filters at all.  So if you've got a document with a low-cost filter you are likely to take much more of a hit than you should.

Comment 1

[Mike Schroepfer]

Attachment: Shark profile of first testcase

Comment 2

[Mike Schroepfer]

Attachment: SVG testcase with filter element completely removed

Comment 3

[Mike Schroepfer]

Attachment: Shark profile of second testcase

Comment 4

[Robert Longson [:longsonr]]

The slowdown is due to the fact that for a filter you render to an offscreen surface then copy the offscreen surface to the drawing surface rather than writing to the drawing surface directly.

What if you set filterRes="2" would you expect that to be ignored as there are no filters or give you a pixellated result? Gecko should produce a pixellated result.

So the question is do we specially optimise for all the things that should make a filter undetectable i.e. no filterRes, no xlink:href (which I don't hink we support yet) etc. In the high level SVG code I think we should leave things as they are and not try for some special fast and potentially buggy code path.

Of course there may be some way to speed up what cairo is doing when it copies images.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We shouldn't optimize for the uncommon noop case, but this kind of shows that the general -baseline- speed for using any filter is very very slow.  Improving this case (without optimizations that just skip the work) would result in an improvement for every filter.

Something that could help a lot is the acquire_image_surface/XShm API that should be getting into cairo somewhat soonish, but even up to that point, there may be additional improvements that could be done.

We may even want to add some code to return a CGImageRef from a cairo surface on OSX -- that way we could even do custom code to use Core Image etc. to do the filtering...

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I think we need to try and figure out some improvement here.  Any additions to cairo to help out in this case we can do (short of adding in a whole filter framework, but that shouldn't be necessary; it sounds like we just need a faster way to get at the image bits).

Comment 7

[Robert Longson [:longsonr]]

Attachment: on second thoughts...

On second thoughts we can do something as such filters are invalid. With this patch we render http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-felem-01-b.html correctly.

Comment 8

[Robert Longson [:longsonr]]

Attachment: update to tip and add reftests

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

While I agree filters could use a lot of speeding up, I actually think the performance of Schrep's testcase is OK for 1.9. What I'm really concerned about is problems like performance on jresig's original testcase http://ejohn.org/files/ecma-cloud.svg ... which seems to hang the browser for a minute or two when you load it.

Comment 10

[Robert Longson [:longsonr]]

That's bug 403932. This patch is more about correctness than speed up anyway, with it we pass one more of the w3c testcases.

Comment 11

[tor]

With this patch we won't draw an element that references a nonexistent filter/mask/clipPath.  Is that really what we should be doing?

Comment 12

[Robert Longson [:longsonr]]

From http://www.w3.org/TR/SVG/masking.html#EstablishingANewClippingPath

It is an error if the 'clip-path' property references a non-existent object or if the referenced object is not a 'clipPath' element (see Error processing).

Equivalent exists in http://www.w3.org/TR/SVG/masking.html#Masking

I think the relevant piece from Error processing is...

The document shall be rendered up to, but not including, the first element which has an error

http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-felem-01-b.html 
Seems to me to confirm that this is what we should be doing.

Comment 13

[Robert Longson [:longsonr]]

Comment on attachment 299832 [details] [diff] [review]
update to tip and add reftests

simple fixes for one of the w3c svg testcases

Comment 14

[Robert Longson [:longsonr]]

Comment on attachment 299832 [details] [diff] [review]
update to tip and add reftests

oops blocking

Comment 15

[Robert Longson [:longsonr]]

checked in.

Comment 16

[Robert Longson [:longsonr]]

Checked in reftests:
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/filter-basic-01.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/filter-basic-02.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/filter-basic-03.svg