Bug 19991 - WebKit needs cross-platform filter system
Summary: WebKit needs cross-platform filter system
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords: SVGHitList
Depends on: 19835
Blocks: 6022 5526 6033
  Show dependency treegraph
 
Reported: 2008-07-10 16:15 PDT by Oliver Hunt
Modified: 2009-06-13 13:13 PDT (History)
12 users (show)

See Also:


Attachments
Initial working patch (24.48 KB, patch)
2008-07-18 06:23 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
Patch (75.85 KB, patch)
2008-08-05 14:48 PDT, Alex Mathews
oliver: review-
Details | Formatted Diff | Diff
Updated Patch (80.87 KB, patch)
2008-08-17 21:51 PDT, Alex Mathews
oliver: review-
Details | Formatted Diff | Diff
prepare SVG FilterEffect (55.69 KB, patch)
2009-05-12 23:22 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
mainline SVG patch (64.15 KB, patch)
2009-05-13 02:06 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
initial SVG Filter patch (92.74 KB, patch)
2009-05-17 11:43 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
initial SVG Filter patch (95.33 KB, patch)
2009-05-20 13:02 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
repare filter primitives for new svg filter system (32.14 KB, patch)
2009-05-22 12:19 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
add new source input and filterBuilder (18.89 KB, patch)
2009-05-23 00:46 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
add new source input and filterBuilder (19.48 KB, patch)
2009-05-23 08:28 PDT, Dirk Schulze
zimmermann: review-
Details | Formatted Diff | Diff
add new source input and filterBuilder (20.33 KB, patch)
2009-05-24 12:30 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
add new source input and filterBuilder (20.50 KB, patch)
2009-05-24 15:38 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
svg independen filters (131.41 KB, patch)
2009-05-28 15:30 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
feFloos need input effect (5.75 KB, patch)
2009-05-29 09:37 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
independent filter system (47.47 KB, patch)
2009-05-31 00:13 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
SVG filter system without subRegions (66.83 KB, patch)
2009-05-31 15:25 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Rename FilterBuilder to SVGFilterBuilder (26.20 KB, patch)
2009-05-31 16:59 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
remove old SVG filter code (186.42 KB, patch)
2009-06-01 04:12 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
subRegion Code + feFlood (4.52 KB, patch)
2009-06-01 14:45 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
subRegion Code (65.87 KB, patch)
2009-06-10 11:54 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
subRegion Code (66.40 KB, patch)
2009-06-11 00:04 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
subRegion Code (66.92 KB, patch)
2009-06-11 03:17 PDT, Dirk Schulze
eric: review+
Details | Formatted Diff | Diff
subRegion Code (60.18 KB, patch)
2009-06-12 11:20 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
subRegion Code (58.11 KB, patch)
2009-06-12 16:23 PDT, Dirk Schulze
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2008-07-10 16:15:54 PDT
Master bug for implementing cross platform filter support.
Comment 1 Alex Mathews 2008-07-18 06:23:00 PDT
Created attachment 22357 [details]
Initial working patch

Inital working patch. I got a slightly hacked FEFlood working with this code. I removed the hack that would display it however because it broke everything else :-D. This isn't ready for prime-time but I thought I should put something up.
Comment 2 Alex Mathews 2008-08-05 14:48:41 PDT
Created attachment 22660 [details]
Patch

Here is some work that I have done. Any suggestions/comments would be much appreciated. 

Subregions and filter bounding boxes work more nicely now. Many of the changes are really simple change propagations that were needed to allow FEFlood and FEImage to work without specific hacks for those two. Once I get this off my tree I'll go back and do a quick clean up patch that removes some now obsolete code.
Comment 3 Oliver Hunt 2008-08-05 15:25:50 PDT
Comment on attachment 22660 [details]
Patch

FilterBuilder::getEffectById
  id == 0 should be id.isEmpty()
  Should m_lastEffect be guaranteed to be non-null?  (eg. shouldn't it initially be a wrapper around with the original input?)

I'm also not sure about the changes to WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp -- i would appreciate it if you could get eric to have a look.

Other than those issues it looks good -- i'll have to spend real time reviewing to to give a more thorough review though, but i'll leave that until after these above issues are dealt with.
Comment 4 Alex Mathews 2008-08-17 21:51:02 PDT
Created attachment 22846 [details]
Updated Patch

Here is an updated patch that should take care of all issues pointed out in the previous comment. This should fix all bounding box related issues as LayoutTests/svg/batik/filters/filterRegions.svg now renders correctly :-). So at least obvious bbox errors shouldn't be a problem.

Any comments would be appreciated.
Comment 5 Nikolas Zimmermann 2008-08-18 03:27:11 PDT
(In reply to comment #4)
> Created an attachment (id=22846) [edit]
> Updated Patch
> 
> Here is an updated patch that should take care of all issues pointed out in the
> previous comment. This should fix all bounding box related issues as
> LayoutTests/svg/batik/filters/filterRegions.svg now renders correctly :-). So
> at least obvious bbox errors shouldn't be a problem.
> 
> Any comments would be appreciated.

Excellent, that you took care of filterRegions.svg, it really tests all subregion cases possibe.
Didn't actually review the patch, just waned to confess that I'm happy about your progresss :-)

Greetings,
Niko
> 
Comment 6 Oliver Hunt 2008-08-18 23:56:07 PDT
Comment on attachment 22846 [details]
Updated Patch

getFilterById is returning a bogus pointer, as ownership is not taken, so the passrefptr will delete the object.

Various implementations of apply(ImageBuffer*) name the parameter, but don't use it, which will result in a compile error on windows at least.

I want eric to look at the SVGResourceFilter stuff as it doesn't look correct to me as it currently stands, but i don't know what should be happening so can't make any suggestion as to what you should do.
Comment 7 Alex Mathews 2008-08-19 00:44:24 PDT
OK, the first two issues Oliver had were quickly resolved but there is still the third.

