NEON intrinsics should be used with gaussian blur filter

• Component: SVG
• Status: RESOLVED FIXED

[Gabor Rapcsanyi]

Rewrite hand written assembly to NEON intrinsics.

[Gabor Rapcsanyi]

Created attachment 149938 [details]
patch

patch

[Nikolas Zimmermann]

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.

[Gabor Rapcsanyi]

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

[Gabor Rapcsanyi]

Created attachment 150397 [details]
patch2

Typos fixed.

[Zoltan Herczeg]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Gabor Rapcsanyi]

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.

[Zoltan Herczeg]

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.

[Gabor Rapcsanyi]

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