There are potentially large performance wins for dynamic SVG here, plus it will help us unify SVG effects (including transforms etc) with non-SVG.

Betting you meant to take the bug, rather than the QA contact.

Why do you want to add a configure option for this?

To put this rewrite behind a flag so we have a strategy for regressions on our branches.

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=.

(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.)

OK, that's fine by me.

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 on attachment 534619 [details] [diff] [review] Some doc changes Review of attachment 534619 [details] [diff] [review]: -----------------------------------------------------------------

(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?

(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.

We can land this part early to simplify the following patches.

Wouldn't this repeated boilerplate be better as a macro?

It's compact anyways, so I'd prefer to have the function names exposed clearly.

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.

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.

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 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.

(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.

> > > > 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.

OK. Done locally.

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.

Still patches left to land...

roc, does this address your comments?

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 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

Sorry if this is obvious, but I'm confused. Has this patch hit firefox-trunk?

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.

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?

Option #1 sounds OK.

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.

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 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 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

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

Separate bug for particular performance issues, please.

(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.)

Can some of these patches be checked in?

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.

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.

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.

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.

Or if we really had to, we could do the double calculation I was doing prior to comment 37.

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.

(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.

(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.

https://hg.mozilla.org/mozilla-central/rev/a439c947dc99 https://hg.mozilla.org/mozilla-central/rev/073b91c24a1e

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/

(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 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?

Does it matter? This is temporary, throw away code, it's already written, and there's very little of it.

Ah heck, here you go. :)

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b11e443f04

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.

(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 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 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.

(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.

(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?

Overall I'm OK with the patch.

Still cleaning this up, but here's a snapshot since Jet asked me to attach it to the bug.

I've not been good at marking work I've spun out into separate bugs as blocking this bug - rectifying that now.

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.

(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.

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 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.

(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.

(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.

(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.

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 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.

(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");

(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.

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?

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.

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

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

Does this bug is in any way related to bug 788093?