Closed Bug 1021085 Opened 7 years ago Closed 7 years ago

Progressive tile painting causes flickering when editing Gaia home-screen

Categories

(Core :: Graphics: Layers, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: cwiiis, Assigned: kats)

References

Details

Attachments

(9 files, 9 obsolete files)

Log
165.03 KB, text/plain
Details
5.16 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
18.31 KB, patch
cwiiis
: review+
botond
: feedback+
Details | Diff | Splinter Review
1.47 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
3.65 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
19.10 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
3.56 KB, patch
cwiiis
: review+
BenWa
: review+
Details | Diff | Splinter Review
23.79 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
3.17 MB, video/mp4
Details
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.
blocking-b2g: --- → 2.0?
Duplicate of this bug: 1021503
this doesnt seem to affect the original horizontal homescreen.
Chris, I'm sure you're right, so can you comment how you know this is "progressive tile painting" related?
(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.
QA Whiteboard: [VH-FC-blocking+]
blocking-b2g: 2.0? → 2.0+
Assignee: nobody → bugmail.mozilla
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.
Blocks: 1020845
Chris, do you still see this with bug 1025562 fixed? I was seeing this earlier but not anymore.
Flags: needinfo?(chrislord.net)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(chrislord.net)
Hm. What device is that? I did the exact same thing on the Flame and didn't see any glitching.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Duplicate of this bug: 1027346
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
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.
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.
Attached file Log
For my reference, line 206 in this file has the bad "transformed compositor bounds" as far as I can tell.
Attachment #8443257 - Attachment is patch: true
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.
Attachment #8443259 - Flags: feedback?(chrislord.net)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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)
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*
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.
Attachment #8443258 - Attachment is obsolete: true
Rebased
Attachment #8443259 - Attachment is obsolete: true
Attachment #8443259 - Flags: feedback?(chrislord.net)
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).
Updating all patches to keep the attachments in order :)
Attachment #8443257 - Attachment is obsolete: true
Attachment #8443614 - Flags: review?(chrislord.net)
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.
Attachment #8443509 - Attachment is obsolete: true
Attachment #8443615 - Flags: review?(chrislord.net)
Attachment #8443615 - Flags: feedback?(botond)
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.
Attachment #8443617 - Flags: review?(chrislord.net)
Attachment #8443510 - Attachment is obsolete: true
Attachment #8443620 - Flags: review?(chrislord.net)
Attachment #8443512 - Attachment is obsolete: true
Attachment #8443622 - Flags: review?(chrislord.net)
Attachment #8443513 - Attachment is obsolete: true
Attachment #8443626 - Flags: review?(chrislord.net)
Attachment #8443626 - Flags: review?(bgirard)
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.
Attachment #8443635 - Flags: review?(chrislord.net)
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.
Attachment #8443626 - Flags: review?(bgirard) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
I'm building this now to test it, but I'll begin reviewing immediately.
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.
Attachment #8443614 - Flags: review?(chrislord.net) → review+
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)?
Attachment #8443615 - Flags: review?(chrislord.net) → review+
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).
Attachment #8443617 - Flags: review?(chrislord.net) → review+
Comment on attachment 8443620 [details] [diff] [review]
Part 4 - Remove unnecessary transform left over

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

LGTM.
Attachment #8443620 - Flags: review?(chrislord.net) → review+
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.
Attachment #8443622 - Flags: review?(chrislord.net) → review+
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.
Attachment #8443626 - Flags: review?(chrislord.net) → review+
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 :|
Attachment #8443635 - Flags: review?(chrislord.net) → review+
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 :)
(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.
No longer blocks: 1028271
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!
Attachment #8443615 - Flags: feedback?(botond) → feedback+
Attached video 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
You need to log in before you can comment on or make changes to this bug.