Eric could you take a look at my bounding box related changes and give me feed back. The functions in question are in SVGResourceFilter (filterBBoxForItemBBox) and FilterEffect (primitiveBBoxForFilterBBox).

Thanks much!
Comment 8 Alex Mathews 2008-09-02 19:30:04 PDT
Sorry for the disappearance. I had to move to a new apartment and get through my first week of classes for the fall semester but I am now back.

I had a long chat with Eric about the bounding box code two weeks ago now. The main thing that came out of it was that Eric wanted to move the calculation logic out of the separate functions that exist and place it where the raw attribute values are brought in, at least for now. The two functions in question are SVGResourceFilter::filterBBoxForItemBBox, which takes the element bbox and returns the filter bbox/rect for that element, and FilterEffect::primitiveBBoxForFilterBBox which takes a bunch of args and sets the subregion bbox/rect for that particular effect.

I have started to move the logic within these two functions out and placing it closer where the raw attribute values are coming in. I moved FilterEffect::primitiveBBoxForFilterBBox into SVGFilterPrimitiveStandardAttributes::setStandardAttributes and am now trying to get the necessary information (read: the element bbox) into SVGResourceFilter so I can calculate the filter rect there.

Apologies if that didn't make much sense, I'm exhausted atm, but I'd be happy to entertain questions or comments. :-D
Comment 9 Dirk Schulze 2009-05-12 23:22:49 PDT
Created attachment 30261 [details]
prepare SVG FilterEffect

FilterEffects need an own ImageBuffer to make it possible to reuse the result. SVGResourceFilter calculates the filterRect and the primitiveRect and draws the referenced object into a ImageBuffer. We need this ImageBuffer to get the SourceGraphic and SourceAlpha.
Comment 10 Eric Seidel (no email) 2009-05-12 23:37:09 PDT
Comment on attachment 30261 [details]
prepare SVG FilterEffect

There seems to be no need to copy/paste so much code.  All these effects share a base class.

I would think the first thing you'd want to do would be to get SVG_FILTERS turned back on in mainline (by ripping otu the broken code, or spliting the SVG_FILTERS ifdef into pieces).  But maybe we need more cleanup before we can do that?
Comment 11 Dirk Schulze 2009-05-13 02:06:15 PDT
Created attachment 30267 [details]
mainline SVG patch

This is a mainline patch to get filters basically working and takes the ideas of "Updated patch". I uploaded this patch to show the main concept and to discuss further steps.
Comment 12 Dirk Schulze 2009-05-17 11:43:11 PDT
Created attachment 30424 [details]
initial SVG Filter patch

This patch brings up the filter system.
Comment 13 Oliver Hunt 2009-05-19 23:33:32 PDT
Comment on attachment 30424 [details]
initial SVG Filter patch

> +
> +    class StringImpl;
> +
> +    class FilterBuilder : RefCounted<FilterBuilder> {
should be public RefCounted<FilterBuilder>

>          HashMap<StringImpl*, RefPtr<FilterEffect> > m_namedEffects;

I *think* (though maciej would know for sure) that this should in reality be RefPtr<StringImpl> (i realise this isn't new code but i think it's wrong)

>  void SVGFEImageElement::notifyFinished(CachedResource* finishedObj)
>  {
> -    if (finishedObj == m_cachedImage && m_filterEffect)
> -        m_filterEffect->setCachedImage(m_cachedImage.get());
> +    //if (finishedObj == m_cachedImage)
> +    //    m_filterEffect->setCachedImage(m_cachedImage.get());
>  }

Commented code == bad

I'd like tests to cover heavily nested filters as well (heavy nesting == >1000 nestings or something)
Comment 14 Dirk Schulze 2009-05-20 13:02:31 PDT
Created attachment 30513 [details]
initial SVG Filter patch

updated version
Comment 15 Eric Seidel (no email) 2009-05-22 08:09:18 PDT
Comment on attachment 30513 [details]
initial SVG Filter patch

 48 FloatRect SVGResourceFilter::filterBBoxForItemBBox(const FloatRect& itemBBox) const

really takes an objectBoundingBox in spec terms.  We really should make it impossible to pass it something that isn't an objectBoundingBox, but that's for a later patch.

Seems:
m_filterBuilder->lastFilter()
belongs in a local here:
128         if (m_filterBuilder->lastFilter()->resultImage())
 129             context->drawImage(m_filterBuilder->lastFilter()->resultImage()->image(), 
 130                             m_filterBuilder->lastFilter()->subRegion());

Don't we already have a conversion function for this?
     Color color = Color(floodColor().red() / 255.0f,
 83                         floodColor().green() / 255.0f,
 84                         floodColor().blue() / 255.0f,
 85                         floodColor().alpha() / 255.0 * floodOpacity());

If you transform the rect, you don't need to save/restore (which is expensive):
 80     filterContext->save();
 81     filterContext->translate(-subRegion().x(), -subRegion().y());
 82     Color color = Color(floodColor().red() / 255.0f,
 83                         floodColor().green() / 255.0f,
 84                         floodColor().blue() / 255.0f,
 85                         floodColor().alpha() / 255.0 * floodOpacity());
 86     filterContext->fillRect(subRegion(), color);
 87     filterContext->restore();

This would have been easier to review if the:
 virtual void apply();
60          virtual void dump();
 60         void apply(SVGResourceFilter*);
 61         void dump();
changes could have been done in a first-pass.

In general this looks great!  So glad you're taking this up.  I think we should see this one more round before landing though.  r- for the nits above.
Comment 16 Dirk Schulze 2009-05-22 12:19:21 PDT
Created attachment 30588 [details]
repare filter primitives for new svg filter system

This is a part of the previous patch and prepares filter primitves to the new filter system.
Comment 17 Eric Seidel (no email) 2009-05-22 18:31:58 PDT
Comment on attachment 30588 [details]
repare filter primitives for new svg filter system

You should mention that filters are turned off, thus no additional tests when landing.  Otherwise this looks fantastic!
Comment 18 Dirk Schulze 2009-05-23 00:46:56 PDT
Created attachment 30608 [details]
add new source input and filterBuilder

Adds missing files of the last patch. Mainly the source input and the FilterBuilder.
Comment 19 Oliver Hunt 2009-05-23 00:58:10 PDT
Comment on attachment 30608 [details]
add new source input and filterBuilder

r=me
Comment 20 Eric Seidel (no email) 2009-05-23 01:01:03 PDT
Comment on attachment 30608 [details]
add new source input and filterBuilder

There are tabs in this file:
 42             m_lastEffect = effect.get();
 43 
 44             if(!id.isEmpty())
 45                 m_namedEffects.set(id.impl(), effect.get()); 


This seems wrong, memory management wise:
 45                 m_namedEffects.set(id.impl(), effect.get()); 


You shouldn't be calling effect.get() the second time I dont' think.  Or you shouldn't be passing it in as a PassRefPtr.  You're never taking ownership?  or at least the code doesn't look like it.

Otherwise looks fine.  r- for the memory issue.
Comment 21 Nikolas Zimmermann 2009-05-23 02:59:01 PDT
(In reply to comment #18)
> Created an attachment (id=30608) [review]
> add new source input and filterBuilder

Nice work Dirk! Looking forward to the next patches :-) A pity that I was too slow to review it :-)
Comment 22 Dirk Schulze 2009-05-23 08:28:50 PDT
Created attachment 30620 [details]
add new source input and filterBuilder

