879203 - [Homescreen] The setting opacity to overlay in the page is not smooth and make a reducing fps.

Status: RESOLVED FIXED

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

Description

[Leo]

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

[Cristian Rodriguez (:crdlc)]

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

Comment 2

[Cristian Rodriguez (:crdlc)]

From my point of view, we cannot do more from front-end

Comment 3

[Gabriele Svelto [:gsvelto]]

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.

Comment 4

[Leo]

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.

Comment 5

[Gabriele Svelto [:gsvelto]]

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

Comment 6

[Eric O'Connor [:oconnore]]

Attachment: Pull request to increase granularity of opacity step function

I attached a pull request.

10 -> 40 steps seems to make the transition appear smooth without harming FPS significantly.

Comment 7

[Eric O'Connor [:oconnore]]

Gabriele is profiling the patched transition performance -- thanks!

Comment 8

[Gabriele Svelto [:gsvelto]]

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.

Comment 9

[Cristian Rodriguez (:crdlc)]

Comment on attachment 767014 [details] [review]
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

Comment 10

[Eric O'Connor [:oconnore]]

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

Comment 11

[Cristian Rodriguez (:crdlc)]

yep, perfect! only one question on the commit, thanks

Comment 12

[Gabriele Svelto [:gsvelto]]

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.

Comment 13

[Cristian Rodriguez (:crdlc)]

Comment on attachment 767014 [details] [review]
Pull request to increase granularity of opacity step function

Great work!

Comment 14

[Julien Wajsberg [:julienw]]

master: d45b4889fa5ba6dc6400aedf1310521d5d09948b

Comment 15

[Julien Wajsberg [:julienw]]

v1-train: 41a03e0439742f61f901c91770e395a87483b456

Comment 16

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

v1.1.0hd: 41a03e0439742f61f901c91770e395a87483b456