888184 - Remove the canvas elements from the statusbar which are preventing hardware composition from working

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto] (PTO)]

+++ This bug was initially created as a clone of Bug #836372 +++

In bug 822345 we've landed a Gaia patch that replaced animated icons in the status bar with canvas elements plus JavaScript code to refresh them. The change was made to overcome the poor performance we experienced with animated images. This workaround unfortunately triggered bug 792966 for which we had to deploy another workaround which disabled allocating very small canvases in GPU memory. This in turn prevents the hardware compositor from kicking in when the network status or system download icons are visible (bug 884188 and bug 885245).

To fix this issue :roc suggested a different approach using CSS transforms coupled with display:block; see bug 836372 comment 9 and the following discussion.

Comment 1

[Jason Smith [:jsmith]]

Do we need to fix this to resolve the two bugs this bug is blocking? If so, this bug would be blocking a blocker.

Comment 2

[Julien Wajsberg [:julienw]]

Yep Jason, that is both the safest and cleanest approach.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Pull request for gaia/master

Pointer to Github pull-request

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 769693 [details]
Pull request for gaia/master

This PR is a work-in-progress that reverts the canvases introduced in the statusbar by the workaround for bug 822345; despite my best efforts it still exhibits some significant issues:

- I've re-created the animated images and made them properly transparent to be consistent with bug 863710. The process should have been essentially loss-less as I've started from the original animated GIF, added an appropriate alpha channel and then turned into APNG. I've verified in Firefox and desktop B2G that they look identical however on the device they appear different. A small bit from either the right or bottom part is missing sometimes and I have no idea why.

- Despite the tests we made in bug 836372 and despite having verified that animated images with transform: perspective(1px) and display: block consume less CPU than canvases + timers with this patch applied we seem to be consuming more CPU. I haven't had the time to profile it yet but I can't explain why is this happening.

- The signal searching icon was partially corrupted, the one in this PR is not transparent and I have to fix it yet.

The part which I'm more dubious about are my CSS changes. I'm not exactly sure they're correct and I fear I might have done something wrong which is causing the aforementioned problems.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're using a DIV with a background-image; did your other experiments use an IMG? Mine did.

Can you try using an IMG instead of background-image? I guess it shouldn't matter but it's possible we have unresolved issues with background-image.

(In reply to Gabriele Svelto [:gsvelto] from comment #4)
> - I've re-created the animated images and made them properly transparent to
> be consistent with bug 863710. The process should have been essentially
> loss-less as I've started from the original animated GIF, added an
> appropriate alpha channel and then turned into APNG. I've verified in
> Firefox and desktop B2G that they look identical however on the device they
> appear different. A small bit from either the right or bottom part is
> missing sometimes and I have no idea why.

Can you demonstrate that in a simple Web page? If so, file a separate bug on it, CC joedrew and Milan and we'll get someone on that right away.

Your CSS looks OK to me, but I'm not sure why the sizing of these icons is done the way you do it.

It's not clear to me why a transparent status bar requires transparent icons. If we make the status bar solid black and the icons have a black background, but make the status bar itself 'opacity:0.5' or something, it should be fine --- as long as the icons are descendants of the status bar element.

Comment 6

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 769693 [details]
Pull request for gaia/master

it's better to me for less javascript + more css/apng, but I have no idea about performance and problems you mention. I tried it on nightly desktop browser and looks same as before.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> You're using a DIV with a background-image; did your other experiments use
> an IMG? Mine did.
> 
> Can you try using an IMG instead of background-image? I guess it shouldn't
> matter but it's possible we have unresolved issues with background-image.

Your comment was spot on, I've just switched div elements with img and it's now working fine. We now have the lowest CPU usage I've ever measured on this particular scenario \o/

> Can you demonstrate that in a simple Web page? If so, file a separate bug on
> it, CC joedrew and Milan and we'll get someone on that right away.

It's bizarre, it shows up only on the devices, not on the emulator or desktop client. I'll attach snapshots and file a separate bug for it.

> Your CSS looks OK to me, but I'm not sure why the sizing of these icons is
> done the way you do it.

It should be because we're trying to make everything DPI-independent (and we have dedicated icons for high-DPI scenarios).

> It's not clear to me why a transparent status bar requires transparent
> icons. If we make the status bar solid black and the icons have a black
> background, but make the status bar itself 'opacity:0.5' or something, it
> should be fine --- as long as the icons are descendants of the status bar
> element.

When in the lock-screen the status-bar is invisible but the icons remain visible and are blended against the wallpaper. I'm not too fond of using APNG files because it means the animations are Gecko-only at this stage but we might find a better way to implement them down the road.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 769693 [details]
Pull request for gaia/master

(In reply to Yuren Ju [:yurenju] from comment #6)
> it's better to me for less javascript + more css/apng, but I have no idea
> about performance and problems you mention. I tried it on nightly desktop
> browser and looks same as before.

Thanks for your feedback; I've refreshed the patch with :roc's suggestion and now we're hitting the right performance target. I've also fixed the signal-searching icon which was corrupted. All the icons that appear in the PR are exact copies of the old ones so I don't think we need to flag UX.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Pull request for gaia/v1-train

Pointer to Github pull-request

Comment 10

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 769693 [details]
Pull request for gaia/master

better if Alive take this review.

Comment 11

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

Comment on attachment 769693 [details]
Pull request for gaia/master

My only question:
Won't the v1-train patch regress https://bugzilla.mozilla.org/show_bug.cgi?id=822345?
It's still using gif not png.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Alive Kuo [:alive] from comment #11)
> My only question:
> Won't the v1-train patch regress
> https://bugzilla.mozilla.org/show_bug.cgi?id=822345?
> It's still using gif not png.

We have PNGs in master because we need them to be transparent there and show up in the lock-screen. AFAIK v1-train will keep using GIFs because we don't want to uplift those changes there. Thanks for the review!

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

Merged on master:

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

... and v1-train:

https://github.com/mozilla-b2g/gaia/commit/187b6de7c87e667cefbc5bc5972f6a21c381311a

Comment 14

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

v1.1.0hd: 187b6de7c87e667cefbc5bc5972f6a21c381311a
v1.1.0hd: e610519c0fec98d15f04ba75b84f647c3faae64b