Overflow areas for intermediate frames in a preserve-3d context are complicated, because they need to be represented in 3d to be meaningful. If you have two nested 90degree rotations around the X axis, then the overflow area at the intermediate stage is empty in 2d, and only useful in 3d. Our current strategy for dealing with this is to transform coordinates from a preserve-3d leaf all the way up to the root of the preserve-3d context, and then subtract the frame offsets to convert back into the child's coordinate space. This has the nice property that collecting child overflow areas to compute the overflow area of a parent just involves taking the overflow area of the child and offsetting it into local coordinate space (like we do for normal children everywhere else). The down side is that this stored overflow area on all but the root preserve-3d frame isn't really meaningful in its own right, and although the numbers are often useful we can't rely on it. Most code that would normally use these areas already has to deal with this possibility (see [1]), but places that don't cause corner case bugs that are hard to track down. The code for computing all the overflow areas is pretty horribly complex too. I think we instead should make it that preserve-3d frames only contribute to the overflow area of the preserve-3d root. A preserve-3d leaf would have a normal pre-transform overflow area, but empty normal overflow areas. Intermediate frames have the pre-transform area containing all non-transformed children, but again an empty normal overflow area. Only the root preserve-3d frame gets the overflow area of all the transformed content. This is a lot simpler to compute and understand, and it flushes out the subtle bugs by making them fail on every case. The following patches clean up a bunch of code that has been annoying me, and implements this. [1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/layout/generic/nsFrame.cpp#2326

With an offset of nsPoint(0, 0), BASIS_AT_ORIGIN and OFFSET_BY_ORIGIN are the same thing (a no-op). This switches all the callers of the former to the latter so we can get rid of it later, and removes some unused paramters.

This no longer seems to be necessary (the reftest passes, and I tested manually with paint flashing). I suspect it was [1] that made the transform code less insane and fixed this. [1] https://hg.mozilla.org/mozilla-central/rev/885889d182fd

Comment on attachment 8755404 [details] [diff] [review] Part 1: Remove callers of BASIS_AT_ORIGIN Review of attachment 8755404 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ -5326,2 @@ > nullptr, aOutAncestor); > - // XXXjwatt: seems like this will double count offsets in the face of preserve-3d: This isn't a problem since this caller doesn't pass INCLUDE_PRESERVE3D_ANCESTORS.

This doesn't add much value for a single caller.

