User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/534.52.7 (KHTML, like Gecko) Version/5.1.2 Safari/534.52.7 Steps to reproduce: Combining CSS 3D perspective + overflow: scroll in WebKit browsers results in a fluid parallax effect which lends a holographic quality to the page, particularly when using analog scrolling. This effect can enhance the reader's intuitive understanding of 3D graphs (i.e. math, science, engineering) and is preferable to a JavaScript / WebGL effect for responsiveness and efficiency reasons. See: http://acko.net/files/dump/parallax.html Trying to do the same in Firefox 10 beta fails. Actual results: The full length of the scrollable element is used to calculate the field-of-view, which looks unnatural and skewed. The effective FOV approaches 180 degrees, and the perspective origin is located halfway down the page. Additionally, rendering on OS X is so choppy as to make the page unusable. Expected results: The field of view of the perspective should be calculated based on the scrollable viewport, and the perspective origin should be locked in a 'position: fixed' manner. Rendering should be smooth and hardware accelerated.

Seeing this with Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:13.0a1) Gecko/20120208 Firefox/13.0a1 ID:20120208031253 too.

This seems to be multiple different bugs and should be filed as such.

Aaron: It's a single use case, achieved by combining two CSS properties on a parent with CSS 3D elements inside. Just because fixing it implies tweaks in multiple locations doesn't mean there shouldn't be an umbrella bug to track if this stuff is actually useful. User-centric vs code-centric development. It's also not very useful to state a bug should be split up without saying how it should be split up.

A valid point, but since bugzilla is primarily a developer tool it is usually aimed at being code-centric. Anyway, the incorrect rendering is because of bug 721082, which I've just put a fix up for. This gets Firefox rendering the page identically to WebKit for me. Lets focus this bug on the poor performance when scrolling this complex 3d screen. I hope that these 2 bugs cover everything from the original report. Profiling data coming soon. :)

Fix for an obvious pain point. With this patch applied, scrolling is somewhat improved. There seems to be two main cases, scrolling with the mouse pointer over the main content, or scrolling with the mouse at the edge of the page where it doesn't hit any interesting content. The former is *much* slower and what I've been looking at when profiling. Almost 60% of the profile is spent under nsLayoutUtils::GetFramesForArea. 34% building display lists, and 23% under HitTest. I can't figure out how to get Instruments to tell me the cumulative time for a function, but I can see at least two callers to nsDisplayTransform::GetResultingTransformMatrix at 10% of the profile each. I'm going to look and see if we can cache the results of this on the frame instead of the display item.

It sounds like you're hitting processing of synthetic mouse events? Do you have bug 675015 fixed in your build? that should be suppressing those events while you scroll.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > It sounds like you're hitting processing of synthetic mouse events? Do you > have bug 675015 fixed in your build? that should be suppressing those events > while you scroll. I do have that changeset in my tree, I still seem to be getting GetFramesForArea called from [ChildView scrollWheel].

I'm less convinced with this patch, it doesn't help all that much. Recursing into GetResultingTransformMatrix (as we do with preserve-3d) is more expensive than reading the actual transformed. Also, can the style context or pres context change for a frame? The caching is assuming these will always be constant.

Comment on attachment 595870 [details] [diff] [review] Use cached transform as much as possible Review of attachment 595870 [details] [diff] [review]: ----------------------------------------------------------------- Can we get rid of UntransformRect entirely?

