Bug 5527 - Filters need subregion support
: Filters need subregion support
Status: RESOLVED FIXED
: WebKit
SVG
: 420+
: Macintosh Mac OS X 10.4
: P3 Normal
Assigned To:
:
: NeedsReduction
:
: 5976 6774 6904 12569
  Show dependency treegraph
 
Reported: 2005-10-28 00:54 PST by
Modified: 2007-10-28 16:36 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2005-10-28 00:54:56 PST
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 From 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 From 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 From 2005-11-21 01:42:36 PST -------
Moving to Erick who may already have a fix.
------- Comment #4 From 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 From 2006-01-26 14:46:15 PST -------
Reassigning to "webkit-unassigned".
------- Comment #6 From 2006-01-26 16:24:39 PST -------
*** Bug 6774 has been marked as a duplicate of this bug. ***
------- Comment #7 From 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 From 2007-08-21 11:43:24 PST -------
Created an attachment (id=16060) [details]
Initial patch

This brings back SVG filters in a useable state. Fixes numerous other bugs, too. See ChangeLog.
------- Comment #9 From 2007-08-21 11:43:58 PST -------
Created an attachment (id=16061) [details]
LayoutTest results
------- Comment #10 From 2007-08-21 14:01:05 PST -------
(From update of attachment 16060 [details])
r=me, provided you deal with th e following issues:

Can we use RetainPtr for 
------- Comment #11 From 2007-08-21 14:05:53 PST -------
(From update of attachment 16060 [details])
r=me, provided you deal with th e following issues:

Can we use RetainPtr for 
------- Comment #12 From 2007-08-21 14:08:57 PST -------
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 From 2007-08-21 14:13:00 PST -------
(From update of attachment 16061 [details])
rubber stamped
------- Comment #14 From 2007-08-21 14:16:38 PST -------
(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 From 2007-08-21 14:34:08 PST -------
Landed in r25175.
------- Comment #16 From 2007-10-26 01:32:33 PST -------
This seems broken on TOT. At least the test case for bug 6774.
------- Comment #17 From 2007-10-28 14:34:53 PST -------
(From update of attachment 16061 [details])
Clearing flag, since this patch was already landed before the bug was reopened.
------- Comment #18 From 2007-10-28 14:35:02 PST -------
(From update of attachment 16060 [details])
Clearing flag, since this patch was already landed before the bug was reopened.
------- Comment #19 From 2007-10-28 16:36:56 PST -------
My mistake.  This appears to work fine on TOT.  I must have been testing with Safari 3 by accident.