842218 - Homescreen: lag when panning right to app list

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Michael Vines [:m1] [:evilmachines]]

There is a noticeable delay (~200ms?) between the time that one pans right on the "time screen" to the first page of the app list.

This makes one feel that they are /fighting/ the UI to get the app list.

Panning left to e.me also has this problem but is dwarfed by bug 842213.

Comment 1

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

This patch is based on the idea of a dedicated method for changing opacity carefully written to avoid as much as possible allocations and condition checks. Moreover we set the opacity with only one decimal. Thanks to those changes we achieve 8-10 fps more than before in pages affected by opacity transformations.

Comment 2

[Michael Vines [:m1] [:evilmachines]]

With this patch, we don't see much change in the initial lag (around 150-200 ms).

During the lag, we see a paint occurring with no updates to the compositor.

Comment 3

[Cristian Rodriguez (:crdlc)]

Hi Michael, to test this patch you should go to landing page and then move your finger around in a small circle as fast as you can. The frame rate increases about 8-10 fps with this patch in compare to master. Maybe the lag is due to set the opacity... OK, this is one thing, now, please could you give your STR to calculate that lag? Could you confirm us that my patch improves the frame rate?

thanks a lot

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 715509 [details]
Patch v1

I made some comments but I would also like to wait for Michael to explain how they measure this lag /fps so we check if the patch is useful or not.

Comment 5

[Cristian Rodriguez (:crdlc)]

The lag no idea :) but fps increases a lot!! If the patch doesn't resolve this perhaps we could open other about fps because I wouldn't like to loose it. Thanks for your help

Comment 6

[Michael Vines [:m1] [:evilmachines]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #4)
> Comment on attachment 715509 [details]
> Patch v1
> 
> I made some comments but I would also like to wait for Michael to explain
> how they measure this lag /fps so we check if the patch is useful or not.

This patch is definitely useful (thanks!) but doesn't help what the issue reported by this bug, which is the *initial* lag we one swipes/pans right from the "clock screen" to the first page of apps.  Panning makes this lag more obvious.  We can measure this by adding tracepoints in the system at the time that input is received and when a display update occurs.   During this time we see a ~100ms paint happening with no display updates.  We need this to occur in 100/60 ms to prevent stutter

Comment 7

[Cristian Rodriguez (:crdlc)]

Do you mean that we are translated pages with display none during 100ms, isn't it? thanks

Comment 8

[Michael Vines [:m1] [:evilmachines]]

Not sure quite, let me try again:
1) User initiates pan and input is received by Gecko
2) Gecko renders a bunch of stuff (for ~100ms)
3) First display update occurs 

If we can make #2 much smaller than we'll get a better UX when the user swipes/pans into the app list.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michael Wu [:mwu] from comment #1)
> I had a patch but it's gone now. However, the hack was pretty simple - in
> apps/homescreen/js/grid.js, I changed all instances of
> togglePagesVisibility(start, end); to togglePagesVisibility(start - 1, end +
> 1); so we always show the next and previous pages. The problem is the next
> and previous pages default to being positioned on the current page, so they
> all pile on top of each other until you move them out of the way.

This (shouldn't) pin the pages visible, because once they're repositioned by gaia they'll be offscreen and gecko will nuke them again.

Comment 11

[Cristian Rodriguez (:crdlc)]

I am going to provide a new patch in a couple of hours

Comment 12

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v2

This patch is based on the idea about having visibles the current, previous and next pages. Maximum are three pages with display block, the rest are not available (display none). Then, we don't force re-painting and translations during the first touchmove event. At that point, we have the pages ready to pan. Please, Michael, could you test it?

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Stupid gecko tricks (WIP; foiled by spaghetti)

This is the spec for a change that will make a big different in the lag on switching screens.  It uses some specialized gecko knowledge to force all the homescreen pages to stay rendered at all times, and then uses opacity to "hide" them when they're "offscreen".

The patch here is a kind of "declarative spec" for the change.  In a nutshell, we want to
 - wrap page <div>s in an outer div to enable the opacity hack.  Transform and transition this new outer div.
 - stop using |visibility: visible/none| to hide offscreen pages
 - instead of translating offscreen pages off the screen, put them at translateX(0) and give them a negligible opacity

The change seems relatively simple to implement but I'm flailing around trying to make the .js changes :(.  We edit page.container/movableContainer styles from a bunch of different places, and I don't know which places to change, and how.  So I didn't include the WIP .js changes.

Can anyone provide guidance on how to apply this change?

Is there a bug on file for cleaning up the code here? :)

Comment 14

[Cristian Rodriguez (:crdlc)]

Could somebody check my new patch?

Comment 15

[Cristian Rodriguez (:crdlc)]

working on chris comments

Comment 16

[Cristian Rodriguez (:crdlc)]

After working on different approaches I have these results:

1) Patch based on Chris Jones comments

Branch: https://github.com/crdlc/gaia/tree/lag-cjones-comments
Commit : https://github.com/crdlc/gaia/commit/82423d7059afb579dc62724d6f6cbe1cbb97325d

Maybe (or I am sure :)) I forgot some comments because the result are not so good

* Lag disappear. First goal achieved 
* Re-paint issue is still present
* After navigating for all pages and ev.me the fps are worst

