Closed Bug 418063 Opened 17 years ago Closed 15 years ago

We shouldn't invalidate the entire foreignObject area when its children change

Categories

(Core :: SVG, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: jwatt, Assigned: dbaron)

References

Details

(Keywords: perf, regression)

Attachments

(1 file)

The checkin for bug 401112 caused us to invalidate the *entire* foreignObject area whenever its children change, no matter how small an area the children take up. What happened is that nsSVGUtils::FindFilterInvalidation was changed to return the frame's mRect if the frame is not filtered. As a result nsSVGForeignObjectFrame::FlushDirtyRegion will never invalidate anything less than the entire nsSVGForeignObjectFrame's covered area, regardless of how big or small mDirtyRegion actually is.
I'm wondering whether we should actually try to fix this for Firefox 3. Invalidating the entire area may be the only way to hide the invalidation bugs that occur when elements that require a view change inside a foreignObject. E.g. see bug 350698.
Keywords: perf
Flags: blocking1.9?
Assignee: nobody → jwatt
Version: unspecified → Trunk
Yeah, we should probably just leave it for safety's sake.
Clearing blocking1.9?, but I would like to patch this so that the fact we're doing whole-area invalidation is explicit, not hidden.
Flags: blocking1.9?
Does this bug cover the code that's now in nsSVGForeignObjectFrame::InvalidateDirtyRect : // XXX invalidate the entire covered region // See bug 418063 rect.UnionRect(rect, mRect); ? That UnionRect is causing pretty substantial performance problems with the animation in http://dbaron.org/mozilla/invert-colors#http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry I wonder if we should try fixing this now; views are much less of an issue.
(In reply to comment #5) > Does this bug cover the code that's now in > nsSVGForeignObjectFrame::InvalidateDirtyRect : > // XXX invalidate the entire covered region > // See bug 418063 > rect.UnionRect(rect, mRect); > ? > Indeed, those lines are the ones that should go to fix this bug. bug 378879, bug 403443 and bug 350698 contain testcases that currently work due to this bodge but have previously failed without it.
> That UnionRect is causing pretty substantial performance problems with the > animation in > http://dbaron.org/mozilla/invert-colors#http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry > > I wonder if we should try fixing this now; views are much less of an issue. FWIW we do support color-interpolation-filters: sRGB
If the bodge is removed then I get: bug 350698 and bug 378879 WFM. bug 403443 still broken. It looks like some transform is missing from the area to refresh in bug 403443 as the screen is getting cleared in horizontal and vertically aligned rectangles while the redraw is aligned to the rotated foreignObject element.
Attached patch patch (deleted) — Splinter Review
Thanks for testing all of those. I attached a simple patch to bug 403443 that fixes it when this change is made. Here's the removal for this bug. Requesting review from jwatt to confirm that he agrees that this is what this bug is about.
Attachment #422773 - Flags: review?(jwatt)
Fixing bug 541188 will be needed to actually get most of the performance benefit, though.
(In reply to comment #8) > If the bodge is removed then I get: > bug 350698 and bug 378879 WFM. > bug 403443 still broken. With that and bug 541188, then: * bug 350698 is still fixed; probably fixed by disuse of views * I see bug 378879 with or without the patches * bug 403443 is still fixed by the patch I put there
Attachment #422773 - Flags: review?(jwatt) → review+
Assignee: jwatt → dbaron
Status: NEW → RESOLVED
Closed: 15 years ago
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: