Bug 52200

Summary: Repaint SVG elements with filter instead of relayout where possible
Product: WebKit Reporter: Zoltan Herczeg <zherczeg>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: eric, krit, oliver, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch after update
none
patch after update
none
patch
krit: review-, krit: commit-queue-
patch
krit: review-, krit: commit-queue-
patch
krit: review-
patch again
krit: review-
patch
none
patch
none
final patch krit: review+

Description Zoltan Herczeg 2011-01-11 02:42:06 PST
Working towards adding primitive renderers in small steps.
Comment 1 Zoltan Herczeg 2011-01-11 02:53:52 PST
Created attachment 78507 [details]
patch
Comment 2 Zoltan Herczeg 2011-01-11 03:58:44 PST
Created attachment 78514 [details]
patch after update
Comment 3 WebKit Review Bot 2011-01-11 03:59:26 PST
Attachment 78514 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:329:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:330:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:30:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 4 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Zoltan Herczeg 2011-01-11 04:43:10 PST
Created attachment 78517 [details]
patch after update
Comment 5 Dirk Schulze 2011-01-11 10:03:46 PST
Comment on attachment 78517 [details]
patch after update

View in context: https://bugs.webkit.org/attachment.cgi?id=78517&action=review

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:333
> +void RenderSVGResourceFilter::repaintFilterPrimitve(RenderObject* object)
> +{
> +    RenderObject* filter = object->parent();
> +    if (!filter || !filter->isSVGResourceFilter())
> +        return;
> +    filter->repaint();
> +}

This function is never called right now, can describe what you plan to do with it please?

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:43
> +    SVGFilter* filter = reinterpret_cast<SVGFilter*>(effect->filter());

We normally use static_casts

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:51
> +    void invalidate(bool repaint = false);

do you really mean repaint or relayout?

> Source/WebCore/svg/SVGFEOffsetElement.cpp:69
> +    if (attrName == SVGNames::dxAttr || attrName == SVGNames::dyAttr)
> +        invalidate(true);

This is unclear to me. Do we want to repaint or relayout? Changing dx or dy definitely affects the repaintRect size of following filter primitives, ergo relayout, right?

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:58
>          if (RenderObject* primitiveRenderer = renderer())
> -            RenderSVGResource::markForLayoutAndParentResourceInvalidation(primitiveRenderer);
> +            reinterpret_cast<RenderSVGResourceFilterPrimitive*>(primitiveRenderer)->invalidate(repaint);
>      }

Why no static_cast?
Comment 6 Zoltan Herczeg 2011-01-12 00:45:42 PST
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:333
> > +void RenderSVGResourceFilter::repaintFilterPrimitve(RenderObject* object)
> > +{
> > +    RenderObject* filter = object->parent();
> > +    if (!filter || !filter->isSVGResourceFilter())
> > +        return;
> > +    filter->repaint();
> > +}
> 
> This function is never called right now, can describe what you plan to do with it please?

Repainting somehow. No idea yet. But somehow when a change happens, which does not affect layout, but needs repaint, this function should invalidate all rects, where the filter is used (absoluteBoundingBoxes). Since it would be too difficult to determine its affected subregion, I would invalidate the whole (at least for now -> image copy is cheap). I could remove this function if you wish, not important for now. Unfortunately clippedOverflowRectForRepaint can return only 1 rect. I could unite all absoluteBoundingBoxes, but I would prefer something more clever.

> > Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:51
> > +    void invalidate(bool repaint = false);
> 
> do you really mean repaint or relayout?

Repaint. Anyway I have no strong opinion here. Should I reverse them? (bool relayout = true) ?

> > Source/WebCore/svg/SVGFEOffsetElement.cpp:69
> > +    if (attrName == SVGNames::dxAttr || attrName == SVGNames::dyAttr)
> > +        invalidate(true);
> 
> This is unclear to me. Do we want to repaint or relayout? Changing dx or dy definitely affects the repaintRect size of following filter primitives, ergo relayout, right?

