Rewrite hand written assembly to NEON intrinsics.
Created attachment 149938 [details] patch patch
Comment on attachment 149938 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=149938&action=review Looks sane to me in general, but I prefer that Zoltan looks at the NEON stuff, he can give r+ or r- then :-) > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:127 > + boxBlurNEON(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); > + else > + boxBlur(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); You could expand isAlphaImage() here. > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:138 > + boxBlur(src, dst, kernelSizeY, dyLeft, dyRight, stride, 4, paintSize.height(), paintSize.width(), isAlphaImage()); boxBlurNEON? Otherwise this makes no sense. You could also expand isAlphaImage() here.
(In reply to comment #2) > (From update of attachment 149938 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149938&action=review > > Looks sane to me in general, but I prefer that Zoltan looks at the NEON stuff, he can give r+ or r- then :-) > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:127 > > + boxBlurNEON(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); > > + else > > + boxBlur(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); > > You could expand isAlphaImage() here. > Because this patch just speed up the 4 channel mode. The alpha image is much complicated to optimize. Now I'm working on that problem, but anyway the hand written assembly didn't improve performance to much so that's why I fall back to the origin boxBlur when the image is an alpha image. > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:138 > > + boxBlur(src, dst, kernelSizeY, dyLeft, dyRight, stride, 4, paintSize.height(), paintSize.width(), isAlphaImage()); > > boxBlurNEON? Otherwise this makes no sense. > You could also expand isAlphaImage() here. Yes, you're right I mistyped it, this is boxBlurNEON.
Created attachment 150397 [details] patch2 Typos fixed.
Comment on attachment 150397 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=150397&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:127 > + if (!isAlphaImage()) > + boxBlurNEON(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); > + else > + boxBlur(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); Since isAlphaImage() is known at this point, we should pass true to boxBlur and remove the bool argument from boxBlurNEON. If we ever make an alpha based version, it will have a different name, since the code cannot be shared. > Source/WebCore/platform/graphics/filters/arm/FEGaussianBlurNEON.h:32 > +using namespace std; We should not add using to a header file... We should use std:: if it is really needed.
Comment on attachment 150397 [details] patch2 Attachment 150397 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13129185 New failing tests: fast/images/embed-does-not-propagate-dimensions-to-object-ancestor.html
Created attachment 150443 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 150605 [details] patch3 (In reply to comment #5) > (From update of attachment 150397 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150397&action=review > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:127 > > + if (!isAlphaImage()) > > + boxBlurNEON(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); > > + else > > + boxBlur(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); > > Since isAlphaImage() is known at this point, we should pass true to boxBlur and remove the bool argument from boxBlurNEON. If we ever make an alpha based version, it will have a different name, since the code cannot be shared. Yes, I removed it. > > > Source/WebCore/platform/graphics/filters/arm/FEGaussianBlurNEON.h:32 > > +using namespace std; > > We should not add using to a header file... We should use std:: if it is really needed. Removed as well.
Comment on attachment 150605 [details] patch3 r=me View in context: https://bugs.webkit.org/attachment.cgi?id=150605&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:127 > + boxBlur(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); isAlphaImage() is true here! > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:140 > + boxBlur(src, dst, kernelSizeY, dyLeft, dyRight, stride, 4, paintSize.height(), paintSize.width(), isAlphaImage()); ditto.
(In reply to comment #9) > (From update of attachment 150605 [details]) > r=me > > View in context: https://bugs.webkit.org/attachment.cgi?id=150605&action=review > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:127 > > + boxBlur(src, dst, kernelSizeX, dxLeft, dxRight, 4, stride, paintSize.width(), paintSize.height(), isAlphaImage()); > > isAlphaImage() is true here! > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:140 > > + boxBlur(src, dst, kernelSizeY, dyLeft, dyRight, stride, 4, paintSize.height(), paintSize.width(), isAlphaImage()); > > ditto. Thanks, I fixed those as well. The patch landed. (http://trac.webkit.org/changeset/121900)