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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Hack

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.

Comment 2

[Timothy Nikkel (:tnikkel)]

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?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch to repro failure on OS X

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 7

[Timothy Nikkel (: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.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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

Comment 9

[Timothy Nikkel (:tnikkel)]

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

Comment 10

[Timothy Nikkel (:tnikkel)]

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.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: patch

This basically extends bug 1008915 to the preserve 3d case.

Comment 12

[Matt Woodrow (:mattwoodrow)]

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.

Comment 13

[Timothy Nikkel (:tnikkel)]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f239e122ce45