(In reply to Matt Woodrow (:mattwoodrow) from comment #9) > Also, can the style context or pres context change for a frame? The caching > is assuming these will always be constant. Prescontext can't change. Style context definitely can change. DidSetStyleContext gets called when that happens.

(In reply to Matt Woodrow (:mattwoodrow) from comment #9) > Created attachment 595873 [details] [diff] [review] > Cache the result of ReadTransforms on a frame > > I'm less convinced with this patch, it doesn't help all that much. Recursing > into GetResultingTransformMatrix (as we do with preserve-3d) is more > expensive than reading the actual transformed. > > Also, can the style context or pres context change for a frame? The caching > is assuming these will always be constant. So what's the problem here? Deeply nested transforms? Can you explain what the remaining problems are, assuming the reviewed patches are applied?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > Can we get rid of UntransformRect entirely? Not quite, it's still used in BuildDisplayListForStackingContext - before we create the nsDisplayTransform. The main high level problem, is that we're spending a ridiculous amount of time doing hittest during scrolling. 67% of my last profile to be precise. That's split between generating a new display list (39%), and running HitTest on it(25%). This is a fairly complex 3d scene, so i think transform is always going to slow up fairly heavily here. It's definitely not only transforms - nsFrame::DisplayBorderBackgroundOutline during display list construction is 6.3% (and that's from one caller, there might be more instance of it). In terms of transform specific things, we're at least calculating the transforms twice during this process. Once during display list construction as mentioned above, and then we compute it again during HitTest. We could try make these share the transform, but we don't have an nsDisplayTransform to store the cache after the first computation, and it's computed with a different offset, so the actual transform is different. Most of the transforms here are nested 3 deep, and we're doing things like checking for possible perspective at all 3 levels (I'll email www-style and ask about this - the spec isn't clear). For this particular test, the ideal behaviour would be have the entire nested transform cached apart from the perspective component since it's the only piece that should change. This seems like really complex caching behaviour to implement, since any of the 3 frames could also have animated transforms, changing size etc. There's other small things, like detecting simple transforms and special casing the multiplication - Applying the perspective could probably do this. Though it appears that GetParentStyleContextFrame() is one of the single most expensive functions we call when generating transforms.

(In reply to Matt Woodrow (:mattwoodrow) from comment #13) > The main high level problem, is that we're spending a ridiculous amount of > time doing hittest during scrolling. 67% of my last profile to be precise. > That's split between generating a new display list (39%), and running > HitTest on it(25%). How can we be spending more time building the display list for hit-testing than for painting the window? Especially given the latter builds a display list for the entire window, but the former only builds a display list for a single point (or very small rectangle)? > In terms of transform specific things, we're at least calculating the > transforms twice during this process. Once during display list construction > as mentioned above, and then we compute it again during HitTest. We could > try make these share the transform, but we don't have an nsDisplayTransform > to store the cache after the first computation, and it's computed with a > different offset, so the actual transform is different. Now I'm really confused. What's computed with a different offset?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > (In reply to Matt Woodrow (:mattwoodrow) from comment #13) > > The main high level problem, is that we're spending a ridiculous amount of > > time doing hittest during scrolling. 67% of my last profile to be precise. > > That's split between generating a new display list (39%), and running > > HitTest on it(25%). > > How can we be spending more time building the display list for hit-testing > than for painting the window? Especially given the latter builds a display > list for the entire window, but the former only builds a display list for a > single point (or very small rectangle)? My only guess is that we're calling it much more frequently? > > > In terms of transform specific things, we're at least calculating the > > transforms twice during this process. Once during display list construction > > as mentioned above, and then we compute it again during HitTest. We could > > try make these share the transform, but we don't have an nsDisplayTransform > > to store the cache after the first computation, and it's computed with a > > different offset, so the actual transform is different. > > Now I'm really confused. What's computed with a different offset? When calculating transforms, we passing the offset from the frame origin to the actual origin - Usually the result of ToReferenceFrame(). In this case we pass nsPoint(0,0) because the rect we're transforming is in a different coordinate space.

We are indeed calling it much more frequently, up to 16x as much in some cases. Filed bug 725851 for this issue.

We probably should also work on making these hit-test calculations faster, so that document.elementFromPoint is efficient. What is the size of the display list generated each time? It must be fairly small, surely, for the single point? I still don't understand why this would be slow.

When we have preserve-3d, we're dumping the passed in dirty rect and taking the overflow area. Aside from being way too big, I think this is still just-plain-wrong since the child transforms in a preserve-3d hierarchy are relative to the parent of the topmost frame. This passes the transformed rect down normally (so that's its used for untransformed children as expected), and also passes down the untouched dirty rect for preserve-3d children to work with. Scrolling performance is much much improved.

Comment on attachment 595926 [details] [diff] [review] Calculate correct dirty rects during display list construction for preserve-3d frames Review of attachment 595926 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +1422,5 @@ > bool aIgnoreRootScrollFrame) > { > nsresult rv; > nsAutoTArray<nsIFrame*,8> outFrames; > + printf("GetFrameForPoint\n"); Oops, consider this gone :)

Comment on attachment 595926 [details] [diff] [review] Calculate correct dirty rects during display list construction for preserve-3d frames Review of attachment 595926 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +1747,5 @@ > return NS_OK; > > bool applyAbsPosClipping = > ApplyAbsPosClipping(aBuilder, disp, this, &absPosClip); > + nsRect dirtyRect = Preserves3D() ? aBuilder->GetPreserve3DRect() : aDirtyRect; Instead of doing this, couldn't we just mess with the dirty rect computed by nsDisplayTransform::UntransformRect below so that we don't untransform when the child needs the same dirty rect? @@ +1754,5 @@ > bool inTransform = aBuilder->IsInTransform(); > if ((mState & NS_FRAME_MAY_BE_TRANSFORMED) && > disp->HasTransform()) { > + if (!hitTestRect && > + nsDisplayTransform::ShouldPrerenderTransformedContent(aBuilder, this)) { Instead of hitTestRect, you should make this test aBuilder->IsForPainting(). @@ +1764,4 @@ > if (!nsDisplayTransform::UntransformRect(dirtyRect, this, nsPoint(0, 0), &dirtyRect)) { > + // we have a singular transform - surely we must be drawing nothing. > + dirtyRect.SetEmpty(); > + } else if (hitTestRect) { Do we need this "if (hitTestRect)" path?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > Instead of doing this, couldn't we just mess with the dirty rect computed by > nsDisplayTransform::UntransformRect below so that we don't untransform when > the child needs the same dirty rect? Children of this frame that aren't part of the preserve-3d chain want the transformed region though. But since the transform could be singular (in 2d), yet still be valid for further transformed preserve-3d children, the transformed rect could be empty. I'm not sure what to do here, because the current code will detect this empty rect as not intersecting with children, and won't even get to the right child. This patch causes a few reftests failures, they all look like this same underlying issue. > > @@ +1754,5 @@ > > bool inTransform = aBuilder->IsInTransform(); > > if ((mState & NS_FRAME_MAY_BE_TRANSFORMED) && > > disp->HasTransform()) { > > + if (!hitTestRect && > > + nsDisplayTransform::ShouldPrerenderTransformedContent(aBuilder, this)) { > > Instead of hitTestRect, you should make this test aBuilder->IsForPainting(). Didn't know this existed, will do. > > @@ +1764,4 @@ > > if (!nsDisplayTransform::UntransformRect(dirtyRect, this, nsPoint(0, 0), &dirtyRect)) { > > + // we have a singular transform - surely we must be drawing nothing. > > + dirtyRect.SetEmpty(); > > + } else if (hitTestRect) { > > Do we need this "if (hitTestRect)" path? Not with the above change

(In reply to Matt Woodrow (:mattwoodrow) from comment #21) > Children of this frame that aren't part of the preserve-3d chain want the > transformed region though. I see. OK. Tricky... Perhaps we can adapt MarkAbsoluteFramesForDisplayList: walk the frame tree and mark preserve-3d frame chains that we have to "force-traverse"? The same mechanism can also store custom dirty rects for those frames.

https://tbpl.mozilla.org/?tree=Try&rev=0c95268d2d11 for first two patches

Reviewed try push with roc and decided that the first two parts can land: http://hg.mozilla.org/integration/mozilla-inbound/rev/3cf4980b7bd1 http://hg.mozilla.org/integration/mozilla-inbound/rev/371abe433192

https://hg.mozilla.org/mozilla-central/rev/3cf4980b7bd1 https://hg.mozilla.org/mozilla-central/rev/371abe433192

Comment on attachment 595873 [details] [diff] [review] Cache the result of ReadTransforms on a frame With this patch I see perf improvement for http://romaxa.info/css3d/cube3d/cube.html test, - ~4% CPU

Comment on attachment 595926 [details] [diff] [review] Calculate correct dirty rects during display list construction for preserve-3d frames with this patch cube.html render text(numbers) only for 1 and 2, other sides are empty

Comment on attachment 595926 [details] [diff] [review] Calculate correct dirty rects during display list construction for preserve-3d frames This causes reftest failures: REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/extra/checkouts/mozilla-central/layout/reftests/transform-3d/preserve3d-1a.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/extra/checkouts/mozilla-central/layout/reftests/transform-3d/preserve3d-2b.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/extra/checkouts/mozilla-central/layout/reftests/transform-3d/preserve3d-4a.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/extra/checkouts/mozilla-central/layout/reftests/transform-3d/preserve3d-5a.html | image comparison (==) (Also note that both of the two remaining patches conflict with bug 727805 and bug 731959, which renamed some variables and methods, so they need to be tweaked so that they apply.)

Oops, the reftest failures were already noted in comment 21. Sorry, never mind me.

We're just waiting for Matt to have some time to get back to finish this.

Bit of a rewrite, since I kept hitting issues with the existing approach. This seems to work nicely, and improves performance considerably. It's still not fantastic, but the hit testing display lists are reasonable now. We could take the intersection of overflow/GetDirtyRect() and untransform that, to get a smaller overflow rect. Since it'll be a rect guaranteed to be within the bounds of the layer, this should work correctly. I doubt this is worth the cost of a matrix inversion though.

Useful debugging code.

Comment on attachment 620550 [details] [diff] [review] Print 3d transform properties on nsBlockFrame Review of attachment 620550 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsBlockFrame.cpp @@ +447,5 @@ > + if (ChildrenHavePerspective()) { > + fprintf(out, " perspective"); > + } > + if (Preserves3DChildren()) { > + fprintf(out, " preserves-3d"); preserves-3d-children No reason to not be consistent here. @@ +450,5 @@ > + if (Preserves3DChildren()) { > + fprintf(out, " preserves-3d"); > + } > + if (Preserves3D()) { > + fprintf(out, " preserving-3d"); preserves-3d

Comment on attachment 620549 [details] [diff] [review] Calculate correct dirty rects during display list construction for preserve-3d frames v2 Review of attachment 620549 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsDisplayList.h @@ +145,5 @@ > PLUGIN_GEOMETRY, > OTHER > }; > + nsDisplayListBuilder(nsIFrame* aReferenceFrame, const nsRect& aDirtyRect, > + Mode aMode, bool aBuildCaret); What is aDirtyRect relative to? Document it. @@ +191,5 @@ > * establishes the coordinate system for the display list > */ > nsIFrame* ReferenceFrame() const { return mReferenceFrame; } > + > + const nsRect& GetDirtyRect() const { return mDirtyRect; } Document its coordinate system here too @@ +358,5 @@ > const nsFrameList& aFrames, > const nsRect& aDirtyRect); > + > + void MarkPreserve3DFramesForDisplayList(nsIFrame* aDirtyFrame, > + const nsRect& aDirtyRect); And what is this dirty rect relative to? You should explain what this method does.

Comment on attachment 620568 [details] [diff] [review] Calculate correct dirty rects during display list construction for preserve-3d frames v3 Review of attachment 620568 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsDisplayList.h @@ +134,5 @@ > /** > * @param aReferenceFrame the frame at the root of the subtree; its origin > + * is the origin of the reference coordinate system for this display list. > + * @param aDirtyRect Dirty rect of the display list, relative to > + * aReferenceFrame. Better also explain how we use this; that content outside the dirty rect is not rendered. But is that true? Because in nsGfxScrollFrame we render outside the dirty rect due to displayports! So what exactly does the dirty rect mean? I think displayports could be a problem with this approach... @@ +365,5 @@ > + * Mark all child frames that Preserve3D() as needing display. > + * Because these frames include transforms set on their parent, dirty rects > + * for intermediate frames may be empty, yet child frames could still be visible. > + */ > + void MarkPreserve3DFramesForDisplayList(nsIFrame* aDirtyFrame); Call this "MarkPreserve3DChildrenForDisplayList"

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36) > Better also explain how we use this; that content outside the dirty rect is > not rendered. But is that true? Because in nsGfxScrollFrame we render > outside the dirty rect due to displayports! So what exactly does the dirty > rect mean? I think displayports could be a problem with this approach... Hrm, good point. Not sure what to do here. My other approach had real issues with absolute position frames. The idea was to save the untransformed dirty rect in BuildDisplayListForStackingContext, and the retrieve it in BuildDisplayListForChild. Child frames that aren't part of the preserve-3d chain would just use the transformed dirty rect. Absolutely positioned frames also use a saved rect in BuildDisplayListForChild, and don't adjust it by the childs offset, normally done here: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1974 This means that the final rect passed to the leaf node in the preserve-3d chain (shifted by aBuilder->ToReferenceFrame()), is no longer in the viewport coordinate system, and our intersection tests fail. I tried moving the offset to below the retrieval of the placeholder dirty rect, which fixed my bug and created a whole lot of new ones.

Just to clarify, we need a dirty rectangle in screen coordinates for preserve-3d since perspective transforms depend on the x/y position for determining the size.

(In reply to Matt Woodrow (:mattwoodrow) from comment #37) > The idea was to save the untransformed dirty rect in > BuildDisplayListForStackingContext, and the retrieve it in > BuildDisplayListForChild. That sounds like the right approach. Can't you make the dirty rect relative to the child frame before saving it on the frame? That's what nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay does.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39) > Can't you make the dirty rect relative to the child frame before saving it > on the frame? That's what nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay > does. The problem is with the saved absolute rect, rather than the saved preserve-3d rect. I've attached a simplified patch to explain this better. In this we don't ever untransform for Preserves3DChildren() and pass the dirty rect on unmodified. This is correct for the transformed frames, and doesn't require saving an extra rect, but wrong for other child frames that aren't part of the preserve-3d hierarchy. That shouldn't matter for the moment. When we hit a node (Preserves3D()), we take the dirty rect and offset it by aBuilder->ToReferenceFrame(this). This normally gives us a rect in the reference frame coordinates, and looks like nsIntRect(0, 65, width, height). We intersect this against the transformed overflow (again, in reference frame coordinates) and decide if it hits. The above works for normal cases, but breaks down when the Preserve3D() is absolutely positioned. Taken from the above testcase: Dirty rect (and the offset we would apply) to the parent of the node, looks correct. (gdb) p aDirtyRect $8 = (const nsRect &) @0x7fff5fbec588: { <mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = { x = -32790, y = -7620, width = 101580, height = 58260 }, <No data fields>} (gdb) p aBuilder->ToReferenceFrame(aChild) $9 = (const nsPoint &) @0x7fff5fbfac08: { <mozilla::gfx::BasePoint<int, nsPoint>> = { x = 32790, y = 11520 }, <No data fields>} Absolute positioned rect we load from the frame property: (gdb) p dirty $27 = { <mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = { x = -5042, y = 2966, width = 1460, height = 1813 }, <No data fields>} (gdb) p aBuilder->ToReferenceFrame(child) $25 = (const nsPoint &) @0x7fff5fbfac08: { <mozilla::gfx::BasePoint<int, nsPoint>> = { x = 49890, y = 19620 }, <No data fields>} Adding these together doesn't give us a rect with TopLeft() of 0,65 and thus we incorrectly decide the frame isn't visible some times. So, assuming that the rect computed ($27) is correct, how do I compute the screen space rect from it?

My reduced test case showing the incorrect dirty rect being calculated and the shape disappearing when it shouldn't.

Is the problem that nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay needs to take transforms into account when converting the parent's dirty rect to be relative to the child?

Moved the saved rect loading (In BuildDisplayListForChild) to after the out of flow rect loading. The out of flow rect isn't right for preserve-3d children, since it's intersected with the frames overflow rect. This doesn't really work with our overflow areas for preserve-3d (since they aren't really in the right coordinate space). Another reason to switch to ns3DRect for overflow areas!

Comment on attachment 622229 [details] [diff] [review] Calculate correct dirty rects during display list construction for preserve-3d frames v4 Review of attachment 622229 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +1782,5 @@ > + nsRect overflow = GetVisualOverflowRectRelativeToSelf(); > + overflow += aBuilder->ToReferenceFrame(this); > + overflow = nsDisplayTransform::TransformRect(overflow, this, aBuilder->ToReferenceFrame(this)); > + > + dirtyRect += aBuilder->ToReferenceFrame(this); make only 1 call to ToReferenceFrame(this) instead of 3

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfd45c1b48e https://hg.mozilla.org/integration/mozilla-inbound/rev/287dc7b3152b

https://hg.mozilla.org/mozilla-central/rev/3bfd45c1b48e https://hg.mozilla.org/mozilla-central/rev/287dc7b3152b