Bug 5192 – WebKit+SVG is missing support for many Filter Elements

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Oliver said he'd take a stab at this.

[Eric Seidel (no email)]

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...

[Eric Seidel (no email)]

Created attachment 4251 [details]
Another pass at Obj-C and C++ binding code for Oliver

[Eric Seidel (no email)]

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.

[Oliver Hunt]

Created attachment 4569 [details]
current state of lighting patch pt1

This is the portion of the lighting patch that resides in kcanvas/device

[Oliver Hunt]

Created attachment 4570 [details]
current state of lighting pt2

This is the portion of the lighting patch that lives inside ksvg2/svg

[Oliver Hunt]

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 :)

[Eric Seidel (no email)]

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!

[Oliver Hunt]

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)

[Oliver Hunt]

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

[Oliver Hunt]

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

[Oliver Hunt]

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..)

[Oliver Hunt]

Created attachment 4739 [details]
more lighting

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

beware the dragons

[Eric Seidel (no email)]

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.

[Oliver Hunt]

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).

[Oliver Hunt]

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

[Oliver Hunt]

Created attachment 4832 [details]
Lighting patch, with more formatting fixes

more formatting changes, rearranged how
SVGFEDiffuse/SpecularLightingElementImpl get the light source.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Landed Oliver's lighting patch.  I'm going to close this bug out and open up a new one to track the 
remaining filters.