WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
73949
Use Skia's Gaussian blur implementation for the FEGaussianBlur filter
https://bugs.webkit.org/show_bug.cgi?id=73949
Summary
Use Skia's Gaussian blur implementation for the FEGaussianBlur filter
Stephen White
Reported
2011-12-06 13:58:12 PST
Call out to Skia's implementation of Gaussian blur.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Stephen White
Comment 1
2011-12-06 14:04:17 PST
Created
attachment 118109
[details]
Patch
Zoltan Herczeg
Comment 2
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?
Stephen White
Comment 3
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.
Dirk Schulze
Comment 4
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).
Zoltan Herczeg
Comment 5
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.
Stephen White
Comment 6
2011-12-08 09:28:10 PST
Created
attachment 118406
[details]
Move skia-specific code to FEGaussianBlur.cpp
Stephen White
Comment 7
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).
Stephen White
Comment 8
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.
Dirk Schulze
Comment 9
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.
Stephen White
Comment 10
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.
Stephen White
Comment 11
2011-12-08 11:33:19 PST
Created
attachment 118434
[details]
Patch
Zoltan Herczeg
Comment 12
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+.
Zoltan Herczeg
Comment 13
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.
Stephen White
Comment 14
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.
Zoltan Herczeg
Comment 15
2011-12-08 11:58:21 PST
Comment on
attachment 118434
[details]
Patch r=me
WebKit Review Bot
Comment 16
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
>
WebKit Review Bot
Comment 17
2011-12-08 14:30:01 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug