Bug 28141 - SVG Filter feGaussianBlur implementation is missing
Summary: SVG Filter feGaussianBlur implementation is missing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 68469 26389
  Show dependency treegraph
 
Reported: 2009-08-10 02:48 PDT by Dirk Schulze
Modified: 2014-05-12 05:54 PDT (History)
3 users (show)

See Also:


Attachments
naive feGaussian-blur (6.39 KB, patch)
2009-08-14 03:18 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
feGaussianBlur implementation (5.96 KB, patch)
2009-10-04 11:15 PDT, Dirk Schulze
oliver: review-
Details | Formatted Diff | Diff
feGaussianBlur implementation (6.67 KB, patch)
2009-10-06 05:40 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
feGaussianBlur implementation (6.67 KB, patch)
2009-10-06 22:07 PDT, Dirk Schulze
oliver: review-
Details | Formatted Diff | Diff
feGaussianBlur implementation (31.16 KB, patch)
2009-10-07 02:35 PDT, Dirk Schulze
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.