Bug 73029 - SVG Gaussian blur in 1-dimension is incorrect
Summary: SVG Gaussian blur in 1-dimension is incorrect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-23 10:19 PST by Stephen White
Modified: 2011-12-01 01:03 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.88 KB, patch)
2011-11-29 13:39 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (2.95 KB, patch)
2011-11-30 05:01 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (62.64 KB, patch)
2011-11-30 10:14 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (62.54 KB, patch)
2011-11-30 11:37 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (63.15 KB, patch)
2011-11-30 12:52 PST, Florin Malita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2011-11-23 10:19:21 PST
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).
Comment 1 Stephen White 2011-11-23 10:27:33 PST
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.
Comment 2 Florin Malita 2011-11-29 13:39:21 PST
Created attachment 117038 [details]
Patch
Comment 3 Simon Fraser (smfr) 2011-11-29 13:56:29 PST
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?
Comment 4 Florin Malita 2011-11-29 14:01:42 PST
(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.
Comment 5 Florin Malita 2011-11-30 04:53:30 PST
(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.
Comment 6 Florin Malita 2011-11-30 05:01:47 PST
Created attachment 117172 [details]
Patch
Comment 7 Simon Fraser (smfr) 2011-11-30 09:25:09 PST
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.
Comment 8 Florin Malita 2011-11-30 10:14:09 PST
Created attachment 117213 [details]
Patch
Comment 9 Florin Malita 2011-11-30 10:19:09 PST
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?
Comment 10 Simon Fraser (smfr) 2011-11-30 11:04:46 PST
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.
Comment 11 Florin Malita 2011-11-30 11:37:05 PST
Created attachment 117238 [details]
Patch
Comment 12 Florin Malita 2011-11-30 11:39:11 PST
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.
Comment 13 Simon Fraser (smfr) 2011-11-30 12:11:52 PST
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.
Comment 14 Florin Malita 2011-11-30 12:52:55 PST
Created attachment 117255 [details]
Patch

Updated.
Comment 15 WebKit Review Bot 2011-11-30 21:24:52 PST
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
Comment 16 Florin Malita 2011-11-30 23:22:30 PST
Comment on attachment 117255 [details]
Patch

Not sure what went wrong there, AFAICT it was a cq problem.
Comment 17 Nikolas Zimmermann 2011-12-01 00:37:33 PST
Comment on attachment 117255 [details]
Patch

Setting cq+ again on Florins request.
Comment 18 WebKit Review Bot 2011-12-01 01:03:09 PST
Comment on attachment 117255 [details]
Patch

Clearing flags on attachment: 117255

Committed r101638: <http://trac.webkit.org/changeset/101638>
Comment 19 WebKit Review Bot 2011-12-01 01:03:15 PST
All reviewed patches have been landed.  Closing bug.