Closed
Bug 879203
Opened 11 years ago
Closed 11 years ago
[Homescreen] The setting opacity to overlay in the page is not smooth and make a reducing fps.
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect, P1)
Tracking
(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)
People
(Reporter: leo.bugzilla.gaia, Assigned: oconnore)
References
(Depends on 1 open bug)
Details
(Whiteboard: TaipeiWW)
Attachments
(1 file)
When the page is moved from land page, the opacity is set as a transition. But the transition is not smooth and make a reducing 2~3 drag fps. In the leo's device if we remove the transition (setOpacityToOverlay and applyEffectOverlay in grid.js), the performance is 55.74 fps. But if we use it, it is 53.25 fps. I think that we need to impove it.
Comment 1•11 years ago
|
||
setOpacityToOverlay and applyEffectOverlay methods are implemented carefully in order to avoid allocations...
https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L283
Furthermore, we implemented the change of the opacity with only one decimal (x.y) so we have a good performance 53.25fps instead of 40 fps when the number of decimals was bigger.
The opacity change is a visual improvement very important from the UX point of view and 2 fps less is not extremely wrong in my honest opinion.
At least, I don't know how to improve this, do you think what it is really needed? I would like to know the Vivien's opinion who is my colleague as owner of this app, please Vivien, what do you think about it?
Thanks
Flags: needinfo?(21)
blocking-b2g: leo? → leo+
Flags: needinfo?(gsvelto)
Whiteboard: TaipeiWW
Target Milestone: --- → 1.1 QE3 (24jun)
Comment 2•11 years ago
|
||
From my point of view, we cannot do more from front-end
Comment 3•11 years ago
|
||
I'll make a profile of the transition and check what's happening on the platform said. This should tell us if it's possible to improve this or not in a reasonable amount of time.
Flags: needinfo?(gsvelto)
If I change the code to update the opacity more times, I think that the movement is improved for UX, but fps is reduced from 53 to 50.
in handleEvent funtion of grid.js file ,
overlayStyle.opacity = Math.round(opacity * 10) / 10;
--> overlayStyle.opacity = opacity.toFixed(2);
What is your opinion? Please check it.
Flags: needinfo?(gsvelto)
Flags: needinfo?(crdlc)
Comment 5•11 years ago
|
||
(In reply to Leo from comment #4)
> If I change the code to update the opacity more times, I think that the
> movement is improved for UX, but fps is reduced from 53 to 50.
What do you mean with "the movement is improved for UX"?
Anyway I've done my tests and here's the relevant profiles. This is scrolling between the homescreen and the first pane with the unmodified code in v1-train:
http://people.mozilla.com/~bgirard/cleopatra/#report=25eaab96419d9515cb62ace2660d94e30c54ed22
And this is once the setOpacityToOverlay() and applyEffectOverlay() functions have been turned into no-ops:
http://people.mozilla.com/~bgirard/cleopatra/#report=0d3e159e88af5e39da2d78980b742596a32266c8
In a nutshell what's happening is that when we set the opacity repainting the screen takes twice the time and apparently this is down to the display list being rebuilt (look for nsDisplayList::PaintRoot()). This is not happening when we don't set the opacity and it accounts almost entirely for the slowdown you're seeing.
especially because this code is far beyond mozilla-central and we might need a mozilla-
I need more time to diagnose the root cause and see if we can do something about it but it might be tricky (or impossible depending on how repainting works).
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 6•11 years ago
|
||
I attached a pull request.
10 -> 40 steps seems to make the transition appear smooth without harming FPS significantly.
Attachment #767014 -
Flags: review?(crdlc)
Assignee | ||
Comment 7•11 years ago
|
||
Gabriele is profiling the patched transition performance -- thanks!
Comment 8•11 years ago
|
||
I'm adding bug 796697 as a dependency because that's what need to be fixed to speed up opacity changes. That bug might require significant work so we cannot except to have it fixed by the end of the week.
The step-function was originally introduced in bug 843521 to improve performance; bug 843521 comment 1 implied an 8-10FPS improvement because of this change. However while testing it I could verify two things:
- The FPS improvement is there but you can only see it if you're swiping _very_ slowly, precisely because you're swiping slowly though the steps become visible making the result unpleasant
- If you swipe quickly having the coarser step function doesn't change a thing as the JS code is triggered only a couple of times during the whole transition and the FPS are unaffected
From my perspective Eric's patch represents some decent middle-ground where we have a smooth transition and a minor FPS degradation only when swiping very slowly which I think is perfectly acceptable.
BTW I've left some comments for you on the PR.
Depends on: 796697
Comment 9•11 years ago
|
||
Comment on attachment 767014 [details]
Pull request to increase granularity of opacity step function
Hi and sorry but it is not working on my Unagi
git pull https://github.com/oconnore/gaia.git 879203-homescreen-opacity
rm -Rf profile/
make reset-gaia
E/GeckoConsole( 1791): [JavaScript Error: "ReferenceError: opacitySteps is not defined" {file: "app://homescreen.gaiamobile.org/js/grid.js" line: 303}]
Ant the last thing, in my honest opinion, that part was implemented in order to avoid allocations and right now you add a closure. If this patch changes the steps, it is ok for me, but in that case the patch should be composed by only two lines. The first one to define the new constant and the second to change the /opacity =/ assignment
Thanks
Attachment #767014 -
Flags: review?(crdlc) → review-
Assignee | ||
Comment 10•11 years ago
|
||
Sorry Cristian, I left my pull request in a bad state while I did other things.
Is this better? It requires more than two lines, and I believe the step function should be factored out.
https://github.com/oconnore/gaia/commit/0a2a2de0259de70a4a3b0762be4a46a3ab7cd9e2
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → eric
Comment 11•11 years ago
|
||
yep, perfect! only one question on the commit, thanks
Comment 12•11 years ago
|
||
I love the fix too :) Bug 843521 mentioned other places where we used a similar pattern. It might be worth tracking them down and checking if they may also benefit from a similar change.
Assignee | ||
Updated•11 years ago
|
Attachment #767014 -
Flags: review- → review?(crdlc)
Comment 13•11 years ago
|
||
Comment on attachment 767014 [details]
Pull request to increase granularity of opacity step function
Great work!
Attachment #767014 -
Flags: review?(crdlc) → review+
Comment 14•11 years ago
|
||
master: d45b4889fa5ba6dc6400aedf1310521d5d09948b
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(crdlc)
Flags: needinfo?(21)
Resolution: --- → FIXED
Comment 15•11 years ago
|
||
v1-train: 41a03e0439742f61f901c91770e395a87483b456
status-b2g18:
--- → fixed
Comment 16•11 years ago
|
||
v1.1.0hd: 41a03e0439742f61f901c91770e395a87483b456
status-b2g-v1.1hd:
--- → fixed
Updated•11 years ago
|
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in
before you can comment on or make changes to this bug.
Description
•