1130681 - layout/reftests/position-dynamic-changes/vertical/topA-heightN-bottomA.html? Failing with vsync refresh driver

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

From https://treeherder.mozilla.org/#/jobs?repo=try&revision=65c501042f81

Reftest R15 failing with error:

http://10.0.2.2:8888/tests/layout/reftests/position-dynamic-changes/vertical/topA-heightN-bottomA.html?margin_abspos | image comparison (==), max difference: 255, number of differing pixels: 3000

Find and fix the problem

Comment 1

[Mason Chang [Inactive] [:mchang]]

We're failing with a differing number of pixels = 3000 rather than 1000.

Comment 2

[Mason Chang [Inactive] [:mchang]]

From recent commit:

73b8ce222f9f: Bug 1129360 - backout the part of the patch from bug 1127289 that causes intermittent r14 failures on the emulator. r=me

Repushed to try with latest master to see if these issues are resolved.

Comment 3

[Mason Chang [Inactive] [:mchang]]

From https://treeherder.mozilla.org/#/jobs?repo=try&revision=579a1053bc44

The orange factor got a little better from the latest m-c with 227931:f8ce4cf2e71d, but still pretty bad :/

Comment 4

[Mason Chang [Inactive] [:mchang]]

Hey Nical, any idea what we should be looking for in the R14/R15 reftest failures? Thanks!

Comment 5

[Nicolas Silva [:nical]]

(In reply to Mason Chang [:mchang] from comment #4)
> Hey Nical, any idea what we should be looking for in the R14/R15 reftest
> failures? Thanks!

I have no clue. This mysteriously started with a patch from bug 1127289 but persisted after the backout. R14 intermittents are blocking the landing of DrawTargetTiled on b2g so I'll probably see clearer as I keep investigating on that front.

Comment 6

[Mason Chang [Inactive] [:mchang]]

So we found an interesting bug while running this test on a mac and running tiling like we do on b2g[1]. This test about 1/20 times will be run without tiles. This is due to the hint provided to CreatePaintedLayerWithHint. Most of the time, tiling works due to the animated geometry root being scrollable [2]. Even though the test doesn't actually scroll anything. When it is scrollable, the display item being processed is a CANVAS_BACKGROUND_COLOR. When it is not scrollable, and we therefore do not do tiling, we're generating a SOLID_COLOR display item. 

A few things are confusing.
1) Why is a CANVAS_BACKGROUND_COLOR considered scrollable? Or what other information do I need to dig up. 
2) Why would it change if we paint a SOLID_COLOR display item.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientPaintedLayer.cpp?from=ClientPaintedLayer.cpp&case=true#162
[2] https://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?from=FrameLayerBuilder.cpp#1688

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Display list dump with tiling

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Display list dump w/o tiling

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Display List dump w/ tiling

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: Display list dump w/o tiling

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Frame dump with tiling

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: Frame dump w/o tiling

Comment 13

[Mason Chang [Inactive] [:mchang]]

Force disabling tiling, hwc composition and culling seems to work - https://treeherder.mozilla.org/#/jobs?repo=try&revision=923e986f2705

bissecting.

Comment 14

[Mason Chang [Inactive] [:mchang]]

Attachment: Patch to mimic b2g tiling

Comment 15

[Mason Chang [Inactive] [:mchang]]

Hi Matt,

We're hitting a strange problem on this reftest / the ones like it such as mozilla-central/layout/reftests/position-dynamic-changes/horizontal/leftA-widthN-rightA.html even without vsync. The problem is that sometimes the test executes with tiling and sometimes not on OS X if we change tiling to be like b2g from attachment 8561992 [details] [diff] [review]. I'm executing the test multiple times by just refreshing the page.

Most of the time, we detect a scrollable frame when finding a frame for a CANVAS_BACKGROUND_COLOR display item here [1]. In these cases, we use tiling on os x. Every once in a while, we find a SOLID_COLOR instead and disable tiling since we no longer have a scrollable hint. The test itself is not scrollable and is just a request animation frame that makes a div wider by 10px every rAF. 

The frame trees and display lists dumps generally look the same. Any ideas on what else to look for or why we would sometimes not have an animated geometry root? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?from=CreateOrRecyclePaintedLayer&case=true#1688

