Bug 14003

Summary: WebKit invalidates *way* too much during JS based SVG animation
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: ml, oliver, zimmermann
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
URL: http://hixie.ch/tests/adhoc/svg/perf/001.xml
Attachments:
Description Flags
fix (exposes a bug in text repaint code)
none
fix (and avoids bad RenderBlock code) oliver: review+

Description Eric Seidel (no email) 2007-06-05 13:19:53 PDT
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.
Comment 1 Eric Seidel (no email) 2007-06-05 14:53:13 PDT
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.
Comment 2 Rob Buis 2007-08-18 13:16:06 PDT
(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.
Comment 3 Eric Seidel (no email) 2007-08-19 12:44:28 PDT
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.
Comment 4 Eric Seidel (no email) 2007-09-26 12:14:44 PDT
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
Comment 5 Eric Seidel (no email) 2007-09-30 19:49:21 PDT
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.
Comment 6 Oliver Hunt 2007-10-05 00:04:24 PDT
Alas you patch destroys text input repainting :(
Comment 7 Eric Seidel (no email) 2007-10-05 17:25:55 PDT
Created attachment 16555 [details]
fix (and avoids bad RenderBlock code)
Comment 8 Oliver Hunt 2007-10-05 17:52:16 PDT
Comment on attachment 16555 [details]
fix (and avoids bad RenderBlock code)

While this doesn't resolve all issues, it's certainly improved things dramatically
Comment 9 Eric Seidel (no email) 2007-10-05 18:12:46 PDT
Landed as r26075 on the feature-branch