Bug 51309
| Summary: | Consolidate blur code in FEGaussianBlur and ContextShadow | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> |
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | ariya.hidayat, helder, jhanssen, krit, sam, simon.fraser, zherczeg, zimmermann |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | OS X 10.5 | ||
| Bug Depends on: | |||
| Bug Blocks: | 68469, 26389 | ||
Simon Fraser (smfr)
FEGaussianBlur and ContextShadow should share the same blur code.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Dirk Schulze
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).
Simon Fraser (smfr)
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.
Dirk Schulze
(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.
Simon Fraser (smfr)
(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.
Ariya Hidayat
Change the bug title.
Zoltan Herczeg
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)?
Ariya Hidayat
(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.
Jan Erik Hanssen
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?
Dirk Schulze
(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.
Jan Erik Hanssen
(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.