Bug 41605 - Fixes to the gaussian blur algorithm
: Fixes to the gaussian blur algorithm
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
:
: 40793
  Show dependency treegraph
 
Reported: 2010-07-05 04:28 PST by
Modified: 2010-08-11 01:44 PST (History)


Attachments
Proposed patch (17.04 KB, patch)
2010-07-06 12:29 PST, Alejandro G. Castro
krit: review-
Review Patch | Details | Formatted Diff | Diff
New result of the feGaussianBlur.svg (35.30 KB, image/png)
2010-07-07 05:08 PST, Alejandro G. Castro
no flags Details
Proposed patch (18.51 KB, patch)
2010-07-09 09:23 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Pixel results (988.35 KB, application/octet-stream)
2010-07-10 01:30 PST, Dirk Schulze
no flags Details
Proposed patch (18.53 KB, patch)
2010-07-14 05:03 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Patch includes new results (1.39 MB, patch)
2010-08-05 13:52 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
images in a zip (736.87 KB, patch)
2010-08-06 04:13 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (1.93 MB, patch)
2010-08-10 02:24 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (1.39 MB, patch)
2010-08-10 04:59 PST, Alejandro G. Castro
krit: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-05 04:28:42 PST
We have to modify the current implementation of the blur filter with the recommendations of the current SVG standard. Basically, review box sizes according to http://www.w3.org/TR/SVG/filters.html#feGaussianBlurElement.

For more information check bug 40793.
------- Comment #1 From 2010-07-06 12:29:33 PST -------
Created an attachment (id=60652) [details]
Proposed patch

This patch unskips one test for gtk but does not change the pixel tests for other platforms that could require a new baseline. I think it is a good option to upload it check the bots and upload bugs with skips so people can create the new baselines, actually it is something we usually do with gtk+ and helps the developers not to have every platform.
------- Comment #2 From 2010-07-06 14:07:46 PST -------
Attachment 60652 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3392345
------- Comment #3 From 2010-07-06 22:24:45 PST -------
(In reply to comment #2)
> Attachment 60652 [details] [details] did not build on mac:
> Build output: http://webkit-commit-queue.appspot.com/results/3392345

cc1plus: warnings being treated as errors
/Users/eseidel/Projects/MacEWS/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:38: warning: implicit conversion shortens 64-bit value into a 32-bit value
------- Comment #4 From 2010-07-06 22:27:06 PST -------
Sorry, was a mistake in my first review, shouldn't be piDouble, but piFloat:
static const float gGaussianKernelFactor = (3 * sqrt(2 * piFloat) / 4.f); should work.
------- Comment #5 From 2010-07-06 22:45:56 PST -------
(From update of attachment 60652 [details])
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:38
 +  static const float gGaussianKernelFactor = (3 * sqrt(2 * piDouble) / 4);
Again, was my mistake, see comment above. Can you delete #include <math.h> as well please? Shouldn't be needed.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:150
 +      unsigned kernelSizeX = static_cast<unsigned>(floor(max(m_stdX, static_cast<float>(2.0)) * filter->filterResolution().width() * gGaussianKernelFactor + 0.5));
Isn't it enough to write 2.f instead of static_cast<float>(2.0)? Please use 0.5f. Only if you have a whole-number, you can omit .0 or .f (not sure about the denominator on a division).

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:151
 +      unsigned kernelSizeY = static_cast<unsigned>(floor(max(m_stdY, static_cast<float>(2.0)) * filter->filterResolution().height() * gGaussianKernelFactor + 0.5));
dito

To the LayoutTests.. You changes behavior, so you need some more layoutTests to cover the changes. I suggest a test, with a stdDerivation of "0", "0 3" and "3 0". According to the new spec, we should see results in all three cases (not the case at the moment).
You changed the result of one of the tests, so that other platforms don't need new test results? That's not the regular case to do so :-) If you don't have a Mac, or no access to a Mac, I can create the results for you.
The patch misses a ChangeLog entry for LayoutTests.

Great patch! Hope to see an update soon. r- for the few style issues and the missing LayoutTests/Changelog-entry.
------- Comment #6 From 2010-07-07 04:44:20 PST -------
(In reply to comment #5)
> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:150
>  +      unsigned kernelSizeX = static_cast<unsigned>(floor(max(m_stdX, static_cast<float>(2.0)) * filter->filterResolution().width() * gGaussianKernelFactor + 0.5));
> Isn't it enough to write 2.f instead of static_cast<float>(2.0)? Please use 0.5f. Only if you have a whole-number, you can omit .0 or .f (not sure about the denominator on a division).
> 

2.f was the original code :), I modified it this way because it was part of your suggestions for the last patch.