2) Patch based on my idea (only current, previous and next pages are displayed, the rest are out of memory)

Branch: https://github.com/crdlc/gaia/tree/bug-842218-v2
Commit: https://github.com/crdlc/gaia/commit/c01cda3711314b8ec6af9befc68fa8ca8369b1ba

Results:

* Seems that totally the lag goes away
* Re-painted issue disappeared
* After navigating for all pages and ev.me the fps are the same

Could you test both approaches and give me your feedback?

Thanks a lot Chris

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to crdlc from comment #16)
> After working on different approaches I have these results:
> 
> 1) Patch based on Chris Jones comments
> 
> Branch: https://github.com/crdlc/gaia/tree/lag-cjones-comments
> Commit :
> https://github.com/crdlc/gaia/commit/82423d7059afb579dc62724d6f6cbe1cbb97325d
> 
> Maybe (or I am sure :)) I forgot some comments because the result are not so
> good

This doesn't implement the "don't move pages offscreen" part of comment 13 (when that happens, gecko will throw away the rendered content), so I'm not surprised the perf is bad :).

Comment 19

[Cristian Rodriguez (:crdlc)]

Hi,

   I updated both branches and I have more or less the same results with your tricks. Has it been implemented correctly?

https://github.com/crdlc/gaia/tree/lag-cjones-comments
https://github.com/crdlc/gaia/commit/776ba9ccb805317fdf0faa0e27343dd48adbb947

   My other patch in my honest opinion works fine, did you test?

https://github.com/crdlc/gaia/tree/bug-842218-v2
https://github.com/crdlc/gaia/commit/71e7f9cdebd3b24631b745f0cd6e50697a99ec87

Thanks

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Hi crdlc, the second patch works fine for me.

Comment 21

[Cristian Rodriguez (:crdlc)]

ok thanks for your feedback Jan! Michael, could you test as well the lag? thanks a lot

Comment 22

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v2

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 717864 [details]
Patch v2

That sounds much smarter to update page visibility after the transition than before the pan..

Comment 24

[Cristian Rodriguez (:crdlc)]

https://github.com/mozilla-b2g/gaia/commit/07b2f27bb7a94035bcc33e1c2a99ce7b61c4e6aa

Comment 25

[Cristian Rodriguez (:crdlc)]

Comment on attachment 717864 [details]
Patch v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: more lag, less FPS and sometimes pages are not re-painted
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch:

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 717864 [details]
Patch v2

Based on Jan's testing we can take this fix for uplift to v1-train.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm happy to go with whichever implementatioon Vivien prefers.

Comment 28

[Michael Vines [:m1] [:evilmachines]]

(In reply to crdlc from comment #21)
> Michael, could you test as well the lag?

Yep, definitely helps.  Thanks!

Comment 29

[John Ford [:jhford] CET/CEST Berlin Time]

Commit 07b2f27bb7a94035bcc33e1c2a99ce7b61c4e6aa does not apply to v1-train.  This means that there are merge conflicts which need to be resolved.  If there are dependencies that are not approved for branch landing, or have yet to land on master, please let me know

If a manual merge is required, a good place to start might be:
  cd gaia
  git checkout v1-train
  git cherry-pick -x -m1 07b2f27bb7a94035bcc33e1c2a99ce7b61c4e6aa
  <RESOLVE MERGE CONFLICTS>

Comment 30

[Cristian Rodriguez (:crdlc)]

v1-train has this patch

https://github.com/mozilla-b2g/gaia/commit/faa2cd3042ab0a6d2c6d73b64d7991fee811735f

Comment 31

[Michael Vines [:m1] [:evilmachines]]

Requesting uplift to 1.0.1.  This patch was a part of the MWC build.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

To ensure that this patch works well on its own (seeing it just landed in v1-train) can we get some QA confirmation that this fix does what it states in out builds - side by side comparison?

Comment 33

[Jason Smith [:jsmith]]

Going to find a new assignee to the verification.

Comment 34

[pallavi]

This issue was compared on both Unagi build Id:20130227070202 V1 and on Unagi, build id:20130225070200 V1.0.1. Couldn't notify any big difference.

For making it much clear,below is the link, which shows comparision on both V1 and V1.0.1
http://www.youtube.com/watch?v=6I4x5Vfdi88&feature=youtu.be

Comment 35

[Cristian Rodriguez (:crdlc)]

Please create three pages with icons and go to second page and then move your finger around in a small circle as fast as you can. You can see as next and previous ones are not repainted correctly

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

Adding QA wanted again for comment 35

Comment 37

[Jason Smith [:jsmith]]

pallavi - can you address comment 35?

Comment 38

[pallavi]

Yes, I can see the noticeable difference between V1 and V1.0.1

On Unagi,build id:20130225070200 V1.0.1 the pages are not repainting correctly

On Unagi, build id:20130228070204 V1 Issue was fixed

Comment 39

[Marcelino Veiga Tuimil [:sonmarce]]

It blocks bug 827130, that is tef+ and cannot be merged in v1.0.1, so this bug should be tef+

Comment 40

[Michael Vines [:m1] [:evilmachines]]

(jhford -- uplift to v1.0.1 please?)

Comment 41

[John Ford [:jhford] CET/CEST Berlin Time]

v1.0.1: 5c24368d8f221d3ef9d0b7147ab05775ead49ddc