Bug 5192 - WebKit+SVG is missing support for many Filter Elements
Summary: WebKit+SVG is missing support for many Filter Elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks: 5857
  Show dependency treegraph
 
Reported: 2005-09-29 15:37 PDT by Eric Seidel (no email)
Modified: 2005-11-28 15:33 PST (History)
0 users

See Also:


Attachments
First pass at Obj-C and C++ bindings of Olivers filter code. (47.62 KB, patch)
2005-10-06 04:38 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Another pass at Obj-C and C++ binding code for Oliver (62.02 KB, patch)
2005-10-08 01:45 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
current state of lighting patch pt1 (56.36 KB, patch)
2005-11-02 19:32 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
current state of lighting pt2 (18.45 KB, patch)
2005-11-02 19:33 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Lighting patch v3 (109.58 KB, patch)
2005-11-08 01:58 PST, Oliver Hunt
eric: review-
Details | Formatted Diff | Diff
Yet another version of the lighting patch (106.92 KB, patch)
2005-11-19 17:11 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
more lighting (103.47 KB, patch)
2005-11-20 05:27 PST, Oliver Hunt
eric: review-
Details | Formatted Diff | Diff
Yet another update to the lighting patch (142.45 KB, patch)
2005-11-27 19:57 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch updated to work with current ToT (143.07 KB, patch)
2005-11-27 20:47 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Lighting patch, with more formatting fixes (121.13 KB, patch)
2005-11-28 04:55 PST, Oliver Hunt
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-09-29 15:37:55 PDT
WebKit+SVG is missing support for many SVG filter elements:

FE_DISTANT_LIGHT 
FE_POINT_LIGHT
FE_SPOT_LIGHT 
FE_COMPONENT_TRANSFER 
FE_CONVOLVE_MATRIX 
FE_DIFFUSE_LIGHTING 
FE_DISPLACEMENT_MAP
FE_MORPHOLOGY 
FE_SPECULAR_LIGHTING 
FE_TURBULENCE 

Most of these would be very easy to support by writing a small CoreImage filter, and requires someone 
with a bit of OpenGL knowledge to write about 5 lines of code. :)  Some of these may even be possible 
w/ existing CoreImage filters.

FE_IMAGE support also exists, but is broken.
Comment 1 Eric Seidel (no email) 2005-10-03 01:56:20 PDT
Oliver said he'd take a stab at this.
Comment 2 Eric Seidel (no email) 2005-10-06 04:38:31 PDT
Created attachment 4230 [details]
First pass at Obj-C and C++ bindings of Olivers filter code.

Oliver began from one end (the gl shading code) and I began from the other
(with the obj-c and c++ wrappers & kcanvas integration).  Hopefully we'll meet
in the middle eventually :)  This is my "first pass" at the necessary wrappers.
I have not tested compiling yet though...
Comment 3 Eric Seidel (no email) 2005-10-08 01:45:52 PDT
Created attachment 4251 [details]
Another pass at Obj-C and C++ binding code for Oliver
Comment 4 Eric Seidel (no email) 2005-10-27 00:52:07 PDT
Comment on attachment 4251 [details]
Another pass at Obj-C and C++ binding code for Oliver

This code is far from complete, but I would like to land it so as to put Oliver
in a better position to finish his implementation.  Landing this does not break
any existing layout tests.
Comment 5 Oliver Hunt 2005-11-02 19:32:21 PST
Created attachment 4569 [details]
current state of lighting patch pt1

This is the portion of the lighting patch that resides in kcanvas/device
Comment 6 Oliver Hunt 2005-11-02 19:33:24 PST
Created attachment 4570 [details]
current state of lighting pt2

This is the portion of the lighting patch that lives inside ksvg2/svg
Comment 7 Oliver Hunt 2005-11-08 01:58:32 PST
Created attachment 4626 [details]
Lighting patch v3

With many examples of poor programming practice, and incomplete path info this
is probably not a useful patch for most purposes :)
Comment 8 Eric Seidel (no email) 2005-11-08 02:55:26 PST
Comment on attachment 4626 [details]
Lighting patch v3

Ok, my raft of comments on your patch:

GREAT WORK.  Amazing screen shot you sent around earlier.

