Homescreen: lag when panning right to app list

VERIFIED FIXED

Status

Firefox OS
Gaia::Homescreen
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: m1, Assigned: crdlc)

Tracking

unspecified
ARM
All
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:shira+, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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

Updated

5 years ago
Severity: normal → major
(Assignee)

Updated

5 years ago
Assignee: nobody → crdlc
(Assignee)

Comment 1

5 years ago
Created attachment 715509 [details]
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.
Attachment #715509 - Flags: review?(21)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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 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.
Attachment #715509 - Flags: review?(21)
(Assignee)

Comment 5

5 years ago
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
(Reporter)

Comment 6

5 years ago
(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
(Assignee)

Comment 7

5 years ago
Do you mean that we are translated pages with display none during 100ms, isn't it? thanks
(Reporter)

Comment 8

5 years ago
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.
Duplicate of this bug: 843263
(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.
(Assignee)

Comment 11

5 years ago
I am going to provide a new patch in a couple of hours
(Assignee)

Comment 12

5 years ago
Created attachment 716459 [details]
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?
Attachment #715509 - Attachment is obsolete: true
Attachment #716459 - Flags: review?(21)
Created attachment 716790 [details] [diff] [review]
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? :)
(Assignee)

Comment 14

5 years ago
Could somebody check my new patch?
Flags: needinfo?(mvines)
(Assignee)

Comment 15

5 years ago
working on chris comments
Flags: needinfo?(mvines)
(Assignee)

Comment 16

5 years ago
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
Flags: needinfo?(jones.chris.g)
(Assignee)

Updated

5 years ago
Attachment #716459 - Attachment is obsolete: true
Attachment #716459 - Flags: review?(21)
(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 :).
Flags: needinfo?(jones.chris.g)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 844762
(Assignee)

Comment 19

5 years ago
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
Flags: needinfo?(jones.chris.g)
Hi crdlc, the second patch works fine for me.
(Assignee)

Comment 21

5 years ago
ok thanks for your feedback Jan! Michael, could you test as well the lag? thanks a lot
Flags: needinfo?(mvines)
(Assignee)

Comment 22

5 years ago
Created attachment 717864 [details]
Patch v2
(Assignee)

Updated

5 years ago
Attachment #717864 - Flags: review?(21)
Attachment #717864 - Flags: feedback?(mvines)
Comment on attachment 717864 [details]
Patch v2

That sounds much smarter to update page visibility after the transition than before the pan..
Attachment #717864 - Flags: review?(21) → review+
(Assignee)

Comment 24

5 years ago
https://github.com/mozilla-b2g/gaia/commit/07b2f27bb7a94035bcc33e1c2a99ce7b61c4e6aa
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(mvines)
Resolution: --- → FIXED
(Assignee)

Comment 25

5 years ago
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:
Attachment #717864 - Flags: approval-gaia-v1?
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
tracking-b2g18: --- → +
Comment on attachment 717864 [details]
Patch v2

Based on Jan's testing we can take this fix for uplift to v1-train.
Attachment #717864 - Flags: approval-gaia-v1? → approval-gaia-v1+
I'm happy to go with whichever implementatioon Vivien prefers.
Flags: needinfo?(jones.chris.g)
(Reporter)

Comment 28

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

Yep, definitely helps.  Thanks!
(Reporter)

Updated

5 years ago
Attachment #717864 - Flags: feedback?(mvines) → feedback+
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>
(Reporter)

Comment 31

5 years ago
Requesting uplift to 1.0.1.  This patch was a part of the MWC build.
blocking-b2g: --- → tef?
status-b2g18: affected → fixed
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?
Keywords: qawanted
QA Contact: jsmith
Going to find a new assignee to the verification.
QA Contact: jsmith

Updated

5 years ago
Keywords: qawanted → verifyme

Comment 34

5 years ago
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
Keywords: verifyme
QA Contact: pgorantla
(Assignee)

Comment 35

5 years ago
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
Adding QA wanted again for comment 35
Keywords: qawanted
pallavi - can you address comment 35?
Keywords: qawanted → verifyme

Comment 38

5 years ago
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
It blocks bug 827130, that is tef+ and cannot be merged in v1.0.1, so this bug should be tef+
(Reporter)

Updated

5 years ago
blocking-b2g: tef? → shira+
status-b2g18-v1.0.1: wontfix → affected
(Reporter)

Comment 40

5 years ago
(jhford -- uplift to v1.0.1 please?)
Flags: needinfo?(jhford)
v1.0.1: 5c24368d8f221d3ef9d0b7147ab05775ead49ddc
status-b2g18-v1.0.1: affected → fixed
Flags: needinfo?(jhford)
You need to log in before you can comment on or make changes to this bug.