1091775 - network-activity.png framerate is too high (30), should be lower (~4)

Status: RESOLVED FIXED

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

Description

[Benoit Girard (:BenWa)]

Updating network-activity.png at 30 FPS isn't useful to the user and it's consuming a disproportionate amount of resources, particularly while app are working to process the incoming network data like webRTC for instance.

We should reduce the framerate of this as low as possible. This will improve performance and battery life.

Comment 1

[Benoit Girard (:BenWa)]

:gsvelto do you know who can look into this?

Comment 2

[Benoit Girard (:BenWa)]

Attachment: network-activity.png

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: apngs.zip

This zip file contains versions of the apngs with the duplicate frames removed and the delays adjusted by 'apngopt'. We should be able to just drop these in as replacements.

Comment 4

[Glenn Randers-Pehrson]

Similar: bug #939488

Comment 5

[Gabriele Svelto [:gsvelto]]

I had requested already a couple of time for the icon to be replaced with either a static indicator (like on Android) or something less CPU intensive but somehow these always fell through. I've personally manipulated the animation twice already (converting it from GIF to APNG and then making it transparent) but I've never altered the frame-rate or anything else as I didn't have green light from UX for that.

I don't know who might be best suited to make this decision though. I'm needinfo'ing Mike Tsai because he's more likely to know who might help us with this.

Comment 6

[Benoit Girard (:BenWa)]

[Blocking Requested - why for this release]: This was found a year ago and the fix still hasn't shipped. It's a small thing to fix and has benefit for performance and battery. Sure we have been shipping with this for awhile (since the start?) but this is the lowest of the lowest hanging fruits. Also it's blocking bug 1091240.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> I had requested already a couple of time for the icon to be replaced with
> either a static indicator (like on Android) or something less CPU intensive
> but somehow these always fell through. I've personally manipulated the
> animation twice already (converting it from GIF to APNG and then making it
> transparent) but I've never altered the frame-rate or anything else as I
> didn't have green light from UX for that.

The current png has an apparent frame rate of 5 fps but has 6 duplicate frames per frame which gives an effective frame rate of 30 fps. We can change to the 5 fps version without needing anything from UX because there's no visible change only a 6x reduction in cpu usage.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8514489 [details]
apngs.zip

Acting somewhat preemptively...  And from the comments I'm presuming the same issue applies to 2.0.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: High CPU use and high network RTT in WebRTC calls.

Testing completed: Needs to be verified on 2.0 & 2.1 before landing

Risk to taking this patch (and alternatives if risky): very low risk - removing spurious duplicate frames from an APNG.

String or UUID changes made by this patch: None

Comment 9

[GoodMike]

n? UX designer Eric Pan for help. Thanks, Eric.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

With the pngs provided by Randell I see a huge improvement. Using Firefox Hello 1.1 on a Flame running 2.0:

User 51%, System 21%, IOW 0%, IRQ 2%
User 61 + Nice 164 + Sys 94 + Idle 111 + IOW 0 + IRQ 0 + SIRQ 10 = 440

  PID   TID PR CPU% S     VSS     RSS PCY UID      Thread          Proc
 2499  2958  1   8% R 190588K  80656K     u0_a2499 MediaStreamGrph /system/b2g/plugin-container
 1419  1450  1   7% S 211776K  88936K     root     Compositor      /system/b2g/b2g
 2499  3320  0   6% S 190588K  80656K     u0_a2499 opensl_rec_thre /system/b2g/plugin-container
 2499  2964  1   5% S 190588K  80656K     u0_a2499 AudioGUM        /system/b2g/plugin-container
 1419  1419  0   5% R 211776K  88936K     root     b2g             /system/b2g/b2g
 2499  2499  1   4% S 190588K  80656K     u0_a2499 Firefox Hello   /system/b2g/plugin-container
 1419  1431  0   3% S 211776K  88936K     root     Socket Thread   /system/b2g/b2g
 2499  2503  1   3% S 190588K  80656K     u0_a2499 Socket Thread   /system/b2g/plugin-container

As you can see, the compositor has gone down to 7% and b2g has gone down to 5% (still up from the 2% without the network indicator, but much better).

Comment 11

[Daniel Coloma:dcoloma]

[Blocking Requested - why for this release]: I am requesting this to block 2.0 as it affects a lot the performance of Loop, by a simple change (a graphical asset) the full experience of Loop as per comment 10. 

Note that any application using network extensively, will also be improved the performance, e.g. the browser when loading "heavy" websites, FB/Google import activities, etc...

Comment 12

[Eric Pang [:epang] UX]

(In reply to GoodMike from comment #9)
> n? UX designer Eric Pan for help. Thanks, Eric.

Hi Randell,

Just a quick note, I took a look at the apngs and noticed one problem..

signal-searching@2.25x_opt.png shows the first signal bar always highlighted, so they will need to be fixed.

All other apngs look good to me.  Great win for the performance!

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Thanks Eric!

Forwarding NI to Jeff - can you correct that one PNG and update the patch?  Thanks!

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to Eric Pang [:epang] from comment #12)
> (In reply to GoodMike from comment #9)
> > n? UX designer Eric Pan for help. Thanks, Eric.
> 
> Hi Randell,
> 
> Just a quick note, I took a look at the apngs and noticed one problem..
> 
> signal-searching@2.25x_opt.png shows the first signal bar always
> highlighted, so they will need to be fixed.
> 
> All other apngs look good to me.  Great win for the performance!

The original [e-mail] has the same problem.

Comment 15

[bhavana bajaj [:bajaj]]

Blocking given the low risk perf win here, but uplifts will only happen after this has landed on central

Comment 16

[Daniel Coloma:dcoloma]

Eric, what is required to move this forward based on comment 14? We start IOT next week and I'd like to be sure vendor builds include this fix.

Thanks!

Comment 17

[Eric Pang [:epang] UX]

Attachment: [e-mail]

Hi Jeff, I've updated the original apng to be correct.  It's ready for optimization :).  Thanks!

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Attachment: [e-mail]

It looks like the original here is already at the right frame rate but here's an optimized version anyways.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8514489 [details]
apngs.zip

I reviewed these using TweakPng and they appear correct (with the one late correction in the other patch).

Comment 20

[Oscar Patiño González]

Attachment: proposed patch

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8517297 [details] [review]
proposed patch

what's tv_apps change for.......?

Comment 22

[Oscar Patiño González]

mistakenly added a commit that had nothing to do with this patch. Sorry

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 8517297 [details] [review]
proposed patch

stamping since Alive's comment was addressed, cheers!

Comment 24

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Etienne Segonzac (:etienne) from comment #23)
> Comment on attachment 8517297 [details] [review]
> proposed patch
> 
> stamping since Alive's comment was addressed, cheers!

Thanks Etienne!

Landed on Gaia master branch at:

https://github.com/mozilla-b2g/gaia/commit/7918024c737c4570cacd784f267e28737ae05dea

Comment 25

[Maria Angeles Oteo (:oteo)]

Comment on attachment 8517297 [details] [review]
proposed patch

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):None
[User impact] if declined:See comment 10 and comment 11
[Testing completed]: Manual testing
[Risk to taking this patch] (and alternatives if risky): no risks, assets change

[String changes made]:no

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/9658b93b412bdcc0f953d668e8c8e68318c99fb8
v2.0: https://github.com/mozilla-b2g/gaia/commit/5ee26701a4d8db266bfb203b2179f686ce14d8b6