Bug 41605 – Fixes to the gaussian blur algorithm

• Component: SVG
• Status: RESOLVED FIXED

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

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.

[Eric Seidel (no email)]

Attachment 60652 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3392345

[Dirk Schulze]

(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

[Dirk Schulze]

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.

[Dirk Schulze]

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.

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

(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.

[Dirk Schulze]

(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?

[Alejandro G. Castro]

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.

[Dirk Schulze]

(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 :-/

[Nikolas Zimmermann]

Changed component to SVG, so it shows up in my all-svg-bugs search.

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

(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.

[Dirk Schulze]

(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.

[Dirk Schulze]

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.

[Dirk Schulze]

btw, I had to make static const float gGaussianKernelFactor = (3 * sqrtf(2 * piFloat) / 4.f); to get it build on Mac.

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

(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?

[Alejandro G. Castro]

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?

[Dirk Schulze]

(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?

[Alejandro G. Castro]

(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.

[Dirk Schulze]

(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?

[Alejandro G. Castro]

(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.

[Dirk Schulze]

(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.

[Dirk Schulze]

Created attachment 63630 [details]
Patch includes new results 

Results after patch for  Alejandro. The results look great!

[Alejandro G. Castro]

(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?

[Dirk Schulze]

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.

[Alejandro G. Castro]

(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)

[Alejandro G. Castro]

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 :)

[Dirk Schulze]

(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.

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

Created attachment 63999 [details]
Proposed patch

Now using svn-create-patch

[Dirk Schulze]

Comment on attachment 63999 [details]
Proposed patch

Looks goo. r=me.

[Alejandro G. Castro]

patch landed r65138