Bug 73029 – SVG Gaussian blur in 1-dimension is incorrect

• Component: SVG
• Status: RESOLVED FIXED

[Stephen White]

If only the stdX or stdY parameter of an FEGaussianBlur filter is non-zero, only two box blurs are applied (the third pass is computed, but ignored).

[Stephen White]

From FEGaussianBlur::platformApplyGeneric():

    for (int i = 0; i < 3; ++i) {
        if (kernelSizeX) {
            kernelPosition(i, kernelSizeX, dxLeft, dxRight);
            boxBlur(srcPixelArray, tmpPixelArray, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage());
        } else {
            ByteArray* auxPixelArray = tmpPixelArray;
            tmpPixelArray = srcPixelArray;
            srcPixelArray = auxPixelArray;
        }

        if (kernelSizeY) {
            kernelPosition(i, kernelSizeY, dyLeft, dyRight);
            boxBlur(tmpPixelArray, srcPixelArray, kernelSizeY, dyLeft, dyRight, stride, 4, paintSize.height(), paintSize.width(), isAlphaImage());
        } else {
            ByteArray* auxPixelArray = tmpPixelArray;
            tmpPixelArray = srcPixelArray;
            srcPixelArray = auxPixelArray;
        }
    }

For both kernelSizeX and kernelSizeY non-zero, this will do:

src -> tmp
tmp -> src
src -> tmp
tmp -> src
src -> tmp
tmp -> src

Leaving the result (as expected) in src.

For kernelSizeX nonzero, kernelSizeY zero:

this will blur src -> tmp, (swap)
               tmp -> src, (swap)
               src -> tmp

The passed-in src buffer contains the result of the second pass.  The third pass (in passed-in tmp) is ignored.

This is exercised by the 2nd and 3rd samples of the svg/filters/feGaussianBlur layout test.

[Florin Malita]

Created attachment 117038 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 117038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117038&action=review

Code looks good but needs a test.

> Source/WebCore/ChangeLog:8
> +        No new tests.

Why not? Won't a pixel test be sufficient for this?

[Florin Malita]

(In reply to comment #3)
> (From update of attachment 117038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117038&action=review
> 
> Code looks good but needs a test.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests.
> 
> Why not? Won't a pixel test be sufficient for this?

Will do, thanks.

[Florin Malita]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/ChangeLog:8
> > > +        No new tests.
> > 
> > Why not? Won't a pixel test be sufficient for this?
> 
> Will do, thanks.

Actually, I just realized that svg/filters/feGaussianBlur.svg catches this nicely: it has two one-dimensional blurs that will require rebaselining.

I'll add a comment and re-spin the patch.

[Florin Malita]

Created attachment 117172 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 117172 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117172&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests: svg/filters/feGaussianBlur.svg covers this and will require reabaselining.

So you should include new pixel results with this patch.

[Florin Malita]

Created attachment 117213 [details]
Patch

[Florin Malita]

Comment on attachment 117172 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117172&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests: svg/filters/feGaussianBlur.svg covers this and will require reabaselining.
> 
> So you should include new pixel results with this patch.

I've included updated results for chromium-win. Is there a way to get all the other platform specific results before committing?

[Simon Fraser (smfr)]

Comment on attachment 117213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117213&action=review

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:126
> +            ByteArray* tmp = src;
> +            src = dst;
> +            dst = tmp;

I think you could use std::swap<>here.

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:134
> +            ByteArray* tmp = src;
> +            src = dst;
> +            dst = tmp;

Ditto.

[Florin Malita]

Created attachment 117238 [details]
Patch

[Florin Malita]

Comment on attachment 117213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117213&action=review

>> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:126
>> +            dst = tmp;
> 
> I think you could use std::swap<>here.

Done.

>> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:134
>> +            dst = tmp;
> 
> Ditto.

Done.

[Simon Fraser (smfr)]

Comment on attachment 117238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117238&action=review

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:124
> +            std::swap(src, dst);

We normally put a 'using namespace std' at the top of the file and then drop the namespace here.

[Florin Malita]

Created attachment 117255 [details]
Patch

Updated.

[WebKit Review Bot]

Comment on attachment 117255 [details]
Patch

Rejecting attachment 117255 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
111575 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium
Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 109.
Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees'
No such file or directory at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2020.

Full output: http://queues.webkit.org/results/10687471

[Florin Malita]

Comment on attachment 117255 [details]
Patch

Not sure what went wrong there, AFAICT it was a cq problem.

[Nikolas Zimmermann]

Comment on attachment 117255 [details]
Patch

Setting cq+ again on Florins request.

[WebKit Review Bot]

Comment on attachment 117255 [details]
Patch

Clearing flags on attachment: 117255

Committed r101638: <http://trac.webkit.org/changeset/101638>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.