Bug 5527 – Filters need subregion support

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Filters need subregion support

All of the filter elements need to support subregions:
http://www.w3.org/TR/SVG/filters.html#FilterPrimitiveSubRegion

ours do not currently, however this should be really easy to do with a few strategic 
FE_QUARTZ_CROP_TO_RECT calls. :)

I've added the NeedsReduction flag as this will need some test cases.

[Erick Tryzelaar]

so it looks like this bug is also contributing to filters-tile-01-b.svg not working correctly. Two things 
need to be fixed as far as I can see:

1) We need to properly read in the subregion bounds. If nothing is specified for the filter subregion, it 
should default to {0%,0% 100%,100%} of the filter region according to the spec. Right now everything is 
defaulting to {0,0 0,0} if it's not specified. 

2) We need to properly clip the subregion. FE_QUARTZ_CROP_TO_RECT calls could work well enough, or 
we could pull out the subregion specification into the cross platform section to share more code, but 
I'm not sure how to do that yet. 

Eric, do you know what would need to be changed in order to fix the first bullet? I haven't been able to 
figure out what would need to be done in order to fix it.

[Eric Seidel (no email)]

(In reply to comment #1)
> so it looks like this bug is also contributing to filters-tile-01-b.svg not working correctly. Two things 
> need to be fixed as far as I can see:
> 
> 1) We need to properly read in the subregion bounds. If nothing is specified for the filter subregion, it 
> should default to {0%,0% 100%,100%} of the filter region according to the spec. Right now everything 
is 
> defaulting to {0,0 0,0} if it's not specified. 

Well, for now we'd probably have to interpret 0,0,0,0 (or !myQRect.isValid()) as being "the default" and 
just make the code know about the 100% default...

Why we'd have to do this, is because the KCanvas doesnt' currently have any concept of "relative 
lengths" like the khtml render tree does.  Eventually this will change, and relative length computations 
will be made in the ksvg2 render tree instead of the dom.

Another solution would be to make the DOM know about this default (this would fit better with the 
current model) and pass absolute coordinates into KCanvas (as it expects).  You could model this off of 
other dom classes which handle relative lengths.  I believe there are existing methods on 
KCanvasRenderingStyle and KSVG::Helper to help you with this conversion.

> 2) We need to properly clip the subregion. FE_QUARTZ_CROP_TO_RECT calls could work well enough, 
or 
> we could pull out the subregion specification into the cross platform section to share more code, but 
> I'm not sure how to do that yet. 

Well,  I don't think clipping can be done in the cross platform section of the code... however, storing of 
the clipping data should be done there.  It should be added to the FEStandardAttributes class if it isn't 
there already.

[Eric Seidel (no email)]

Moving to Erick who may already have a fix.

[Eric Seidel (no email)]

http://bugzilla.opendarwin.org/show_bug.cgi?id=6774 has a good test case for this.

[Eric Seidel (no email)]

Reassigning to "webkit-unassigned".

[Eric Seidel (no email)]

*** Bug 6774 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

A nice test case is attached to:
http://bugzilla.opendarwin.org/show_bug.cgi?id=6774

[Nikolas Zimmermann]

Created attachment 16060 [details]
Initial patch

This brings back SVG filters in a useable state. Fixes numerous other bugs, too. See ChangeLog.

[Nikolas Zimmermann]

Created attachment 16061 [details]
LayoutTest results

[Oliver Hunt]

Comment on attachment 16060 [details]
Initial patch

r=me, provided you deal with th e following issues:

Can we use RetainPtr for

[Oliver Hunt]

Comment on attachment 16060 [details]
Initial patch

r=me, provided you deal with th e following issues:

Can we use RetainPtr for

[Oliver Hunt]

That should have said can we use RetainPtr for SVGResourceFilter::m_imagesByName

I'm unhappy with DEBUG_OUTPUT_IMAGE/dumpOutputImage.  Our usual approach would be to have dumpOutputImage available in debug builds, and to then set a break pooint wherever, and call it from the debugger, rather than having yet another flag that effects runtime behaviour.

There are also a couple of places you change my copyright notices, while your at it you can update the email address to oliver <at> nerget <dot> com :D

[Oliver Hunt]

Comment on attachment 16061 [details]
LayoutTest results

rubber stamped

[Nikolas Zimmermann]

(In reply to comment #12)
> That should have said can we use RetainPtr for
> SVGResourceFilter::m_imagesByName
To be discussed on IRC.

> 
> I'm unhappy with DEBUG_OUTPUT_IMAGE/dumpOutputImage.  Our usual approach would
> be to have dumpOutputImage available in debug builds, and to then set a break
> pooint wherever, and call it from the debugger, rather than having yet another
> flag that effects runtime behaviour.
And passing a filename where to store the output? I'm rather fine with this solution
as I'm using it in quite a lot tricky places (ie. SVGRootInlineBox & DEBUG_CHUNK_BUILDING).

It's just a per-file specific define, don't see a big problem with it.
 
> There are also a couple of places you change my copyright notices, while your
> at it you can update the email address to oliver <at> nerget <dot> com :D
Okay.

Greetings,
Niko

[Nikolas Zimmermann]

Landed in r25175.

[Eric Seidel (no email)]

This seems broken on TOT. At least the test case for bug 6774.

[Darin Adler]

Comment on attachment 16061 [details]
LayoutTest results

Clearing flag, since this patch was already landed before the bug was reopened.

[Darin Adler]

Comment on attachment 16060 [details]
Initial patch

Clearing flag, since this patch was already landed before the bug was reopened.

[Eric Seidel (no email)]

My mistake.  This appears to work fine on TOT.  I must have been testing with Safari 3 by accident.