Bug 28141

Summary: SVG Filter feGaussianBlur implementation is missing
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: eric, jeffschiller, oliver
Priority: P2    
Version: 525.x (Safari 3.1)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 68469, 26389    
Description Flags
naive feGaussian-blur
feGaussianBlur implementation
oliver: review-
feGaussianBlur implementation
feGaussianBlur implementation
oliver: review-
feGaussianBlur implementation oliver: review+

Description Dirk Schulze 2009-08-10 02:48:23 PDT
SVG Filter feGaussianBlur implementation is missing.

Details for implementing:

The Specification http://www.w3.org/TR/SVG11/filters.html#feGaussianBlur give an alternative for a real gaussian blur, if stdDeviation is >= 2.0. Many other SVG Implementations take this alternative for every stdDeviation >= 0. It looks like even FireFox just uses the alternative. We might follow this becaues most SVG's with gaussianBlur take a stdDeviation > 2 and it reduces the complexity of calculation.
Comment 1 Dirk Schulze 2009-08-14 03:18:43 PDT
Created attachment 34824 [details]
naive feGaussian-blur

A very simlpe feGaussian-blur with box blur.
Comment 2 Dirk Schulze 2009-08-16 06:13:52 PDT
calculateDrawingIntRect will be introduced by bug 28133 , the last feBlend patch.
Comment 3 Dirk Schulze 2009-10-04 11:15:57 PDT
Created attachment 40597 [details]
feGaussianBlur implementation

Implementation of feGaussianBlur.
Comment 4 Eric Seidel (no email) 2009-10-05 09:17:15 PDT
This looks like a patch a graphics person like Oliver should comment on.
Comment 5 Dirk Schulze 2009-10-05 22:42:35 PDT
Nikolas asked why I call boxBlur twice in the for loop. I would like to explain it for the reviewer. The first call is for the horizontal bluring, the second call is for the vertical bluring. This is the same like a bluring with a complete kernel, but faster.
The reason for bluring the image 3 times is, that the spec want us to.
Comment 6 Oliver Hunt 2009-10-05 23:01:29 PDT
Comment on attachment 40597 [details]
feGaussianBlur implementation

There are a few places where blurring is misspelled as bluring.

double(sum / dx) seems bogus -- both sum and dx are integer types, so you're just doing an integer divide and casting to double, which is either superfluous, or not what you intended to do.

What happens if i apply this filter to a 3x3 image?  it looks to me like there's potential for overflow...
Comment 7 Dirk Schulze 2009-10-06 05:40:23 PDT
Created attachment 40713 [details]
feGaussianBlur implementation

I changed the double conversion to unsigned char. And fixed the writing of blur(r)ing :-). I also missed an early return, if one dimension of the kernel has the size of 0.

You were right with the overflow. There is a overflow on filling the kernel if the kernel size is bigger than the width/height of the effect. I double checked the rest of the code and didn't see further problems. I calculated some examples manualy (e.g. A kernel with 3x3) and it works as expected. An overflow on the blurring itself seems to me impossible, since I check if the next or previous value is not catched if it isn't in the current row/column (between 0 and width/height of a column/row).

I compared many other examples with FF and Opera and did not see any differences.
Comment 8 Dirk Schulze 2009-10-06 22:07:38 PDT
Created attachment 40762 [details]
feGaussianBlur implementation

> +                if (x + dxRight < effectHeight)

mixed up effectHeight with effectWidth here. So it just works if effectHeight == effectWidth. This patch fixes this issue.
Comment 9 Oliver Hunt 2009-10-06 23:38:40 PDT
Comment on attachment 40762 [details]
feGaussianBlur implementation

Code looks good, but i want additional tests for filtering 0x0, x*y with x!=y, and x*y with x<3, y > 3 and vice versa.
Comment 10 Dirk Schulze 2009-10-07 02:35:30 PDT
Created attachment 40777 [details]
feGaussianBlur implementation

added test with different kernel sizes. No change to the source.
Comment 11 Dirk Schulze 2009-10-09 13:53:29 PDT
landed in r49402.