Bug 90166 - NEON intrinsics should be used with gaussian blur filter
Summary: NEON intrinsics should be used with gaussian blur filter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-28 05:58 PDT by Gabor Rapcsanyi
Modified: 2012-07-05 06:23 PDT (History)
5 users (show)

See Also:


Attachments
patch (35.33 KB, patch)
2012-06-28 06:31 PDT, Gabor Rapcsanyi
no flags Details | Formatted Diff | Diff
patch2 (35.33 KB, patch)
2012-07-02 04:24 PDT, Gabor Rapcsanyi
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (319.05 KB, application/zip)
2012-07-02 10:21 PDT, WebKit Review Bot
no flags Details
patch3 (35.79 KB, patch)
2012-07-03 07:24 PDT, Gabor Rapcsanyi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Rapcsanyi 2012-06-28 05:58:57 PDT
Rewrite hand written assembly to NEON intrinsics.
Comment 1 Gabor Rapcsanyi 2012-06-28 06:31:41 PDT
Created attachment 149938 [details]
patch

patch
Comment 2 Nikolas Zimmermann 2012-06-28 09:24:54 PDT
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.
Comment 3 Gabor Rapcsanyi 2012-06-29 01:51:23 PDT
(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.
Comment 4 Gabor Rapcsanyi 2012-07-02 04:24:33 PDT
Created attachment 150397 [details]
patch2

Typos fixed.
Comment 5 Zoltan Herczeg 2012-07-02 04:33:05 PDT
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 6 WebKit Review Bot 2012-07-02 10:21:30 PDT
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
Comment 7 WebKit Review Bot 2012-07-02 10:21:33 PDT
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
Comment 8 Gabor Rapcsanyi 2012-07-03 07:24:19 PDT
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 9 Zoltan Herczeg 2012-07-05 05:05:35 PDT
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.
Comment 10 Gabor Rapcsanyi 2012-07-05 06:23:02 PDT
(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)