updated version of the last patch. I hope this is the right use of RefPtr. I tested it and didn't see any problems.
Comment 23 Nikolas Zimmermann 2009-05-23 09:17:33 PDT
Comment on attachment 30620 [details]
add new source input and filterBuilder

Hi Dirk,

nice job, I have some comments though:

> Index: WebCore/platform/graphics/filters/SourceAlpha.cpp
...
> +PassRefPtr<SourceAlpha> SourceAlpha::create()
> +{
> +    return adoptRef(new SourceAlpha());
> +}
I'd prefer "new SourceAlpha" (without braces) here, not in the style guide but I think it's general practice.

> Index: WebCore/platform/graphics/filters/SourceGraphic.cpp
...
> +PassRefPtr<SourceGraphic> SourceGraphic::create()
> +{
> +    return adoptRef(new SourceGraphic());
> +}
Ditto.

> Index: WebCore/svg/FilterBuilder.cpp
...
> + *  Copyright (C) Dirk Schulze <krit@webkit.org>
Year is missing?

> +FilterBuilder::FilterBuilder() {
> +    m_sourceAlpha = SourceAlpha::create();
> +    m_sourceGraphic = SourceGraphic::create();
> +}
Hm, I wonder wheter lazy creation of these objects would make sense? I think this could save some time.

> +void FilterBuilder::add(const String& id, RefPtr<FilterEffect> effect)
> +{
> +            m_lastEffect = effect.get();
> +
> +            if(!id.isEmpty())
> +                m_namedEffects.set(id.impl(), m_lastEffect); 
> +}
Wrong indention and if(!id) -> if (!id).

> +FilterEffect* FilterBuilder::getEffectById(const String& id) const
> +{   
> +    if(id.isEmpty() && m_lastEffect)
> +        return m_lastEffect.get();
> +
> +    // If id isn't declared and lastEffect is still null then FilterBuilder will return
> +    // a SourceGraphic object as per the Spec.
> +    if(id.isEmpty() && !m_lastEffect)
> +        return m_sourceGraphic.get();
> +
> +    if (id == String("SourceGraphic"))
> +        return m_sourceGraphic.get();
> +    if (id == String("SourceAlpha"))
> +        return m_sourceAlpha.get();
> +    // FIXME: needs to be implemented
> +    if (id == String("BackgroundGraphic"))
> +        return m_sourceGraphic.get();
> +    if (id == String("BackgroundAlpha"))
> +        return m_sourceGraphic.get();
> +    if (id == String("FillPaint"))
> +        return m_sourceGraphic.get();
> +    if (id == String("StrokePaint"))
> +        return m_sourceGraphic.get();
> +
> +    return m_namedEffects.get(id.impl()).get();
> +}
Indention is wrong here, if( -> if ( has to be changed.
That function doesn't look particulary fast to me. I'd suggest to delegate the work:

I'd suggest to add a new function to all FilterEffects: (example for SourceGraphic)
<code>
static const AtomicString& SourceGraphic::filterName()
{
    DEFINE_STATIC_LOCAL(const AtomicString, s_filterName, ("SourceGraphic"));
    return s_filterName;
}
</code>

Then the getEffectById function could be written much nicer:

<code>
typedef HashMap<AtomicStringImpl*, FilterEffect*> FilterEffectMap;

FilterEffect* FilterBuilder::getEffectById(const AtomicString& id)
{
    DEFINE_STATIC_LOCAL(FilterEffectMap, s_filterEffects, ());

    if (s_filterEffects.isEmpty()) {
        s_filterEffects.set(SourceGraphic::filterName(), m_sourceGraphic.get());
        s_filterEffects.set(SourceAlpha::filterName(), m_sourceAlpha.get());
        ...
    }

    // If id isn't declared...
    if (id.isEmpty())
        return m_lastEffect ? m_lastEffect.get() : m_sourceGraphic.get();

    return s_filterEffects.get(id.impl());
}
</code>

If we'd go for lazy creation of the FilterEffect objects (m_sourceGraphic, etc..) the HashMap could point to functions creating the FilterEffect, ie:
s_filterEffects.set(SourceAlpha::filterName(), &FilterBuilder::createSourceAlpha())

FilterEffect* FilterBuilder::createSourceAlpha()
{
    if (m_sourceAlpha)
        return m_sourceAlpha.get();
    m_sourceAlpha = SourceAlpha::create()
    return m_sourceAlpha.get();
}
...

What do you think of this approach?

r- because of the ineffiency of the getEffectById function. It can easily be fixed though.
Comment 24 Dirk Schulze 2009-05-23 10:32:13 PDT
(In reply to comment #23)
> (From update of attachment 30620 [details] [review])
Hi Nikolas,

Thanks for your review. I'm realy happy to hear of you again :-)

About your comments. It sounds to me like breaking a fly on the wheel.  Do you realy think we should add this to every effect? It makes the code a bit harder to read. I agree with you that we shouldn't call if (id ==...), if possible but IIRC the code style of webkit doesn't allow "else if".
My goal was to make the new filter system not only workable but also more understandable for someone who is new to webkit. And I think I should admit that your style makes the code a bit more mystery to me ;-)

I'll change the code if you think it's the best way. But the drawing/rendering is still much slower than the if's.
Comment 25 Nikolas Zimmermann 2009-05-23 10:54:38 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 30620 [details] [review] [review])
> Hi Nikolas,
> 
> Thanks for your review. I'm realy happy to hear of you again :-)
You're welcome :-)

> 
> About your comments. It sounds to me like breaking a fly on the wheel.  Do you
> realy think we should add this to every effect? It makes the code a bit harder
> to read. I agree with you that we shouldn't call if (id ==...), if possible but
> IIRC the code style of webkit doesn't allow "else if".
The point is not the if / else if coding style. The point is speed, the hash map lookup is faster then these constructions!
If you want to avoid adding filterName() everywhere, you could at least define all the names in FilterBuilder's getEffectById() function.

> My goal was to make the new filter system not only workable but also more
> understandable for someone who is new to webkit. And I think I should admit
> that your style makes the code a bit more mystery to me ;-)
Heh, it's not my style it's WebKit-fast-string-handling-style :-)