Before this patch: All callers now pass OFFSET_BY_ORIGIN, and we switch to BASIS_AT_ORIGIN when recursing for preserve-3d frames. GetAccumulatedPreserve3DTransform passes in the offset from the current frame to the reference frame of the preserve-3d root, and all callers pass nsPoint(0,0). If we take an example where we have 3 nested preserve-3d frames then the GetAccumulatedPreserve3DTransform code is passing in the sum of ToReferenceFrame() for each of the 3 (since all preserve-3d frames are reference frames and thus ToReferenceFrame() is the same as frame->GetPosition() for the inner 2). GetResultingTransformMatrixInternal computes the combined transform as: frame1transform * (frame1offset+frame2offset+frame3offset) * -(frame2offset+frame3offset) * frame2transform * (frame2offset+frame3offset) * -(frame3offset) * frame3transform * (frame3offset) The first term only has one translation (since it's OFFSET_BY_ORIGIN), and the rest have positive/minus pairs (since they're BASIS_AT_ORIGIN). We subtract the frame->GetPosition() from our offset each time we recurse, which drops one offset each time. This simplifies to: frame1transform * frame1offset * frame2transform * frame2offset * frame3transform * frame3offset. This has the effect of taking coordinates in the coordinate space for children of frame 1, and converting them into the post-transform display-list coordinate space for frame 3. This matches what we do when creating layers (compute each frame's transform separately and offset by ToReferenceFrame() - see nsDisplayTransform::GetTransform). So far so good. For the callers passing 0,0 as the offset we get: frame1transform * (0) * (frame1offset) * frame2transform * -(frame1offset) * (frame1offset+frame2offset) * frame3transform * -(frame1offset+frame2offset) This simplifies to: frame1transform * frame1offset * frame2transform * frame2offset * frame3transform * -(frame1offset+frame2offset) This looks really weird, but actually matches what I described in comment #0 as the desired output when computing overflow areas, which explains how things manage to work currently. Other callers that don't expect this (like nsDisplayListBuilder::GetPreserves3DDirtyRect which is used for a call to UntransformRect) have to manually move us back into a sane coordinate space. Yet others (like UnionBorderBoxes) are probably just broken with preserve-3d. This patch reorders things so we just compute the sane transform that we want the first time without the simplification. The default behaviour is to output a transform in the local coordinate space of frame3, and you manually pass in the value of frame3offset if you want that appended to the end. This removes the crazyness from GetPreserves3DDirtyRect and changes GetAccumulatedPreserve3DTransform to only pass in frame3offset instead of the sum of the three. It also kills BASIS_AT_ORIGIN since that's no longer used (and was hard to understand). It does however break the overflow rect callers (since they really did want a result shifted back to the child position), but the next patch fixes that.

As above, the overflow rects for preserve-3d hid subtle bugs. When we build nsDisplayOpacity in the middle of a preserve-3d context, then we shouldn't rely on the overflow areas to be correct.

Please see comment 0, and the comments in the bug for a full description of what this is doing and why.

Comment on attachment 8755405 [details] [diff] [review] Part 2: Remove unnecessary optimization combining translations Review of attachment 8755405 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsDisplayList.cpp @@ -5693,5 @@ > - !hasPerspective) { > - // We can fold the final translation by roundedOrigin into the first matrix > - // basis change translation. This is more stable against variation due to > - // insufficient floating point precision than reversing the translation > - // afterwards. How does removing this avoid regressing bug 1008915?

Comment on attachment 8755415 [details] [diff] [review] Part 4: Reorder how we compute transforms for preserve-3d Review of attachment 8755415 [details] [diff] [review]: ----------------------------------------------------------------- I don't know this code very well and this is some pretty tricky stuff. How good is our test coverage in this area? Is it worth going through the history of this code and manually checking that we're not regressing any old fixed bugs?

(In reply to Markus Stange [:mstange] from comment #10) > Comment on attachment 8755415 [details] [diff] [review] > Part 4: Reorder how we compute transforms for preserve-3d > > Review of attachment 8755415 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't know this code very well and this is some pretty tricky stuff. How > good is our test coverage in this area? Is it worth going through the > history of this code and manually checking that we're not regressing any old > fixed bugs? I'll get thinker to review this as well, since I agree that it's tricky and not very well understood (by anyone). I like to think the result of the patch is much easier to follow though. We failed a lot of tests when I made mistakes writing this, so I think test coverage is decent these days. I definitely don't want to uplift this though. (In reply to Markus Stange [:mstange] from comment #9) > Comment on attachment 8755405 [details] [diff] [review] > Part 2: Remove unnecessary optimization combining translations > > Review of attachment 8755405 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsDisplayList.cpp > @@ -5693,5 @@ > > - !hasPerspective) { > > - // We can fold the final translation by roundedOrigin into the first matrix > > - // basis change translation. This is more stable against variation due to > > - // insufficient floating point precision than reversing the translation > > - // afterwards. > > How does removing this avoid regressing bug 1008915? I mentioned a changeset in comment 2, the full explanation for that changeset is in bug 1168263 comment 54. That simplified the transform code significantly, and I believe made it no longer necessary.

Comment on attachment 8755415 [details] [diff] [review] Part 4: Reorder how we compute transforms for preserve-3d Review of attachment 8755415 [details] [diff] [review]: ----------------------------------------------------------------- It looks good for me with the changes of part 1 ~ 3.

Comment on attachment 8755419 [details] [diff] [review] Part 6: Make preserve-3d frames only contribute to the overflow area of the preserve-3d root frame This patch could have used more detailed description of what it was doing in terms of code changes. But since I've delayed a week, I'd feel bad about just cancelling the request and asking for that at this point. >+ if (ChildrenHavePerspective() && sizeChanged) { >+ nsRect newBounds(nsPoint(0, 0), aNewSize); >+ RecomputePerspectiveChildrenOverflow(this, &newBounds); >+ } I guess the logic for not doing the same thing for children with perspective (making them have no transform, and then recomputing it in the parent) is that it wouldn't really be any simpler, and the parent's size is sometimes right to start with? (Still, do we sometimes end up with too much overflow on the parent as a result of the child having the wrong overflow the first time around?) Still, it seems like some of the same logic might apply to perspective, no? (Changing that could certainly be a separate bug, though.) > nsRect childVisual; > nsRect childScrollable; These are now unused; remove them. My memory of how transform matrices work for 3D-transformed frames (i.e., that they include the transform up to the root of the 3D context) agrees with what this is doing, although I didn't check it. I'm happy to see this change; this approach makes more sense than the old way. r=dbaron

Comment on attachment 8755405 [details] [diff] [review] Part 2: Remove unnecessary optimization combining translations Was my explanation in comment 11 sufficient? We can try chat on irc or skype if you'd prefer.

Comment on attachment 8755405 [details] [diff] [review] Part 2: Remove unnecessary optimization combining translations Ok good. Simpler code + the reftest passes so I'm happy. Sorry for forgetting about this patch.

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e081fc6f3f90 Part 1: Remove callers of BASIS_AT_ORIGIN. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/56ededfbdee6 Part 2: Remove unnecessary optimization combining translations. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/bb0230c4bb6d Part 3: Remove aOutAncestor. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/9cda37e5a581 Part 4: Reorder how we compute transforms for preserve-3d. r=mstange,thinker https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac50a46557b Part 5: Don't trust the visible region for opacity within preserve-3d. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/15b5f8019521 Part 6: Make preserve-3d frames only contribute to the overflow area of the preserve-3d root frame. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/c5c9bc65a408 Part 7: Clean up unecessary parameter for RecomputePerspectiveChildrenOverflow. r=dbaron

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bd393975aadd. So far for sure, Win7 and Win8 opt https://treeherder.mozilla.org/logviewer.html#?job_id=29381541&repo=mozilla-inbound. Maybe, dunno yet, https://treeherder.mozilla.org/logviewer.html#?job_id=29381248&repo=mozilla-inbound

And https://treeherder.mozilla.org/logviewer.html#?job_id=29382278&repo=mozilla-inbound. Do snakebites come in threes?

More than threes. I think I'm going to be able to not star anything else in there as fixed-by-commit that isn't yours, so call https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&tochange=bd393975aadd0769b1d67b595470d7b002fc15f4&fromchange=c5c9bc65a408fa0c26ec742c0950b6fc484cb635&filter-failure_classification_id=2 the things I think are yours.

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6168f50aef0 Part 1: Remove callers of BASIS_AT_ORIGIN. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/c1132f9a4432 Part 2: Remove unnecessary optimization combining translations. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/5a3e1572fbd2 Part 3: Remove aOutAncestor. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/175627605d6b Part 4: Reorder how we compute transforms for preserve-3d. r=mstange,thinker https://hg.mozilla.org/integration/mozilla-inbound/rev/346262b1c844 Part 5: Don't trust the visible region for opacity within preserve-3d. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/3c80b670cfe6 Part 6: Make preserve-3d frames only contribute to the overflow area of the preserve-3d root frame. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/656235d7f868 Part 7: Clean up unecessary parameter for RecomputePerspectiveChildrenOverflow. r=dbaron

https://hg.mozilla.org/mozilla-central/rev/d6168f50aef0 https://hg.mozilla.org/mozilla-central/rev/c1132f9a4432 https://hg.mozilla.org/mozilla-central/rev/5a3e1572fbd2 https://hg.mozilla.org/mozilla-central/rev/175627605d6b https://hg.mozilla.org/mozilla-central/rev/346262b1c844 https://hg.mozilla.org/mozilla-central/rev/3c80b670cfe6 https://hg.mozilla.org/mozilla-central/rev/656235d7f868