Comment 16

[Matt Woodrow (:mattwoodrow)]

I suspect the root frame (that paints the CANVAS_BACKGROUND_COLOR) is always being treated as scrollable.

The difference is probably that we're getting an intermediate active geometry root for the frame being animated. 

You should debug nsDisplayListBuilder::IsAnimatedGeometryRoot for the case where we don't get tiling, though I suspect we're hitting the code at [1]. I'm not entirely sure why though, since I thought that was only for animating top/left not width.

These animated geometry root are temporary (based on activity) and timeout after no changes for a while, so could explain the intermittent nature of this bug.


[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1160

Comment 17

[Mason Chang [Inactive] [:mchang]]

From https://treeherder.mozilla.org/#/jobs?repo=try&revision=650667e91f4f - Disabling tiling fixes this intermittent.

Comment 18

[Mason Chang [Inactive] [:mchang]]

Force tiling everywhere shows the intermittent, so might not even be an issue with the scrollable hint. - https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b75761733a8

Comment 19

[Mason Chang [Inactive] [:mchang]]

Some notes, disabling gralloc seems to always pass this test. Backing out bug 1127289 fixes this as well. Commit 3c88c70e1e64, the one before bug 1127289 landed - https://treeherder.mozilla.org/#/jobs?repo=try&revision=41f28c5950f7

Comment 20

[Mason Chang [Inactive] [:mchang]]

Adding the unlock back from bug 1127289 let's us pass - https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5dc20cc26f0

Comment 21

[Mason Chang [Inactive] [:mchang]]

Very long story, the vsync refresh driver really isn't the problem but got tripped up at a bad commit range.

bug 1127289 landed and made R14 failures on b2g occur, but android was ok. 
Android - https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a171f0b3092
b2g - https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=5fc092347f17&tochange=8977d94c9d9a&filter-searchStr=b2g_emulator_vm%29

At hg commit 73b8ce222f9f, part of bug 1127289 was backed out, but caused android test failures, but b2g was ok.

When adding the part of bug 1127289 that fixed the R14 failures plus at commit 4bf1ae49cc11, bug 1118876, Android test failures starting occuring. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea3bd637e5cd

Currently testing part of bug 1127289 and bug 1118876 backed out to test if android + b2g are ok.

Comment 22

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Add unlock back to ClientTiledLayerBuffer. r=jrmuizel

Comment 23

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Backout bug 1118876 for android reftest failures. r=jrmuizel

Comment 24

[Mason Chang [Inactive] [:mchang]]

Try with the two patches - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e64aa2c0f66

Was going to try to push w/ the refresh driver, but try closed :/

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8563903 [details] [diff] [review]
Part 1: Add unlock back to ClientTiledLayerBuffer. r=jrmuizel

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

::: gfx/layers/client/TiledContentClient.cpp
@@ +1293,5 @@
>    // Intersect this area with the portion that's invalid.
>    tileRegion.SubOut(GetValidRegion());
>    tileRegion.SubOut(aDirtyRegion); // Has now been validated
>  
> +  backBuffer->Unlock();

Add a comment about how this doesn't seem necessary but taking it out was causing test failures on B2G.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8563904 [details] [diff] [review]
Part 2: Backout bug 1118876 for android reftest failures. r=jrmuizel

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

Ok. Better let nical know.

Comment 27

[Mason Chang [Inactive] [:mchang]]

Letting you know, bug 1118876 and 1127289 are getting backed out - https://bugzilla.mozilla.org/show_bug.cgi?id=1130681#c21

Comment 28

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c6bec02690
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cf76d4021ce

Comment 29

[Nicolas Silva [:nical]]

(In reply to Mason Chang [:mchang] from comment #27)
> Letting you know, bug 1118876 and 1127289 are getting backed out -
> https://bugzilla.mozilla.org/show_bug.cgi?id=1130681#c21

Do you know what's wrong with the patch from bug 1118876?

Comment 30

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nicolas Silva [:nical] from comment #29)
> (In reply to Mason Chang [:mchang] from comment #27)
> > Letting you know, bug 1118876 and 1127289 are getting backed out -
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1130681#c21
> 
> Do you know what's wrong with the patch from bug 1118876?

Nope. We weren't able to figure it out yet.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cf76d4021ce