> 
> I'll change the code if you think it's the best way. But the drawing/rendering
> is still much slower than the if's.
Of course, but getEffectById() may be called repeatedly. You don't want hundreds of useless if comparisions every time, but a central hash map lookup :-)
Comment 26 Oliver Hunt 2009-05-23 13:05:01 PDT
> > I'll change the code if you think it's the best way. But the drawing/rendering
> > is still much slower than the if's.
> Of course, but getEffectById() may be called repeatedly. You don't want
> hundreds of useless if comparisions every time, but a central hash map lookup
> :-)
> 

I disagree, handling SourceAlpha, etc effectively requires a new instance of the filter at each node in the tree, also getEffectById() should not be being called in any hot code as the filter graph should be construted once (on first use) and only updated in future if the filter graph is actually changed.
Comment 27 Dirk Schulze 2009-05-24 12:30:51 PDT
Created attachment 30632 [details]
add new source input and filterBuilder

I talked with Nikolas on IRC. I hope I address all of the problems Eric and Nikolas mentioned.
FilterBuilder has two hashmaps. One for the standard inputs and one for the custom filter effects with a name. This avoids creating new strings on every call of getFilterEffect().
Comment 28 Nikolas Zimmermann 2009-05-24 14:56:53 PDT
Comment on attachment 30632 [details]
add new source input and filterBuilder

Clearing review flag, as Dirk is going to upload a revised version.
Comment 29 Dirk Schulze 2009-05-24 15:38:32 PDT
Created attachment 30636 [details]
add new source input and filterBuilder

revised version :-)
Comment 30 Nikolas Zimmermann 2009-05-24 15:40:06 PDT
Comment on attachment 30636 [details]
add new source input and filterBuilder

Looks great. r=me.
Comment 31 Dirk Schulze 2009-05-24 15:53:48 PDT
Comment on attachment 30636 [details]
add new source input and filterBuilder

landed part2 in r44118. clearing r+ flag of nikolas
Comment 32 Sam Weinig 2009-05-24 16:09:10 PDT
Comment on attachment 30636 [details]
add new source input and filterBuilder

