Last Comment Bug 614732 - Implement SVG display lists
: Implement SVG display lists
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: mozilla17
Assigned To: Jonathan Watt [:jwatt]
:
: Jet Villegas (:jet)
Mentors:
https://wiki.mozilla.org/SVG:SVG-disp...
Depends on: 824464 876234 541270 660216 725897 725903 726928 729562 731959 732429 732819 734082 762679 765505 766227 767734 767823 769103 769242 769514 769612 769645 769742 769902 769970 772017 772246 773837 774095 774133 775697 775735 776393 776747 777585 778012 780963 788831 791675 793658 794098 808318 813531 818435 822014 829825 875329 916515 958160
Blocks: 360148 554004 270264 612250 614564 672413 733764 776054
  Show dependency treegraph
 
Reported: 2010-11-24 18:52 PST by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2014-01-11 07:39 PST (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add --enable-svg-dlists option (2.05 KB, patch)
2011-05-22 07:31 PDT, Jonathan Watt [:jwatt]
khuey: review-
Details | Diff | Splinter Review
Add MOZ_SVG_DLISTS flag (1.99 KB, patch)
2011-05-23 16:31 PDT, Jonathan Watt [:jwatt]
khuey: review+
jwatt: checkin+
Details | Diff | Splinter Review
Some doc changes (2.68 KB, patch)
2011-05-23 16:34 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
Make sure SVG frames that shouldn't directly display don't create display list items (7.60 KB, patch)
2011-08-11 07:37 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
stop recalculating the contribution of markers on every GetCoveredRegion() (3.48 KB, patch)
2011-09-07 13:21 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
put all bounds calculation code in GetBBoxContribution in a way that it can be reused (28.20 KB, patch)
2011-09-07 13:37 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
put all bounds calculation code in GetBBoxContribution in a way that it can be reused (27.65 KB, patch)
2011-09-29 18:03 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
Kill covered regions (make mRect for SVG frames relative to user space) (15.86 KB, patch)
2011-11-30 09:30 PST, Jonathan Watt [:jwatt]
roc: review+
Details | Diff | Splinter Review
Implement option #1 from comment 32 (7.59 KB, patch)
2011-12-06 09:33 PST, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
Kill covered regions (make mRect for SVG frames relative to user space) (15.08 KB, patch)
2011-12-19 20:31 PST, Jonathan Watt [:jwatt]
jwatt: review+
jwatt: checkin+
Details | Diff | Splinter Review
Implement option #1 from comment 32 (6.63 KB, patch)
2011-12-19 20:32 PST, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
mark tests as random (1.69 KB, patch)
2012-02-09 11:18 PST, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
Add support for a couple of live prefs to enable/disable the display list paths (3.12 KB, patch)
2012-05-16 09:45 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
Add support for a couple of live prefs to enable/disable the display list paths (4.43 KB, patch)
2012-05-16 18:58 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
Implement display lists for painting and hit-testing of SVG features involved in hit-testing (67.19 KB, patch)
2012-05-17 04:00 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
Implement display lists for painting and hit-testing of SVG features involved in hit-testing (63.75 KB, patch)
2012-06-27 08:28 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch - run reftests with SDL enabled, but their refs with SDL disabled (2.88 KB, patch)
2012-07-09 08:22 PDT, Jonathan Watt [:jwatt]
roc: review+
Details | Diff | Splinter Review
patch - Implement display list based painting and hit-testing for SVG (68.14 KB, patch)
2012-07-10 12:37 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch - Implement display list based painting and hit-testing for SVG (69.28 KB, patch)
2012-07-15 23:38 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch - Implement display list based painting and hit-testing for SVG (69.35 KB, patch)
2012-07-16 05:21 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch - flip prefs to enable SVG display lists (1016 bytes, patch)
2012-07-16 05:42 PDT, Jonathan Watt [:jwatt]
roc: review+
Details | Diff | Splinter Review
patch - Implement display list based painting and hit-testing for SVG (68.31 KB, patch)
2012-07-20 08:32 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2010-11-24 18:52:07 PST
There are potentially large performance wins for dynamic SVG here, plus it will help us unify SVG effects (including transforms etc) with non-SVG.
Comment 1 Phil Ringnalda (:philor) 2011-03-18 20:10:44 PDT
Betting you meant to take the bug, rather than the QA contact.
Comment 2 Jonathan Watt [:jwatt] 2011-05-22 07:31:35 PDT
Created attachment 534296 [details] [diff] [review]
Add --enable-svg-dlists option
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-22 11:05:03 PDT
Why do you want to add a configure option for this?
Comment 4 Jonathan Watt [:jwatt] 2011-05-22 15:16:35 PDT
To put this rewrite behind a flag so we have a strategy for regressions on our branches.
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-22 15:32:40 PDT
Comment on attachment 534296 [details] [diff] [review]
Add --enable-svg-dlists option

>@@ -6408,16 +6409,28 @@ fi
>+MOZ_SVG_DLISTS=
>+MOZ_ARG_ENABLE_BOOL(svg-dlists,
>+[  --enable-svg-dlists    Enable SVG display list support],
>+    MOZ_SVG_DLISTS=1,
>+    MOZ_SVG_DLISTS= )

You can do that by removing the above five lines, and then whatever MOZ_SVG_DLISTS is set to earlier in configure will be substituted into the makefiles and defined in the preprocessor if appropriate.

Adding visible configure options is generally a bad idea.  People will flip them for all sorts of ridiculous reasons.  Also, to disable on aurora or beta we'd have to change the mozconfigs the machines use, and it's easier just to change a MOZ_SVG_DLISTS=1 to MOZ_SVG_DLISTS=.
Comment 6 Daniel Holbert [:dholbert] 2011-05-22 15:43:58 PDT
(FWIW, khuey's suggestion is exactly what dbaron did for CSS animations. We've got...
> MOZ_CSS_ANIMATIONS=1
...in configure.in, and everything else goes off of that, with no --enable/disable mozconfig option.)
Comment 7 Jonathan Watt [:jwatt] 2011-05-23 16:31:33 PDT
Created attachment 534618 [details] [diff] [review]
Add MOZ_SVG_DLISTS flag

OK, that's fine by me.
Comment 8 Jonathan Watt [:jwatt] 2011-05-23 16:33:27 PDT
For those that are interested I've put my mercurial queue online at:

http://hg.mozilla.org/users/jwatt_jwatt.org/svg-display-list-patches/file/tip

It's not currently interesting, but I'll mention here when I push more interesting updates to it.
Comment 9 Jonathan Watt [:jwatt] 2011-05-23 16:34:37 PDT
Created attachment 534619 [details] [diff] [review]
Some doc changes
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-23 17:47:28 PDT
Comment on attachment 534619 [details] [diff] [review]
Some doc changes

Review of attachment 534619 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 11 Jonathan Watt [:jwatt] 2011-05-25 02:13:53 PDT
(In reply to comment #3)
> Why do you want to add a configure option for this?

So I guess the reason I made it a configure flag was because I wanted to have it turned it off by default, while avoiding having to touch configure.in to turn it on locally while developing. Having an mq patch that touches configure.in to turn it on means that every time I pull and rebuild I end up rebuilding everything because popping and pushing the queue touches configure.in. Any thoughts on how best to avoid this?
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-27 15:34:22 PDT
(In reply to comment #11)
> (In reply to comment #3)
> > Why do you want to add a configure option for this?
> 
> So I guess the reason I made it a configure flag was because I wanted to
> have it turned it off by default, while avoiding having to touch
> configure.in to turn it on locally while developing. Having an mq patch that
> touches configure.in to turn it on means that every time I pull and rebuild
> I end up rebuilding everything because popping and pushing the queue touches
> configure.in. Any thoughts on how best to avoid this?

There's no good way to avoid this.  You have the same problem if you add a flag, or do anything touching configure.in.
Comment 13 Jonathan Watt [:jwatt] 2011-08-11 07:37:31 PDT
Created attachment 552370 [details] [diff] [review]
Make sure SVG frames that shouldn't directly display don't create display list items

We can land this part early to simplify the following patches.
Comment 14 Robert Longson 2011-08-11 07:41:56 PDT
Wouldn't this repeated boilerplate be better as a macro?
Comment 15 Jonathan Watt [:jwatt] 2011-08-11 08:51:33 PDT
It's compact anyways, so I'd prefer to have the function names exposed clearly.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-11 16:07:32 PDT
Comment on attachment 552370 [details] [diff] [review]
Make sure SVG frames that shouldn't directly display don't create display list items

Review of attachment 552370 [details] [diff] [review]:
-----------------------------------------------------------------

It's not going to be boilerplate for long anyway.
Comment 17 Jonathan Watt [:jwatt] 2011-09-07 13:21:11 PDT
Created attachment 558923 [details] [diff] [review]
stop recalculating the contribution of markers on every GetCoveredRegion()

I'm changing mRect in SVG frames to hold values more compatible with box model layout, and as part of that I have two preliminary patches. This one moves the stuff in nsSVGPathGeometryFrame::GetCoveredRegion that should be in nsSVGPathGeometryFrame::UpdateCoveredRegion to the latter. We already have marker invalidation tests that should catch any problems, just in case that's useful for any potential future regression tracking.
Comment 18 Jonathan Watt [:jwatt] 2011-09-07 13:37:04 PDT
Created attachment 558935 [details] [diff] [review]
put all bounds calculation code in GetBBoxContribution in a way that it can be reused

This preliminary patch puts all bounds calculation code in the file nsSVGPathGeometryFrame.cpp under nsSVGPathGeometryFrame::GetBBoxContribution in a way that it can be reused. That requires adding a flags argument to GetBBoxContribution to allow callers to specify whether e.g. stroke or markers should also be taken into account. The plan for SVG 2 is to allow the DOM method getBBox() to also specify flags like these, so this work should be useful for that too.
Comment 19 Robert Longson 2011-09-07 14:28:43 PDT
Comment on attachment 558935 [details] [diff] [review]
put all bounds calculation code in GetBBoxContribution in a way that it can be reused

> EXPORTS = \
> 	nsSVGEffects.h \
> 	nsSVGFilterInstance.h \
> 	nsSVGForeignObjectFrame.h \
> 	nsSVGIntegrationUtils.h \
> 	nsSVGUtils.h \
>+	nsISVGChildFrame.h \
> 	$(NULL)

Why? All the changes are in layout/svg especially if you move the enum as suggested below.

>+  enum BBoxFlags {
>+    eBBoxIncludeFill          = 1 << 0,
>+    eBBoxIgnoreFillIfNone     = 1 << 1,
>+    eBBoxIncludeStroke        = 1 << 2,
>+    eBBoxIgnoreStrokeIfNone   = 1 << 3,
>+    eBBoxIncludeMarkers       = 1 << 4
>+  };
>+

Perhaps the enum would be better located in nsSVGUtils.h I really don't think we want to increase any use of nsISVGChildFrame.

> 
>+gfxRect
>+nsSVGMarkerFrame::GetMarksBBoxContrib(const gfxMatrix &aToBBoxUserspace,

I think we ought to make you type the word Contribution in full here.
Comment 20 Jonathan Watt [:jwatt] 2011-09-07 14:40:08 PDT
(In reply to Robert Longson from comment #19)
> Comment on attachment 558935 [details] [diff] [review]
> put all bounds calculation code in GetBBoxContribution in a way that it can
> be reused
> 
> > EXPORTS = \
> > 	nsSVGEffects.h \
> > 	nsSVGFilterInstance.h \
> > 	nsSVGForeignObjectFrame.h \
> > 	nsSVGIntegrationUtils.h \
> > 	nsSVGUtils.h \
> >+	nsISVGChildFrame.h \
> > 	$(NULL)
> 
> Why? All the changes are in layout/svg especially if you move the enum as
> suggested below.

Right, but the flags are used in nsSVGUtils.h, and that's used outside layout.

> >+  enum BBoxFlags {
> >+    eBBoxIncludeFill          = 1 << 0,
> >+    eBBoxIgnoreFillIfNone     = 1 << 1,
> >+    eBBoxIncludeStroke        = 1 << 2,
> >+    eBBoxIgnoreStrokeIfNone   = 1 << 3,
> >+    eBBoxIncludeMarkers       = 1 << 4
> >+  };
> >+
> 
> Perhaps the enum would be better located in nsSVGUtils.h I really don't
> think we want to increase any use of nsISVGChildFrame.

I'd suggest that we have another base class to replace nsISVGChildFrame, in which case the flags would go there. I think they belong in a frame class, but I guess they could go in nsSVGUtils.h if roc thinks so too.

> > 
> >+gfxRect
> >+nsSVGMarkerFrame::GetMarksBBoxContrib(const gfxMatrix &aToBBoxUserspace,
> 
> I think we ought to make you type the word Contribution in full here.

Not sure if that's a friendly jibe at my tendency to ask for names to be clear, or if you really feel that way. I didn't think spelling it out made it any clearer in this case, and since it would make the already messy calling points even messier, I didn't. Again though, I could change that.
Comment 21 Robert Longson 2011-09-07 15:03:34 PDT
> > 
> > I think we ought to make you type the word Contribution in full here.
> 
> Not sure if that's a friendly jibe at my tendency to ask for names to be
> clear, or if you really feel that way. I didn't think spelling it out made
> it any clearer in this case, and since it would make the already messy
> calling points even messier, I didn't. Again though, I could change that.

lil bit ;-) I do like consistency and as you've typed the word in full in other functions you should do so here too.
Comment 22 Jonathan Watt [:jwatt] 2011-09-07 15:13:51 PDT
OK. Done locally.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-07 17:06:45 PDT
Comment on attachment 558935 [details] [diff] [review]
put all bounds calculation code in GetBBoxContribution in a way that it can be reused

Review of attachment 558935 [details] [diff] [review]:
-----------------------------------------------------------------

I think the flags should be in nsSVGUtils. GetBBox is the primary API here, the rest are just helpers.

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +637,5 @@
>  }
>  
>  gfxRect
> +nsSVGGlyphFrame::GetBBoxContribution(const gfxMatrix &aToBBoxUserspace,
> +                                     PRUint32 aFlags)

Don't you need to change something here to get stroke extents taken into account?

::: layout/svg/base/src/nsSVGPathGeometryFrame.cpp
@@ +307,5 @@
> +      bbox = context->GetUserStrokeExtent();
> +      bbox += gfxPoint(bbox.width, bbox.height) / 2;
> +      bbox.SizeTo(gfxSize(0, 0));
> +    }
> +    bbox = nsSVGUtils::PathExtentsToMaxStrokeExtents(bbox, this);

I'm confused. I think this would be clearer if you actually took the union with the previous value of bbox here, and referred to fillExtents instead of bbox for the path extents.
Comment 25 Robert Longson 2011-09-08 14:15:18 PDT
Still patches left to land...
Comment 26 Jonathan Watt [:jwatt] 2011-09-29 18:03:10 PDT
Created attachment 563617 [details] [diff] [review]
put all bounds calculation code in GetBBoxContribution in a way that it can be reused

roc, does this address your comments?
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-29 19:40:10 PDT
Comment on attachment 563617 [details] [diff] [review]
put all bounds calculation code in GetBBoxContribution in a way that it can be reused

Review of attachment 563617 [details] [diff] [review]:
-----------------------------------------------------------------

great!
Comment 28 Jonathan Watt [:jwatt] 2011-09-30 04:05:46 PDT
Comment on attachment 563617 [details] [diff] [review]
put all bounds calculation code in GetBBoxContribution in a way that it can be reused

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/8ddd09649e13
Comment 30 Daniel Roesler 2011-11-02 01:15:37 PDT
Sorry if this is obvious, but I'm confused. Has this patch hit firefox-trunk?
Comment 31 Jonathan Watt [:jwatt] 2011-11-02 01:39:44 PDT
There are several patches attached to this bug which are listed above under "Attachments". Those with "checkin+" have been checked in. If what you really meant to ask was "is this bug finished", no, there are more patches still to come.
Comment 32 Jonathan Watt [:jwatt] 2011-11-30 09:30:21 PST
Created attachment 577996 [details] [diff] [review]
Kill covered regions (make mRect for SVG frames relative to user space)

This seems to pretty much work, but there is one rather significant problem. When an element moves, we need to invalidate its old and new area. Currently SVG doesn't separate these two steps very strongly; both steps happen in UpdateAndInvalidateCoveredRegion _after_ the attribute/property in question  has changed. We get away with this only because the bounds we store in mRect are relative to the frame's nsSVGOuterSVGFrame. Thus when UpdateAndInvalidateCoveredRegion is called we can still get the old nsSVGOuterSVGFrame relative bounds to invalidate it, update the bounds, then invalidate the new bounds. The problem is, by making the bounds stored in mRect relative to the SVG frame's userspace instead of the nsSVGOuterSVGFrame, we can't reliably get the old bounds relative to the nsSVGOuterSVGFrame any more. Transforming the old _userspace_ relative bounds by passing it up the parent chain is wrong, since the element's transform attribute or a transform anywhere along the parent chain could be the thing that has changed.

It seems like long term we still want mRect on SVG frames to contain the user space relative bounds, since once we're in a world where invalidation is done using display list comparison, we no longer care that we can't get the old bounds.

In the meantime this issue is still a problem though. I can see two ways to handle it.

1) We could maintain another nsRect on SVG leaf frames to store the old nsSVGOuterSVGFrame relative bounds. That means doing twice the work in UpdateCoveredRegion though.

2) We could make the internal types for SVG's DOM representations of its attributes (nsSVGLength2 et. al.) send an invalidation notification _before_ they are changed. That is, add WillChangeLength methods to compliment the DidChangeLength methods et. al.

Option #2 seems a bit insane since that would be a lot of work and be code we'd probably throw away once display list invalidation is implemented. So I'm thinking option #1.

roc?
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-30 15:07:57 PST
Option #1 sounds OK.
Comment 34 Jonathan Watt [:jwatt] 2011-12-06 09:20:50 PST
Ignore the two lines:

  // XXX maybe just pass destRect blindly if rendering is broken

I've removed those since no tests seem to fail as a result of that part of the patch.
Comment 35 Jonathan Watt [:jwatt] 2011-12-06 09:33:13 PST
Created attachment 579339 [details] [diff] [review]
Implement option #1 from comment 32

And here's a followup implementing option #1 from comment 32 to temporarily work around the invalidation issue introduced by killing SVG covered regions. This can die once bug 539356 is fixed.
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-06 15:52:06 PST
Comment on attachment 577996 [details] [diff] [review]
Kill covered regions (make mRect for SVG frames relative to user space)

Review of attachment 577996 [details] [diff] [review]:
-----------------------------------------------------------------

Basically looks good. In fact, surprisingly non-disruptive!

::: layout/svg/base/src/nsSVGImageFrame.cpp
@@ +391,5 @@
>          aContext->GetRenderingContext(this),
>          mImageContainer,
>          nsLayoutUtils::GetGraphicsFilterForFrame(this),
>          destRect,
> +        // XXX maybe just pass destRect blindly if rendering is broken

Not sure what this means

::: layout/svg/base/src/nsSVGUtils.h
@@ +445,5 @@
> +  // This is a temporary helper we should no longer need after bug 614732 is
> +  // fixed.
> +  static nsPoint
> +  TransformAppPoint(nsPoint aPoint, gfxMatrix aCanvasTM,
> +                    nsPresContext* aPresContext);

aFrame isn't a parameter here.

Also, this needs to say what it's transforming from and to. Say "TransformOuterSVGPointToInnerFrame", and call aCanvasTM aFrameToCanvasTM?

Use const gfxMatrix&.

@@ +449,5 @@
> +                    nsPresContext* aPresContext);
> +
> +  static nsRect
> +  LeafRectToCanvas(const nsRect& aRect, const gfxMatrix& aMatrix,
> +                   nsPresContext* aPresContext);

Same here ... TransformFrameRectToOuterSVG?
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-06 15:54:46 PST
Comment on attachment 579339 [details] [diff] [review]
Implement option #1 from comment 32

Review of attachment 579339 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +532,5 @@
>      mRect = nsSVGUtils::ToAppPixelRect(PresContext(), extent);
>    }
>  
> +  // See bug 614732.
> +  // Repeat, but this time with GetCanvasTM() for mCoveredRegion:

