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).
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.
Created attachment 117038 [details] Patch
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?
(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.
(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.
Created attachment 117172 [details] Patch
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.
Created attachment 117213 [details] Patch
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 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.
Created attachment 117238 [details] Patch
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 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.
Created attachment 117255 [details] Patch Updated.
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 on attachment 117255 [details] Patch Not sure what went wrong there, AFAICT it was a cq problem.
Comment on attachment 117255 [details] Patch Setting cq+ again on Florins request.
Comment on attachment 117255 [details] Patch Clearing flags on attachment: 117255 Committed r101638: <http://trac.webkit.org/changeset/101638>
All reviewed patches have been landed. Closing bug.