Bug 41605 - Fixes to the gaussian blur algorithm
Summary: Fixes to the gaussian blur algorithm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 40793
  Show dependency treegraph
 
Reported: 2010-07-05 04:28 PDT by Alejandro G. Castro
Modified: 2010-08-11 01:44 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2010-07-05 04:28:42 PDT
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 Alejandro G. Castro 2010-07-06 12:29:33 PDT
Created attachment 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 Eric Seidel (no email) 2010-07-06 14:07:46 PDT
Attachment 60652 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3392345
Comment 3 Dirk Schulze 2010-07-06 22:24:45 PDT
(In reply to comment #2)
> Attachment 60652 [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 Dirk Schulze 2010-07-06 22:27:06 PDT
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 Dirk Schulze 2010-07-06 22:45:56 PDT
Comment on attachment 60652 [details]
Proposed patch

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 Alejandro G. Castro 2010-07-07 04:44:20 PDT
(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 Alejandro G. Castro 2010-07-07 04:57:48 PDT
(In reply to comment #5)
>
> [...]
>
> (From update of attachment 60652 [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 Dirk Schulze 2010-07-07 05:00:42 PDT
(In reply to comment #7)
> (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.

What is the result of this test? Can you make a screenshot?
Comment 9 Alejandro G. Castro 2010-07-07 05:08:22 PDT
Created attachment 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 Dirk Schulze 2010-07-07 05:33:05 PDT
(In reply to comment #9)
> 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.

right, shouldn't blur on 0 :-/
Comment 11 Nikolas Zimmermann 2010-07-09 07:29:42 PDT
Changed component to SVG, so it shows up in my all-svg-bugs search.
Comment 12 Alejandro G. Castro 2010-07-09 09:23:27 PDT
Created attachment 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 Alejandro G. Castro 2010-07-09 09:24:56 PDT
(In reply to comment #12)
> 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.

Is it possible to generate those pixel test results from gtk platform? Not sure about this.
Comment 14 Dirk Schulze 2010-07-09 09:31:56 PDT
(In reply to comment #13)
> (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.

No, the results differ.

I'll run pixel tests tomorrow and upload the results as a patch.
Comment 15 Dirk Schulze 2010-07-10 01:30:30 PDT
Created attachment 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 Dirk Schulze 2010-07-10 01:45:36 PDT
btw, I had to make static const float gGaussianKernelFactor = (3 * sqrtf(2 * piFloat) / 4.f); to get it build on Mac.
Comment 17 Alejandro G. Castro 2010-07-12 01:18:42 PDT
(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 Alejandro G. Castro 2010-07-12 11:51:08 PDT
(In reply to comment #15)
> 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.

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 Alejandro G. Castro 2010-07-14 05:03:53 PDT
Created attachment 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 Dirk Schulze 2010-08-03 23:42:26 PDT
(In reply to comment #19)
> 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?

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 Alejandro G. Castro 2010-08-04 00:01:31 PDT
(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 Dirk Schulze 2010-08-04 00:13:38 PDT
(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 Alejandro G. Castro 2010-08-04 00:24:40 PDT
(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 Dirk Schulze 2010-08-04 00:38:49 PDT
(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 Dirk Schulze 2010-08-05 13:52:13 PDT
Created attachment 63630 [details]
Patch includes new results 

Results after patch for  Alejandro. The results look great!
Comment 26 Alejandro G. Castro 2010-08-06 04:06:38 PDT
(In reply to comment #25)
> Created an attachment (id=63630) [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 Dirk Schulze 2010-08-06 04:13:47 PDT
Created attachment 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 Alejandro G. Castro 2010-08-06 04:54:51 PDT
(In reply to comment #27)
> 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.

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 Alejandro G. Castro 2010-08-10 02:24:01 PDT
Created attachment 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 Dirk Schulze 2010-08-10 02:54:19 PDT
(In reply to comment #29)
> 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 :)

You may need a new version compatible with trunk.
Comment 31 Alejandro G. Castro 2010-08-10 03:07:41 PDT
(In reply to comment #30)
> (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.

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 Alejandro G. Castro 2010-08-10 04:59:35 PDT
Created attachment 63999 [details]
Proposed patch

Now using svn-create-patch
Comment 33 Dirk Schulze 2010-08-10 07:35:47 PDT
Comment on attachment 63999 [details]
Proposed patch

Looks goo. r=me.
Comment 34 Alejandro G. Castro 2010-08-11 01:44:12 PDT
patch landed r65138