Why can't we just call LeafRectToCanvas on mRect here?

::: layout/svg/base/src/nsSVGImageFrame.cpp
@@ +500,5 @@
> +  context2.IdentityMatrix();
> +  extent = context.GetUserPathExtent();
> +  if (!extent.IsEmpty()) {
> +    mCoveredRegion = nsSVGUtils::ToAppPixelRect(PresContext(), extent);
> +  }

Again, why not just call LeafRectToCanvas on mRect?

::: layout/svg/base/src/nsSVGPathGeometryFrame.cpp
@@ +219,5 @@
> +  extent = GetBBoxContribution(GetCanvasTM(),
> +    nsSVGUtils::eBBoxIncludeFill | nsSVGUtils::eBBoxIgnoreFillIfNone |
> +    nsSVGUtils::eBBoxIncludeStroke | nsSVGUtils::eBBoxIgnoreStrokeIfNone |
> +    nsSVGUtils::eBBoxIncludeMarkers);
> +  mCoveredRegion = nsSVGUtils::ToAppPixelRect(PresContext(), extent);

Ditto
Comment 38 Helder "Lthere" Magalhães 2011-12-07 16:42:59 PST
Sorry if this sounds a bit noisy and/or off-scope. I've been working in an implementation of cross-browser zoom and pan controls (very preliminary demo available [1]) which uses 'currentScale' and 'currentTranslate' as basis. As I've been experiencing performance issues in Firefox (not in comparison with the competition but mostly with "good old" IE+ASV), I've been crawling for applicable issues and documentation... Maybe it's time to start asking questions:

1. Would a couple testcases be useful in the scope of this bug? Should a separate bug be created?

2. The docs at [2] state that 'nsDisplayTransform' changes are likely to improve manipulation of 'currentScale' and 'currentTranslate', right?

3. I'd expect that a change in 'currentTranslate' might be heavy as all elements transform change but profiling showed that changes in 'currentScale' seem to take similar amounts of time. Shouldn't a translate only invalidate elements near the viewport edges and simply move the canvas? Or is this a too naive approach?

4. In [3] one reads "Find cases of SVG where there are a lot of elements on the screen. How common is this?". I'd like to state that, even if there may not be lots of examples online, surely there are many use-cases which fit. I can at least recall schematic drawings (static, CAD-based, as well as dynamic, for monitoring [4] and such) and many clipart I've seen are in deed heavy duty. Just my 2 cents...


[1] http://heldermagalhaes.com/stuff/svg/demos/SVGObject-HTMLCanvasInteractors/
[2] https://wiki.mozilla.org/SVG:SVG-display-lists#nsDisplayTransform
[3] https://wiki.mozilla.org/SVG:SVG-display-lists#Follow-up_post-completion
[4] http://www.svgopen.com/2008/?section=abstracts_and_proceedings#paper_44
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2011-12-07 17:03:19 PST
Separate bug for particular performance issues, please.
Comment 40 Jonathan Watt [:jwatt] 2011-12-19 20:29:35 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> In fact, surprisingly non-disruptive!

