Bug 68457

Summary: Style changes applied to filtered elements do not invalidate the element
Product: WebKit Reporter: Tim Horton <thorton>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: krit, simon.fraser, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
repro
none
patch!
krit: review-, simon.fraser: commit-queue-
patch, fixed the comment and the test darin: review+

Description Tim Horton 2011-09-20 12:07:47 PDT
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!
Comment 1 Tim Horton 2011-09-20 12:08:07 PDT
Created attachment 108039 [details]
repro
Comment 2 Radar WebKit Bug Importer 2011-09-20 12:08:10 PDT
<rdar://problem/10154777>
Comment 3 Tim Horton 2011-09-20 12:08:46 PDT
Note, the "from" color (the initial color of the animation) *is* getting applied.
Comment 4 Dirk Schulze 2011-09-22 00:28:32 PDT
Without looking into the code, I assume that we do not invalidate the filter for the example.
Comment 5 Tim Horton 2011-09-23 18:20:46 PDT
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.
Comment 6 Tim Horton 2011-09-26 13:47:25 PDT
Created attachment 108717 [details]
patch!

Happens with all style changes, not just <animateColor>.
Comment 7 Simon Fraser (smfr) 2011-09-26 13:57:43 PDT
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?
Comment 8 Tim Horton 2011-09-26 14:00:19 PDT
(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.
Comment 9 Dirk Schulze 2011-09-26 14:05:21 PDT
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?
Comment 10 Tim Horton 2011-09-26 14:12:23 PDT
(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.
Comment 11 Tim Horton 2011-09-26 15:14:54 PDT
(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.
Comment 12 Tim Horton 2011-09-26 15:16:13 PDT
Created attachment 108730 [details]
patch, fixed the comment and the test
Comment 13 Tim Horton 2011-09-26 17:35:56 PDT
Landed in 96052.
Comment 14 Dirk Schulze 2011-09-26 23:55:11 PDT
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?
Comment 15 Nikolas Zimmermann 2011-09-27 11:55:08 PDT
(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.
Comment 16 Tim Horton 2011-09-27 12:02:22 PDT
(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?
Comment 17 Tim Horton 2011-09-27 15:10:50 PDT
Performance fix goes here ---> https://bugs.webkit.org/show_bug.cgi?id=68941