Bug 68457 – Style changes applied to filtered elements do not invalidate the element

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Tim Horton]

Open the attached test case. Note how the color of the blurred ellipse doesn't pulse.

Remove the filter. Note how the animation works fine.

Open the original test case in Opera. Also works fine, and is blurred!

[Tim Horton]

Created attachment 108039 [details]
repro

[Radar WebKit Bug Importer]

<rdar://problem/10154777>

[Tim Horton]

Note, the "from" color (the initial color of the animation) *is* getting applied.

[Dirk Schulze]

Without looking into the code, I assume that we do not invalidate the filter for the example.

[Tim Horton]

So it looks like most style changes don't cause filter invalidation. Anything that requires relayout does, because then we go SVGResourcesCache::clientLayoutChanged -> RenderSVGResourceFilter::removeClientFromCache and remove the filter from RenderSVGResourceFilter::m_filter, which then causes us not to bail early in RenderSVGResourceFilter::applyResource (if we don't removeClientFromCache, we bail in the m_filter.contains(object) check).

Style changes don't end up calling removeClientFromCache, so the filter is still in m_filter, so we don't invalidate.

I'm not sure what the downsides to calling removeClientFromCache from SVGResourcesCache::clientStyleChanged might be (but have confirmed that copying the body of SVGResourcesCache::clientLayoutChanged (which does call removeClientFromCache) does solve this problem).

I'll fix it next week.

[Tim Horton]

Created attachment 108717 [details]
patch!

Happens with all style changes, not just <animateColor>.

[Simon Fraser (smfr)]

Comment on attachment 108717 [details]
patch!

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

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:131
> +    // In this case the proper SVGFE*Element will imply whether the modified CSS properties require a relayout or repaint.

I don't understand the comment. How does an element imply anything?

[Tim Horton]

