Bug 5527 - Filters need subregion support
: Filters need subregion support
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 420+
: Macintosh Mac OS X 10.4
: P3 Normal
Assigned To: Nikolas Zimmermann
: NeedsReduction
Depends on:
Blocks: 5976 6774 6904 12569
  Show dependency treegraph
 
Reported: 2005-10-28 00:54 PDT by Eric Seidel
Modified: 2007-10-28 16:36 PDT (History)
2 users (show)

See Also:


Attachments
Initial patch (169.24 KB, patch)
2007-08-21 11:43 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
LayoutTest results (79.26 KB, text/plain)
2007-08-21 11:43 PDT, Nikolas Zimmermann
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2005-10-28 00:54:56 PDT
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.
Comment 1 Erick Tryzelaar 2005-11-17 23:34:23 PST
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.
Comment 2 Eric Seidel 2005-11-18 13:54:26 PST
(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.

Comment 3 Eric Seidel 2005-11-21 01:42:36 PST
Moving to Erick who may already have a fix.
Comment 4 Eric Seidel 2006-01-26 03:45:21 PST
http://bugzilla.opendarwin.org/show_bug.cgi?id=6774 has a good test case for this.
Comment 5 Eric Seidel 2006-01-26 14:46:15 PST
Reassigning to "webkit-unassigned".
Comment 6 Eric Seidel 2006-01-26 16:24:39 PST
*** Bug 6774 has been marked as a duplicate of this bug. ***
Comment 7 Eric Seidel 2006-01-26 16:25:07 PST
A nice test case is attached to:
http://bugzilla.opendarwin.org/show_bug.cgi?id=6774
Comment 8 Nikolas Zimmermann 2007-08-21 11:43:24 PDT
Created attachment 16060 [details]
Initial patch

This brings back SVG filters in a useable state. Fixes numerous other bugs, too. See ChangeLog.
Comment 9 Nikolas Zimmermann 2007-08-21 11:43:58 PDT
Created attachment 16061 [details]
LayoutTest results
Comment 10 Oliver Hunt 2007-08-21 14:01:05 PDT
Comment on attachment 16060 [details]
Initial patch

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

Can we use RetainPtr for
Comment 11 Oliver Hunt 2007-08-21 14:05:53 PDT
Comment on attachment 16060 [details]
Initial patch

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

Can we use RetainPtr for
Comment 12 Oliver Hunt 2007-08-21 14:08:57 PDT
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
Comment 13 Oliver Hunt 2007-08-21 14:13:00 PDT
Comment on attachment 16061 [details]
LayoutTest results

rubber stamped
Comment 14 Nikolas Zimmermann 2007-08-21 14:16:38 PDT
(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
Comment 15 Nikolas Zimmermann 2007-08-21 14:34:08 PDT
Landed in r25175.
Comment 16 Eric Seidel 2007-10-26 01:32:33 PDT
This seems broken on TOT. At least the test case for bug 6774.
Comment 17 Darin Adler 2007-10-28 14:34:53 PDT
Comment on attachment 16061 [details]
LayoutTest results

Clearing flag, since this patch was already landed before the bug was reopened.
Comment 18 Darin Adler 2007-10-28 14:35:02 PDT
Comment on attachment 16060 [details]
Initial patch

Clearing flag, since this patch was already landed before the bug was reopened.
Comment 19 Eric Seidel 2007-10-28 16:36:56 PDT
My mistake.  This appears to work fine on TOT.  I must have been testing with Safari 3 by accident.