Bug 51309 - Consolidate blur code in FEGaussianBlur and ContextShadow
Summary: Consolidate blur code in FEGaussianBlur and ContextShadow
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 68469 26389
  Show dependency treegraph
 
Reported: 2010-12-19 15:23 PST by Simon Fraser (smfr)
Modified: 2014-05-12 05:54 PDT (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-12-19 15:23:50 PST
FEGaussianBlur and ContextShadow should share the same blur code.
Comment 1 Dirk Schulze 2010-12-19 23:02:28 PST
Difficult. IIRC ContextShadow just supports one channel blurring and copies all temporary results in one of the 4 channels. For GaussianBlur we need to blur all 4 channels (with one, rarely used exception: SourceAlpha).
Comment 2 Simon Fraser (smfr) 2010-12-20 08:50:25 PST
Must the alpha-only blur not touch the other channels?

Even if we don't share code, it would be good to have all the blurring code in the same file.
Comment 3 Dirk Schulze 2010-12-20 10:16:31 PST
(In reply to comment #2)
> Must the alpha-only blur not touch the other channels?
No, for source alpha the image is filled with solid black. Just the alpha channel differs.

> 
> Even if we don't share code, it would be good to have all the blurring code in the same file.
All FE*-files (as well as FEGaussianBlur) need FILTERs enabled, while ContextShadow doesn't. So if you want to share code, FEGaussianBlur needs access to ContextShadow for alpha blurring. Does ContextShadow clear all other channels afterwards, or are they still filled with the temporary data of the blurring? I guess it doesn't clear the data, so it would be necessary to use a composite operator afterwards (source-in?), and I'm not sure if all platforms support accessing the GraphicsContext after a pixel manipulation on the ImageBuffer (Cairo does, and the others?). So we either need a second round for filling all channels other than alpha with zero, or need a second temp imageBuffer to make the compositing.
Comment 4 Simon Fraser (smfr) 2010-12-20 10:24:27 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Must the alpha-only blur not touch the other channels?
> No, for source alpha the image is filled with solid black. Just the alpha channel differs.
> 
> > 
> > Even if we don't share code, it would be good to have all the blurring code in the same file.
> All FE*-files (as well as FEGaussianBlur) need FILTERs enabled, while ContextShadow doesn't. So if you want to share code, FEGaussianBlur needs access to ContextShadow for alpha blurring.

Another approach would be to just put the blurring methods into their own file.

> Does ContextShadow clear all other channels afterwards, or are they still filled with the temporary data of the blurring?

It does not clear them.

> I guess it doesn't clear the data, so it would be necessary to use a composite operator afterwards (source-in?), and I'm not sure if all platforms support accessing the GraphicsContext after a pixel manipulation on the ImageBuffer (Cairo does, and the others?).

CG, at least, is fine using source-in composting to just extract the alpha channel.

> So we either need a second round for filling all channels other than alpha with zero, or need a second temp imageBuffer to make the compositing.

I'm fine with keeping FEGaussianBlur's alpha-only blur in a separate function from the more optimized shadow blur. I just want them all in the same place, so that any optimizations that are made (e.g. use of vImage) are more likely do be done on all of them.
Comment 5 Ariya Hidayat 2010-12-20 11:11:57 PST
Change the bug title.
Comment 6 Zoltan Herczeg 2010-12-20 11:26:26 PST
Templates can do miracles, probably here as well. Would it be possible to create all the different versions depending on a template argument (bool or int)?
Comment 7 Ariya Hidayat 2010-12-20 11:28:53 PST
(In reply to comment #6)
> Templates can do miracles, probably here as well. Would it be possible to create all the different versions depending on a template argument (bool or int)?

It will be very hard.
Comment 8 Jan Erik Hanssen 2010-12-20 12:36:19 PST
I'm looking at this but it turns out ContextShadow.cpp is BSD licensed while FEGaussianBlur.cpp is GPL licensed.. not quite sure what to do here. Comments?
Comment 9 Dirk Schulze 2011-01-05 03:00:48 PST
(In reply to comment #8)
> I'm looking at this but it turns out ContextShadow.cpp is BSD licensed while FEGaussianBlur.cpp is GPL licensed.. not quite sure what to do here. Comments?

Just let the license for the files as they are and move the code around, where ever it belongs to.
Comment 10 Jan Erik Hanssen 2011-01-05 09:08:53 PST
(In reply to comment #9)
> (In reply to comment #8)
> > I'm looking at this but it turns out ContextShadow.cpp is BSD licensed while FEGaussianBlur.cpp is GPL licensed.. not quite sure what to do here. Comments?
> 
> Just let the license for the files as they are and move the code around, where ever it belongs to.

The plan was to create a new file with code from two existing files. But those two files have different licenses.