Closed Bug 461863 Opened 11 years ago Closed 11 years ago

Nested filtering is broken (inner filtered elements are clipped or don't paint)

Categories

(Core :: SVG, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: wormsxulla, Assigned: jwatt)

References

()

Details

(Keywords: fixed1.9.1, regression, testcase)

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081010 SeaMonkey/2.0a2pre
Build Identifier: 

The URL http://corpsmoderne.net/~eve97/vertex/pretzel_fixed.svg should display a pretzel with "salt" grains on it. 
They used to show in SeaMonkey 2.0a1pre...

They are totally missing in my SeaMonkey build :
Build identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081010 SeaMonkey/2.0a2pre

They do appear in Firefox 3.0.3 and in Opera 9.60 and in Iceape 1.1.15.

N.B.: The vertical scrollbar is missing, too, but it's also missing in Opera 9.60 so I suspect this is another error (bug) in the SVG with some filters. See for comparison: http://corpsmoderne.net/~eve97/vertex/pretzel.svg which has an error in the shadow (not rendered completely on the right and at the bottom, but does have a vertical scrollbar - but no salt!

Reproducible: Always

Steps to Reproduce:
1. Go to http://corpsmoderne.net/~eve97/vertex/pretzel_fixed.svg
2. Look around for a salted pretzel
3.
Actual Results:  
The "salt" objects are missing.

Expected Results:  
A salted pretzel
Can you make a minimised testcase please? This is far too big to debug.
I just can't succeed an upload of a minimal testcase as an attachment in Bugzilla tonight, so here it is:

http://www.petaimg.com/uploads/1225196748.svg

or (same file):

http://corpsmoderne.net/~eve97/vertex/pretzel_f.svg
I was hoping for something much more minimal than that. Just a rect that should display and doesn't because of how it's transformed. 

A regression range would be nice too.
Thanks Martin.
Ah. Well, thanks for looking into that, it's kind of difficult for me to do so
on the eee pc...

All I can say is that the object (a pinkish square) shows in Firefox 3.0.3
(released on 2008, Sep. 26th) but do not show in latest trunk of Firefox 3.1
beta 1 (checked by doublec on #svg).

I have also simplified the file again (getting rid of unecessary layers), but I
can't go any further I think.
Attached image testcase (obsolete) —
Thanks for the smaller file, worms.
I've made a smaller testcase, but I'm not sure if this is about this issue at all. This shows a green circle in branch builds, but not in current trunk build. But this didn't regress between 2008-09-10 and 2008-09-11 as what is happening with the url.
Wow. No green circle in Opera 9.60 either with the testcase https://bugzilla.mozilla.org/attachment.cgi?id=345207 !

(Not that I am comparing stuff, but this attachment seems to be a different case from the one I submitted)
Attached image testcase2
Yeah, that's why I wondered about the testcase too (maybe it is somehow invalid, I guess).

But this testcase shows probably what this bug is about. It would show a vertical line in the 2008-09-10 trunk build (and does so in Opera), but the vertical line is much smaller in the 2008-09-10 build and in current trunk build nothing shows up.
Attachment #345207 [details] is invalid because the <filter> element has no filter primitive children.
Flags: blocking1.9.1?
Attachment #345207 - Attachment is obsolete: true
I think we're getting the bounding box of the g element filter wrong somehow so the data is all clipped out. Possibly connected to bug 460946.
Fixing bug 460946 does not help this.
Flags: blocking1.9.1? → blocking1.9.1+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → jwatt
OS: Linux → All
Hardware: PC → All
jwatt, are you on this?
Yes, I'm digging into this. I expect to have a patch in a day or two.
Thanks.  :)
Summary: Some objects are not displayed at all in the SVG → Nested filtering is broken (inner filtered elements are clipped or don't paint)
Attached patch patchSplinter Review
Sorry for the delay on this.

The problem here is that when we filter an element that has a filtered ancestor, the convertion of the dirty rect to device space for the painting of the inner filtered element goes wrong. This happens because the ctor of the nsAutoFilterInstance created for the filtered ancestor has called SetMatrixPropagation(PR_FALSE) an the ancestor (it won't be set back to PR_TRUE until the dtor is called), so the GetCanvasTM call in the nsAutoFilterInstance ctor for the inner filtered element fails to get the transform all the way back to device space. As a result, finiM in the nsAutoFilterInstance ctor contains a bogus transform, and therefore the calculation for dirtyOutputRect is wrong.

I was going to try and temporarily fix this by making a version of GetCanvasTM that always gets the transform to the root (or make GetCanvasTM take an argument to indicate that's what we want). That turned out to be messy though since it turns out to be tricky to figure out what all the GetCanvasTM calls actually want (often they want different things depending on state or what we're doing at the time). Happily roc realized that we can change the filter code fairly simply to not only avoid that mess, but actually get us one step closer to getting rid of the whole SetMatrixPropagation mess.

So essentially what the current filter code does is to set an initial "user space to filter space" transform on the gfxContext the filtered element paints to, and matches that by short circuiting GetCanvasTM so that it will return the transform to the filtered element's user space during painting of the filtered element. What this patch does is change the filter code to set a "device space to filter space" transform on the gfxContext instead, which allows us to eliminate the short circuiting code so that GetCanvasTM can continue to return the transform to device space, even while painting the filtered element.
Attachment #361261 - Flags: superreview?(roc)
Attachment #361261 - Flags: review?(roc)
+    // clip their insainly large filterRes.

You need a spell checker. s/insainly/insanely/
Sadly Xcode doesn't have one built in. :-) Thanks, fixed locally.
Is this why nested masks don't work either? (bug 457156)
I don't think so. The only place where we wrap large parts of code in SetMatrixPropagation calls is in the filter code. It doesn't look like the other SetMatrixPropagation calls could cause mask problems.
Attachment #361261 - Flags: superreview?(roc)
Attachment #361261 - Flags: superreview+
Attachment #361261 - Flags: review?(roc)
Attachment #361261 - Flags: review+
Comment on attachment 361261 [details] [diff] [review]
patch

+    static_cast<nsSVGLength2*>(filter->mLengthAttributes), bbox, aTarget);

Cast should not be necessary. Although I think I used the &elem[0] trick for a reason, so just removing the cast probably doesn't work. I know it's a little confusing but what I had might be the best.

+fails == filter-marked-line-01.svg pass.svg

Cite a bug number here.

We should really warn whenever anyone uses a non-percentage value with boundingBox units, and the spec should say that's deprecated. It's just too confusing for a bare number to be sometimes interpreted as a fraction of the bounding box and sometimes as user-space units.

+    // Hmm, what if the author did not specify filterRes?

Then I don't think we can get into this state, unless the filter region is empty, in which case not rendering is fine.
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
Flags: in-testsuite+
Jonathan, can you backport this to the 1.9.1 branch? It's not completely trivial due to the do_QueryFrame changes that have happened on trunk.
Pushed http://hg.mozilla.org/mozilla-central/rev/2afb4a746869 to GECKO191b2_20081125_RELBRANCH
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
that is not the right branch
Crap. Thanks.
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
You need to log in before you can comment on or make changes to this bug.