(In reply to comment #7)
> (From update of attachment 108717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108717&action=review
> 
> > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:131
> > +    // In this case the proper SVGFE*Element will imply whether the modified CSS properties require a relayout or repaint.
> 
> I don't understand the comment. How does an element imply anything?

Ooh, good point, I meant to change that word while I was fixing the comment too. Pretty sure "determine" (or "decide") is the intended word.

[Dirk Schulze]

Comment on attachment 108717 [details]
patch!

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

r- because I'd like an even more reduced test case. Also have a question to your patch itself.

Btw. Don't you have commit rights? Why do you set cq?

> LayoutTests/svg/filters/animate-fill.svg:6
> +    <filter id="filt">
> +        <feGaussianBlur stdDeviation="3" />
> +    </filter>

Shouldn't we still use the filter code path if we don't have an effect at all? If not, even an offset effect (feOffset) without attributes would be enough.

> LayoutTests/svg/filters/animate-fill.svg:9
> +    <clipPath id="clip">
> +        <rect width="50px" height="50px" x="25" y="25" />
> +    </clipPath>

Is it just reproducible with a clipPath? If you just used it to have a solid filled rect, the suggestion above should help.

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:152
> +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer);
> +    if (!resources)
> +        return;
> +
> +    resources->removeClientFromCache(renderer);

Do we still need to invalidate the client for clipPath? Is there a CSS property that could influence any visual affect on clipPath? IIRC SVGResourceCache is used for PaintServers as well (pattern and gradient), can you reproduce the problem for these resources as well?

[Tim Horton]

(In reply to comment #9)
> (From update of attachment 108717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108717&action=review
> 
> r- because I'd like an even more reduced test case. Also have a question to your patch itself.

Ok.

> Btw. Don't you have commit rights? Why do you set cq?

I don't know, good question! Habit? Convenience? (is there any downside?)

> > LayoutTests/svg/filters/animate-fill.svg:6
> > +    <filter id="filt">
> > +        <feGaussianBlur stdDeviation="3" />
> > +    </filter>
> 
> Shouldn't we still use the filter code path if we don't have an effect at all?

But then we won't draw anything!

> If not, even an offset effect (feOffset) without attributes would be enough.
> 
> > LayoutTests/svg/filters/animate-fill.svg:9
> > +    <clipPath id="clip">
> > +        <rect width="50px" height="50px" x="25" y="25" />
> > +    </clipPath>
> 
> Is it just reproducible with a clipPath? If you just used it to have a solid filled rect, the suggestion above should help.

You're inside my head! (i.e., I just used it to have a solid filled rect and make the test work more places).

It does appear to reproduce with feOffset, so I'll use that; that'll make the test a lot cleaner.

> > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:152
> > +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer);
> > +    if (!resources)
> > +        return;
> > +
> > +    resources->removeClientFromCache(renderer);
> 
> Do we still need to invalidate the client for clipPath? Is there a CSS property that could influence any visual affect on clipPath? IIRC SVGResourceCache is used for PaintServers as well (pattern and gradient), can you reproduce the problem for these resources as well?

Hmm, that's a good question. I'll take a look and report back.

[Tim Horton]

(In reply to comment #9)
> (From update of attachment 108717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108717&action=review
> 
> r- because I'd like an even more reduced test case. Also have a question to your patch itself.
> 
> Btw. Don't you have commit rights? Why do you set cq?
> 
> > LayoutTests/svg/filters/animate-fill.svg:6
> > +    <filter id="filt">
> > +        <feGaussianBlur stdDeviation="3" />
> > +    </filter>
> 
> Shouldn't we still use the filter code path if we don't have an effect at all? If not, even an offset effect (feOffset) without attributes would be enough.
> 
> > LayoutTests/svg/filters/animate-fill.svg:9
> > +    <clipPath id="clip">
> > +        <rect width="50px" height="50px" x="25" y="25" />
> > +    </clipPath>
> 
> Is it just reproducible with a clipPath? If you just used it to have a solid filled rect, the suggestion above should help.
> 
> > Source/WebCore/rendering/svg/SVGResourcesCache.cpp:152
> > +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer);
> > +    if (!resources)
> > +        return;
> > +
> > +    resources->removeClientFromCache(renderer);
> 
> Do we still need to invalidate the client for clipPath? Is there a CSS property that could influence any visual affect on clipPath? IIRC SVGResourceCache is used for PaintServers as well (pattern and gradient), can you reproduce the problem for these resources as well?

Doesn't reproduce with the others. Perhaps a better approach would be to change the way we check for invalidation in RenderSVGResourceFilter::applyResource? Or clear the resource from m_filter elsewhere? I'll look for a different solution if you think this one is overkill.

[Tim Horton]

Created attachment 108730 [details]
patch, fixed the comment and the test

[Tim Horton]

Landed in 96052.

[Dirk Schulze]

Sad that it was landed. I indeed think that this is overkilling! We definitely don't need to invalidate masker, pattern, gradients and clip path on style changes of the target!

Can you please come up with a followup patch that would just clean the resource if it is a filter?

[Nikolas Zimmermann]

(In reply to comment #14)
> Sad that it was landed. I indeed think that this is overkilling! We definitely don't need to invalidate masker, pattern, gradients and clip path on style changes of the target!
> 
> Can you please come up with a followup patch that would just clean the resource if it is a filter?

I share Dirks concerns as well. Sorry for not commenting earlier, I took some days off...
Anyhow: We have to be very careful when we release anything from the cache, style changes may happen more frequently than you'd expect!

An easy test would be to add a fprintf(stderr, "INVALIDATE!") before your newly added removeClientFromCache() call - then run all SVG pixel tests and look in the layout-test-results directory for all *stderr.txt files containing "INVALIDATE". I bet it's now firing a lot more often than desired.

Can you please find this out? I fear this is a large perf regression.

[Tim Horton]

(In reply to comment #15)
> (In reply to comment #14)
> > Sad that it was landed. I indeed think that this is overkilling! We definitely don't need to invalidate masker, pattern, gradients and clip path on style changes of the target!
> > 
> > Can you please come up with a followup patch that would just clean the resource if it is a filter?
> 
> I share Dirks concerns as well. Sorry for not commenting earlier, I took some days off...
> Anyhow: We have to be very careful when we release anything from the cache, style changes may happen more frequently than you'd expect!
> 
> An easy test would be to add a fprintf(stderr, "INVALIDATE!") before your newly added removeClientFromCache() call - then run all SVG pixel tests and look in the layout-test-results directory for all *stderr.txt files containing "INVALIDATE". I bet it's now firing a lot more often than desired.
> 
> Can you please find this out? I fear this is a large perf regression.

Ok, I'll fix this today. I've been looking around trying to come up with cases where target style changes would require a master/pattern/gradient/clipPath and could come up with nothing, so sure, we'll only do it in the case of filters. Does that work?

[Tim Horton]

Performance fix goes here ---> https://bugs.webkit.org/show_bug.cgi?id=68941