Repaint broken when children of filtered SVG elements are updated

• Component: SVG
• Status: RESOLVED FIXED

[Joel Webber]

When a parent (e.g., <g>) SVG element has a filter applied, and one of its children is updated (in such a way that only a sub-rectangle of the outer element would normally need to be repainted), webkit re-renders the element incorrectly. You can see this in the attached HTML file -- both squares should be green, but the second usually turns up red, because the element doesn't repaint correctly when the child's fill color is updated (on a 1ms timer).

This bug manifests in several different ways -- the attached example is the simplest reproduction I could get. Generally speaking, what's happening is as follows:
- On the first paint, the SVG filter code allocates an ImageBuffer, paints the results of the filter into it, then caches it.
  - Subsequent repaints just use the cached ImageBuffer.
- When you update the child, it kicks off a repaint for the bounds of the child's RenderObject.
- When that paint comes back down the pipeline, the <g> with the filter applied allocates a new ImageBuffer.
- The PaintInfo specifies that only the child's bounds needs to be repainted.
- The filter's *new* ImageBuffer is only partially painted.
- This mostly works, because the part of the filter's ImageBuffer that overlaps the paint rectangle is copied to the screen.
- But the *next* time we repaint the <g> from this ImageBuffer cache, it only contains a subset of the pixels it needs.
- This causes seemingly-random parts of the <g> to get blown away visually.

[Julien Chaffraix]

