Advanced Layers is having trouble rendering two of our 3d transform tests. It looks like layout computes the wrong visible region for one of the container layers, which causes a good chunk of the intended draw area to be clipped. The existing compositor accidentally bypasses this problem via PostProcessLayers [3]. It accumulates child visible regions for occlusion culling and effectively ignores the container's visible region. [1] https://hg.mozilla.org/mozilla-central/raw-file/eac0c056235e/layout/reftests/transform-3d/opacity-preserve3d-1-ref.html [2] https://hg.mozilla.org/mozilla-central/raw-file/eac0c056235e/layout/reftests/transform-3d/backface-visibility-3.html [3] http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/gfx/layers/composite/LayerManagerComposite.cpp#363

The outer transform here is adding a scale factor of 2x to all child layers, and we're not taking that into account when converting app units to pixels. This is relatively rare, since most preserve-3d content has complex (perspective) transforms that disable our attempts to propagate scale factors down.

Comment on attachment 8867587 [details] [diff] [review] scale-preserve-3d Review of attachment 8867587 [details] [diff] [review]: ----------------------------------------------------------------- |ScaleToOutsidePixels()| is spread over the functions. I think it might be possible and worth to move all scaling to |PostProcessRetainedLayers()|.

The reason we do the scaling during FrameLayerBuilder, is because we try push scale factors down the tree to leaves, and if we end up having a scale on a PaintedLayer, then we adjust the resolution that we do the rasterization at. We can't do this in the compositor as rasterization has already be finished at this point (though this might no longer be true in a future WebRender world). We also try compute accurate visible/opaque regions on layers, so that we can do occlusion between layers that share the same AGR (like having a video occlude a PaintedLayer behind it). We then repeat this work on the compositor (in PostProcessLayers), once any async changes have been applied, so that we can compute the final occluded regions and do minimal compositing.

This fixes a bug with layout/reftests/transform-3d/opacity-preserve3d-1-ref.html where we computed the visible regions for layers wrong, but then PostProcessLayers (in the compositor) fixed it again. David's AdvancedLayers doesn't use PostProcessLayers so it exposed this issue. The problem is that our checks to see if we're preserving 3d use the frame parent, so for positioned frames jump directly to the containing block. In the testcase, the containing block has transform-style:'preserve-3d', but the style for the parent element has opacity:0.5 (and no preserve-3d). At the frame level we thought we were preserving 3d (and computed overflow areas accordingly), but display list building correctly inserts the flattening nsDisplayOpacity. The confusion between these two caused us to compute incorrect visibility values. Frame tree: https://pastebin.mozilla.org/9021813 Note that the block frames at lines 38 and 40 both think that they're 'preserve-3d' even though they are within the opacity (line 24).

Confirmed that with these patches AL renders both test cases correctly.

Did you happen to check to see what effect this has on webrender-enabled reftests? opacity-preserve3d-1.html is currently failing with webrender enabled and this might help make it pass.

(In reply to Matt Woodrow (:mattwoodrow) from comment #3) > The reason we do the scaling during FrameLayerBuilder, is because we try > push scale factors down the tree to leaves, and if we end up having a scale > on a PaintedLayer, then we adjust the resolution that we do the > rasterization at. We can't do this in the compositor as rasterization has > already be finished at this point (though this might no longer be true in a > future WebRender world). I meant to move calls to |ScaleToOuterPixels()| to |PostProcessRetainedLayers()|, not to remove scaling parameters.

So what's supposed to happen if the parent is something (e.g., scroll frame, multicol, table cell) that has a wrapper frame between it and its children? (Scroll frame might be a bad example because I think it flattens 3-D, but I don't think multicol or table cell do.) I'm worried about that because there's a discrepancy between the title of your patch (which seems to claim that you want the DOM parent) and the code that it contains (which only traverses to the parent of the placeholder, which could still be the frame for an anonymous box like ::-moz-cell-content).

Right, that's a good point. Should I be doing GetContent()->GetParent()->GetPrimaryFrame() instead? Or possibly walking up the frame tree (via placeholders) until I find the nearest ancestor that belongs to a different element (and isn't anonymous)? It seems like those two options would give different results, not sure which one I'd actually want here.

