WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
ASSIGNED
68478
feGaussianBlur CoreImage implementation
https://bugs.webkit.org/show_bug.cgi?id=68478
Summary
feGaussianBlur CoreImage implementation
Tim Horton
Reported
2011-09-20 14:31:26 PDT
Still very rough, I just wanted to see how it'd go. Haven't tested to see if it's actually faster yet, either, though I suspect it is. Also moves feGaussianBlur to using compiled-in-per-platform platformApply like everywhere else in WebCore, instead of dispatching to suffix'd functions.
Attachments
initial (i.e. no changelogs, some tests fail, etc.) patch
(24.13 KB, patch)
2011-09-20 14:31 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
style fixes
(24.10 KB, patch)
2011-09-20 14:36 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
new patch, with Simon's changes and WKSI update
(406.22 KB, patch)
2011-09-20 17:55 PDT
,
Tim Horton
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
example of brokenness
(41.97 KB, image/png)
2011-09-21 14:46 PDT
,
Tim Horton
no flags
Details
"example of brokenness" SVG source
(701 bytes, image/svg+xml)
2011-09-21 14:54 PDT
,
Tim Horton
no flags
Details
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Tim Horton
Comment 1
2011-09-20 14:31:57 PDT
Created
attachment 108053
[details]
initial (i.e. no changelogs, some tests fail, etc.) patch
Radar WebKit Bug Importer
Comment 2
2011-09-20 14:31:57 PDT
<
rdar://problem/10155876
>
Tim Horton
Comment 3
2011-09-20 14:36:30 PDT
Created
attachment 108055
[details]
style fixes
Tim Horton
Comment 4
2011-09-20 14:42:04 PDT
I seem to need to update :-\ Also, some of the blurs in the tests are slightly different, I'm not sure what that's about. It's like... the gamma of the blur, or something, if that makes sense.
Simon Fraser (smfr)
Comment 5
2011-09-20 14:43:47 PDT
Comment on
attachment 108055
[details]
style fixes View in context:
https://bugs.webkit.org/attachment.cgi?id=108055&action=review
> Source/WebCore/platform/graphics/filters/ci/FECITools.h:1 > +/*
I suggest filters/coreimage/, not 'ci'
> Source/WebCore/platform/graphics/filters/ci/FECITools.h:36 > +#import <QuartzCore/CIImagePrivate.h> // TODO: WKSI? (this is for kCIFormatRGBA8)
Yep.
> Source/WebCore/platform/graphics/filters/ci/FECITools.mm:33 > +CIImage* ciImageFromByteArray(ByteArray* src, const IntSize& size)
This should return a RetainPtr
> Source/WebCore/platform/graphics/filters/ci/FEGaussianBlurCI.mm:38 > + if(kernelSizeX != kernelSizeY)
Space after if, brace on new line.
Sam Weinig
Comment 6
2011-09-20 16:17:21 PDT
Is there a measurable performance gain? If so, what is it?
Tim Horton
Comment 7
2011-09-20 16:57:45 PDT
(In reply to
comment #6
)
> Is there a measurable performance gain? If so, what is it?
The frame rate bump I'm seeing on a 300x300 ellipse being blurred with stdDeviation=20 appears to be a little over 2x, on otherwise equivalent release builds, on my MacBook Pro. Also appears to be a significant decrease (80% of a core to 40% of a core) in CPU usage.
Dean Jackson
Comment 8
2011-09-20 17:37:27 PDT
(In reply to
comment #4
)
> Also, some of the blurs in the tests are slightly different, I'm not sure what that's about. It's like... the gamma of the blur, or something, if that makes sense.
premultiplied vs non? Looking at this I wonder if we need a way to store the CIFilter between applications. That would save us the compile and linking step (although maybe CI is really smart about it).
Tim Horton
Comment 9
2011-09-20 17:40:03 PDT
(In reply to
comment #8
)
> (In reply to
comment #4
) > > > Also, some of the blurs in the tests are slightly different, I'm not sure what that's about. It's like... the gamma of the blur, or something, if that makes sense. > > premultiplied vs non?
Possible, good call, I'll check that.
> Looking at this I wonder if we need a way to store the CIFilter between applications. That would save us the compile and linking step (although maybe CI is really smart about it).
Hadn't considered that; I had wondered if it would be nice if we could pass the CIImage (as platform-specific userdata ala other parts of WebCore) through the filter chain instead of recreating it for each, but that might get messy, so I'm avoiding it for now.
Tim Horton
Comment 10
2011-09-20 17:55:58 PDT
Created
attachment 108092
[details]
new patch, with Simon's changes and WKSI update
WebKit Review Bot
Comment 11
2011-09-20 18:22:59 PDT
Comment on
attachment 108092
[details]
new patch, with Simon's changes and WKSI update
Attachment 108092
[details]
did not pass cr-mac-ews (chromium): Output:
http://queues.webkit.org/results/9772487
Zoltan Herczeg
Comment 12
2011-09-20 18:59:27 PDT
Tim, the purpose of platformApply, platformApplyGeneric grouping is dynamicaly detectable features. I.e: platformApply: if (RunOnARM && hasNEON) platformApplyNEON(...) if (hasOpenCLI) platformApplyOpenCLI(...) platformApplyGeneric(...) I know this is not yet implemented, but planned work this way. So the generic code always call platformApply which might call a single function on some platforms, or do feature dependent selection on others. So your man function could look like: platformApply: #if MAC if (kernelX == kernelY) platformApplyCF() #endif platformApplyGeneric(...)
Tim Horton
Comment 13
2011-09-21 11:21:57 PDT
(In reply to
comment #12
)
> Tim, the purpose of platformApply, platformApplyGeneric grouping is dynamicaly detectable features. > > I.e: > > platformApply: > if (RunOnARM && hasNEON) > platformApplyNEON(...) > if (hasOpenCLI) > platformApplyOpenCLI(...) > platformApplyGeneric(...) > > I know this is not yet implemented, but planned work this way. So the generic code always call platformApply which might call a single function on some platforms, or do feature dependent selection on others. > > So your man function could look like: > platformApply: > #if MAC > if (kernelX == kernelY) > platformApplyCF() > #endif > platformApplyGeneric(...)
Ah, ok. I was quite confused because it seemed both a) different than everywhere else, and b) unnecessary, but it makes sense knowing that there were eventually plans to use it more seriously. I'll switch that part back for now.
Tim Horton
Comment 14
2011-09-21 14:46:10 PDT
Created
attachment 108237
[details]
example of brokenness Attaching an example of what's going wrong. Here there's a white rectangle and a black rectangle on a gray background, the two rects overlap and are *together* in a blurred group. It's not a premultiplication problem here since there's no alpha involved. Not sure what's up.
Tim Horton
Comment 15
2011-09-21 14:46:43 PDT
Err, maybe I'm just getting the radius wrong. But I'm not sure why.
Tim Horton
Comment 16
2011-09-21 14:48:51 PDT
No, nevermind, the radius is fine, obviously, since only tests with overlapping elements blurred together fail.
Tim Horton
Comment 17
2011-09-21 14:54:32 PDT
Created
attachment 108240
[details]
"example of brokenness" SVG source
Zoltan Herczeg
Comment 18
2011-09-21 22:58:30 PDT
This is really interesting. If you check the curve at the bottom-right corner of the white box, the "+patch" image clearly has an extra blurred circle. I mean the blur looks like: | Normal: | / _____/ | +Path: / ___ / \____/
Dirk Schulze
Comment 19
2011-09-22 00:33:34 PDT
Even if 2x speedup is quite good. I'd expect better results with a OpenCL implementation of filters (with a better usage of the GPU). OpenCL gets also supported by more and more SoCs. I wouldn't be surprised if the next iPhone ships with support for OpenCL. But we can try to go both ways.
Dean Jackson
Comment 20
2011-09-22 03:40:15 PDT
(In reply to
comment #19
)
> Even if 2x speedup is quite good. I'd expect better results with a OpenCL implementation of filters (with a better usage of the GPU). OpenCL gets also supported by more and more SoCs. I wouldn't be surprised if the next iPhone ships with support for OpenCL. But we can try to go both ways.
I would take a bet that we'd get a bigger speedup if we could avoid copying the CIImage back for rendering by CG. However this would require some surgery in SVG rendering.
Dirk Schulze
Comment 21
2011-09-22 03:47:57 PDT
(In reply to
comment #20
)
> (In reply to
comment #19
) > > Even if 2x speedup is quite good. I'd expect better results with a OpenCL implementation of filters (with a better usage of the GPU). OpenCL gets also supported by more and more SoCs. I wouldn't be surprised if the next iPhone ships with support for OpenCL. But we can try to go both ways. > > I would take a bet that we'd get a bigger speedup if we could avoid copying the CIImage back for rendering by CG. However this would require some surgery in SVG rendering.
Maybe yes, but you'd need to do a compete new filters implementation. Means you need a new filter chain implemented in parallel. If you like to do that, it would be very hard to share any filter effect related work with other platforms (even with Safari Win). However, the code to do that is still in the revision history of SVN. You wouldn't need to start from scratch. IIRC it was removed even before I started working on WebKit (more than 5 years ago?).
Zoltan Herczeg
Comment 22
2011-09-30 01:13:48 PDT
Might be related:
https://bugs.webkit.org/show_bug.cgi?id=68899
Stephen White
Comment 23
2011-11-09 12:48:32 PST
I've noticed that some of the filters (e.g., FEOffset) use GraphicsContext and ImageBuffer directly. Since several ports already have accelerated implementations of GraphicsContext already, I think it's possible to trivially accelerate these filters today, simply by setting the "Accelerated" flag on ImageBuffer creation. Inspired by that, for the remaining filters, what do people think about either placing the required new APIs (e.g., gaussianBlur) on GraphicsContext itself? If that seems out of place on GraphicsContext (most of these operations would be essentially image-to-image, and would ignore most GraphicsContext state), another alternative would be to provide a platform-independant CoreImage-like API for other ports to implement. For example, we could have a class, call it ImageContext, which wraps a CIContext as GraphicsContext wraps a CGContext. Among other things, I think this would allow the Mac port to do efficient CGImage -> CIImage conversions, and also allow other ports to implement the filters using other backends, with only platform-independent code in the platform/graphics/filters directory. For example, the Skia backend has some image processing capabilities, but does not have a distinction between images used for image processing or drawing, so it would be nice to take advantage of that if possible.
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