Closed Bug 1107843 Opened 7 years ago Closed 7 years ago

Event regions display items affect rendering in the presence of preserve-3d transforms

Categories

(Core :: Layout, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: kats, Assigned: tnikkel)

References

Details

Attachments

(5 files)

Attached file Display list dumps
+++ This bug was initially created as a clone of Bug #1107280 +++

Breaking out the reftest failure for layout/reftests/bugs/1081185-1.html from bug 1107280 to a new bug.

What appears to be happening in this case is that the event regions display items are messing with the WrapPreserve3DListInternal function. It's not clear to me what this function does but I'm attaching some display list dumps of the layout/reftests/bugs/1081185-1.html page with and without event regions enabled. When event regions are enabled there are two nsDisplayTransform items instead of just one; it appears that the extra one is created to wrap an event regions item.
Attached patch HackSplinter Review
The attached patch makes the test failure go away on my local OS X machine, but the patch is almost certainly not correct since it basically discards the event-regions item that causes the new nsDisplayTransform to appear. I assume that instead of discarding it we really want it to just sit in the WrapList without getting wrapped into an nsDisplayTransform.
Not wrapping the layer event regions items in a display transform item would be wrong because the regions on the layer event regions item are indeed transformed.

Why does the existence of these extra items cause the reftest to fail?
Attached file Layer dumps
Yeah that's a good point. Looking further it looks like the extra nsDisplayTransform triggers the creation of a new ClientTiledPaintedLayer, which in turns causes the page to render with an intermediate surface. I'm attaching layer dumps too.

Without event regions the layer 0x11d313000 which has the 0.5 opacity has no intermediate surface, and the corresponding layer with event regions does have an intermediate surface, and an extra child layer.
The code inside the loop in nsDisplayList::ComputeVisibilityForSublist looks almost identical to the code in nsDisplayItem::RecomputeVisibility, except that when nsDisplayItem::ComputeVisibility returns false the sublist version doesn't reset mVisibleRect back to empty. Do you know if that's intentional?
This is the patch I'm using locally to reproduce this failure on OS X. After applying this patch you can run the test using:

mach reftest --filter 1081185 layout/reftests/bugs/reftest.list
tn, can you take this whenever you get back? I'm not making much progress here because I don't understand conceptually what the correct behaviour in this case is. It's not clear to me if the event regions item should end up creating a new painted layer or not and if so, if the intermediate surface behaviour is correct as well.
Flags: needinfo?(tnikkel)
Whatever the bug I don't think it's related to event regions at all. You can get the same bug without event regions by just putting a div inside the "bottom" div. And I have just reproduced with event regions disabled completely. I think it's a general bug with how we compute visible regions; it is quite different when we have preserve3d (like in the test) and when we have a non-preserve3d transform like in the reference.
The different in my mind is that when you add another div, it's expected to change the layer structure and result in an intermediate surface being used, because both divs have visible content and need to be layered just so with preserve-3d and opacity. Event regions however have no visible content and so should not cause an intermediate surface to be used because it's a waste of memory.

Turning on event regions causes the condition at [1] to be true, because the container layer has multiple children with non-empty visible regions. If the visible region of the layer with just the event regions were empty this wouldn't happen. If you think it's correct for event regions to affect the layer's visible region then this is not a bug and we can just fuzz the test.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp?rev=c080f66ed9d2#1061
(I meant to post this before comment 8 but it got mid-aired and I didn't notice until much later when I came back to the tab.)

I think what's happening is in the preserve3d case we pass down the dirty region directly to the child and use that to calculate the dirty rect for the "bottom" div transform. But in the non-preserve3d case we transform the dirty region for the perspective, then transform that again for the transform on the "bottom" div. Transforming the dirty region twice leads to slight inaccuracies and we end up drawing one less row or column of pixels.
The dirty region for the leaf div is the same in both cases, so I think the difference is down to the visible rect we set on the transform item.
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel)
Attached patch patchSplinter Review
This basically extends bug 1008915 to the preserve 3d case.
Attachment #8556745 - Flags: review?(matt.woodrow)
Comment on attachment 8556745 [details] [diff] [review]
patch

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

::: layout/base/nsDisplayList.cpp
@@ +5217,5 @@
> +    if (aOffsetByOrigin) {
> +      result.Translate(-aProperties.mToTransformOrigin);
> +      result.TranslatePost(offsetBetweenOrigins);
> +    } else {
> +      result.ChangeBasis(offsetBetweenOrigins);        

Trailing whitespace.
Attachment #8556745 - Flags: review?(matt.woodrow) → review+
Fixed the whitespace. This only seems to have fixed the fuzzy on Windows, so I left the b2g fuzzy. The issue I was debugging on mac must be the issue we have on Windows, but not the b2g issue.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f239e122ce45
https://hg.mozilla.org/mozilla-central/rev/f239e122ce45
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.