> +    class SourceAlpha : public FilterEffect {
> +    public:        
> +        static PassRefPtr<SourceAlpha> create();
> +
> +        static const AtomicString& effectName();
> +
> +        void apply(SVGResourceFilter*);

It is a layering violation for a platform/ level class to know about SVG.  Can we fix this?
Comment 33 Nikolas Zimmermann 2009-05-24 16:22:54 PDT
(In reply to comment #32)
> (From update of attachment 30636 [details] [review])
> > +    class SourceAlpha : public FilterEffect {
> > +    public:        
> > +        static PassRefPtr<SourceAlpha> create();
> > +
> > +        static const AtomicString& effectName();
> > +
> > +        void apply(SVGResourceFilter*);
> 
> It is a layering violation for a platform/ level class to know about SVG.  Can
> we fix this? 
> 

Oops neither me nor Eric noticed :-)
Will talk to Dirk about a fix tomorrow. This doesn't need a revert IMHO, as SVG Filters are turned off.
Comment 34 Nikolas Zimmermann 2009-05-24 16:26:09 PDT
Now that you talk about it, I'm wondering why we're adding stuff to platform/graphics/filters anyway. As this is cross-platform, we could move all of this into svg/graphics/filters. This would also avoid the layering violation Sam describes.
Comment 35 Dirk Schulze 2009-05-25 05:19:34 PDT
(In reply to comment #34)
> Now that you talk about it, I'm wondering why we're adding stuff to
> platform/graphics/filters anyway. As this is cross-platform, we could move all
> of this into svg/graphics/filters. This would also avoid the layering violation
> Sam describes.
> 

I talked to olliej some time ago and we might use filters in other parts of webkit as well. My plan was to make filters for SVG worable first and work on a svg independent filter system later as a second goal.
Comment 36 Nikolas Zimmermann 2009-05-25 10:59:04 PDT
(In reply to comment #35)
> (In reply to comment #34)
> > Now that you talk about it, I'm wondering why we're adding stuff to
> > platform/graphics/filters anyway. As this is cross-platform, we could move all
> > of this into svg/graphics/filters. This would also avoid the layering violation
> > Sam describes.
> > 
> 
> I talked to olliej some time ago and we might use filters in other parts of
> webkit as well. My plan was to make filters for SVG worable first and work on a
> svg independent filter system later as a second goal.

Sounds reasonable. That doesn't fix the layering violation though. If that's your intention to make SVG independant filters, then we need a different class holding the FilterBuilder than SVGResourceFilter. SVGResourceFilter could reference this new class living in platform/graphics/filter.

Whatever we do, that layering violation has to be resolved soon. If you'd like, we can discuss on IRC tonite.
Comment 37 Dirk Schulze 2009-05-25 12:03:05 PDT
(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > Now that you talk about it, I'm wondering why we're adding stuff to
> > > platform/graphics/filters anyway. As this is cross-platform, we could move all
> > > of this into svg/graphics/filters. This would also avoid the layering violation
> > > Sam describes.
> > > 
> > 
> > I talked to olliej some time ago and we might use filters in other parts of
> > webkit as well. My plan was to make filters for SVG worable first and work on a
> > svg independent filter system later as a second goal.
> 
> Sounds reasonable. That doesn't fix the layering violation though. If that's
> your intention to make SVG independant filters, then we need a different class
> holding the FilterBuilder than SVGResourceFilter. SVGResourceFilter could
> reference this new class living in platform/graphics/filter.
> 
> Whatever we do, that layering violation has to be resolved soon. If you'd like,
> we can discuss on IRC tonite.
> 
FilterBuilder can still be hold by SVGResourceFilter. It's just FilterEffect::apply(..) that needs another input (another reference). I think thats why Alex inclueded Filter.cpp and Filter.h. SVGResourceFilter can create a new Filter object, give all neccessary values to it and FilterEffect::apply() calls Filter instead of SVGResourceFilter. And, if you want, Filter can also hold the FilterBuilder as well. We just need to move Filter.* form svg/ to platform/graphics/filter
Comment 38 David Levin 2009-05-27 11:24:44 PDT
Comment on attachment 30588 [details]
repare filter primitives for new svg filter system

Clearing r+ to remove from the commit queue.

It appears that this was committed as http://trac.webkit.org/changeset/44083
Comment 39 Dirk Schulze 2009-05-28 15:30:17 PDT
Created attachment 30757 [details]
svg independen filters

Make filters in platform/graphics/filters independent from svg. This enables webkit to use filters some where else then SVG.
Comment 40 Nikolas Zimmermann 2009-05-28 15:35:16 PDT
Comment on attachment 30757 [details]
svg independen filters

r=me. As discussed on IRC.
Comment 41 Dirk Schulze 2009-05-28 15:36:43 PDT
Comment on attachment 30513 [details]
initial SVG Filter patch

clear flag. We do the change in smaller steps.
Comment 42 Dirk Schulze 2009-05-28 15:37:30 PDT
Comment on attachment 30757 [details]
svg independen filters

clear flag. landed in r44253.
Comment 43 Dirk Schulze 2009-05-29 09:37:56 PDT
Created attachment 30777 [details]
feFloos need input effect

This patch fixes feFlood. feFlood need an input effect. It helps creating test cases for subRegions, without implementing more filter effects.
Comment 44 Dirk Schulze 2009-05-31 00:13:59 PDT
Created attachment 30811 [details]
independent filter system

make the filter system independent of SVG.
Comment 45 Nikolas Zimmermann 2009-05-31 07:42:24 PDT
Comment on attachment 30777 [details]
feFloos need input effect

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 44264)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> +2009-05-29  Dirk Schulze  <krit@webkit.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        SVG filter feFlood needs an input effect. This change
> +        helps creating test cases, once filters are activated.

Hi Dirk, r=me.
Though the ChangeLog is unclear, how about "Adding 'in1' attribute support for <feFlood>, as specified in SVG 1.1". ?
Comment 46 Nikolas Zimmermann 2009-05-31 07:50:47 PDT
Comment on attachment 30811 [details]
independent filter system


> Index: WebCore/platform/graphics/filters/Filter.h
> ===================================================================
> --- WebCore/platform/graphics/filters/Filter.h	(revision 0)
> +++ WebCore/platform/graphics/filters/Filter.h	(revision 0)
> @@ -0,0 +1,56 @@
> +/*
> + *  Copyright (C) 2009 Dirk Schulze <krit@webkit.org>
> + *
> + *   This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Library General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Library General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Library General Public License
> + *  aint with this library; see the file COPYING.LIB.  If not, write to
> + *  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + *  Boston, MA 02110-1301, USA.
> + */
> +
> +#ifndef Filter_h
> +#define Filter_h
> +
> +#if ENABLE(FILTERS)
> +#include "FilterEffect.h"
> +#include "Image.h"
> +#include "PlatformString.h"
> +#include "StringHash.h"
String includes don't seem to be needed here at all. Same for "FilterEffect.h", can use a class forward for that (you're even doing that, so the include is unnecessary).

> +
> +#include <wtf/PassOwnPtr.h>
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/RefPtr.h>
I don't see any RefPtr/PassRefPtr usage here?

> +
> +namespace WebCore {
> +
> +    class AtomicString;
AtomicString is superflous.

> +    class FilterEffect;
> +
> +    class Filter : public RefCounted<Filter> {
> +    public:
> +
> +        void setSourceImage(PassOwnPtr<Image> image) { m_image = image; }
> +        Image* sourceImage() { return m_image.get(); }
> +
> +        virtual void filterEffectSubRegion(FilterEffect*) = 0;
Isn't this function supposed to return anything?
If not, it should be renamed to sth. like "calculateEffectSubRegion".

> +
> +    private:
> +
> +        OwnPtr<Image> m_image;
You can omit that newline here, after "private:".


> Index: WebCore/platform/graphics/filters/FilterEffect.h
> ===================================================================
> --- WebCore/platform/graphics/filters/FilterEffect.h	(revision 44295)
> +++ WebCore/platform/graphics/filters/FilterEffect.h	(working copy)
> @@ -22,9 +22,9 @@
>  #define FilterEffect_h
>  
>  #if ENABLE(FILTERS)
> +#include "Filter.h"
>  #include "FloatRect.h"
>  #include "ImageBuffer.h"
> -#include "SVGResourceFilter.h"
>  #include "TextStream.h"
>  
>  #include <wtf/PassRefPtr.h>
> @@ -33,6 +33,8 @@
>  
>  namespace WebCore {
>  
> +    class Filter;
> +
>      class FilterEffect : public RefCounted<FilterEffect> {
>      public:
>          virtual ~FilterEffect();
> @@ -56,7 +58,7 @@ namespace WebCore {
>          ImageBuffer* resultImage() { return m_effectBuffer.get(); }
>          void setEffectBuffer(ImageBuffer* effectBuffer) { m_effectBuffer.set(effectBuffer); }
>  
> -        virtual void apply(SVGResourceFilter*) = 0;
> +        virtual void apply(Filter*) = 0;
>          virtual void dump() = 0;
>  
>          virtual TextStream& externalRepresentation(TextStream&) const;
Either use a class forward for "Filter" or an include, not both.


> Index: WebCore/svg/graphics/filters/SVGFilter.h
....

> +        void filterEffectSubRegion(FilterEffect*);
Same comment as above, needs a clarification.

> +
> +    private:
> +        SVGFilter(const FloatRect& itemBox, const FloatRect& filterRect, bool itemBBoxMode, bool filterBBoxMode);
> +
> +        FloatRect m_itemBox;
> +        FloatRect m_filterRect;
> +        bool m_effectBBoxMode;
> +        bool m_filterBBoxMode;
> +    };
Okay the constructor doesn't look that nice. We can leave it as is for now, but it might be nicer to use an enum with flags
and pass that as constructor argument instead of two bools (thinkin of "EffectBoundingBox | FilterBoundingBox"..).

Nontheless, r=me. You can fix above things before landing.
Comment 47 Dirk Schulze 2009-05-31 11:10:41 PDT
Comment on attachment 30811 [details]
independent filter system

Added the changes of Nikolas. 

landed in r44296. Clearing review flag.
Comment 48 Dirk Schulze 2009-05-31 11:33:06 PDT
Comment on attachment 30777 [details]
feFloos need input effect

landed in r44297. Clearing review flag.
Comment 49 Dirk Schulze 2009-05-31 15:25:02 PDT
Created attachment 30820 [details]
SVG filter system without subRegions

Use the new filter system in SVG.
Comment 50 Nikolas Zimmermann 2009-05-31 15:58:25 PDT
Comment on attachment 30820 [details]
SVG filter system without subRegions

Looks good, r=me. Please fix style issue "static_cast<..> (" -> "static_cast<...>(" before landing.
As discussed on IRC, a follow-up patch will also clean up SVGFilterEffect, which contains unncessary things like "String m_result" etc
Comment 51 Dirk Schulze 2009-05-31 16:30:47 PDT
Comment on attachment 30820 [details]
SVG filter system without subRegions

landed in r44299. Clearing review flag.
Comment 52 Dirk Schulze 2009-05-31 16:59:27 PDT
Created attachment 30822 [details]
Rename FilterBuilder to SVGFilterBuilder

FilterBuilder is SVG specific. Move it to svg/graphics/filters and rename it to SVGFilterBuilder.
Comment 53 Dirk Schulze 2009-05-31 22:37:24 PDT
Comment on attachment 30822 [details]
Rename FilterBuilder to SVGFilterBuilder

Reviewed by Nikolas.

landed in r44309. Clearing review flag.
Comment 54 Dirk Schulze 2009-06-01 04:12:07 PDT
Created attachment 30830 [details]
remove old SVG filter code

Remove last pieces of the old SVG filte system. They are not usable with our current filter system. The new filter effects will replace the functionality step by step.
Comment 55 Nikolas Zimmermann 2009-06-01 04:44:53 PDT
Comment on attachment 30830 [details]
remove old SVG filter code

Yay! r=me. If we ever need a special CoreImage/Filter layer again, we still have these files in the history, so it's fine to remove all of them.
Comment 56 Dirk Schulze 2009-06-01 14:40:44 PDT
Comment on attachment 30830 [details]
remove old SVG filter code

landed in r44332. Clearing review flag.
Comment 57 Dirk Schulze 2009-06-01 14:45:01 PDT
Created attachment 30842 [details]
subRegion Code + feFlood

added subregions and the first filter effect: feFlood
Comment 58 Nikolas Zimmermann 2009-06-01 14:50:09 PDT
Comment on attachment 30842 [details]
subRegion Code + feFlood

Hi Dirk,

looks fine.

> +        subRegionBBox = FloatRect(subRegionBBox.x() * useBBox.width() + useBBox.x(),
> +                               subRegionBBox.y() * useBBox.height() + useBBox.y(),
> +                               subRegionBBox.width() * useBBox.width(),
> +                               subRegionBBox.height() * useBBox.height());
Please line up the "subRegion" lines.


Okay, that really needs to be fixed. I'm going to check why this is neede before.

> -    if (m_builtinEffects.contains(idImpl))
> +    if (!idIsEmpty && m_builtinEffects.contains(idImpl))
>          return m_builtinEffects.get(idImpl).get();

Almost r+, let's just investigate in this issue before.
Comment 59 Eric Seidel (no email) 2009-06-01 14:54:56 PDT
Comment on attachment 30842 [details]
subRegion Code + feFlood

I think it's definitely time to move to a new bug.  Normally we try to do one patch per bug. ;)

Why explicit constructor call?
 78     Color color = Color(colorWithOverrideAlpha(floodColor().rgb(), floodOpacity()));


It seems like buffer creation of the proper size is a common thing that filters will want to share code for.

Why are you translating before filling?  Why not just fill FloatRect(FloatSize(), subRegion.size()) instead?

You should explain the
 68     if (!idIsEmpty && m_builtinEffects.contains(idImpl))

change in your ChangeLog.

I assume this is covered by existing tests (which aren't currently being run).  When can we turn on filters again? ;)
Comment 60 Simon Fraser (smfr) 2009-06-02 18:48:14 PDT
How does this relate to bug 19835?
Comment 61 Dirk Schulze 2009-06-10 11:46:27 PDT
(In reply to comment #60)
> How does this relate to bug 19835?
> 

We can maybe close 19835.
Comment 62 Dirk Schulze 2009-06-10 11:54:14 PDT
Created attachment 31136 [details]
subRegion Code

New subRegion code. The test filterRegion of Batik is not good enough to test subRegions too. Thats why the subRegion patch #30842 was wrong. I added 28 tests for filterRegions and subRegions to this bug (splitted into two main tests). This tests are just additions to Batiks filterRegions test.
Comment 63 Dirk Schulze 2009-06-11 00:04:17 PDT
Created attachment 31152 [details]
subRegion Code

The same subRegion code and tests as above. Clipping of the sourceInput was added, to avoid big ImageBuffers for SourceGraphic, SourceAlpha, ... . The ImageBuffer still can get to big for CI if filterRegion _and_ the boundingBox of the referenced object is very big. We may need a workaround for this case to enable filtery by default.

This is the last patch I want to add to this bug report. I think this should get into this report because it is necessary to get filters for SVG working. Every following bug/addition will go into a new report and we can close this one as well as #19835.
Comment 64 Dirk Schulze 2009-06-11 03:17:29 PDT
Created attachment 31157 [details]
subRegion Code

I read a part of the spec over. Every filter needs to be clipped to the filter region at any time, not just at drawing time. This can make the ImageBuffers used by filter effect smaller.  Just a minimal change to the patch above.
Comment 65 Eric Seidel (no email) 2009-06-12 02:37:15 PDT
It's really getting time to retire this bug.  Please use new bugs for future patches.  You should feel free to relate them back to this bug if you like.
Comment 66 Eric Seidel (no email) 2009-06-12 02:37:58 PDT
(In reply to comment #64)
> Created an attachment (id=31157) [review]
> subRegion Code
> 
> I read a part of the spec over. Every filter needs to be clipped to the filter
> region at any time, not just at drawing time. This can make the ImageBuffers
> used by filter effect smaller.  Just a minimal change to the patch above.

Does your test cover this?
Comment 67 Eric Seidel (no email) 2009-06-12 02:46:21 PDT
Comment on attachment 31157 [details]
subRegion Code

 7         <!-- Hier mit flood? -->
 :)  Most of the team doesn't speak german.

No argument name needed:
 60         FloatRect calculateUniteEffectRect(Filter* filter);

It's sad that more of the calculateUniteEffectRect code can't be shared.  It seems like there are two variants of that code.  I wonder if we should put it in a base class instead of copying it to every subclass?  Subclasses could implement some other method which returns a bool to control which of the two common implementations to use?

This if is not needed:
 62     if (m_mergeInputs.size() > 1) {

The for already takes care of that.

This code:
 84     filterContext->fillRect(FloatRect(0.f, 0.f, subRegion().width(), subRegion().height()), color);

would be clearer as:
FloatRect(FloatPoint(), subregion.size())

The explicit color construction is not needed here:
 82     Color color = Color(colorWithOverrideAlpha(floodColor().rgb(), floodOpacity()));

 76     IntSize bufferSize = IntSize(subRegion().width(), subRegion().height());

why not just subRegion().size()?
or enclosingIntSize(...) if necessary?

No need to call .get(), OwnPtr will convert to a bool automatically:
0     if (!filterGraphic.get())
 61         return;

I woudl really like to see a way to get rid of all the calculateUniteEffectRect copy/paste code.

r- for the nits above.  I'd like to see this again... ideally with the calculateUniteEffectRect copy/paste gone if possible. :)
Comment 68 Dirk Schulze 2009-06-12 11:20:14 PDT
Created attachment 31203 [details]
subRegion Code

> r- for the nits above.
Most things are fixed. There is no enclosingIntSize. I had to use enclosingInRect and use the size of this rect. subRegion() is a FloatRect and can't be used with ImageBuffer.

> I woudl really like to see a way to get rid of all the 
> calculateUniteEffectRect
> copy/paste code.
I shared as much code as possible in two new functions in FilterEffect. There are still some effects, that need special code to calculate their subRegions.

> Does your test cover this?
Yes. The tests test clipped effects too.

> It's really getting time to retire this bug.
This is the last topic. Every following patch gets a new bug report.
Comment 69 Eric Seidel (no email) 2009-06-12 12:25:19 PDT
Comment on attachment 31203 [details]
subRegion Code

 74         FloatRect childEffectsUnite(Filter*, FilterEffect*, FilterEffect*);
 75         FloatRect childEffectsUnite(Filter*, FilterEffect*);
The two FilterEffects probably could use names there.

childEffetsUnite! doesn't make much sense except as a revolution slogan for the filter effects workers union.

Did you mean unionOfChildEffectSubRegions?

"unite" is not the noun form. "union" is.  "unite" is the verb form.  Then again, in math you can say "union" as a verb too (or at least I've heard it said).

What's a uniteEffectRect?

How are effects ever children of one another?  I guess one can view this as a tree... so maybe.

Isn't this:
FloatRect FEFlood::calculateEffectRect(Filter* filter)
 68 {
 69     setUniteEffectRect(filter->filterRegion());
 70     filter->calculateEffectSubRegion(this);
 71     return subRegion();
 72 }
just effectRectWithNoChildren()

Ok, so I think I see.  The difference between the various implementations is how many other effect rects to take into account.  Why not just have a calculateEffectRectOfChildren() function which is virtual, and by default returns FloatRect().  In the case where FilterEffects have no children, you don't have to do anything.  In cases where they do, you end up calling calculateEffectRectForChildren(one, two).  obviously merge woudl have its own implementation.

Then you only have *one* implementation of calculateEffectRect in the baseclass which depends on the virtual calculateEffectRectOfChildren()
Comment 70 Dirk Schulze 2009-06-12 14:00:49 PDT
(In reply to comment #69)
> Ok, so I think I see.  The difference between the various implementations is
> how many other effect rects to take into account.  Why not just have a
> calculateEffectRectOfChildren() function which is virtual, and by default
> returns FloatRect().  In the case where FilterEffects have no children, you
> don't have to do anything.  In cases where they do, you end up calling
> calculateEffectRectForChildren(one, two).  obviously merge woudl have its own
> implementation.
> 
> Then you only have *one* implementation of calculateEffectRect in the baseclass
> which depends on the virtual calculateEffectRectOfChildren()

Not sure if I missunderstand you or you me. But at least it seems that I can amuse you :-)
Here is a short explanation of the code.
calculateEffectRect/childEffectsUnite (didn't rename them now to avoid more confusion) have different tasks. 

Before I start: a effect can have no, one, two or multiple childs (if you see the references as tree). Only feMerge can have multiple childs.

The first task is to let the childs calculate their subregion, second: calculate the *union* of this subregions, third calculate the own subregion, depending on the union and the filterRegion, as a preperation for apply() and for the union calculation of the parent.
Most calculations are very simular, with the exception of feTile and the source inputs. My question about your suggestion is how you would call calculateEffectSubRegion() that calculates the effects own subregion?
Not sure, but it sounds like that you'll end up on the same code as above. But with some more reasonable names :-)
Comment 71 Dirk Schulze 2009-06-12 16:23:48 PDT
Created attachment 31219 [details]
subRegion Code

Try to avoid copy/paste code as much as possible.
Comment 72 Eric Seidel (no email) 2009-06-12 16:29:30 PDT
Comment on attachment 31219 [details]
subRegion Code

Ok, two things left to fix.

We label all virtual functions with "virtual" even if it's not strictly required by C++ for subclasses.

I think:
+        FloatRect unionOfChildEffectSubRegions(Filter*, FilterEffect*, FilterEffect*);
+        FloatRect unionOfChildEffectSubRegions(Filter*, FilterEffect*);
should be called:
uniteChildEffectSubRegions() (yes, that's the same name as the virtual function, I know) since they do something/calculate something they should start with a verb.

I don't need to see this again.  please fix the above when landing.

Thanks again!
Comment 73 Nikolas Zimmermann 2009-06-13 07:49:08 PDT
Comment on attachment 31219 [details]
subRegion Code

Hey Dirk,

looking forward to see subregion support in trunk :-)
Still, I have some more comments before landing:

> +        FloatRect uniteChildEffectSubregions(Filter* filter) { return unionOfChildEffectSubRegions(filter, m_in.get(), m_in2.get()); }

I think it's incosistent to use "Subgregions" / "SubRegions". Please stick with one writing style in all places.

> +FloatRect FEMerge::uniteEffectRect(Filter* filter)
> +{
> +    FloatRect uniteEffectRect;
> +    if (m_mergeInputs.isEmpty())
> +        return FloatRect();
> +    else
> +        uniteEffectRect = m_mergeInputs[0]->calculateEffectRect(filter);
I'd suggest using, early return style without allocating the FloatRect here.
if (m_mergeInputs.isEmpty())
    return FloatRect();

FloatRect uniteEffectRect = m_mergeInputs.first()->calculateEffectRect(filter);
..

Other than these minor issues, it looks great, and much nicer after Erics suggestions.
Keep up the good work!
Comment 74 Eric Seidel (no email) 2009-06-13 10:42:04 PDT
Since subregion is a single word, Niko's right that we should be writing it Subregion (not SubRegion) in function names.  Thanks niko.
Comment 75 Dirk Schulze 2009-06-13 10:59:30 PDT
(In reply to comment #72)
> (From update of attachment 31219 [details] [review])
> Ok, two things left to fix.
> 
> We label all virtual functions with "virtual" even if it's not strictly
> required by C++ for subclasses.
> 
> I think:
> +        FloatRect unionOfChildEffectSubRegions(Filter*, FilterEffect*,
> FilterEffect*);
> +        FloatRect unionOfChildEffectSubRegions(Filter*, FilterEffect*);
> should be called:
> uniteChildEffectSubRegions() (yes, that's the same name as the virtual
> function, I know) since they do something/calculate something they should start
> with a verb.
The effects can't find unionOfChildEffectSubRegions if it gets the same name as the virtual function. Do you have a better name? :-P
Comment 76 Eric Seidel (no email) 2009-06-13 11:01:43 PDT
I'm surprised by this.  It also could be a static method on the class, since it doesn't use an member variables.

Wasn't the suggesion to name it unite* not union* anyway?  You could change it to caclculateUnion* :)
Comment 77 Dirk Schulze 2009-06-13 13:13:34 PDT
landed subRegion patch in r44655. I close this bug for now and open new bug reports for other topics, since the main part on a cross-platform filter system is done.