Bug 28141

Summary: SVG Filter feGaussianBlur implementation is missing
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
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    
Attachments:
Description Flags
naive feGaussian-blur
none
feGaussianBlur implementation
oliver: review-
feGaussianBlur implementation
none
feGaussianBlur implementation
oliver: review-
feGaussianBlur implementation oliver: review+

Dirk Schulze
Reported 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.
Attachments
naive feGaussian-blur (6.39 KB, patch)
2009-08-14 03:18 PDT, Dirk Schulze 		no flags
DetailsFormatted DiffDiff
feGaussianBlur implementation (5.96 KB, patch)
2009-10-04 11:15 PDT, Dirk Schulze 		oliver: review-
DetailsFormatted DiffDiff
feGaussianBlur implementation (6.67 KB, patch)
2009-10-06 05:40 PDT, Dirk Schulze 		no flags
DetailsFormatted DiffDiff
feGaussianBlur implementation (6.67 KB, patch)
2009-10-06 22:07 PDT, Dirk Schulze 		oliver: review-
DetailsFormatted DiffDiff
feGaussianBlur implementation (31.16 KB, patch)
2009-10-07 02:35 PDT, Dirk Schulze 		oliver: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Dirk Schulze
Comment 1 2009-08-14 03:18:43 PDT
Created attachment 34824 [details] naive feGaussian-blur A very simlpe feGaussian-blur with box blur.
Dirk Schulze
Comment 2 2009-08-16 06:13:52 PDT
calculateDrawingIntRect will be introduced by bug 28133 , the last feBlend patch.
Dirk Schulze
Comment 3 2009-10-04 11:15:57 PDT
Created attachment 40597 [details] feGaussianBlur implementation Implementation of feGaussianBlur.
Eric Seidel (no email)
Comment 4 2009-10-05 09:17:15 PDT
This looks like a patch a graphics person like Oliver should comment on.
Dirk Schulze
Comment 5 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.
Oliver Hunt
Comment 6 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...
Dirk Schulze
Comment 7 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.
Dirk Schulze
Comment 8 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.
Oliver Hunt
Comment 9 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.
Dirk Schulze
Comment 10 2009-10-07 02:35:30 PDT
Created attachment 40777 [details] feGaussianBlur implementation added test with different kernel sizes. No change to the source.
Dirk Schulze
Comment 11 2009-10-09 13:53:29 PDT
landed in r49402.
Note You need to log in before you can comment on or make changes to this bug.