There are a bunch of little style issues to fix:
if( -- space between if and (
foo(){} -- space between ) and {
foo() {}; -- no need for ;
if (foo) bar();  -- No one-line ifs,  bar() needs to have its' own line,
indented.

KCanvasPoint3F makePoint() should be part of KCanvas3F, not a static function
in ksvg2.

Long constructor functions should be moved into the .cpp files, instead of the
headers.

Constructors should use c++ style inits m_foo(foo) instead of m_foo = foo;


Some functional issues too:

<feDiffuseLighting> and <feSpecularLighting> can have other nodes than just a
single lightsource in them.  So you need to walk the child nodes, looking for
the first source and use that.

Your custom filter code should take CIImage inputs instead of CISamplers and
should make the appropriate CISamplers from CIFilters in the apply function.

Don't call +initialize directly.  That's done for you at the first mention of
the class.  Call something innocous like [MyFilter class]; instead.

Yeah, in general, don't pass around CISamplers... think of that as an
"internal" class which CIFilters use.

It's better not to use convenience functions like:
outputImageWithNormals, but rather to lookup filters by name, reset them to
defaults, set each default, and then grab the output image (a CIImage) to pass
to the next filter.  Folowing this format makes for easier to read code, and
code to which it's easier to add things like Filter sub-region support.

We should consider removing FE_POINT_LIGHT from the constants for filter types,
if they are not ever going to be constructed as filters by the device.

We still need to come up with a slick solution for either loading the kernel
files at runtime, or compiling the kernel source into WebKit as a static
string.  Writing a simple perl script to take in cikernel file and output a
header file with a single static string defined would be a slick solution.

convolve still should be changed to use 3 3-vectors instead of 9 individual
float lookups.

New files you created should have the same Apple-style BSD license header,
except with your name on them.	Files you copied from ksvg, should carry the
ksvg style LGPL header w/ your name (and the names from the file you copied). 
All files need a up-to-date copyright header however.

All and all, the patch looks GREAT!  There are just a few little nits which we
should clean up before we land this one.

Thanks again for all your hard work!
Comment 9 Oliver Hunt 2005-11-18 21:28:13 PST
We also need support for the arithmetic operator of feComposite -- this should take only  a few 
minutes and a fairly simple shader -- adding the shader should be completely contrained to the shader 
wrappers, and kcanvasfiltersquartz.mm (i think that's the file)
Comment 10 Oliver Hunt 2005-11-19 17:11:19 PST
Created attachment 4738 [details]
Yet another version of the lighting patch

Still not ready for landing but a number of style problems have been fixed. 
Spotlights now go (albeit without working defaults).  

Also adds support for the arithmetic operation for feComposite
Comment 11 Oliver Hunt 2005-11-19 17:12:42 PST
Comment on attachment 4738 [details]
Yet another version of the lighting patch

Still has a number of style problems, but may as well catch any *wrong* fixes
before i do them everywhere
Comment 12 Oliver Hunt 2005-11-20 04:02:48 PST
our implementation of FE_GAUSSIAN_BLUR doesn't match the output of batik, I'm inclined to think this is 
because we're using an area blur rather than that wanted by the blur spec (we use standard deviation as 
radius, which is , ahem, wrong..)
Comment 13 Oliver Hunt 2005-11-20 05:27:22 PST
Created attachment 4739 [details]
more lighting

Primarily reformatting, but some code corrections exist (missing breaks)

beware the dragons
Comment 14 Eric Seidel (no email) 2005-11-23 04:23:28 PST
Comment on attachment 4739 [details]
more lighting

We've dicsussed this some over irc, issues include:

* copyright headers
* some code could be shorter:
+    CIFilter *filter;
+    filter = [[self alloc] init];
+    return [filter autorelease];
* newlines:
\ No newline at end of file
* no need for this header:
+#import <stdio.h>

I believe there were other things we discussed via irc.
Comment 15 Oliver Hunt 2005-11-27 19:57:04 PST
Created attachment 4824 [details]
Yet another update to the lighting patch

Okay this patch fixes:
  * many formatting issues
  * copyright notices (no more "copyright foo goes here")
  * many lighting issues, including incorrect z-depth calculation (which no one
noticed)
Unfortunately it has shown "issues" 
  * Images created by SVG
(http://www.w3.org/TR/2002/CR-SVG11-20020430/images/filters/filters01.svg) seem
to be flipped relative to images loaded with feImage (at least when there
normals are  calculated).
Comment 16 Oliver Hunt 2005-11-27 20:47:32 PST
Created attachment 4827 [details]
Patch updated to work with current ToT

Same as above, with a few fixes to keep up to date with ToT
Comment 17 Oliver Hunt 2005-11-28 04:55:48 PST
Created attachment 4832 [details]
Lighting patch, with more formatting fixes

more formatting changes, rearranged how
SVGFEDiffuse/SpecularLightingElementImpl get the light source.
Comment 18 Eric Seidel (no email) 2005-11-28 10:25:52 PST
Comment on attachment 4832 [details]
Lighting patch, with more formatting fixes

Here are my thoughts:

Still have one stray
#import <stdio.h> in WKDiffuseLightingFilter.mm

+// Temporary HACK
+// FIXME, more here...
Such comments should be either made more clear or removed.

Still a couple spacing mistakes:
KCLightSource(KCLightType a_type):m_type(a_type) {}
(missing spaces around the : )
and
+    const KCLightSource * lightSource() const { return m_lightSource; }
(too many spaces around * )

Why switch back to CIGaussianBlur?  Does that offer better correctness?

On the whole, this looks nearly goood enough to land.
Comment 19 Eric Seidel (no email) 2005-11-28 11:47:38 PST
Comment on attachment 4832 [details]
Lighting patch, with more formatting fixes

The paths are doubled for the new filter files:

kcanvas/device/quartz/device/quartz/filters/WKArithmeticFilter.cikernel
Comment 20 Eric Seidel (no email) 2005-11-28 15:18:29 PST
Comment on attachment 4832 [details]
Lighting patch, with more formatting fixes

I fixed the issues I mentioned above locally (including adding these to the
project file, and making a change log entry).  I'll land my fixed patch, and
post for posterity.
Comment 21 Eric Seidel (no email) 2005-11-28 15:33:25 PST
Landed Oliver's lighting patch.  I'm going to close this bug out and open up a new one to track the 
remaining filters.