Bug 14003 – WebKit invalidates *way* too much during JS based SVG animation

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

[Eric Seidel (no email)]

WebKit invalidates *way* too much during JS based SVG animation

See Hixie's perf test.  Watch how we end up redrawing the entire tiger every time, even though we should only be redrawing the area where the circle was before.

[Eric Seidel (no email)]

Ok, it turns out the problem is due to this line:

    while (child) {
        if (!child->isRenderPath() || static_cast<RenderPath*>(child)->hasRelativeValues())
            child->setNeedsLayout(true);


We end up causing any container child to re-layout, even when not necessary.  Instead the RenderSVGContainer code needs to be smart enough to see if it's width/height has changed, and then (and only then) bother to mark it's relative-length path or container kids as needing layout.

[Rob Buis]

(In reply to comment #0)
> WebKit invalidates *way* too much during JS based SVG animation
> 
> See Hixie's perf test.  Watch how we end up redrawing the entire tiger every
> time, even though we should only be redrawing the area where the circle was
> before.
> 
Seems fixed on fb?

Cheers,

Rob.

[Eric Seidel (no email)]

Do you remember which patch had the fix?  I've not been watching the checkins closely *at all* but closing this would require the "relayout every child when doing a layout" bug described above to have been fixed.

[Eric Seidel (no email)]

The bad code still exists in RenderSVGContainer:

    while (child) {
        // FIXME: This check is bogus, see http://bugs.webkit.org/show_bug.cgi?id=14003
        if (!child->isRenderPath() || static_cast<RenderPath*>(child)->hasRelativeValues())
            child->setNeedsLayout(true);

line 243

[Eric Seidel (no email)]

Created attachment 16479 [details]
fix (exposes a bug in text repaint code)

So this fixes our overzealous repainting, however it exposes a repaint bug in RenderSVGText's use of RenderBlock::layout().  RenderBlock::layout() will attempt to do a partial repaint when the block itself has not changed size.  The calculations used for the partial repaint were initially not SVG transform aware (this patch fixes that), but even with this patch there seem to be int vs. float issues regarding those repaints.  With this patch applied you can view the perf01.xml example with Quartz Debug's "Flash Screen Updates" enabled and see how as you resize the window *sometimes* the repaint rect overlaps with the changing text, and sometimes it doesn't.  :(

I should note that only RenderSVGRoot needs this change (to check if the bounds changed before forcing kids to layout), due to the fact that RenderSVGRoot determines its own bounds, and RenderSVGContainer does not (RenderSVGContainer's bounds are determined by its kids).  Actually, as I type that, I realize that might not always be true.  <g>'s bounds are determined by its kids, but other containers might determine their own bounds, in which case this change is still not quite correct.

[Oliver Hunt]

Alas you patch destroys text input repainting :(

[Eric Seidel (no email)]

Created attachment 16555 [details]
fix (and avoids bad RenderBlock code)

[Oliver Hunt]

Comment on attachment 16555 [details]
fix (and avoids bad RenderBlock code)

While this doesn't resolve all issues, it's certainly improved things dramatically

[Eric Seidel (no email)]

Landed as r26075 on the feature-branch