Hm, forgot about it. Thanks for mentioning it. I should find another filter to work on this feature. Perhaps diffuseConstant for lightning. Remove this from the future patch.
Comment 7 Zoltan Herczeg 2011-01-12 06:26:14 PST
Created attachment 78684 [details]
patch
Comment 8 Dirk Schulze 2011-01-13 03:14:47 PST
Comment on attachment 78684 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78684&action=review

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:334
> +void RenderSVGResourceFilter::repaintFilterPrimitve(RenderObject* object)
> +{
> +    RenderObject* filter = object->parent();
> +    if (!filter || !filter->isSVGResourceFilter())
> +        return;
> +    filter->repaint();
> +}
> +

We'll definitely need this function later. But the policy wants us not to commit dead code. And it is not in use atm.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:43
> +    SVGFilter* filter = static_cast<SVGFilter*>(effect->filter());

Please add an assert for effect->filter() before this line.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:76
> +    RefPtr<FilterEffect> effectReference = passEffectReference;
>      ASSERT(!m_effectReferences.contains(effectReference));
> +    ASSERT(object && !m_effectRenderer.contains(object));

You're right, a functions should always take the ownership, but IIRC you don't touch passEffectReference in this function, but just save it in m_effectRenderer. So do we need to store it in effectReference? Can't we just store it m_effectRenderer? Also, do all callers of appendEffectToEffectRef...() release the passed effect? - Ah you store RefPtr in two member variables, m_effectReferences and m_effectRenderer. Hm, not sure what behavior is correct here.
Comment 9 Zoltan Herczeg 2011-01-13 05:53:27 PST
Thanks for the review.

> > Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:76
> > +    RefPtr<FilterEffect> effectReference = passEffectReference;
> >      ASSERT(!m_effectReferences.contains(effectReference));
> > +    ASSERT(object && !m_effectRenderer.contains(object));
> 
> You're right, a functions should always take the ownership, but IIRC you don't touch passEffectReference in this function, but just save it in m_effectRenderer. So do we need to store it in effectReference? Can't we just store it m_effectRenderer? Also, do all callers of appendEffectToEffectRef...() release the passed effect? - Ah you store RefPtr in two member variables, m_effectReferences and m_effectRenderer. Hm, not sure what behavior is correct here.

No idea about the best solution here. I think passing RefPtr-s is against the style (not sure...). Directly using PassRefPtr-s cause a segfault, so I need to define a local variable before use. Of course I could revert these lines to the original one if you prefer them.
Comment 10 Zoltan Herczeg 2011-01-13 06:41:28 PST
Created attachment 78804 [details]
patch
Comment 11 Dirk Schulze 2011-01-17 00:12:32 PST
Comment on attachment 78804 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78804&action=review

I'd like to see Niko to look at this patch as well. Maybe he has some more proposals.

PS: Why do you add cq flag? You have commit rights and in my opinion it is better to land a patch yourself, you have to be available on landing anyway to fix problems.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:114
> +        builder->appendEffectToEffectReferences(effect, effectElement->renderer());

effect needs to be release if you change RefPtr to PassRefPtr in appendEffectToEffectReferences(...). Because with PassRefPtr you give over the ownership. This is maybe not what you want.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:48
> +    // They depend on the RenderObject argument of RenderSVGResourceFilter::applyResource.

They is unclear, do you mean FilterEffects or the subregion of a filter effect?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:71
> +void SVGFilterBuilder::appendEffectToEffectReferences(PassRefPtr<FilterEffect> passEffectReference, RenderObject* object)

I wouldn't change RefPtr to PassRefPtr, as long as we don't know who should take the ownership.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:57
> +    inline PassRefPtr<FilterEffect> getEffectByRenderer(RenderObject* object) { return m_effectRenderer.get(object); }

getters don't begin with 'get' for WebCore, just effectByRenderer. You're not calling this function anywhere, as far as I can see. Why did you add this here? Can you change the name of getEffectReferences() as well?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:76
>      HashMap<RefPtr<FilterEffect>, FilterEffectSet> m_effectReferences;
> +    HashMap<RenderObject*, RefPtr<FilterEffect> > m_effectRenderer;

Do both hashmaps need to take RefPtr<FilterEffect>? Can we garantee somehow, that if A has the element B has it as well? Would be better to save a pointer in at least one map.
Comment 12 Zoltan Herczeg 2011-01-17 06:47:29 PST
> > Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:57
> > +    inline PassRefPtr<FilterEffect> getEffectByRenderer(RenderObject* object) { return m_effectRenderer.get(object); }
> 
> You're not calling this function anywhere, as far as I can see. Why did you add this here? 

