Bug 73949 – Use Skia's Gaussian blur implementation for the FEGaussianBlur filter

• Component: SVG
• Status: RESOLVED FIXED

[Stephen White]

Call out to Skia's implementation of Gaussian blur.

[Stephen White]

Created attachment 118109 [details]
Patch

[Zoltan Herczeg]

Just a question since you didn't set r?: adding new fields to GraphicsContext makes a burden for all platforms. If this is intended for SVG use only, why don't you put this change there?

[Stephen White]

(In reply to comment #2)
> Just a question since you didn't set r?: adding new fields to GraphicsContext makes a burden for all platforms. If this is intended for SVG use only, why don't you put this change there?

(I assume you mean a maintenance burden, not a memory burden, since this is only a (non-virtual) member function, so it shouldn't cause additional memory bloat.)  The idea was that other ports could also implement this API.  For instance, the CoreGraphics port could use CoreImage to implement it.

I could put the skia-specific code in platform/graphics/filters, but that seems unfortunate, and would pepper platform/graphics/filters with platform-specific code.  I feel it's better to abstract it behind an API.  It would also allow us to re-use this API in other contexts (for example, as a webkit-specific extension to do gaussian blurs in <canvas>), although that's not the thrust of this work.

[Dirk Schulze]

Comment on attachment 118109 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:195
> +void GraphicsContext::blurImage(Image*, const IntPoint&, float, float)
> +{
> +    ASSERT_NOT_REACHED();
> +}
> +void GraphicsContext::blurImageBuffer(ImageBuffer*, const IntPoint&, float, float)
> +{
> +    ASSERT_NOT_REACHED();
> +}

I wonder if it makes more sense to move this to either Image or ImageBuffer (later might make more sense).

[Zoltan Herczeg]

> I could put the skia-specific code in platform/graphics/filters, but that seems unfortunate, and would pepper platform/graphics/filters with platform-specific code.  I feel it's better to abstract it behind an API.  It would also allow us to re-use this API in other contexts (for example, as a webkit-specific extension to do gaussian blurs in <canvas>), although that's not the thrust of this work.

We have already put platform specific code there, see the ARM directory. This is the Platform directory after all! I know we could put everything to GraphicsContext but this is not the WebKit way. It is hard for porting GraphicsContext as it is now, so we should not add things because it might be useful for somebody sometimes in the future. I think the optimization itself is usefult, just puting it into a central data structure without a strong reason is just an extra maintainability burden without any advantage.

[Stephen White]

Created attachment 118406 [details]
Move skia-specific code to FEGaussianBlur.cpp

[Stephen White]

(In reply to comment #4)
> (From update of attachment 118109 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118109&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:195
> > +void GraphicsContext::blurImage(Image*, const IntPoint&, float, float)
> > +{
> > +    ASSERT_NOT_REACHED();
> > +}
> > +void GraphicsContext::blurImageBuffer(ImageBuffer*, const IntPoint&, float, float)
> > +{
> > +    ASSERT_NOT_REACHED();
> > +}
> 
> I wonder if it makes more sense to move this to either Image or ImageBuffer (later might make more sense).

Could do that, but ImageBuffer::draw() and drawPattern() are private, so there would still be a need for public wrapper functions in GraphicsContext (or at least, that's the current precedent).

[Stephen White]

(In reply to comment #5)
> > I could put the skia-specific code in platform/graphics/filters, but that seems unfortunate, and would pepper platform/graphics/filters with platform-specific code.  I feel it's better to abstract it behind an API.  It would also allow us to re-use this API in other contexts (for example, as a webkit-specific extension to do gaussian blurs in <canvas>), although that's not the thrust of this work.
> 
> We have already put platform specific code there, see the ARM directory. This is the Platform directory after all! I know we could put everything to GraphicsContext but this is not the WebKit way. It is hard for porting GraphicsContext as it is now, so we should not add things because it might be useful for somebody sometimes in the future. I think the optimization itself is usefult, just puting it into a central data structure without a strong reason is just an extra maintainability burden without any advantage.

OK, fair enough.  I've moved it to FEGaussianBlur.

[Dirk Schulze]

Comment on attachment 118406 [details]
Move skia-specific code to FEGaussianBlur.cpp

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

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:279
> +#if USE(SKIA)
> +    if (filter()->renderingMode() == Accelerated) {
> +        ImageBuffer* resultImage = createImageBufferResult();
> +        if (!resultImage)
> +            return;

I wonder if you can move this code into a new file, just like it is done for the ARM port (see platformApplyNeon()).

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:293
> +        paint.setImageFilter(new SkBlurImageFilter(stdX, stdY))->unref();

As far as I know, the Skia blur filter was just a simple box blur, no gaussian Blur. If this is the case, you will get significant differences on the blur result. The SVG specification doesn't allow a to big delta. Can you confirm that? You should see bigger differences on pixel results on some tests. If you see differences, this would mean a standard violation. But if you can confirm that it is a box blur filter, you just need to run it three times and the result should look like a gaussian blur.

[Stephen White]

(In reply to comment #9)
> (From update of attachment 118406 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118406&action=review
> 
> > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:279
> > +#if USE(SKIA)
> > +    if (filter()->renderingMode() == Accelerated) {
> > +        ImageBuffer* resultImage = createImageBufferResult();
> > +        if (!resultImage)
> > +            return;
> 
> I wonder if you can move this code into a new file, just like it is done for the ARM port (see platformApplyNeon()).

OK, will give it a shot.  It will look a little different from the Neon case, since I want to avoid use of the ByteArrays (being GPU-accelerated), so I'll need to hook in a little earlier than the call to platformApply(), 

> > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:293
> > +        paint.setImageFilter(new SkBlurImageFilter(stdX, stdY))->unref();
> 
> As far as I know, the Skia blur filter was just a simple box blur, no gaussian Blur. If this is the case, you will get significant differences on the blur result. The SVG specification doesn't allow a to big delta. Can you confirm that? You should see bigger differences on pixel results on some tests. If you see differences, this would mean a standard violation. But if you can confirm that it is a box blur filter, you just need to run it three times and the result should look like a gaussian blur.

Nope, it's now a Gaussian:  triple-box on the CPU side, and true Gaussian on the GPU side.

[Stephen White]

Created attachment 118434 [details]
Patch

[Zoltan Herczeg]

> OK, fair enough.  I've moved it to FEGaussianBlur.

Thanks!

> I wonder if you can move this code into a new file, just like it is done for the ARM port (see platformApplyNeon()).

Not sure this can be done without much code duplication. Maybe the body of the "if" statement could be moved to an inline function, but you would still need an "if", since SKIA is not enough, the condition must also be satisfied.

Still the body of the platformApplySoftware would be more readable if we would move the body of the "if" statement into an inline function.

Otherwise the patch is ok for me. Just this minor change, and I will give you an r+.

[Zoltan Herczeg]

Mid air collision :)

Ok, your latest patch is fine by me. Do you wish to make it an inline function (turn the .cpp into a .h)? Either is ok for me, inline is faster, no other difference.

[Stephen White]

(In reply to comment #13)
> Mid air collision :)
> 
> Ok, your latest patch is fine by me. Do you wish to make it an inline function (turn the .cpp into a .h)? Either is ok for me, inline is faster, no other difference.

I prefer to leave it out-of-line, unless it shows up as a hot spot in profiling (which I doubt).

Thanks for your review.

[Zoltan Herczeg]

Comment on attachment 118434 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 118434 [details]
Patch

Clearing flags on attachment: 118434

Committed r102385: <http://trac.webkit.org/changeset/102385>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.