Last Comment Bug 541188 - nsSVGForeignObjectFrame::PaintSVG doesn't pass aDirtyRect through
: nsSVGForeignObjectFrame::PaintSVG doesn't pass aDirtyRect through
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla1.9.3a1
Assigned To: David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
:
Mentors:
http://dbaron.org/mozilla/invert-colo...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-21 12:49 PST by David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
Modified: 2010-01-30 11:31 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.81 KB, patch)
2010-01-21 16:08 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
the original testcase (invert-colors.xhtml before I changed it to avoid foreignObject) (3.62 KB, application/xhtml+xml)
2010-01-21 17:39 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
mac reftest failure (14.64 KB, patch)
2010-01-22 19:20 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (2.83 KB, patch)
2010-01-26 18:28 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
jwatt: review+
roc: superreview+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-21 12:49:03 PST
I was investigating why fixing bug 418063 didn't make much of an improvement on the performance problems in:
http://dbaron.org/mozilla/invert-colors#http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry

I think I've found the next (maybe even last, I hope) problem, which is that nsSVGForeignObjectFrame::PaintSVG doesn't pass a transformed version of aDirtyRect through to the call to nsLayoutUtils::PaintFrame; it just tells PaintFrame that everything is dirty.
Comment 1 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-21 16:08:38 PST
Created attachment 422861 [details] [diff] [review]
patch

This patch seems to work; it makes things involving repainting in foreignObject feel *much* faster.

I found this code (and the GetFrameForPoint code) somewhat confusing, but I think I understand it now.  However, now that I understand it, I'm not quite sure how to make it simpler, though I think it should be.

I've tested that the repainting from selection and link hovering works in http://starkravingfinkle.org/blog/wp-content/uploads/2007/07/foreignobject-transform.svg (including when zoomed), and in a similar testcase that has a viewbox.  I should probably have some automated tests for those combinations, though...
Comment 2 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-21 17:15:52 PST
(In reply to comment #0)
> I was investigating why fixing bug 418063 didn't make much of an improvement on
> the performance problems in:
> http://dbaron.org/mozilla/invert-colors#http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry

That's no longer a testcase for this, since I reworked it to avoid using foreignObject (it uses 'filter' on HTML elements instead).
Comment 3 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-21 17:39:24 PST
Created attachment 422884 [details]
the original testcase (invert-colors.xhtml before I changed it to avoid foreignObject)
Comment 4 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-21 20:40:09 PST
When I pushed to try, I got this failure on Mac only:
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/sendchange-macosx-unittest/mozilla/layout/reftests/svg/moz-only/foreignObject-zoom-01.svg |
... seems like it could be a real problem.  (Not sure if it's this bug or bug 418063's patch.)
Comment 5 Robert Longson 2010-01-22 00:02:36 PST
testcases for this patch are in bug 426980 bug 428379 and bug 484677

It may be possible to turn the first two into reftests using roc's zoomed reftest feature. If the first two bugs still work then it's pretty certain that bug 484677 still will.
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-22 19:20:16 PST
Created attachment 423118 [details] [diff] [review]
mac reftest failure

This is the reftest failure from try server; I confirmed it is due to this patch, and it's happened twice.  It's Mac-only.

The problem is a very slight color difference down the right edge of the viewport.

I expect I need a call to ToOutsidePixels somewhere.
Comment 7 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-26 18:28:13 PST
Created attachment 423720 [details] [diff] [review]
patch

The cause of the reftest failure was that I was inadvertently using the default conversion (truncate) of floats to nscoord rather than using NSToCoordFloor/Ceil as appropriate.
Comment 8 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-26 18:31:57 PST
Also, I wonder if we should really be intersecting the dirty rect with the kid's rect (which is what we used to pass as the dirty rect); maybe we shouldn't.  I should write some tests...
Comment 9 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-27 14:59:41 PST
(In reply to comment #8)
> Also, I wonder if we should really be intersecting the dirty rect with the
> kid's rect (which is what we used to pass as the dirty rect); maybe we
> shouldn't.  I should write some tests...

It seems like we're always clipping the overflow, so it doesn't matter.
Comment 10 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-01-30 11:31:18 PST
http://hg.mozilla.org/mozilla-central/rev/bbc58a03cab9

Note You need to log in before you can comment on or make changes to this bug.