1021085 - Progressive tile painting causes flickering when editing Gaia home-screen

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

When editing the icons in the new vertical Gaia home-screen, the screen often flickers blank areas.

STR:

1. Install Gaia with either bug 1009530 fixed, or the attached patch
2. On the home-screen, long-press on an icon and release to enter edit mode
3. Scroll to the bottom or near the bottom of the apps list
4. Long-press and hold to move an icon round
5. Observe flickering as the animations for moving the icons occur

I've noticed that the flickering never seems to happen when the scroll offset is 0,0. There are more ways than the listed STR to get the flickering to occur, but that's the easiest/most reproduceable.

Comment 2

[Tony Chung [:tchung]]

this doesnt seem to affect the original horizontal homescreen.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Chris, I'm sure you're right, so can you comment how you know this is "progressive tile painting" related?

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> Chris, I'm sure you're right, so can you comment how you know this is
> "progressive tile painting" related?

Disabling the pref 'layers.progressive-paint' fixes the issue. It seems the calculations that try to figure out the coherent update region are not correct in this situation, and so you see tile updates that it thinks would be off-screen.

The flickering technically occurs due to the combination of progressive rendering and layerisation - if items weren't switching layers and were just updating content, you would see seams due to this bug but because the old layer is destroyed and a new layer is created, you see flickering instead.

Comment 5

[Jim Porter (:squib)]

I *think* this is the same underlying issue, but I've noticed that you can occasionally get some flickering in the Settings app if you tap an item with a full-row highlight (most of the stuff on the main screen) if it sits across a tile boundary. It's usually fast enough that you don't notice it, but I can make it more obvious by scrolling immediately prior (presumably by increasing CPU/GPU load).

This probably isn't a huge amount of help, but it seems to be more widespread than just the vertical homescreen.

Comment 6

[Johan Lorenzo [:jlorenzo]]

https://moztrap.mozilla.org/manage/case/13739/

Comment 7

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

Chris, do you still see this with bug 1025562 fixed? I was seeing this earlier but not anymore.

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Chris, do you still see this with bug 1025562 fixed? I was seeing this
> earlier but not anymore.

I certainly don't see it as prevalently as I did before, but I do still see it - I'll record a video, but the STR are basically scroll down, long press and hold an icon and rearrange it with the icons above it a few times.

Comment 9

[Chris Lord [:cwiiis]]

Video: https://www.dropbox.com/s/y1n6uqszrb52zs5/VID_0004.3gp

Comment 10

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

Hm. What device is that? I did the exact same thing on the Flame and didn't see any glitching.

Comment 11

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Hm. What device is that? I did the exact same thing on the Flame and didn't
> see any glitching.

This is an Open C because I left my Flame at home today, but is almost identical in configuration to the Flame. I just newly flashed this this morning - it's running stock Gecko (git commit 9f140ae3ceca706e536bb6569dcee210c17257b4 on git.mozilla.org) and not-quite-stock Gaia (but it soon will be stock and I think we'd agree that it'd be quite hard to cause this kind of glitching at that level).

I'll check on the Flame tomorrow with updated stock everything, but I don't expect different results.

Comment 12

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

Can you also confirm it still goes away with progressive paint disabled? I'll try a few other devices to see if I can repro it.

Comment 14

[Benoit Girard (:BenWa)]

Here's a video profile that happens to show the flicker quite clearly. If you zoom in to the relevant part of the profile you see can multiple 'Rasterize' phases indicative of a progressive paint:

http://people.mozilla.org/~bgirard/cleopatra/#report=04102901652b7363c3969fefaec593230204e870

Comment 15

[Chris Lord [:cwiiis]]

I can confirm that this does still happen on master (I was testing on the wrong tree before, so not sure it had the necessary patches), and that disabling progressive paint fixes the issue.

Comment 16

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

I got a log of what's happening here and it seems like the transformed composition bounds in TiledContentClient::ComputeProgressiveUpdateRegion is wrong. Not a big surprise to be honest, because I've recently fiddled with the corresponding transforms in BeginPaint() and the only reason I didn't fiddle with this one was because it made even less sense :)

For one thing I don't think it even gets the "compositor-side" composition bounds from the same layer that it gets the "content-side" composition bounds from, at least for B2G. I'll look over this code some more and try to fix it up.

Comment 17

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

Attachment: Log

For my reference, line 206 in this file has the bad "transformed compositor bounds" as far as I can tell.

Comment 18

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

Attachment: Part 1 - Refactor to use the same scrolling ancestor everywhere

Comment 19

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

Attachment: Part 2 - Fix the transform used to compute the compositor-side composition bounds

Comment 20

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

Attachment: Part 3 - Remove unnecessary transform left over