(In reply to Matt Woodrow (:mattwoodrow) from comment #9) > Right, that's a good point. > > Should I be doing GetContent()->GetParent()->GetPrimaryFrame() instead? Or > possibly walking up the frame tree (via placeholders) until I find the > nearest ancestor that belongs to a different element (and isn't anonymous)? I suspect maybe GetContent()->GetFlattenedTreeParent()->GetPrimaryFrame() is better than your first option. It's not immediately clear to me in what cases it would be different from the second. It feels more straightforward than trying to walk through the tree, though. Note that you need to null-check the result of GetFlattenedTreeParent(), so it's not *quite* that expression.

The previous patch wasn't correct (and failed a reftest) because the scale factors on the current ContainerState are for the layers within the outer ContainerLayer. That's coordinate *outside* of the transform of the nsDisplayTransform/ContainerLayer we just built. We're trying to use the visible area for the children, which is coordinate within the transform, so we want to scale by the scale factors chosen by the ContainerState we recursed into. We don't have access to those, and it's not easy to get them. How about instead we just disable scaling for preserve-3d (slightly more than we do already), and then we don't need to scale. This changed the fuzzy slightly on my machine and made the test still fail, so I just simplified the fuzzy condition.

Reimplemented using your suggestion. We still need to figure out what to do for bug 1362543 (which will likely change this exact same code again), and probably add some new tests matching whatever behavior we go with.

Comment on attachment 8867587 [details] [diff] [review] scale-preserve-3d Review of attachment 8867587 [details] [diff] [review]: ----------------------------------------------------------------- Here is some explanations of what is wrong in the previous patch. I don't spend much time on this bug, so not sure if I have missed anything. ::: layout/painting/FrameLayerBuilder.cpp @@ +4372,5 @@ > item->Frame()->HasPerspective()))) { > // Give untransformed visible region as outer visible region > // to avoid failure caused by singular transforms. > newLayerEntry->mUntransformedVisibleRegion = true; > + newLayerEntry->mVisibleRegion = ScaleToOutsidePixels(item->GetVisibleRectForChildren()); |mUntransformedVisibleRegion| being true means the |mVisibleRegion| is already in the coordination within the container layer, it is also what |GetVisiblRectForChildren()| is defined. In another word, it would not be projected back, calling |ProjectRectBounds()|. So, |ScaleToOutsidePixels()| is unnecessary here. However, |GetVisibleRectForChildren()| does not count the pre-scale of |ownLayer|, should be a container layer, here. So, what we need to do is to apply reversed pre-scale on the value returned by |GetVisibleRectForChildren()|. @@ +4385,5 @@ > bool useChildrenVisible = > itemType == nsDisplayItem::TYPE_TRANSFORM && > (item->Frame()->IsPreserve3DLeaf() || > item->Frame()->HasPerspective()); > + const nsIntRegion &visible = useChildrenVisible ? ScaleToOutsidePixels(item->GetVisibleRectForChildren()) : |useChildrenVisible|, here, means the same thing as |mUntransformedVisibleRegion| above.

I can not provide a useful review on this patch.

The bug is that we are currently converting app units to pixels without taking the scaling factor into account. The initial patch used the scaling factor for the ContainerLayer outside the transformed layer, yet we're converting coordinates for the visible region which is within the transformed layer. What we really need to know is what scale factors were picked by the ContainerState within the transformed layer, but that's no longer on the stack at the point this code runs. The fix is either to add new plumbing to report back the scale factors chosen, or just to change ChooseScaleAndSetTransform such that it always chooses a scaling of 1.0 in this situation. I went for the latter, since it's much simpler.

(In reply to Matt Woodrow (:mattwoodrow) from comment #15) > What we really need to know is what scale factors were picked by the > ContainerState within the transformed layer, but that's no longer on the > stack at the point this code runs. Are |Layer::GetPreXScale()| and |Layer::GetPreYScale()| not what you need?

Comment on attachment 8868378 [details] [diff] [review] fix-scaling-preserve-3d Review of attachment 8868378 [details] [diff] [review]: ----------------------------------------------------------------- I believe you and it does seem very simple.

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3eec11c6b37 Use the DOM-ordering parent frame when deciding if a frame combines its transform with ancestors. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea810dbc4af Scale visible region for preserve-3d layers correctly. r=thinker

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4f29bdedb50c Remove failing annotation for WR since it no longer fails

sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=105399904&repo=mozilla-inbound&lineNumber=2699

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/08d8f714ec4d Backed out changeset 4f29bdedb50c https://hg.mozilla.org/integration/mozilla-inbound/rev/91ce014868c5 Backed out changeset 5ea810dbc4af https://hg.mozilla.org/integration/mozilla-inbound/rev/6a84d1b25433 Backed out changeset a3eec11c6b37 for reftest failures in group-opacity-surface-size-1.html

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3074555ab01 Use the DOM-ordering parent frame when deciding if a frame combines its transform with ancestors. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8e12048edb Scale visible region for preserve-3d layers correctly. r=thinker

Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/16167ba06cbb Remove failing annotation for WR since it no longer fails

https://hg.mozilla.org/mozilla-central/rev/d3074555ab01 https://hg.mozilla.org/mozilla-central/rev/7c8e12048edb https://hg.mozilla.org/mozilla-central/rev/16167ba06cbb

Matt, could this have caused bug 1397671?

Ni? to dbaron, who looks like initial reviewer. Do you know if this could've caused the regression in bug 1397671?

Seems likely.