Optimize FEGaussian blur

[Simon Fraser (smfr)]

I've been playing with FEGaussian blur for CSS shadows, and have some optimizations.

[Simon Fraser (smfr)]

Created attachment 76311 [details]
Patch

[WebKit Review Bot]

Attachment 76311 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7000058

[Simon Fraser (smfr)]

Created attachment 76312 [details]
Patch

[Build Bot]

Attachment 76311 [details] did not build on win:
Build output: http://queues.webkit.org/results/6918061

[Simon Fraser (smfr)]

Using the fixed-point math (BlurSumShift) from ContextShadow gets us to 2x performance, with some very minor pixel differences.

[Sam Weinig]

Comment on attachment 76312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76312&action=review

> WebCore/ChangeLog:15
> +        These changes giave a 1.42x performance improvement.

Typo: giave.

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:133
> +inline void boxBlurAlphaOnly(ByteArray* srcPixelArray, ByteArray* dstPixelArray,
> +                    unsigned dx, int dxLeft, int dxRight, int stride, int strideLine, int effectWidth, int effectHeight)

Weird alignment here.

[Zoltan Herczeg]

How and on what test case you measured the perf impovement? You could also add a new WebCore/manual-test.

[Dirk Schulze]

I'd also like to see a perf test for gaussian blur in general, not only for this patch.

(In reply to comment #5)
> Using the fixed-point math (BlurSumShift) from ContextShadow gets us to 2x performance, with some very minor pixel differences.

Did your last patch still cause some pixel differences? Alex and others spend a lot of time in making FEGaussianBlur as compatible and comparable  as possible with other viewers. Would like to know if the difference is noticeable.
Even so, can you update the pixel results please?

PS: Thanks for working on filters :-).

[Simon Fraser (smfr)]

The patch posted here is math-identical with the current code.

I'll update pixel results if I post another patch with the fixed-point math.

[Simon Fraser (smfr)]

I was measuring perf with some custom code that blurred a 382x382 rectangle.

[Eric Seidel (no email)]

Attachment 76312 [details] was posted by a committer and has review+, assigning to Simon Fraser for commit.

[Dirk Schulze]

Any news on this bug? Simon, do you land the patch?

[Zoltan Herczeg]

I doubt it is still apply...

[Darin Adler]

Comment on attachment 76312 [details]
Patch

Clearing review flag on this old patch.

[Simon Fraser (smfr)]

Bah, I pretty much rewrote this patch from scratch today.

[Dirk Schulze]

(In reply to comment #15)
> Bah, I pretty much rewrote this patch from scratch today.

Where is this patch? Can we mark this one as duplicated or do you reuse this bug report?

[Zoltan Herczeg]

Looks nice.

I am just thinking that the following part:

// Fill the kernel
for (int i = 0; i < maxKernelSize; ++i) {
    unsigned offset = line + i * stride;
    unsigned char* srcPtr = srcPixelArray->data() + offset;
    sumR += *srcPtr++;
    sumG += *srcPtr++;
    sumB += *srcPtr++;
    sumA += *srcPtr;
}

could be changed to:

unsigned char* srcPtr = srcPixelArray->data() + line;
for (int i = 0; i < maxKernelSize; ++i, srcPtr += stride) {
    sumR += srcPtr[0];
    sumG += srcPtr[1];
    sumB += srcPtr[2];
    sumA += srcPtr[3];
} 

or:
unsigned char* srcPtr = srcPixelArray->data() + line;
unsigned char* end = srcPtr + maxKernelSize * stride;
while (srcPtr < end) {
    sumR += srcPtr[0];
    sumG += srcPtr[1];
    sumB += srcPtr[2];
    sumA += srcPtr[3];
    srcPtr += stride
}

Something similar could be done in the internal loop.

[Simon Fraser (smfr)]

The real killer right now is bug 120813, which swamps any other optimization twiddling we try to do in this function.

[Adenilson Cavalcanti Silva]

Created attachment 227342 [details]
Patch

[Adenilson Cavalcanti Silva]

I observed an improvement of 7 to 10% in SVG perf test MtSaintHelens.

[Adenilson Cavalcanti Silva]

Created attachment 227343 [details]
6 executions (3 vanilla, 3 patched)

[Dirk Schulze]

Comment on attachment 227342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227342&action=review

smfr or dino, do you have further input?

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:94
> +inline void boxBlurAlphaOnly(Uint8ClampedArray* srcPixelArray, Uint8ClampedArray* dstPixelArray,

is it really not possible to combine that with the code following? Looks sad that we have a separate code path just for SourceAlpha cases.

Ah, looked at the following code again... So you save iterating 4 times through the image data.

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:142
> +    if (alphaImage) {
> +        return boxBlurAlphaOnly(srcPixelArray, dstPixelArray, dx, dxLeft, dxRight, stride, strideLine,
> +            effectWidth, effectHeight, maxKernelSize);
> +    }

You can omit braces here.

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:225
> +

no unnecessary new lines

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:244
> +

no unnecessary new lines

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:259
> +

Ditto.

[Simon Fraser (smfr)]

Comment on attachment 227342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227342&action=review

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:95
> +inline void boxBlurAlphaOnly(Uint8ClampedArray* srcPixelArray, Uint8ClampedArray* dstPixelArray,
> +    unsigned dx, int& dxLeft, int& dxRight, int& stride, int& strideLine, int& effectWidth, int& effectHeight, const int& maxKernelSize)

The Uint8ClampedArray* should be Uint8ClampedArray& (in the caller too).

[Dirk Schulze]

(In reply to comment #23)
> (From update of attachment 227342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227342&action=review
> 
> > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:95
> > +inline void boxBlurAlphaOnly(Uint8ClampedArray* srcPixelArray, Uint8ClampedArray* dstPixelArray,
> > +    unsigned dx, int& dxLeft, int& dxRight, int& stride, int& strideLine, int& effectWidth, int& effectHeight, const int& maxKernelSize)
> 
> The Uint8ClampedArray* should be Uint8ClampedArray& (in the caller too).

Adenilson investigates in a potential regression. If it turns out to be a false negative r=me as well. Otherwise we need to investigate the reason first.

[Adenilson Cavalcanti Silva]

Created attachment 227457 [details]
filters (css3/svg) and fast tests results in Vanilla (trunk of today)

[Adenilson Cavalcanti Silva]

Created attachment 227459 [details]
Patched resultsfilters (css3/svg) and fast tests results

[Adenilson Cavalcanti Silva]

Created attachment 227465 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 227465 [details]
Patch

Clearing flags on attachment: 227465

Committed r166084: <http://trac.webkit.org/changeset/166084>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.