> 
> To the LayoutTests.. You changes behavior, so you need some more layoutTests to cover the changes. I suggest a test, with a stdDerivation of "0", "0 3" and "3 0". According to the new spec, we should see results in all three cases (not the case at the moment).

I understand, are you talking about pixel tests or dump texts? I guess pixel tests are the best option in this case, in that case we can upload them without baseline so they appear as new to the ports doing pixel tests. BTW, Martin is trying to get the patch adding gtk pixel tests, hopefully we could have them soon :).

> You changed the result of one of the tests, so that other platforms don't need new test results? That's not the regular case to do so :-)

The test that I added to gtk was skipped, it had no result, it was a dump test not pixel test, and the expected result does not change before or after the patch, so it could be the same for the other port. The other option is to skip those tests and upload a bug so the people from the other ports have the opportunity to check it and we do not risk to break the bots.

> If you don't have a Mac, or no access to a Mac, I can create the results for you.

If you could do that for me I'd owe you a beer :). Honestly I'm not sure the best option in these cases, I see a lot of people skipping tests from other ports and I seems kind of sane to me, the other option of having all the platforms each developer looks like not very efficient to me, but I'm no

> The patch misses a ChangeLog entry for LayoutTests.
> 

Oops :).

> Great patch! Hope to see an update soon. r- for the few style issues and the missing LayoutTests/Changelog-entry.

