Bug 73949 - Use Skia's Gaussian blur implementation for the FEGaussianBlur filter
Summary: Use Skia's Gaussian blur implementation for the FEGaussianBlur filter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-06 13:58 PST by Stephen White
Modified: 2011-12-08 14:30 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.59 KB, patch)
2011-12-06 14:04 PST, Stephen White
no flags Details | Formatted Diff | Diff
Move skia-specific code to FEGaussianBlur.cpp (3.50 KB, patch)
2011-12-08 09:28 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (7.12 KB, patch)
2011-12-08 11:33 PST, Stephen White
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2011-12-06 13:58:12 PST
Call out to Skia's implementation of Gaussian blur.
Comment 1 Stephen White 2011-12-06 14:04:17 PST
Created attachment 118109 [details]
Patch
Comment 2 Zoltan Herczeg 2011-12-06 23:22:50 PST
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?
Comment 3 Stephen White 2011-12-07 08:24:30 PST
(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.
Comment 4 Dirk Schulze 2011-12-07 23:52:26 PST
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).
Comment 5 Zoltan Herczeg 2011-12-08 06:12:12 PST
> 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.
Comment 6 Stephen White 2011-12-08 09:28:10 PST
Created attachment 118406 [details]
Move skia-specific code to FEGaussianBlur.cpp
Comment 7 Stephen White 2011-12-08 09:31:18 PST
(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).
Comment 8 Stephen White 2011-12-08 09:33:25 PST
(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.
Comment 9 Dirk Schulze 2011-12-08 09:43:30 PST
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.
Comment 10 Stephen White 2011-12-08 10:36:50 PST
(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.
Comment 11 Stephen White 2011-12-08 11:33:19 PST
Created attachment 118434 [details]
Patch
Comment 12 Zoltan Herczeg 2011-12-08 11:44:33 PST
> 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+.
Comment 13 Zoltan Herczeg 2011-12-08 11:48:38 PST
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.
Comment 14 Stephen White 2011-12-08 11:52:42 PST
(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.
Comment 15 Zoltan Herczeg 2011-12-08 11:58:21 PST
Comment on attachment 118434 [details]
Patch

r=me
Comment 16 WebKit Review Bot 2011-12-08 14:29:56 PST
Comment on attachment 118434 [details]
Patch

Clearing flags on attachment: 118434

Committed r102385: <http://trac.webkit.org/changeset/102385>
Comment 17 WebKit Review Bot 2011-12-08 14:30:01 PST
All reviewed patches have been landed.  Closing bug.