(In reply to comment #0)
> You can see this in the attached HTML file -- both squares should be green, but the second usually turns up red, because the element doesn't repaint correctly when the child's fill color is updated (on a 1ms timer).

Joel, it looks like the attachment did not make it. Could you upload it (again)?

[Joel Webber]

Created attachment 114273 [details]
Missing example

[Joel Webber]

Created attachment 114275 [details]
Patch

[Joel Webber]

My proposed fix for this (in the attached patch) is to modify RenderSVG[Container Root] such that they needsLayout() whenever any of their children needsLayout(). This fixes the immediate problem because the filter's ImageBuffer cache always gets fully repainted whenever a child is modified.

I also believe this will fix other probably-as-yet-unreported bugs with SVG filters, because repainting only the child's bounds is insufficient for filters with kernel sizes > 1px.

I'm not attached to the approach, but I think it's pretty straightforward -- any suggestions on the details are very much welcome.

[Joel Webber]

A couple of notes on the test case:
- This test only fails intermittently on trunk. If you load the attached HTML file, you'll see that it occasionally renders correctly (both squares green), but most of the time you'll see that the bottom-right square is red. It always works correctly with the patch applied.
- I have another test that more directly models the bug, by changing a child of the <g> element, which causes the rest of the cached ImageBuffer to get blown away (as in the bug description). But for the life of me I can't get it to fail when running in test_shell or as a layout test. I believe this has something to do with differences between the painting logic in test_shell and chrome/safari.

[Nikolas Zimmermann]

Comment on attachment 114275 [details]
Patch

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

First round of review, thanks for tackling this!

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:57
> +    SVGRenderSupport::setNeedsLayoutForFilteredContainer(this);

I'd rename this to "setContainerNeedsLayoutIfNeeded".

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:310
> +    if (object->normalChildNeedsLayout()) {

Use early return here.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:312
> +        if (resources && resources->filter())

And here, for any of the cases.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:313
> +            object->setNeedsLayout(true);

This is dangerous, you're mutating the setNeedsLayout state of all containing blocks as well, are you sure you want that? Doing that from within layout() that looks suspicious.
I'd propose to amend RenderSVGContainers way of laying out the children, where we currently do:
    SVGRenderSupport::layoutChildren(this, selfNeedsLayout());

This could just read layoutChildren(this, selfNeedsLayout() || filtersForceContainerLayout())
where filtersForceContainerLayout() contains your object->normalChildNeedsLayout() && resources->filter() check.
Sounds better, eh?

> LayoutTests/svg/repaint/filter-child-repaint.svg:13
> +      <feOffset in="SourceGraphic" dx="0" dy="0" result="topCopy"/>
> +      <feGaussianBlur in="SourceAlpha" stdDeviation="2" result="shadow"/>
> +      <feOffset in="shadow" dx="3" dy="3" result="movedShadow"/>
> +      <feMerge>
> +        <feMergeNode in="movedShadow"/>
> +        <feMergeNode in="topCopy"/>
> +      </feMerge>
> +    </filter>

You could use the new feDropShadow primitive, would simplify the test.

> LayoutTests/svg/repaint/filter-child-repaint.svg:15
> +

Can you add a <title> section, explaining when this tests passes?
Currently its hard to tell, without tracing in mind :-)

> LayoutTests/svg/repaint/filter-child-repaint.svg:16
> +  <g filter='url(#dropShadow)' width='128px' height='128px'>

width/height are useless on <g>, you can remove it.

> LayoutTests/svg/repaint/filter-child-repaint.svg:18
> +    <rect width="64" height="64" style="fill:rgb(0,128,0)"/>
> +    <rect id='poke' x='32' y='32' width="64" height="64" style="fill:rgb(128,0,0)"/>

Also you could just use fill="green" here.

[Joel Webber]

Comment on attachment 114275 [details]
Patch

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

>> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:57

> 
> I'd rename this to "setContainerNeedsLayoutIfNeeded".

Done.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:310
>> +    if (object->normalChildNeedsLayout()) {
> 
> Use early return here.

Done.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:312
>> +        if (resources && resources->filter())
> 
> And here, for any of the cases.

Done.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:313
>> +            object->setNeedsLayout(true);
> 
> This is dangerous, you're mutating the setNeedsLayout state of all containing blocks as well, are you sure you want that? Doing that from within layout() that looks suspicious.
> I'd propose to amend RenderSVGContainers way of laying out the children, where we currently do:
>     SVGRenderSupport::layoutChildren(this, selfNeedsLayout());
> 
> This could just read layoutChildren(this, selfNeedsLayout() || filtersForceContainerLayout())
> where filtersForceContainerLayout() contains your object->normalChildNeedsLayout() && resources->filter() check.
> Sounds better, eh?

Thanks for clarifying that -- it felt a bit odd to be flipping that bit here, and I failed to notice the hierarchical behavior of calling setNeedsLayout(). I've refactored this code as you suggest, and it does indeed feel better.

>> LayoutTests/svg/repaint/filter-child-repaint.svg:13
>> +    </filter>
> 
> You could use the new feDropShadow primitive, would simplify the test.

Done, thanks. That one wasn't in the SVG 1.1 docs, but I see it in WebKit now.
(This awkward implementation, btw, came from a chromium bug I was investigating)

>> LayoutTests/svg/repaint/filter-child-repaint.svg:15
>> +
> 
> Can you add a <title> section, explaining when this tests passes?
> Currently its hard to tell, without tracing in mind :-)

Done. It's a bit hard to tell what the norms are from existing tests :)

>> LayoutTests/svg/repaint/filter-child-repaint.svg:16
>> +  <g filter='url(#dropShadow)' width='128px' height='128px'>
> 
> width/height are useless on <g>, you can remove it.

Done.

>> LayoutTests/svg/repaint/filter-child-repaint.svg:18
>> +    <rect id='poke' x='32' y='32' width="64" height="64" style="fill:rgb(128,0,0)"/>
> 
> Also you could just use fill="green" here.

Done (old habits die hard...).

[Joel Webber]

Created attachment 114526 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 114526 [details]
Patch

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

Looks good, r=me!

> Source/WebCore/rendering/svg/SVGRenderSupport.h:58
> +    // Determines whether a container needs to be laid out because it's filtered and a child is being laid out

Missed period.

[Joel Webber]

Created attachment 114566 [details]
Patch

[Joel Webber]

Please ignore that last patch. There's commit-log diff garbage in there.

[Joel Webber]

Created attachment 114569 [details]
Added the requested '.'

[Joel Webber]

Many thanks for the prompt review, Nikolas. Looks like I've managed to reset the review state by adding that trivial period to the comment and re-uploading. Whenever you have a free moment to R+ it, it would be greatly appreciated.

[Nikolas Zimmermann]

Comment on attachment 114569 [details]
Added the requested '.'

Thanks!

[WebKit Review Bot]

Comment on attachment 114569 [details]
Added the requested '.'

Clearing flags on attachment: 114569

Committed r100036: <http://trac.webkit.org/changeset/100036>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The test added by this patch is failing on Mac (the second box is red).