Thanks for the prompt and helpful reply. I'll review and upload a new revision.
------- Comment #7 From 2010-07-07 04:57:48 PST -------
(In reply to comment #5)
>
> [...]
>
> (From update of attachment 60652 [details] [details])
> To the LayoutTests.. You changes behavior, so you need some more layoutTests to cover the changes. I suggest a test, with a stdDerivation of "0", "0 3" and "3 0". According to the new spec, we should see results in all three cases (not the case at the moment).

Apparently the test svg/filters/feGaussianBlur.svg includes those cases so we can use it:

<filter id="0x0">
    <feGaussianBlur stdDeviation="0"/>
</filter>
<filter id="0x5">
    <feGaussianBlur stdDeviation="0 5"/>
</filter>
<filter id="5x0">
    <feGaussianBlur stdDeviation="5 0"/>
</filter>

Hmm, and it seems although qt and gtk are doing a text dump, mac id doing the proper pixel test.
------- Comment #8 From 2010-07-07 05:00:42 PST -------
(In reply to comment #7)
> (In reply to comment #5)
> >
> > [...]
> >
> > (From update of attachment 60652 [details] [details] [details])
> > To the LayoutTests.. You changes behavior, so you need some more layoutTests to cover the changes. I suggest a test, with a stdDerivation of "0", "0 3" and "3 0". According to the new spec, we should see results in all three cases (not the case at the moment).
> 
> Apparently the test svg/filters/feGaussianBlur.svg includes those cases so we can use it:
> 
> <filter id="0x0">
>     <feGaussianBlur stdDeviation="0"/>
> </filter>
> <filter id="0x5">
>     <feGaussianBlur stdDeviation="0 5"/>
> </filter>
> <filter id="5x0">
>     <feGaussianBlur stdDeviation="5 0"/>
> </filter>
> 
> Hmm, and it seems although qt and gtk are doing a text dump, mac id doing the proper pixel test.

What is the result of this test? Can you make a screenshot?
------- Comment #9 From 2010-07-07 05:08:22 PST -------
Created an attachment (id=60718) [details]
New result of the feGaussianBlur.svg

Now all the filters with deviation 0 appear. I've just checked that maybe there is a bug when we have 5 0 or 0 5, because the minimum deviation so when it reaches the kernelsize calculation it uses 2 and blurs in that direction, urg.
------- Comment #10 From 2010-07-07 05:33:05 PST -------
(In reply to comment #9)
> Created an attachment (id=60718) [details] [details]
> New result of the feGaussianBlur.svg
> 
> Now all the filters with deviation 0 appear. I've just checked that maybe there is a bug when we have 5 0 or 0 5, because the minimum deviation so when it reaches the kernelsize calculation it uses 2 and blurs in that direction, urg.

right, shouldn't blur on 0 :-/
------- Comment #11 From 2010-07-09 07:29:42 PST -------
Changed component to SVG, so it shows up in my all-svg-bugs search.
------- Comment #12 From 2010-07-09 09:23:27 PST -------
Created an attachment (id=61047) [details]
Proposed patch

Fixed the problem with the blur with one deviation set to 0 and the other points commented. I do not mark the patch for review until someone could generate the mac pixel tests results for me, or if you think it is ok I skip those tests and upload a bug so they can do it.
------- Comment #13 From 2010-07-09 09:24:56 PST -------
(In reply to comment #12)
> Created an attachment (id=61047) [details] [details]
> Proposed patch
> 
> Fixed the problem with the blur with one deviation set to 0 and the other points commented. I do not mark the patch for review until someone could generate the mac pixel tests results for me, or if you think it is ok I skip those tests and upload a bug so they can do it.

Is it possible to generate those pixel test results from gtk platform? Not sure about this.
------- Comment #14 From 2010-07-09 09:31:56 PST -------
(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=61047) [details] [details] [details]
> > Proposed patch
> > 
> > Fixed the problem with the blur with one deviation set to 0 and the other points commented. I do not mark the patch for review until someone could generate the mac pixel tests results for me, or if you think it is ok I skip those tests and upload a bug so they can do it.
> 
> Is it possible to generate those pixel test results from gtk platform? Not sure about this.

No, the results differ.

I'll run pixel tests tomorrow and upload the results as a patch.
------- Comment #15 From 2010-07-10 01:30:30 PST -------
Created an attachment (id=61146) [details]
Pixel results

Can you please take a careful look at it? Results with text seem a bit translated and partly more blurred than they should be.
------- Comment #16 From 2010-07-10 01:45:36 PST -------
btw, I had to make static const float gGaussianKernelFactor = (3 * sqrtf(2 * piFloat) / 4.f); to get it build on Mac.
------- Comment #17 From 2010-07-12 01:18:42 PST -------
(In reply to comment #16)
> btw, I had to make static const float gGaussianKernelFactor = (3 * sqrtf(2 * piFloat) / 4.f); to get it build on Mac.

Thanks very much for the results Dirk (I'll pay you the beer as soon as we have an opportunity ;). I'll review this code, update the tests and upload a patch for review.
------- Comment #18 From 2010-07-12 11:51:08 PST -------
(In reply to comment #15)
> Created an attachment (id=61146) [details] [details]
> Pixel results
> 
> Can you please take a careful look at it? Results with text seem a bit translated and partly more blurred than they should be.

I think the reason for the translation (it should happen in all the images) is the way we choose now the left and right parts in the kernel, before we just divide by two and substract, the problem with that is when kernel size is even, we were using 3 box blurs with a bigger right/down side, the effect is that the image is a little bit translated. For instance for kernelSize = 6 we did:

  1 2 3 4 5 6
        ^
        destination

Now we use:

 1 2 3 4 5 6
     ^

 1 2 3 4 5 6
       ^    

 1 2 3 4 5 6 7
       ^

Now the filter should be more centered, those are the steps pointed out in the svg standard. You can check it for instance with the blurred square in resource-invalidate-on-target-update.svg, without the patch the shadow down and right is darker (with std 2, if you increase to a number which gives you a odd kernel size it is more centered). Or if you use filter-on-filter-for-text.svg, change the deviation to 5 (gives an odd kernel size) and you could check how the text is translated. This is one of the reasons I had to fix this to make the tiling work.

Wrt more blurred text, probably you are talking about filter-on-filter-for-text.svg, the problem there is the deviation standard is 1, and the minimum for the algorithm is 2, so what I did was use 2 if the value is under 2. Not sure if this is the right thing to do (if you set 2 in the svg you can check the blur does not change). Any suggestions?
------- Comment #19 From 2010-07-14 05:03:53 PST -------
Created an attachment (id=61505) [details]
Proposed patch

I've allowed deviations of size 2, instead I've controlled that the kernel is not smaller than 2. Dirk could you get me the pixel results then using the patch?
------- Comment #20 From 2010-08-03 23:42:26 PST -------
(In reply to comment #19)
> Created an attachment (id=61505) [details] [details]
> Proposed patch
> 
> I've allowed deviations of size 2, instead I've controlled that the kernel is not smaller than 2. Dirk could you get me the pixel results then using the patch?

Sorry, I must have missed the last comment. What about a deviation of 1? This should be allowed as well and even if I don't know how the result should look like. Normaly it should just draw the input as is. But I think the other SVG viewers blur it somehow. Did you test it?
------- Comment #21 From 2010-08-04 00:01:31 PST -------
(In reply to comment #20)
> Sorry, I must have missed the last comment.

No problem :)

> What about a deviation of 1? This should be allowed as well and even if I don't know how the result should look like. Normaly it should just draw the input as is. But I think the other SVG viewers blur it somehow. Did you test it?

Yes, deviation of 1 is allowed and visual result is similar to other browsers, basically now we are controlling the kernel size that we calculate from the deviation, the minimum kernel size is 2 because 1 would mean no blur.
------- Comment #22 From 2010-08-04 00:13:38 PST -------
(In reply to comment #21)

> Yes, deviation of 1 is allowed and visual result is similar to other browsers, basically now we are controlling the kernel size that we calculate from the deviation, the minimum kernel size is 2 because 1 would mean no blur.

I do agree with the deviation of 1, but IIRC other browsers still blur the effect a bit with deviation of 1. Am I wrong?
------- Comment #23 From 2010-08-04 00:24:40 PST -------
(In reply to comment #22)
> (In reply to comment #21)
> 
> > Yes, deviation of 1 is allowed and visual result is similar to other browsers, basically now we are controlling the kernel size that we calculate from the deviation, the minimum kernel size is 2 because 1 would mean no blur.
> 
> I do agree with the deviation of 1, but IIRC other browsers still blur the effect a bit with deviation of 1. Am I wrong?

Yes they do, and we also do it. Probably I did not explain myself correctly, this patch just controls the kernel size not the deviation, for instance a deviation of 1 or less than 1 (can happen if user sets a radius of 1 in the CSS) will use a kernel size of 2.
------- Comment #24 From 2010-08-04 00:38:49 PST -------
(In reply to comment #23)
> Yes they do, and we also do it. Probably I did not explain myself correctly, this patch just controls the kernel size not the deviation, for instance a deviation of 1 or less than 1 (can happen if user sets a radius of 1 in the CSS) will use a kernel size of 2.

Ok, I'll create new results, once I'am at homoe.
------- Comment #25 From 2010-08-05 13:52:13 PST -------
Created an attachment (id=63630) [details]
Patch includes new results 

Results after patch for  Alejandro. The results look great!
------- Comment #26 From 2010-08-06 04:06:38 PST -------
(In reply to comment #25)
> Created an attachment (id=63630) [details] [details]
> Patch includes new results 
> 
> Results after patch for  Alejandro. The results look great!

Thanks for the patch Dirk, for some reason the binary part of the patch does not apply correctly in my repository, I've tested patch and git apply but it does not work. Not sure what is the problem, could you create a git format-patch?
------- Comment #27 From 2010-08-06 04:13:47 PST -------
Created an attachment (id=63711) [details]
images in a zip

No, don't use git on Mac. I added the results to the zip file. Maybe you replace them yourself.
------- Comment #28 From 2010-08-06 04:54:51 PST -------
(In reply to comment #27)
> Created an attachment (id=63711) [details] [details]
> images in a zip
> 
> No, don't use git on Mac. I added the results to the zip file. Maybe you replace them yourself.

Thanks, that helps :). Just one issue, the zip file does not have the same expected png files, at least it does not have:

svg/batik/text/textProperties-expected.png
svg/batik/text/textFeatures-expected.png
svg/custom/resource-invalidate-on-target-update-expected.png
svg/dynamic-updates/* (all this directory)
------- Comment #29 From 2010-08-10 02:24:01 PST -------
Created an attachment (id=63991) [details]
Proposed patch

svn-apply allowed me to get the results. This is the patch with the new results sent by Dirk (thanks for the results :)
------- Comment #30 From 2010-08-10 02:54:19 PST -------
(In reply to comment #29)
> Created an attachment (id=63991) [details] [details]
> Proposed patch
> 
> svn-apply allowed me to get the results. This is the patch with the new results sent by Dirk (thanks for the results :)

You may need a new version compatible with trunk.
------- Comment #31 From 2010-08-10 03:07:41 PST -------
(In reply to comment #30)
> (In reply to comment #29)
> > Created an attachment (id=63991) [details] [details] [details]
> > Proposed patch
> > 
> > svn-apply allowed me to get the results. This is the patch with the new results sent by Dirk (thanks for the results :)
> 
> You may need a new version compatible with trunk.

Ouch, I think I rebased that branch this morning, probably the problem is the git format-patch file format and svn-apply trying to patch binary files.
------- Comment #32 From 2010-08-10 04:59:35 PST -------
Created an attachment (id=63999) [details]
Proposed patch

Now using svn-create-patch
------- Comment #33 From 2010-08-10 07:35:47 PST -------
(From update of attachment 63999 [details])
Looks goo. r=me.
------- Comment #34 From 2010-08-11 01:44:12 PST -------
patch landed r65138