Bug 52200 – Repaint SVG elements with filter instead of relayout where possible

• Component: SVG
• Status: RESOLVED FIXED

[Zoltan Herczeg]

Working towards adding primitive renderers in small steps.

[Zoltan Herczeg]

Created attachment 78507 [details]
patch

[Zoltan Herczeg]

Created attachment 78514 [details]
patch after update

[WebKit Review Bot]

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.

[Zoltan Herczeg]

Created attachment 78517 [details]
patch after update

[Dirk Schulze]

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?

[Zoltan Herczeg]

> > 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.

[Zoltan Herczeg]

Created attachment 78684 [details]
patch

[Dirk Schulze]

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.

[Zoltan Herczeg]

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.

[Zoltan Herczeg]

Created attachment 78804 [details]
patch

[Dirk Schulze]

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.

[Zoltan Herczeg]

> > 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.

[Nikolas Zimmermann]

(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.

[Zoltan Herczeg]

Created attachment 79273 [details]
patch

[Zoltan Herczeg]

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.

[Dirk Schulze]

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.

[Zoltan Herczeg]

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?

[Dirk Schulze]

(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 :-(

[Zoltan Herczeg]

> 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?

[Dirk Schulze]

(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

[Zoltan Herczeg]

> 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?

[Dirk Schulze]

(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.

[Zoltan Herczeg]

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 :(

[Dirk Schulze]

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

[Zoltan Herczeg]

(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?

[Dirk Schulze]

(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.

[Zoltan Herczeg]

> 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.

[Dirk Schulze]

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.

[Zoltan Herczeg]

> > 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.

[Dirk Schulze]

(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 ;-)

[Zoltan Herczeg]

Created attachment 79914 [details]
patch

[Zoltan Herczeg]

Created attachment 80055 [details]
patch

[Zoltan Herczeg]

Created attachment 80061 [details]
final patch

[WebKit Review Bot]

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.

[Dirk Schulze]

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;

[Zoltan Herczeg]

Landed in http://trac.webkit.org/changeset/76590

Thank you again for the review.