Yes, pleasantly so. :)

> ::: layout/svg/base/src/nsSVGImageFrame.cpp
> @@ +391,5 @@
> >          aContext->GetRenderingContext(this),
> >          mImageContainer,
> >          nsLayoutUtils::GetGraphicsFilterForFrame(this),
> >          destRect,
> > +        // XXX maybe just pass destRect blindly if rendering is broken
> 
> Not sure what this means

I removed that locally (mentioned above), so ignore that.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> Comment on attachment 579339 [details] [diff] [review]
> Implement option #1 from comment 32
> 
> Why can't we just call LeafRectToCanvas on mRect here?

Uh, no good reason. (There's the bounds-of-bounds issue, but that's not a good one.)
Comment 41 Jonathan Watt [:jwatt] 2011-12-19 20:31:08 PST
Created attachment 583055 [details] [diff] [review]
Kill covered regions (make mRect for SVG frames relative to user space)
Comment 42 Jonathan Watt [:jwatt] 2011-12-19 20:32:22 PST
Created attachment 583056 [details] [diff] [review]
Implement option #1 from comment 32
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-02 14:38:37 PST
Can some of these patches be checked in?
Comment 44 Jonathan Watt [:jwatt] 2012-02-06 05:27:16 PST
I just need to fix some failures I got on try - I'll come back to that just as soon as I'm finished with the reviews for bug 629200.
Comment 45 Jonathan Watt [:jwatt] 2012-02-08 18:37:33 PST
I've resolved most of the issues on Try now, but there's one class of failure that needs some discussion. The kill-covered-regions patches no longer use the gfxCanvas to obtain covered regions directly in outer-<svg> device space. We now have an intermediate step whereby we calculate the user space bounds of the leaf, and then convert that to canvas space bounds. It turns out that this causes us to stop rendering SVG that contains large objects that are scaled down to fit within the SVG viewport.

Specifically it causes these tests to fail:

https://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/vector/empty/tall--cover--height.html?raw=1
https://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/vector/empty/tall--cover--width.html?raw=1
https://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/vector/empty/wide--cover--height.html?raw=1
https://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/vector/empty/wide--cover--width.html?raw=1

Taking tall--cover--height.html as an example...when nsSVGPathGeometryFrame::UpdateCoveredRegion() calls GetBBoxContribution(), it now returns a rect with zero width. This is because under GetBBoxContribution, when GeneratePath creates a path for the rect {x=0, y=0, width=214748365, height=1}, it now does so while passing the identity matrix to the gfxContext, not the canvasTM. As a result this super large width is not scaled down to be within cairo's bounds, and so when GetBBoxContribution calls context->GetUserPathExtent() it gets back a rect with zero width.

It's worth noting that these tests work in Chrome and Opera (probably IE too?). (You need to run them locally - mxr doesn't serve the embedded SVG correctly.)

I need to take a step back and think about this some more once I've had some sleep. Right now though, I'm not feeling very keen on going back to the bad old days of hitting cairo's 24.8 fixed point limit (~16777215) more often.
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 18:47:28 PST
I don't think we'll actually hit cairo's fixed-point limit since we pass floating-point coordinates to cairo which then get transformed to device space, so it should all work out in the end. The real issue is that we can only represent user-space values up to 2^30/60 in user space.

I don't think we should worry about this unless we see it in the wild. If you really want to deal with it, we could try setting width/height to nscoord_MAX and treating that as "covered region is infinite" in various places.
Comment 47 Jonathan Watt [:jwatt] 2012-02-09 04:07:35 PST
To clarify, the reason we don't paint is because if mRect/mCoveredRegion contains broken bounds, those bounds are likely not to overlap with the dirty rect passed into nsSVGUtils::PaintFrameWithEffects, and it will return early.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #46)
> I don't think we'll actually hit cairo's fixed-point limit since we pass
> floating-point coordinates to cairo which then get transformed to device
> space, so it should all work out in the end.

We actually do hit cairo's fixed point limit while calculating a leaf's user space bounds. To do so we currently paint to an offscreen (using ScreenReferenceSurface()) with the identity matrix, then ask that gfxContext for the bounds. We may pass floats to gfx at that point, but under gfx, cairo converts them to fixed points to do its calculations.

If fact cairo's limit on coordinates is 8,388,607 (I forgot about the sign bit in my previous comment). I just tested in gdb to confirm gfxContext::GetUserPathExtent() screws up if any points of the path our outside that limit.

> The real issue is that we can
> only represent user-space values up to 2^30/60 in user space.
> 
> I don't think we should worry about this unless we see it in the wild. If
> you really want to deal with it, we could try setting width/height to
> nscoord_MAX and treating that as "covered region is infinite" in various
> places.

The nscoord_MAX trick would work if gfxContext::GetUserPathExtent() returned the correct bounds for large objects and we could check if the bounds would overflow the nscoords. Unfortunately, since it doesn't, there's no easy solution were we to find that people start complaining a lot about our limits again.

Well, I guess we could feasibly add our own float/double-based code for calculating user space limits. That would be easier than it was previously where we would have had to calculate the limits with a transform.
Comment 48 Jonathan Watt [:jwatt] 2012-02-09 04:40:00 PST
Or if we really had to, we could do the double calculation I was doing prior to comment 37.
Comment 49 Jonathan Watt [:jwatt] 2012-02-09 11:18:14 PST
Created attachment 595826 [details] [diff] [review]
mark tests as random