Comment 21

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

Attachment: Part 4 (uncompiled) - Fix up the Fennec code path to match

Comment 22

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

Comment on attachment 8443259 [details] [diff] [review]
Part 3 - Remove unnecessary transform left over

Chris, can you apply patches 1-3 and check if you still see the problem? The log I get with these applied seems to do what I want, and I think it should fix the flickering because now it's using the correct coherent rect.

Comment 23

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

Attachment: Part 4 - Fix up the Fennec code path to match

Comment 24

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Comment on attachment 8443259 [details] [diff] [review]
> Part 3 - Remove unnecessary transform left over
> 
> Chris, can you apply patches 1-3 and check if you still see the problem? The
> log I get with these applied seems to do what I want, and I think it should
> fix the flickering because now it's using the correct coherent rect.

I applied parts 1-3 and I still get some odd effects - it's harder to reproduce, but when you scroll down and rearrange the icons, after dropping, quite often the view visibly blanks, then draws in low-precision, then sometimes doesn't reappear in high precision.

I've recorded a (unfortunately a bit blurry) video to demonstrate. Note, this doesn't happen every time and sometimes it seems to be totally fine - it's definitely better than before, but not completely fixed.

I couldn't reproduce any artifacts while the rearranging is happening, it only seems to happen on drop, which I guess is when the layers change and get flattened (which would still indicate that the coherent region is wrong somewhere in some situation).

Video: https://www.dropbox.com/s/mgukzar454yo2fh/VID_0006.3gp (when I zoom in, it's hard to tell, but the rendering is stuck in low-precision)

Comment 25

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

Ok, thanks. I'm able to repro some issues on Fennec too so hopefully after debugging my way through those it'll fix everything. *fingers crossed*

Comment 26

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

Attachment: Part 2 - Fix the transform used to compute the compositor-side composition bounds

I missed the fact that the transform's Inverse() was stored in the paint data, so the adjustment I was doing was all backwards. Fixed that up.

Comment 27

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

Attachment: Part 3 - Remove unnecessary transform left over

Rebased

Comment 28

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

Attachment: Part 4 - Fix up the Fennec code path to match

Comment 29

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

Attachment: Part 5 - Use the entire transformed composition bounds as the coherent update rect

Comment 30

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

I was able to repro the issue in comment 24; it's a result of rounding error causing AboutToCheckerboard to return true instead of false. We need a FuzzyContains function or something, but doing that right will take time so for now I'll just hack in an Inflate(0.01f).

Comment 31

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

Attachment: Part 1 - Refactor to use the same scrolling ancestor everywhere

Updating all patches to keep the attachments in order :)

Comment 32

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

Attachment: Part 2 - Fix the transform used to compute the compositor-side composition bounds

Would like botond to also review this if he can before he goes on PTO, but if not then I don't want to wait.

Comment 33

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

Attachment: Part 3 - Deal with some rounding error

Ugly but I'll fix it up better in bug 1024126. I'd like to add a FuzzyContains to BaseRect to do this more nicely. I'll do all that as part of bug 1024126 though.

Comment 34

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

Attachment: Part 4 - Remove unnecessary transform left over

Comment 35

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

Attachment: Part 5 - Fix up the Fennec code path to match

Comment 36

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

Attachment: Part 6 - Use a larger coherent update rect on B2G

Comment 37

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

Attachment: Part 7 - Misc cleanups

Dunno why I put in all those "0x"s in before, %p does that automatically. Removing them. Also I renamed a variable and adjusted some logs/comments. Nothing big here, just didn't want to file a new bug for it.

Comment 38

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

https://tbpl.mozilla.org/?tree=Try&rev=3527be750c87

Comment 39

[Benoit Girard (:BenWa)]

Comment on attachment 8443626 [details] [diff] [review]
Part 6 - Use a larger coherent update rect on B2G

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

This looks good, but maybe we really should have this behave the same way on Fennec and do the conservative approach everywhere.

Comment 40

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> Would like botond to also review this if he can before he goes on PTO, but
> if not then I don't want to wait.

I'll try to look at it within the next few days. As this is a high-priority blocker, don't wait up.

Comment 41

[Chris Lord [:cwiiis]]

I'm building this now to test it, but I'll begin reviewing immediately.

Comment 42

[Chris Lord [:cwiiis]]

Comment on attachment 8443614 [details] [diff] [review]
Part 1 - Refactor to use the same scrolling ancestor everywhere

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

Looks good - I wonder if that helper method should actually be a bit higher up (on ThebesLayer, perhaps, or even on Layer) - but I'm fine with it staying here for now.

Comment 43

[Chris Lord [:cwiiis]]