We need this function to find the filter primitive objects for each renderer when they are repainted. Since the FilterEffect objects store the arguments by themself, we need to call their setters with the new value.
Comment 13 Nikolas Zimmermann 2011-01-17 08:28:56 PST
(In reply to comment #11)
> (From update of attachment 78804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78804&action=review
> 
> I'd like to see Niko to look at this patch as well. Maybe he has some more proposals.
> 
Good review, I think it covers all issues I have as well.
Comment 14 Zoltan Herczeg 2011-01-18 06:10:22 PST
Created attachment 79273 [details]
patch
Comment 15 Zoltan Herczeg 2011-01-18 06:19:46 PST
I moved one step further to actually use these new functions, but still calling the relayout. So you can see how I plan to use them.
Comment 16 Dirk Schulze 2011-01-19 01:25:11 PST
Comment on attachment 79273 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79273&action=review

> Source/WebCore/ChangeLog:11
> +        calls relayout (Thus, this is just a performance overhead
> +        at this moment).

Please make a detailed description how you plan to reach the goal and what you're doing right now: What happens if an attribute change? What does the renderer of the primitive element do?

> Source/WebCore/ChangeLog:42
> +        Demoing the new attribute setting mechanism.

Demonstration of ...

> Source/WebCore/ChangeLog:49
> +        Scanning the FilterBuilder list, and calls the necessary callbacks.

s/calls/call/

> Source/WebCore/ChangeLog:57
> +        FilterEffect belongs to this RenderObject.

that belongs or maybe belonging

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:334
> +        HashSet<FilterEffect*>::iterator end = effectReferences.end();
> +        for (HashSet<FilterEffect*>::iterator it = effectReferences.begin(); it != end; ++it)

In other SVG files, we often use 
HashSet<FilterEffect*>::iterator it  = effectReferences.begin();
HashSet<FilterEffect*>::iterator end = effectReferences.end();
for (; it != end; ++it)

but it does not really matter.

Makes it really sense to clear all effects this way? I mean this function just makes sense if you want to clear _all_ effects. But isn't it already covered by calling the DTor of FilterBuilder?

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:339
> +void RenderSVGResourceFilter::setAllFilterEffectsAttribute(RenderObject* object, const QualifiedName& attribute)

s/setAllFilterEffectsAttribute/setAllFilterEffectAttributes/

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:351
> +        if (effect) {
> +            primitve->setFilterEffectAttribute(effect, attribute);
> +            recursiveClearResult(builder, effect);
> +        }
> +    }

if (!effect)
   continue;

...

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:40
> -    explicit RenderSVGResourceFilterPrimitive(SVGFilterPrimitiveStandardAttributes* filterPrimitiveElement)
> +    explicit RenderSVGResourceFilterPrimitive(SVGStyledElement* filterPrimitiveElement)

Why did you change the elements type?

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:108
> +    if (attrName == SVGNames::diffuseConstantAttr
> +        || attrName == SVGNames::surfaceScaleAttr
> +        || attrName == SVGNames::lighting_colorAttr)
> +        setAllFilterEffectsAttribute(attrName);
> +

This should be in a separate patch, together with dynamic update tests.

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:76
> +    // When all filters support this method, it will be changed to a pure virtual method.
> +    ASSERT_NOT_REACHED();

If not all filters implemented this function, don't we run into this assert?

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:61
> +    inline void setAllFilterEffectsAttribute(const QualifiedName& attribute)

arh. setFilterEffectAttribute and setAllFilterEffectsAttribute sounds to simular for me. I even don't see a difference in the behavior if I just look at the names. setAttributeToAllFilterEffects? Hm. even that sounds a bit strange. Of course you should rename the function in the renderer as well.
Comment 17 Zoltan Herczeg 2011-01-19 04:12:06 PST
Krit, you asked me not to introduce unused functions, but you want to see the work in small steps in the same time. This is impossible because of the dependencies.

I try to explain you the concept.

The issue:

   Filter     1 --- 1  Filter Renderer         1 --- n  List of FilterBuilder
      | 1 - n               | 1 - n                              | 1 - n
FilterElement 1 --- 1 Filter Primitve Renderer              FilterEffects

When an attribute of the FilterElement changes, it needs to update its FilterEffects (created by this FilterElement before).

Since only the FilterRenderer access the currently existing FilterEffects, all FilterElements needs to implement this function:

void SVGFE*::setFilterEffectAttribute(FilterEffect* effect, const QualifiedName& attrName)

The function is called when we call setAllFilterEffectsAttribute(attrName) in svgAttributeChanged (thus we can put the assert to SVGFilterPrimitiveStandardAttributes.cpp:76)

Note: the other way, accessing the FilterElement from a FilterEffect would cause layering violation.

This patch demonstrates the concept of updating the FilterEffects when the diffuseConstant of DiffuseLighting updates (and invalidates all filter images dependent of this particular DiffuseLighting FilterEffect - this is done automatically). So the FilterBuilders are not changed at all, only some attributes changed, and some existing images freed.

However, the repainting is not implemented, so we do the relayaout as before. This is the smallest step I can think of. Do you wish to implement this a different way?
Comment 18 Dirk Schulze 2011-01-19 05:15:06 PST
(In reply to comment #17)
> Krit, you asked me not to introduce unused functions, but you want to see the work in small steps in the same time. This is impossible because of the dependencies.
I did not ask you to delete more functions?

> 
> I try to explain you the concept.
> 
> The issue:
> 
>    Filter     1 --- 1  Filter Renderer         1 --- n  List of FilterBuilder
>       | 1 - n               | 1 - n                              | 1 - n
> FilterElement 1 --- 1 Filter Primitve Renderer              FilterEffects
> 
> When an attribute of the FilterElement changes, it needs to update its FilterEffects (created by this FilterElement before).
Right.

> 
> Since only the FilterRenderer access the currently existing FilterEffects, all FilterElements needs to implement this function:
> 
> void SVGFE*::setFilterEffectAttribute(FilterEffect* effect, const QualifiedName& attrName)
> 
> The function is called when we call setAllFilterEffectsAttribute(attrName) in svgAttributeChanged (thus we can put the assert to SVGFilterPrimitiveStandardAttributes.cpp:76)
Ok, looks like I misunderstand your code. For clarification:
* An attribute of an effect changes state, lets say surface-scale on the diffuseLigthning effect.
* SVGFEDiffuseLightingElement::svgAttributeChanged calls setAllFilterEffectsAttribute(attrName)
* inline void setAllFilterEffectsAttribute calls setAllFilterEffectsAttribute(primitiveRenderer, attribute); of the parent renderer RenderSVGFilterResource (primitiveRenderer is the renderer of the effect element)
* here we run through all FilterBuilder and call setFilterEffectAttribute(effect, attribute); in the DiffuseLightning element of above for setting the new value in FilterEffect*, and clear all results of all effects.

A bit confusing. But the problem is that we can have different instances of FilterEffect* for one SVGFE*Element and its RenderSVGResourceFilterPrimitive :-/

I'd like to change your concept a bit:

* SVGFEDiffuseLightingElement::svgAttributeChanged calls renderer()->primitiveAttributeChanged(attrName);
* RenderSVGResourceFilterPrimitive::primitiveAttributeChanged(const QualifiedName& attrName) calls parent()->primitveAttributeChanged(this, const QualifiedName& attrName);
* RenderSVGResourceFilter::primitveAttributeChanged(const RenderSVGResourceFilterPrimitive* renderer, const QualifiedName& attrName) is basically your RenderSVGResourceFilter::setAllFilterEffectsAttribute

Would that work? I think it is better to do it that way instead of calling a function in SVGFilterPrimitiveAttr.

> 
> Note: the other way, accessing the FilterElement from a FilterEffect would cause layering violation.
Would this even be possible? Nevertheless this is not the way to go, since FilterEffect must be independent of SVG. - Ah, thats what you mean with layering violation :-P

> 
> This patch demonstrates the concept of updating the FilterEffects when the diffuseConstant of DiffuseLighting updates (and invalidates all filter images dependent of this particular DiffuseLighting FilterEffect - this is done automatically). So the FilterBuilders are not changed at all, only some attributes changed, and some existing images freed.
Yes. And the last point confused me, but now I understand your concept. There is still a problem I guess. What if an attribute causes the smallest effect rect to be bigger? E.g. The stdDeviation of feGaussianBlur increases. Doesn't it mean, that the results of determineFilterPrimitiveSubregion() are wrong? Doesn't this cause an invalidation, because our code in RenderSVGResourceFilter::applyResource() depends on these results? Something I haven't thought about yet :-(
Comment 19 Zoltan Herczeg 2011-01-19 05:46:50 PST
> Yes. And the last point confused me, but now I understand your concept. There is still a problem I guess. What if an attribute causes the smallest effect rect to be bigger? E.g. The stdDeviation of feGaussianBlur increases. Doesn't it mean, that the results of determineFilterPrimitiveSubregion() are wrong? Doesn't this cause an invalidation, because our code in RenderSVGResourceFilter::applyResource() depends on these results? Something I haven't thought about yet :-(

I think we can leave this later. First, I would focus on attributes which does not change layout, only repainting is enough. Although repainting is still difficult, since one Filter(Primitive)Renderer can be referenced by multiple SVG elements, and renderer->repaint() only support one rectangle. Do you have any idea to invalidate multiple rects?
Comment 20 Dirk Schulze 2011-01-19 05:57:13 PST
(In reply to comment #19)
> > Yes. And the last point confused me, but now I understand your concept. There is still a problem I guess. What if an attribute causes the smallest effect rect to be bigger? E.g. The stdDeviation of feGaussianBlur increases. Doesn't it mean, that the results of determineFilterPrimitiveSubregion() are wrong? Doesn't this cause an invalidation, because our code in RenderSVGResourceFilter::applyResource() depends on these results? Something I haven't thought about yet :-(
> 
> I think we can leave this later. First, I would focus on attributes which does not change layout, only repainting is enough. Although repainting is still difficult, since one Filter(Primitive)Renderer can be referenced by multiple SVG elements, and renderer->repaint() only support one rectangle. Do you have any idea to invalidate multiple rects?

Is this necessary? Your idea was to clear all ImageBuffers an ImageData of the effects that needs to get repainted and of all of its childs, the last filterEffect should be affected as well. RenderSVGResourceFilter asks the last FilterEffect for its ImageBuffer, if it hasn't one, It calls FilterEffect::apply. apply calls FilterEffect::apply recursively up to that filterEffect that already has a result. So there isn't even a change necessary on applyResource or postApply, and repaint isn't necessary as well for the moment, right?  If the last effect isn't affected, we don't need to relayout or repaint at all, because the effect that changed is not needed for our result.

Hope it helps :-P
Comment 21 Zoltan Herczeg 2011-01-19 06:05:42 PST
> Is this necessary? Your idea was to clear all ImageBuffers an ImageData of the effects that needs to get repainted and of all of its childs, the last filterEffect should be affected as well. RenderSVGResourceFilter asks the last FilterEffect for its ImageBuffer, if it hasn't one, It calls FilterEffect::apply. apply calls FilterEffect::apply recursively up to that filterEffect that already has a result. So there isn't even a change necessary on applyResource or postApply, and repaint isn't necessary as well for the moment, right?  If the last effect isn't affected, we don't need to relayout or repaint at all, because the effect that changed is not needed for our result.
> 
> Hope it helps :-P

Exactly, but the issue is not here. Somehow something must tell WebKit, that an area must be repainted. Of course, the repainting itself happens with the unmodified code (applyResource, etc), but still, WebKit should knows about that an area must be repainted. Basically render->repaint() should do that, I think. All renderers has a virtual function, which returns the bounding box when you call this repaint(). However, SVGHiddenContainer returns with an empty box, so no repaint happens on the screen. Still, even one bounding box is not enough. A filter can have multiple painting boxes. I could unite them, but I don't think that is the best option, isn't it?
Comment 22 Dirk Schulze 2011-01-21 00:38:57 PST
(In reply to comment #21)
> Exactly, but the issue is not here. Somehow something must tell WebKit, that an area must be repainted. Of course, the repainting itself happens with the unmodified code (applyResource, etc), but still, WebKit should knows about that an area must be repainted. Basically render->repaint() should do that, I think. All renderers has a virtual function, which returns the bounding box when you call this repaint(). However, SVGHiddenContainer returns with an empty box, so no repaint happens on the screen. Still, even one bounding box is not enough. A filter can have multiple painting boxes. I could unite them, but I don't think that is the best option, isn't it?

For filters we have SVGResource and RenderSVGResource that manages this. We have to think about this after your patch got landed. At the moment, one of the both call 'needsLayout' of the target element. This need to be changed somehow. I think this doesn't influence your patch for now.
Comment 23 Zoltan Herczeg 2011-01-21 06:44:29 PST
Created attachment 79729 [details]
patch again

Full patch including repaint.

We recently updated our mac, and seems I cannot do pixel tests anymore with it :(
Comment 24 Dirk Schulze 2011-01-21 06:59:36 PST
Comment on attachment 79729 [details]
patch again

View in context: https://bugs.webkit.org/attachment.cgi?id=79729&action=review

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:108
> +        primitiveAttributeChanged(attrName);

Did you miss my comment about calling the renderer directly here? https://bugs.webkit.org/show_bug.cgi?id=52200#c18
Comment 25 Zoltan Herczeg 2011-01-21 08:41:55 PST
(In reply to comment #24)
> (From update of attachment 79729 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79729&action=review
> 
> > Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:108
> > +        primitiveAttributeChanged(attrName);
> 
> Did you miss my comment about calling the renderer directly here? https://bugs.webkit.org/show_bug.cgi?id=52200#c18

Hm? Perhaps I misunderstood you, but I added a

inline void primitiveAttributeChanged(const QualifiedName& attribute)
{
if (RenderObject* primitiveRenderer = renderer())
    static_cast<RenderSVGResourceFilterPrimitive*>
      (primitiveRenderer)->primitiveAttributeChanged(attribute);
}

to the SVGFilterPrimitiveStandardAttributes.h. Would you prefer this in a different way?
Comment 26 Dirk Schulze 2011-01-21 09:53:49 PST
(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 79729 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=79729&action=review
> > 
> > > Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:108
> > > +        primitiveAttributeChanged(attrName);
> > 
> > Did you miss my comment about calling the renderer directly here? https://bugs.webkit.org/show_bug.cgi?id=52200#c18
> 
> Hm? Perhaps I misunderstood you, but I added a
> 
> inline void primitiveAttributeChanged(const QualifiedName& attribute)
> {
> if (RenderObject* primitiveRenderer = renderer())
>     static_cast<RenderSVGResourceFilterPrimitive*>
>       (primitiveRenderer)->primitiveAttributeChanged(attribute);
> }
> 
> to the SVGFilterPrimitiveStandardAttributes.h. Would you prefer this in a different way?

Yes, like I wrote it in https://bugs.webkit.org/show_bug.cgi?id=52200#c18. This is the common way like  we do it for all elements.
Comment 27 Zoltan Herczeg 2011-01-21 12:14:14 PST
> Yes, like I wrote it in https://bugs.webkit.org/show_bug.cgi?id=52200#c18. This is the common way like  we do it for all elements.

??? still I don't undestand you.

You said:

* SVGFEDiffuseLightingElement::svgAttributeChanged calls renderer()->primitiveAttributeChanged(attrName);
* RenderSVGResourceFilterPrimitive::primitiveAttributeChanged(const QualifiedName& attrName) calls parent()->primitveAttributeChanged(this, const QualifiedName& attrName);

That is exactly what the code snippet do I copied in my previous comment.

SVGFEDiffuseLightingElement calls its render()->primitiveAttributeChanged(...) but renderer() requires a static_cast to RenderSVGResourceFilterPrimitive, so I made an inline function in the base class to make the code more readable.
Comment 28 Dirk Schulze 2011-01-21 13:21:55 PST
Comment on attachment 79729 [details]
patch again

View in context: https://bugs.webkit.org/attachment.cgi?id=79729&action=review

(In reply to comment #27)
> > Yes, like I wrote it in https://bugs.webkit.org/show_bug.cgi?id=52200#c18. This is the common way like  we do it for all elements.
> 
> ??? still I don't undestand you.
> 
> You said:
> 
> * SVGFEDiffuseLightingElement::svgAttributeChanged calls renderer()->primitiveAttributeChanged(attrName);
> * RenderSVGResourceFilterPrimitive::primitiveAttributeChanged(const QualifiedName& attrName) calls parent()->primitveAttributeChanged(this, const QualifiedName& attrName);
> 
> That is exactly what the code snippet do I copied in my previous comment.
> 
> SVGFEDiffuseLightingElement calls its render()->primitiveAttributeChanged(...) but renderer() requires a static_cast to RenderSVGResourceFilterPrimitive, so I made an inline function in the base class to make the code more readable.

Thats true, I was more thinking about consistency across other elements and avoid as much calls of functions from other files as possible to have a better overview. But in this case, and also because you create the renderer in SVGFilterPrimitiveAttribute) it might be better to use your code as is. The patch already looks great. Still some notes.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:151
> -            return false;
> +            return true;

This is difficult. If you return true, the filtered object continues drawing on to the context: the real context! Means we'll draw the target on to the context and on top of it we draw the filter result. Thats bad because of couple of reasons. At first, because we draw the result of the filter with conpositeOver to the context we may see both drawings. Second, the repaintRect of the target depends on the filter area. If the filter area is outside of the objectBoundingBox from the target, we won't clear the results on animations or what ever. Third, we do unnecessary drawings. The filtered target element might be the root element of thounds of childs, that is inefficient. Why do you have to return true here? Shouldn't it work with false as well? We would enter postApply() even it we return false here.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:300
> +            filterData->builded = true;
> +        }
> +
> +        if (!lastEffect->hasResult()) {
> +            // Always true if filterData is just built.

Why did you move 'filterData->builded = true;' atop of 'if (!lastEffect->hasResult())' Isn't it build after we called apply() the first time? and the 'if' doesn't even check filterData->builded so it shouldn't matter here, but makes more sense in general to set it as true, if we really built it.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:363
> +        repaintRectangle(filterData->absoluteDrawingRegion);

Interesting, but you couldn't check it on your mac?

what if you have another element that is drawn on top of the filtered object? will it get repainted as well? (Unsure right now, a test case would help) Also isn't the parent renderer hierarchy RenderSVGResourceContainer-> RenderSVGHiddenContainer? Does it even affect any redrawings? I'd move this to another patch an invalidate like we did before. So you cann add more tests to check the correct behavior of the code.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:54
> +        static_cast<RenderSVGResourceFilter*>(parent())->primitiveAttributeChanged(this, attribute);

please check if the parent exists and if it is really a RenderSVGResourceFilter, IIRC it don't needs to be a RenderSVGResourceFilter. We even create a renderer in this case:
<g>
<feOffset/>
</g>

parent() would be RenderSVGContainer.
Comment 29 Zoltan Herczeg 2011-01-24 01:01:18 PST
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:151
> > -            return false;
> > +            return true;
> 
> This is difficult. If you return true, the filtered object continues drawing on to the context: the real context! Means we'll draw the target on to the context and on top of it we draw the filter result. Thats bad because of couple of reasons. At first, because we draw the result of the filter with conpositeOver to the context we may see both drawings. Second, the repaintRect of the target depends on the filter area. If the filter area is outside of the objectBoundingBox from the target, we won't clear the results on animations or what ever. Third, we do unnecessary drawings. The filtered target element might be the root element of thounds of childs, that is inefficient. Why do you have to return true here? Shouldn't it work with false as well? We would enter postApply() even it we return false here.

Is it? I think I misunderstood the return value. Hm, it looks like it is working with "return false". Great.

> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:300
> > +            filterData->builded = true;
> > +        }
> > +
> > +        if (!lastEffect->hasResult()) {
> > +            // Always true if filterData is just built.
> 
> Why did you move 'filterData->builded = true;' atop of 'if (!lastEffect->hasResult())' Isn't it build after we called apply() the first time? and the 'if' doesn't even check filterData->builded so it shouldn't matter here, but makes more sense in general to set it as true, if we really built it.

Ok I moved it after the apply(). Anyway, why it is "builded" instead of "built" ?

> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:363
> > +        repaintRectangle(filterData->absoluteDrawingRegion);
> 
> Interesting, but you couldn't check it on your mac?

Yeah, our mac has been recently updated and now pixel tests fail with 0.49% difference. Since the image is the same, I think the font was changed. After a clean build, I will check this again, but I don't expect this will change sincs non-filter related images are fail as well.

> what if you have another element that is drawn on top of the filtered object? will it get repainted as well? (Unsure right now, a test case would help) Also isn't the parent renderer hierarchy RenderSVGResourceContainer-> RenderSVGHiddenContainer? Does it even affect any redrawings? I'd move this to another patch an invalidate like we did before. So you cann add more tests to check the correct behavior of the code.

It is ok for me, but how should the code look like? Keeping the functions, but not modify the DiffuseLighting? Or removing the code? Without repainting, the patch does nothing useful, so I think disabling the call in DiffuseLighting would make sense.
Comment 30 Dirk Schulze 2011-01-24 01:38:24 PST
(In reply to comment #29)
> Ok I moved it after the apply(). Anyway, why it is "builded" instead of "built" ?
Oops! my fault :-) 

> > > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:363
> > > +        repaintRectangle(filterData->absoluteDrawingRegion);
> > 
> > Interesting, but you couldn't check it on your mac?
> 
> Yeah, our mac has been recently updated and now pixel tests fail with 0.49% difference. Since the image is the same, I think the font was changed. After a clean build, I will check this again, but I don't expect this will change sincs non-filter related images are fail as well.
Likely that it is a problem with Nikos patch. :-(

> 
> > what if you have another element that is drawn on top of the filtered object? will it get repainted as well? (Unsure right now, a test case would help) Also isn't the parent renderer hierarchy RenderSVGResourceContainer-> RenderSVGHiddenContainer? Does it even affect any redrawings? I'd move this to another patch an invalidate like we did before. So you cann add more tests to check the correct behavior of the code.
> 
> It is ok for me, but how should the code look like? Keeping the functions, but not modify the DiffuseLighting? Or removing the code? Without repainting, the patch does nothing useful, so I think disabling the call in DiffuseLighting would make sense.
Unsure right now. But like I said before, we have to _mark_ the targets for repaint. And shouldn't call repaint directly. Anyway, make a complete invalidation now and try to find out how to repaint in a second patch ;-)
Comment 31 Zoltan Herczeg 2011-01-24 05:49:06 PST
Created attachment 79914 [details]
patch
Comment 32 Zoltan Herczeg 2011-01-25 05:51:20 PST
Created attachment 80055 [details]
patch
Comment 33 Zoltan Herczeg 2011-01-25 06:53:57 PST
Created attachment 80061 [details]
final patch
Comment 34 WebKit Review Bot 2011-01-25 06:55:27 PST
Attachment 80061 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:60:  The parameter name "effect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Dirk Schulze 2011-01-25 06:58:42 PST
Comment on attachment 80061 [details]
final patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80061&action=review

r=me with two notes. great work!

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:340
> +static void recursiveClearResult(SVGFilterBuilder* builder, FilterEffect* effect)
> +{
> +    if (effect->hasResult()) {
> +        effect->clearResult();
> +
> +        HashSet<FilterEffect*>& effectReferences = builder->effectReferences(effect);
> +        HashSet<FilterEffect*>::iterator end = effectReferences.end();
> +        for (HashSet<FilterEffect*>::iterator it = effectReferences.begin(); it != end; ++it)
> +            recursiveClearResult(builder, *it);
> +    }
> +}

please reomve this before landing

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:105
> +    if (effect->hasResult()) {
> +        effect->clearResult();
> +
> +        HashSet<FilterEffect*>& effectReferences = this->effectReferences(effect);
> +        HashSet<FilterEffect*>::iterator end = effectReferences.end();
> +        for (HashSet<FilterEffect*>::iterator it = effectReferences.begin(); it != end; ++it)
> +            clearResultsRecursive(*it);
> +    }

make a early return here:
if (!effect->hasResult())
  return;
Comment 36 Zoltan Herczeg 2011-01-25 07:12:08 PST
Landed in http://trac.webkit.org/changeset/76590

Thank you again for the review.