I was going to change the large values in the viewBox in nonpercent-width-omitted-height-extreme-viewbox.svg and omitted-width-nonpercent-height-extreme-viewbox.svg from 2147483647 down to 8388607, but it seems from Waldo and the comment in the reftest.list that those tests are really there to check that the super large values don't cause divide by zero.
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-09 13:36:29 PST
(In reply to Jonathan Watt [:jwatt] from comment #47)
> Well, I guess we could feasibly add our own float/double-based code for
> calculating user space limits. That would be easier than it was previously
> where we would have had to calculate the limits with a transform.

Actually, as we switch platforms to use Azure instead of cairo, this problem will go away (except for where Azure uses cairo, but that will probably only be Linux/X).

Other backends may have their own issues, but hopefully they won't have this one.
Comment 51 Jonathan Watt [:jwatt] 2012-02-10 07:02:20 PST
(In reply to Jonathan Watt [:jwatt] from comment #49)
> Created attachment 595826 [details] [diff] [review]
> mark tests as random

I merged this into the 'Kill covered regions' patch for landing, since that's the one that causes the tests to fail.
Comment 53 Jonathan Watt [:jwatt] 2012-05-16 09:45:00 PDT
Created attachment 624422 [details] [diff] [review]
Add support for a couple of live prefs to enable/disable the display list paths

I've been working on the display list stuff, split into two main parts. The first part is all the SVG features that can be involved in hit test (to be clear, support for both hit testing _and_ painting for those features). The second part is everything else (so things like filters and other things that affect painting but don't play any part in hit-testing).

My intention is that we can turn on at least the first part ASAP and get eyes on some of this code while I'm finishing off the second part.

This patch adds support for a couple of live prefs so that we can enable/disable the display list path for hit testing and painting separately:

  svg.display-lists.hit-testing.enabled
  svg.display-lists.painting.enabled

I've also created a restartless add-on to make it easy to flip between the display list/non-display list code paths to make it easy to check for bugs:

  https://builder.addons.mozilla.org/user/jwatt/
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-16 15:25:46 PDT
(In reply to Jonathan Watt [:jwatt] from comment #53)
> I've been working on the display list stuff, split into two main parts. The
> first part is all the SVG features that can be involved in hit test (to be
> clear, support for both hit testing _and_ painting for those features). The
> second part is everything else (so things like filters and other things that
> affect painting but don't play any part in hit-testing).

That is a great idea!
Comment 55 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-16 18:21:04 PDT
Comment on attachment 624422 [details] [diff] [review]
Add support for a couple of live prefs to enable/disable the display list paths

Review of attachment 624422 [details] [diff] [review]:
-----------------------------------------------------------------

Use AddBoolVarCache?
Comment 56 Jonathan Watt [:jwatt] 2012-05-16 18:38:34 PDT
Does it matter? This is temporary, throw away code, it's already written, and there's very little of it.
Comment 57 Jonathan Watt [:jwatt] 2012-05-16 18:58:03 PDT
Created attachment 624624 [details] [diff] [review]
Add support for a couple of live prefs to enable/disable the display list paths

Ah heck, here you go. :)
Comment 58 Jonathan Watt [:jwatt] 2012-05-16 21:07:17 PDT
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b11e443f04
Comment 59 Jonathan Watt [:jwatt] 2012-05-17 04:00:21 PDT
Created attachment 624696 [details] [diff] [review]
Implement display lists for painting and hit-testing of SVG features involved in hit-testing

This is part 1 as described in comment 53, minus support for clipPath. The vast bulk of the work to get to this stage has been pushed off to other bugs, so this patch is pretty readable actually. Also, all patches that this patch strictly depends on other than the "Make sure SVG frames that shouldn't directly display don't create display list items" patch above have been landed now. (I plan to land that one together with this one when this one is ready to land.) So if anyone wants to help out with testing this work it shouldn't be too hard to apply. :-)

roc: requesting review from you just to get your eyes on it, although you may want the clipPath support before this lands? That probably makes logical sense.
Comment 60 Ryan VanderMeulen [:RyanVM] 2012-05-17 11:11:39 PDT
(In reply to Jonathan Watt [:jwatt] from comment #58)
> Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b11e443f04

http://hg.mozilla.org/mozilla-central/rev/d3b11e443f04
Comment 61 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-17 17:33:15 PDT
Comment on attachment 624696 [details] [diff] [review]
Implement display lists for painting and hit-testing of SVG features involved in hit-testing

Review of attachment 624696 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe rlongson should review this too?

::: content/svg/content/src/nsSVGSVGElement.cpp
@@ +1038,5 @@
>                    mCurrentScale != 1.0f));
>  
> +  if (hasChildrenOnlyTransform != mHasChildrenOnlyTransform) {
> +    // Reconstruct the frame tree to handle view creation/destruction and
> +    // stacking context changes:

Incorrect comment. Only stacking context changes matter now.

::: layout/base/nsDisplayItemTypes.h
@@ +44,5 @@
>   * 
>   * This is #included inside nsDisplayItem.
>   */
>  enum Type {
> +  // XXXSDL Do we need to add something here?

Drop this comment

::: layout/generic/nsFrame.cpp
@@ +136,5 @@
>  using namespace mozilla::layers;
>  using namespace mozilla::layout;
>  
> +bool NS_SVGDisplayListHitTestingEnabled();
> +bool NS_SVGDisplayListPaintingEnabled();

Just #include the right header

@@ +1487,5 @@
>  {
>    NS_PRECONDITION(aRect, "Must have aRect out parameter");
>  
> +  if ((!aDisp->IsAbsolutelyPositioned() &&
> +       GetType() != nsGkAtoms::svgInnerSVGFrame) ||

This is extremely hot code. We need to avoid this GetType() virtual call as much as possible. Let's move the NS_STYLE_CLIP_RECT check on mClipFlags up so it happens first.

Even then, I'm not too happy about the hackiness of using GetType(). Can we do IsFrameOfType(eSVG) or something like that instead?

@@ +1833,5 @@
>        nsSVGIntegrationUtils::GetRequiredSourceForInvalidArea(this, dirtyRect);
>    }
>  
>    // Mark the display list items for absolutely positioned children
> +  // XXXSDL not for SVG?

remove comment

@@ +1996,5 @@
>    nsIFrame* child = aChild;
>    if (child->GetStateBits() & NS_FRAME_TOO_DEEP_IN_FRAME_TREE)
>      return NS_OK;
>  
> +  bool notSVG = !(mState & NS_FRAME_SVG_LAYOUT);

negatives in names suck. Call it isSVG.

@@ +2003,5 @@
> +  // XXX It's not clear to me if/how some of the CSS properties handled in this
> +  // method should apply to SVG. Certainly 'float' and 'position' should not
> +  // apply and so should be ignored for SVG. For now I'm keeping things simple
> +  // by having a separate version of this function for SVG that we invoke here.
> +  // We probably want to do something to eliminate the code duplication though.

I'm confused as to why this is #if 0'ed out. I think we should simply be testing !isSVG where necessary.

Well actually I guess you do, so why not just remove this #if 0 block?

@@ +2072,5 @@
>    }
>  
>    // Mark the display list items for absolutely positioned children
> +  // XXXSDL Need this even for SVG in case there are foreignObjects somewhere
> +  // down in our descendants?

remove comment?

@@ +2126,5 @@
>  
> +  bool isPositioned = notSVG && disp->IsPositioned();
> +  if (isVisuallyAtomic || isPositioned || (notSVG && disp->IsFloating()) ||
> +      (child->GetType() == nsGkAtoms::svgInnerSVGFrame &&
> +       (disp->mClipFlags & NS_STYLE_CLIP_RECT)) ||

This is super-hot code. Avoid the GetType() call by testing NS_STYLE_CLIP_RECT first and clean it up as discussed above.

@@ +2265,5 @@
>  }
>  
> +nsresult
> +nsIFrame::BuildDisplayListForSVGChild(nsDisplayListBuilder*   aBuilder,
> +                                      nsIFrame*               aChild,

Remove this?

::: layout/generic/nsFrame.h
@@ +611,5 @@
>        }
>      }
> +
> +    if (aFrame->IsFrameOfType(eSVG)) {
> +      return false;

This is very hot code. Don't add a virtual call here. Can you test the SVG_LAYOUT state bit instead?

::: layout/svg/base/src/nsSVGContainerFrame.cpp
@@ +132,5 @@
> +  nsIFrame* kid = GetFirstPrincipalChild();
> +  while (kid) {
> +    nsresult rv = BuildDisplayListForChild(aBuilder, kid, aDirtyRect, aLists);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    kid = kid->GetNextSibling();

Call nsContainerFrame::BuildDisplayListForNonBlockChildren?

::: layout/svg/base/src/nsSVGForeignObjectFrame.cpp
@@ +196,5 @@
> +  while (kid) {
> +    nsresult rv = BuildDisplayListForChild(aBuilder, kid, aDirtyRect, aLists);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    kid = kid->GetNextSibling();
> +  }

Call nsContainerFrame::BuildDisplayListForNonBlockChildren?

::: layout/svg/base/src/nsSVGPathGeometryFrame.cpp
@@ +90,5 @@
> +    MOZ_COUNT_DTOR(nsDisplaySVGPathGeometry);
> +  }
> +#endif
> + 
> +  // XXXSDL TYPE_SVG_EVENT_RECEIVER? Or add a new type???

Add a new type!

Different classes should not share the same type.
Comment 62 Robert Longson 2012-05-18 02:30:59 PDT
Comment on attachment 624696 [details] [diff] [review]
Implement display lists for painting and hit-testing of SVG features involved in hit-testing

Do we have reftests that have filters that make shapes bigger? If not then we could really do with some.

>+    if (aChange & nsChangeHint_ChildrenOnlyTransform) {
>+      // See comment for similar abort in ProcessRestyledFrames.
>+      nsIFrame *f = aFrame->GetContent()->GetPrimaryFrame();

What's the line above doing? Under what circumstances is f different to aFrame?

Is there anything other than a viewBox that is a childrenOnlyTransform? If not then perhaps
calling it what it is i.e. using ViewBox rather than ChildrenOnly would be clearer.

>   /**
>    * Returns true if we're currently building a display list that's
>-   * directly or indirectly under an nsDisplayTransform or SVG
>-   * foreignObject.
>+   * directly or indirectly under an nsDisplayTransform.
>    */
>   bool IsInTransform() { return mInTransform; }

Could make the above const while you're here.

> bool
> nsIFrame::GetClipPropClipRect(const nsStyleDisplay* aDisp, nsRect* aRect,
>                               const nsSize& aSize) const
> {
>   NS_PRECONDITION(aRect, "Must have aRect out parameter");
> 
>-  if (!aDisp->IsAbsolutelyPositioned() ||
>+  if ((!aDisp->IsAbsolutelyPositioned() &&
>+       GetType() != nsGkAtoms::svgInnerSVGFrame) ||
>       !(aDisp->mClipFlags & NS_STYLE_CLIP_RECT))
>     return false;

What's going on here with the inner svg frame test?

>+nsresult
>+nsIFrame::BuildDisplayListForSVGChild(nsDisplayListBuilder*   aBuilder,
>+                                      nsIFrame*               aChild,
>+                                      const nsRect&           aDirtyRect,
>+                                      const nsDisplayListSet& aLists,
>+                                      PRUint32                aFlags) {
>+  // XXX Probably this SVG version of BuildDisplayListForChild could be
>+  // further simplified, but for now I'm trying to keep it relatively similar
>+  // to BuildDisplayListForChild in case we want to merge the two.
>+
>+  nsIFrame* child = aChild;
>+
>+  const nsStyleDisplay* disp = child->GetStyleDisplay();
>+  // true if this is a real or pseudo stacking context
>+  bool pseudoStackingContext = false;
>+  NS_ABORT_IF_FALSE(!(aFlags & DISPLAY_CHILD_FORCE_PSEUDO_STACKING_CONTEXT),
>+    "No such thing as a pseudo-stacking context in SVG");

The abort could come first as it doesn't require any of the variables above to be set.

>+  // Mark the display list items for absolutely positioned children
>+  //XXX backed out in a10dd1a539db child->MarkAbsoluteFramesForDisplayList(aBuilder, dirty);

What's the comment about? Probably best removed.

>+
>+  // Child is painted atomically if it's transformed, partially transparent, or
>+  // has SVG effects.
>+  // XXX IsTransformed() returns true if there's an implicit viewBox
>+  // transform, so this will make <svg> into a stacking context dependent on
>+  // whether it has a viewBox, which may seem unexpected to content authors.

How would content authors be aware of the stacking context?

>diff --git a/layout/svg/base/src/nsSVGInnerSVGFrame.cpp b/layout/svg/base/src/nsSVGInnerSVGFrame.cpp
>--- a/layout/svg/base/src/nsSVGInnerSVGFrame.cpp
>+++ b/layout/svg/base/src/nsSVGInnerSVGFrame.cpp
>@@ -84,16 +84,20 @@ nsSVGInnerSVGFrame::GetType() const
> 
> //----------------------------------------------------------------------
> // nsISVGChildFrame methods
> 
> NS_IMETHODIMP
> nsSVGInnerSVGFrame::PaintSVG(nsRenderingContext *aContext,
>                              const nsIntRect *aDirtyRect)
> {
>+  // XXXSDL We can currently still hit this if we are nondisplay SVG
>+  // e.g. if we're in a marker.

In that case you could check the NONDISPLAY_CHILD state bit and still have the abort.
Comment 63 Jonathan Watt [:jwatt] 2012-06-13 06:28:26 PDT
(In reply to Robert Longson from comment #62)
> Do we have reftests that have filters that make shapes bigger? If not then
> we could really do with some.

Sure, we have plenty of tests using feGaussianBlur filters, for example.

> >+    if (aChange & nsChangeHint_ChildrenOnlyTransform) {
> >+      // See comment for similar abort in ProcessRestyledFrames.
> >+      nsIFrame *f = aFrame->GetContent()->GetPrimaryFrame();
> 
> What's the line above doing? Under what circumstances is f different to
> aFrame?

See the comment that the comment refers you to. ;-) Specifically:

http://hg.mozilla.org/mozilla-central/annotate/964b11fea7f1/layout/base/nsCSSFrameConstructor.cpp#l7925

> Is there anything other than a viewBox that is a childrenOnlyTransform? If
> not then perhaps
> calling it what it is i.e. using ViewBox rather than ChildrenOnly would be
> clearer.

See the comment documenting nsSVGContainerFrame::HasChildrenOnlyTransform and its implementations (already in the tree) - in short, yes, there are other children-only transforms.

> > bool
> > nsIFrame::GetClipPropClipRect(const nsStyleDisplay* aDisp, nsRect* aRect,
> >                               const nsSize& aSize) const
> > {
> >   NS_PRECONDITION(aRect, "Must have aRect out parameter");
> > 
> >-  if (!aDisp->IsAbsolutelyPositioned() ||
> >+  if ((!aDisp->IsAbsolutelyPositioned() &&
> >+       GetType() != nsGkAtoms::svgInnerSVGFrame) ||
> >       !(aDisp->mClipFlags & NS_STYLE_CLIP_RECT))
> >     return false;
> 
> What's going on here with the inner svg frame test?

The CSS spec says the 'clip' property only applies to absolutely positioned elements, but the SVG spec says that on SVG elements it applies regardless. I'm obeying the CSS spec for outer-<svg>, since that's generally what we do for outer-<svg>, but the SVG spec for other SVG elements to which 'clip' applies. I've added a comment.
Comment 64 Robert Longson 2012-06-13 10:44:21 PDT
(In reply to Jonathan Watt [:jwatt] from comment #63)
> > > 
> > >-  if (!aDisp->IsAbsolutelyPositioned() ||
> > >+  if ((!aDisp->IsAbsolutelyPositioned() &&
> > >+       GetType() != nsGkAtoms::svgInnerSVGFrame) ||
> > >       !(aDisp->mClipFlags & NS_STYLE_CLIP_RECT))
> > >     return false;
> > 
> > What's going on here with the inner svg frame test?
> 
> The CSS spec says the 'clip' property only applies to absolutely positioned
> elements, but the SVG spec says that on SVG elements it applies regardless.
> I'm obeying the CSS spec for outer-<svg>, since that's generally what we do
> for outer-<svg>, but the SVG spec for other SVG elements to which 'clip'
> applies. I've added a comment.

And we can get here with any <svg> frame now, though not any svg frame?
Comment 65 Robert Longson 2012-06-13 12:55:53 PDT
Overall I'm OK with the patch.
Comment 66 Jonathan Watt [:jwatt] 2012-06-27 08:28:20 PDT
Created attachment 637128 [details] [diff] [review]
Implement display lists for painting and hit-testing of SVG features involved in hit-testing

Still cleaning this up, but here's a snapshot since Jet asked me to attach it to the bug.
Comment 67 Jonathan Watt [:jwatt] 2012-06-30 17:44:36 PDT
I've not been good at marking work I've spun out into separate bugs as blocking this bug - rectifying that now.
Comment 68 Jonathan Watt [:jwatt] 2012-07-09 08:22:13 PDT
Created attachment 640228 [details] [diff] [review]
patch - run reftests with SDL enabled, but their refs with SDL disabled

Running both reftests and their references with SVG display lists enabled is a great way to get false positives and think that tests are rendering correctly when in fact they are not. I've been working on this bug with this patch applied to enable SDL for reftest tests and disable SDL for their references to avoid that. It can't be checked in just yet, obviously, but soon after pushing the patches to add SDL support, I think we should also push this.
Comment 69 Jonathan Watt [:jwatt] 2012-07-09 08:25:18 PDT
(In reply to Jonathan Watt [:jwatt] from comment #68)
> soon after pushing the patches to add SDL support, I think we should also
> push this.

And then revert it some time later once we're confident that the display list code paths are working for SVG.
Comment 70 Jonathan Watt [:jwatt] 2012-07-10 12:37:32 PDT
Created attachment 640724 [details] [diff] [review]
patch - Implement display list based painting and hit-testing for SVG

This is really close now. I still need to fix bug 769970, and the nsSVGIntegrationUtils code in this patch isn't quite painting filters correctly for SVG just yet. All other blocker bugs that I've spun work out to are fixed or have patches awaiting review though, so I'm hoping to be ready for review on a fully working version of this patch in a day or two.

Ignore the XXXSDL comments.
Comment 71 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-10 16:44:16 PDT
Comment on attachment 640724 [details] [diff] [review]
patch - Implement display list based painting and hit-testing for SVG

Review of attachment 640724 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayItemTypes.h
@@ +60,2 @@
>    TYPE_SVG_OUTER_SVG,
> +  TYPE_SVG_OUTER_SVG2,

We need a better name here!

::: layout/generic/nsFrame.cpp
@@ +1458,5 @@
> +  if (!(aDisp->mClipFlags & NS_STYLE_CLIP_RECT) ||
> +      (!aDisp->IsAbsolutelyPositioned() &&
> +       (!(mState & NS_FRAME_SVG_LAYOUT) ||
> +        !(mContent->Tag() == nsGkAtoms::svg ||
> +          mContent->Tag() == nsGkAtoms::foreignObject)))) {

Maybe you can rewrite this to reduce the amount of negation and make this easier to understand.

@@ +1805,5 @@
>    }
>  
> +  if (!IsFrameOfType(eSVG) || mContent->Tag() == nsGkAtoms::foreignObject) {
> +    // Mark the display list items for absolutely positioned children.
> +    MarkAbsoluteFramesForDisplayList(aBuilder, dirtyRect);

Why do you need this conditional?

@@ +1921,5 @@
>     */
> +  else if (disp->mOpacity < 1.0f &&
> +           !((mState & NS_FRAME_SVG_LAYOUT) &&
> +             nsSVGUtils::CanOptimizeOpacity(this)) &&
> +           !resultList.IsEmpty()) {

Maybe you can rewrite this to reduce the amount of negation and make this easier to understand.

@@ +2030,5 @@
>    }
>  
> +  if (!isSVG || child->GetContent()->Tag() == nsGkAtoms::foreignObject) {
> +    // Mark the display list items for absolutely positioned children.
> +    child->MarkAbsoluteFramesForDisplayList(aBuilder, dirty);

I don't know why you need this conditional.
Comment 72 Jonathan Watt [:jwatt] 2012-07-15 23:38:01 PDT
Created attachment 642494 [details] [diff] [review]
patch - Implement display list based painting and hit-testing for SVG


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #71)
> > +  if (!IsFrameOfType(eSVG) || mContent->Tag() == nsGkAtoms::foreignObject) {
> > +    // Mark the display list items for absolutely positioned children.
> > +    MarkAbsoluteFramesForDisplayList(aBuilder, dirtyRect);
> 
> ...
>
> > +  if (!isSVG || child->GetContent()->Tag() == nsGkAtoms::foreignObject) {
> > +    // Mark the display list items for absolutely positioned children.
> > +    child->MarkAbsoluteFramesForDisplayList(aBuilder, dirty);
> 
> I don't know why you need this conditional.

It just seemed wasteful to be calling MarkAbsoluteFramesForDisplayList on SVG frames when we know we don't do absolute positioning in SVG.
Comment 73 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-16 05:01:40 PDT
(In reply to Jonathan Watt [:jwatt] from comment #72)
> It just seemed wasteful to be calling MarkAbsoluteFramesForDisplayList on
> SVG frames when we know we don't do absolute positioning in SVG.

MarkAbsoluteFramesForDisplayList just checks a state bit and bails out if it's not set, which it won't be for SVG, so it's highly unlikely there's any win from adding an additional shortcut, so we should we should keep the code simple.
Comment 74 Jonathan Watt [:jwatt] 2012-07-16 05:21:46 PDT
Created attachment 642540 [details] [diff] [review]
patch - Implement display list based painting and hit-testing for SVG


(In reply to Jonathan Watt [:jwatt] from comment #72)
> It just seemed wasteful to be calling MarkAbsoluteFramesForDisplayList on
> SVG frames when we know we don't do absolute positioning in SVG.

Hmm, I thought I checked and it did more, but yeah, the checks I added is pointless as you say.
Comment 75 Jonathan Watt [:jwatt] 2012-07-16 05:42:08 PDT
Created attachment 642546 [details] [diff] [review]
patch - flip prefs to enable SVG display lists

I think it would be a good idea to first land without these prefs enabled, get a nightly and a bunch of test runs checking that I've not broken the non-display list code paths, and then flip the prefs.
Comment 76 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-16 06:01:28 PDT
Comment on attachment 642494 [details] [diff] [review]
patch - Implement display list based painting and hit-testing for SVG

Review of attachment 642494 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGGraphicElement.cpp
@@ +173,5 @@
> +                                             nsChangeHint_UpdateTransformLayer));
> +      } else {
> +        // We changed to a zero length transform list, which is treated as no
> +        // transform.
> +        NS_UpdateHint(retval, nsChangeHint_ReconstructFrame);

What if we changed from a zero-length transform list to a real transform? What causes us to reconstruct then?

::: layout/generic/nsFrame.cpp
@@ +1443,5 @@
>  
>    return DisplayOutlineUnconditional(aBuilder, aLists);
>  }
>  
> +inline static bool IsClipPropClipableSVG(const nsIFrame *aFrame)

This name is pretty bad. Can you think of a better one?

@@ +1452,5 @@
> +  // the CSS spec for outer-<svg> (since that's what we generally do), but
> +  // obey the SVG spec for other SVG elements to which 'clip' applies.
> +  return (aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT) &&
> +    (aFrame->GetContent()->Tag() == nsGkAtoms::svg ||
> +     aFrame->GetContent()->Tag() == nsGkAtoms::foreignObject);

Avoid calling GetContent()->Tag() twice

@@ +2088,5 @@
> +  bool isPositioned = !isSVG && disp->IsPositioned();
> +  if (isVisuallyAtomic || isPositioned || (!isSVG && disp->IsFloating()) ||
> +      (isSVG &&
> +       (child->GetContent()->Tag() == nsGkAtoms::svg ||
> +        child->GetContent()->Tag() == nsGkAtoms::foreignObject) &&

Call your new function to simplify this code. Put the NS_STYLE_CLIP_RECT check first if you want to avoid the function call most of the time. Probably doesn't matter though.

::: layout/svg/base/src/nsSVGIntegrationUtils.cpp
@@ +448,5 @@
> +  nsPoint offsetWithoutSVGGeomFramePos = offset;
> +  nsPoint svgGeomFramePos;
> +  if (aFrame->IsFrameOfType(nsIFrame::eSVGGeometry)) {
> +    // SVG leaf frames apply their offset themselves, we need to unapply it at
> +    // various points below to prevent it being double counted.

We should stop them applying their offset themselves, right? How hard would that be?

@@ +456,5 @@
> +  offset = offset.ToNearestPixels(appUnitsPerDevPixel).
> +                    ToAppUnits(appUnitsPerDevPixel);
> +  offsetWithoutSVGGeomFramePos =
> +    offsetWithoutSVGGeomFramePos.ToNearestPixels(appUnitsPerDevPixel).
> +                                   ToAppUnits(appUnitsPerDevPixel);

this doesn't seem right. Snapping the two values independently could make them snap in different directions which would surely be wrong. I think we should probably try to get rid of these offset adjustments from here.

::: layout/svg/base/src/nsSVGOuterSVGFrame.cpp
@@ +584,5 @@
> +  if (aBuilder->IsPaintingToWindow()) {
> +    state.SetPaintingToWindow(true);
> +  }
> +
> +  mList.PaintForFrame(aBuilder, aContext, mFrame, nsDisplayList::PAINT_DEFAULT);

Why are we calling PaintForFrame here? We shouldn't be! The child display items should be painted directly!

@@ +687,5 @@
> +      NS_SVGDisplayListPaintingEnabled()) {
> +    // XXXjwatt maybe set nonContentList to nsnull in debug builds to catch any
> +    // parts of the SVG code that add to anything other than the content list.
> +    // We can't do that right now because of the way foreignObject is handled
> +    // (it adds to a non-content list).

We shouldn't need to worry about that, there's nothing wrong with having someone add non-content-list items.
Comment 77 Jonathan Watt [:jwatt] 2012-07-16 12:56:43 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76)
> > +inline static bool IsClipPropClipableSVG(const nsIFrame *aFrame)
> 
> This name is pretty bad. Can you think of a better one?

Hmm, I seem to be entering into an infinite "can you do better" loop here. :) Can I say "no"? It says what it is, and I can't think of another name that does that and isn't completely silly.

> Avoid calling GetContent()->Tag() twice

I'm happy to change that, but both GetContent() and Tag() are inline so I thought it was okay. I assumed that would allow the compiler to do that for me to save doing the deref'ing to NameAtom() twice.

> We should stop them applying their offset themselves, right? How hard would
> that be?

It would mean translating by the negative of their Position() in PaintSVG() and GetFrameForPoint(), and some other stuff I forget now. Last time I tried I decided it was easier to do it the way I currently have it, but maybe that's changed. I can try doing that and seeing how easy/ugly it is to work through the test failures.

> this doesn't seem right. Snapping the two values independently could make
> them snap in different directions which would surely be wrong. I think we
> should probably try to get rid of these offset adjustments from here.

Okay, I'll see what I can do with that.

> Why are we calling PaintForFrame here? We shouldn't be! The child display
> items should be painted directly!

For that I'd need to defer to the superclass' implementation of Paint(), but that's nsDisplayWrapList::Paint which just contains the line:

  NS_ERROR("nsDisplayWrapList should have been flattened away for painting");
Comment 78 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-16 20:56:00 PDT
(In reply to Jonathan Watt [:jwatt] from comment #77)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76)
> > > +inline static bool IsClipPropClipableSVG(const nsIFrame *aFrame)
> > 
> > This name is pretty bad. Can you think of a better one?
> 
> Hmm, I seem to be entering into an infinite "can you do better" loop here.
> :) Can I say "no"? It says what it is, and I can't think of another name
> that does that and isn't completely silly.

IsSVGContentWithCSSClip?

> > Avoid calling GetContent()->Tag() twice
> 
> I'm happy to change that, but both GetContent() and Tag() are inline so I
> thought it was okay. I assumed that would allow the compiler to do that for
> me to save doing the deref'ing to NameAtom() twice.

It's also simpler code to not repeat yourself there.

> > Why are we calling PaintForFrame here? We shouldn't be! The child display
> > items should be painted directly!
> 
> For that I'd need to defer to the superclass' implementation of Paint(), but
> that's nsDisplayWrapList::Paint which just contains the line:

No you don't.

Actually, you shouldn't build nsDisplayOuterSVGWithChildren at all, I think. It doesn't do anything except call SetPaintingToWindow(true) which doesn't really make sense since this context might not be used to paint some of the SVG children (e.g. children of transformed elements could be rendered to ThebesLayers using a different gfxContext). So in fact I think we need to revisit SVGAutoRenderState. I think instead of getting properties off the gfxContext we should get those properties off the nsDisplayListBuilder. paintingToWindow is already there, and I guess renderMode might need to be there too.
Comment 79 Jonathan Watt [:jwatt] 2012-07-20 08:32:55 PDT
Created attachment 644353 [details] [diff] [review]
patch - Implement display list based painting and hit-testing for SVG

This passes Try now.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76)
> What if we changed from a zero-length transform list to a real transform?
> What causes us to reconstruct then?

I changed IsSVGTransformed to just depend on the presence/absence of the attributes.

> @@ +448,5 @@
> > +  nsPoint offsetWithoutSVGGeomFramePos = offset;
> > +  nsPoint svgGeomFramePos;
> > +  if (aFrame->IsFrameOfType(nsIFrame::eSVGGeometry)) {
> > +    // SVG leaf frames apply their offset themselves, we need to unapply it at
> > +    // various points below to prevent it being double counted.
> 
> We should stop them applying their offset themselves, right? How hard would
> that be?

Actually, this is hard. The knock on affects go deep into the SVG code to a depth that I haven't found the bottom of yet. The SVG code in general doesn't know about painting with ToReferenceFrame offsets. I guess that's why you solved the problem using a translation when you originally wrote the nsSVGIntegrationUtils code. Maybe I can do that as a follow-up?
Comment 80 Jonathan Watt [:jwatt] 2012-07-20 09:36:29 PDT
Regarding the IsPseudoStackingContextFromStyle XXXSDL comment, David says just to remove that and add a comment to IsPseudoStackingContextFromStyle stating that it doesn't apply to SVG, so that's what I've done locally.
Comment 81 Jonathan Watt [:jwatt] 2012-07-20 11:53:36 PDT
Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/62f19ed60528
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0e3950eb0c

I've split out the flipping of the prefs to enable SVG display lists into bug 776054 since enabling SVG display lists will apparently clash with bug 755084, and roc has asked me not to get in its way since it's high priority.

For anyone wanting to play with SVG display lists in advance, the prefs you need to flip to |true| are:

  svg.display-lists.hit-testing.enabled
  svg.display-lists.painting.enabled
Comment 83 Mathieu Pellerin 2012-09-05 19:17:16 PDT
Does this bug is in any way related to bug 788093?

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