Comment on attachment 8443615 [details] [diff] [review]
Part 2 - Fix the transform used to compute the compositor-side composition bounds

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

This looks good to me and I think I follow it, but I think Botond's review wrt the the transforms would be more valuable than mine.

::: gfx/layers/client/TiledContentClient.h
@@ +263,5 @@
>  
>    /*
> +   * The transform matrix to go from this layer's Layer units to
> +   * the scroll ancestor's ParentLayer units. The "scroll ancestor" is
> +   * the closest ancestor layer which scrolls.

Maybe an extra note here to say that this scroll ancestor is what determines the composition bounds (which helps explain the variable's name)?

Comment 44

[Chris Lord [:cwiiis]]

Comment on attachment 8443617 [details] [diff] [review]
Part 3 - Deal with some rounding error

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

Ok. I'd still rather this match the fennec-side one with the concept of a danger zone (which is basically what that inflate is doing, it's just a very tiny danger zone).

Comment 45

[Chris Lord [:cwiiis]]

Comment on attachment 8443620 [details] [diff] [review]
Part 4 - Remove unnecessary transform left over

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

LGTM.

Comment 46

[Chris Lord [:cwiiis]]

Comment on attachment 8443622 [details] [diff] [review]
Part 5 - Fix up the Fennec code path to match

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

LGTM.

Comment 47

[Chris Lord [:cwiiis]]

Comment on attachment 8443626 [details] [diff] [review]
Part 6 - Use a larger coherent update rect on B2G

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

Sound.

Comment 48

[Chris Lord [:cwiiis]]

Comment on attachment 8443635 [details] [diff] [review]
Part 7 - Misc cleanups

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

LGTM.

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +207,5 @@
>      return false;
>    }
>  
> +  // Only draw progressively when the resolution is unchanged, and we're not
> +  // in a reftest scenario (that's what the HasShadowManager() check is for).

Thanks for adding this, and sorry I didn't in the first place :|

Comment 49

[Chris Lord [:cwiiis]]

I can't easily reproduce any errors when dragging home-screen icons with these patches applied and progressive painting/low precision painting enabled. I can also confirm that low precision rendering appears to be working in the browser, so I guess these patches are good :)

Comment 50

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

(In reply to Chris Lord [:cwiiis] from comment #43)
> This looks good to me and I think I follow it, but I think Botond's review
> wrt the the transforms would be more valuable than mine.

Yeah, he's on PTO at the moment so I'd rather land it and have him look at it when he gets back. (Botond: don't spoil your PTO, this isn't that urgent :p)

> ::: gfx/layers/client/TiledContentClient.h
> > +   * The transform matrix to go from this layer's Layer units to
> > +   * the scroll ancestor's ParentLayer units. The "scroll ancestor" is
> > +   * the closest ancestor layer which scrolls.
> 
> Maybe an extra note here to say that this scroll ancestor is what determines
> the composition bounds (which helps explain the variable's name)?

Good call, done.

Will land as soon as the tree reopens.

Comment 51

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a587744b72f7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1d8bdf77e3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a73ed980626f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/492e015783de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b51b22c6c85
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/341ec472b082
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6aade5365ee1

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a587744b72f7
https://hg.mozilla.org/mozilla-central/rev/cf1d8bdf77e3
https://hg.mozilla.org/mozilla-central/rev/a73ed980626f
https://hg.mozilla.org/mozilla-central/rev/492e015783de
https://hg.mozilla.org/mozilla-central/rev/8b51b22c6c85
https://hg.mozilla.org/mozilla-central/rev/341ec472b082
https://hg.mozilla.org/mozilla-central/rev/6aade5365ee1

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e93838ffb061
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d45ce23b1bb
https://hg.mozilla.org/releases/mozilla-aurora/rev/63f56d26a625
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d72bf6d5c3c
https://hg.mozilla.org/releases/mozilla-aurora/rev/1fede0b2aa98
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f7f6ee7fe2e
https://hg.mozilla.org/releases/mozilla-aurora/rev/e992bf84c5c6

Comment 54

[Botond Ballo [:botond]]

Comment on attachment 8443615 [details] [diff] [review]
Part 2 - Fix the transform used to compute the compositor-side composition bounds

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

LGTM!

Comment 55

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame.MP4

This issue has been verified successfully on Flame 2.0 & 2.1.
See attachment: Verify_Video_Flame.MP4
Reproducing rate: 0/10

Flame v2.0 version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ff1100ba2ab8
Build-ID        20141204000228
Version         32.0

Flame v2.1 version:
Gaia-Rev        5655269098c7e82254e56933f1af05b4abe2a2f3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/86608c9389b5
Build-ID        20141204001201
Version         34.0