Closed Bug 829075 Opened 11 years ago Closed 11 years ago

Lock screen background shows significant banding

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:-, blocking-basecamp:-, b2g18+ wontfix, b2g18-v1.0.1 wontfix)

RESOLVED WORKSFORME
blocking-b2g -
blocking-basecamp -
Tracking Status
b2g18 + wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: cjones, Assigned: rik)

References

Details

(Whiteboard: visual design uxbranch UX-P1)

Attachments

(1 file)

STR
 (1) Unlock phone
 (2) Go to lockscreen

Notice significant banding on background image.

Gecko uses 16bpp surfaces by default on 16 bit displays, but the combination of the default image and the effect applied to it result in more-than-usually noticeable banding.

We should be able to work around this by forcing the image into a 32bpp surface.  Adding |opacity: 0.99| to the <img> should do the trick.

Implementing that background image as a <canvas> would also work.

This is a partner complaint so needs to be a high-priority ux-want.  Please feel free to fix in ux-branch.
Whiteboard: visual design
Assignee: nobody → anthony
Will be done in UX branch
blocking-basecamp: ? → -
blocking-basecamp: - → ?
Whiteboard: visual design → visual design, uxbranch, UX-P2
Sorry, did not mean to renom. Bugzilla inflight collisions weirdness.
blocking-basecamp: ? → -
So we can't use the opacity trick as we're not using <img> elements but background-images for this.

I've done a few testing and it seems using a canvas with |background: -moz-element()|, mozSetImageElement and an off-screen <canvas> does not fix it.
Using a real <canvas> in the DOM works though. Should we fix it with a Gaia workaround or ask the platform to fix it?
Also, this should probably be tracked as a tef blocker.
blocking-b2g: --- → tef?
You can watch my test at http://ricaud.me/tmp/canvas-test.html

First image is <canvas> via mozSetImageElement and shows banding. Second image is <canvas> in DOM and shows no banding.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
See Also: → 802743
This is a pretty big deal, the images look horrible on Otoro... similar to launch hardware.
Whiteboard: visual design, uxbranch, UX-P2 → visual design, uxbranch, UX-P1
Whiteboard: visual design, uxbranch, UX-P1 → visual design, uxbranch, UX-P, ptracking
Anthony, I tried out your fix on Otoro. Its a significant positive change, can we apply this to all our wallpapers?
Patryk: This is the plan. I'd just like a confirmation from Chris that we're going to do a workaround in Gaia rather than fix the issue in Gecko.
Flags: needinfo?(jones.chris.g)
Yes, it's far too late in the v1 game to attempt whole-system 32bpp rendering or dithering bitmaps.
Flags: needinfo?(jones.chris.g)
I did some testing and only the lockscreen is improved by using a canvas. The code is ready, I need to write some tests now.
Summary: System background (and lock screen background) show significant banding → Lock screen background shows significant banding
Comment on attachment 703969 [details] [diff] [review]
Review pointer to https://github.com/mozilla-b2g/gaia/pull/7686

0. Image should apply to panels, not the lockscreen itself because we will horizontally move the panels during the to/from emergency calls panel switching.
1. How does <canvas> prevents bending? An easier fix would be simply ask visuals to provide a dithering gradient mask ...
2. Instead of update the canvas every time the background image loads, save the canvas as a blob into asyncStorage is better in terms of CPU usage. That will partly fix bug 820940 too.
Attachment #703969 - Flags: review?(timdream)
Whiteboard: visual design, uxbranch, UX-P, ptracking → visual design uxbranch UX-P1 ptracking
0. Ah thanks, I tried to find out what difference using one canvas would make and couldn't see any.
1. See comment 0. A dithering gradient mask might help on Otoro but wouldn't it degrade the quality on Unagi?
2. The current patch already helps bug 820940 since it removes the data URIs. I tried storing the result in a blob and then displaying it in a <img> with |opacity: 0.99| and it does not solve the banding. I think we'll have to draw one canvas per panel for now.
(In reply to Anthony Ricaud (:rik) from comment #13)
> 0. Ah thanks, I tried to find out what difference using one canvas would
> make and couldn't see any.
> 1. See comment 0. A dithering gradient mask might help on Otoro but wouldn't
> it degrade the quality on Unagi?
> 2. The current patch already helps bug 820940 since it removes the data
> URIs. I tried storing the result in a blob and then displaying it in a <img>
> with |opacity: 0.99| and it does not solve the banding. I think we'll have
> to draw one canvas per panel for now.

That's not exactly what I have in mind. You could generate the blob with <canvas>, save it, and use |window.URL| to generate the object URL, and set the |background-image| to that URL -- provided that showing the canvas v.s. the image generated by the canvas has no difference.
Sadly, only using a <canvas> in the DOM works at the moment. Using the image generated by a <canvas with a |background-image| or an <img> tag, even with |opacity: 0.99|, doesn't fix the banding.

So it means we'll compute this 3 times (for every lockscreen-panel) at every launch or every time the background changes. That doesn't sound so bad for such a hack.
(In reply to Anthony Ricaud (:rik) from comment #15)
> Sadly, only using a <canvas> in the DOM works at the moment. Using the image
> generated by a <canvas with a |background-image| or an <img> tag, even with
> |opacity: 0.99|, doesn't fix the banding.
> 
> So it means we'll compute this 3 times (for every lockscreen-panel) at every
> launch or every time the background changes. That doesn't sound so bad for
> such a hack.

This is bad, and I worry about the memory consumption of these duplicate <canvas> objects. Since this is not a urgent bug, I would say we should instead investigate the platform issue underneath, rather than further complicating the Gaia codebase.

jlebar, would you worry about the memory consumption of this approach?
cjones, what cause the difference of the blob image and the <canvas> itself that generate it?
It's not totally clear to me what the proposal here is.  Will you have three screen-sized canvases, with stuff drawn on them?

I think a canvas contains a bitmap copy of its image data as RGBA, and I think we never discard that.  If so, on a 320x480 display, you're talking 320*480*4/(1024*1024) bytes = 0.6MB per canvas, so 1.7mb overall.

That's a significant hit, but it would be offset somewhat by the fact that you don't need to keep around the background as a data URI.

But here's a more basic question: If a canvas works for you, why can't you generate the image once using a canvas and then save it as a blob?  You can use a blob in an <img> tag or as a background-image...
(In reply to Justin Lebar [:jlebar] from comment #17)
> It's not totally clear to me what the proposal here is.  Will you have three
> screen-sized canvases, with stuff drawn on them?
Yes.
Although when I think about it, we don't need a background behind the emergency call panel so I think we can go down to 2 <canvas> (one for #lockscreen-panel-main and #lockscreen-panel-passcode). What do you think Tim?

> But here's a more basic question: If a canvas works for you, why can't you
> generate the image once using a canvas and then save it as a blob?  You can
> use a blob in an <img> tag or as a background-image...
Blob in <img> or background-image doesn't remove the banding. Look at my test in comment 5 on an Otoro (it only tests background-image and mozSetImageElement but I've confirmed with blobs yesterday). Only a real <canvas> inserted in the DOM solves our problem.

If we have time, obviously a platform fix is better.
I thought you were saying that a blob in an <img> *overlayed with your radial lightness mask* exhibits banding.

What if we were to compute the image plus the mask using a canvas, then extract the result using toBlob and insert /that/ into an <img>?  Surely that would work, right?
(In reply to Justin Lebar [:jlebar] from comment #19)
> I thought you were saying that a blob in an <img> *overlayed with your
> radial lightness mask* exhibits banding.
> 
> What if we were to compute the image plus the mask using a canvas, then
> extract the result using toBlob and insert /that/ into an <img>?  Surely
> that would work, right?
No, it doesn't. That's what I was trying to say but I feel like I'm not clear. So here's another test: http://ricaud.me/tmp/canvas-test-blob.html.
First image: <img> with blob from <canvas>, shows banding.
Second image: <canvas> in DOM, no banding.

(In reply to Anthony Ricaud (:rik) from comment #20)
> (In reply to Justin Lebar [:jlebar] from comment #19)
> > I thought you were saying that a blob in an <img> *overlayed with your
> > radial lightness mask* exhibits banding.
> > 
> > What if we were to compute the image plus the mask using a canvas, then
> > extract the result using toBlob and insert /that/ into an <img>?  Surely
> > that would work, right?
> No, it doesn't. That's what I was trying to say but I feel like I'm not
> clear. So here's another test: http://ricaud.me/tmp/canvas-test-blob.html.
> First image: <img> with blob from <canvas>, shows banding.
> Second image: <canvas> in DOM, no banding.

Yes. What's happening here is that the canvas is transparent and so stored as 32bit, we then draw this canvas directly with opengl and so we don't ever operate on it explicitly as 16 bit

(In reply to Anthony Ricaud (:rik) from comment #18)
> If we have time, obviously a platform fix is better.

There is not really a platform fix possible here other than switching everything to 32bit like android has done.
> There is not really a platform fix possible here other than switching everything to 32bit like 
> android has done.

...or using <canvas> like <img>, which is unfortunate because it uses more memory.
(In reply to Anthony Ricaud (:rik) from comment #18)
> (In reply to Justin Lebar [:jlebar] from comment #17)
> > It's not totally clear to me what the proposal here is.  Will you have three
> > screen-sized canvases, with stuff drawn on them?
> Yes.
> Although when I think about it, we don't need a background behind the
> emergency call panel so I think we can go down to 2 <canvas> (one for
> #lockscreen-panel-main and #lockscreen-panel-passcode). What do you think
> Tim?
> 

Right, with input from comment 22 and comment 21 (thanks!), <canvas> is unavoidable for removing the bending, at the end of the day. Please go ahead with the fix (with a comment in the code).
Attachment #703969 - Flags: review?(timdream)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 836293
Comment on attachment 703969 [details] [diff] [review]
Review pointer to https://github.com/mozilla-b2g/gaia/pull/7686

This is a common partner complaint, so there's high value in uplifting.  I haven't looked at the commit so I can't judge the risk.
Attachment #703969 - Flags: approval-gaia-v1?(21)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #26)
> Comment on attachment 703969 [details] [diff] [review]
> Review pointer to https://github.com/mozilla-b2g/gaia/pull/7686
> 
> This is a common partner complaint, so there's high value in uplifting.  I
> haven't looked at the commit so I can't judge the risk.

I have been told that there are some issues with background sizing? Rik has those been fixed? And if yes where?
Flags: needinfo?(anthony)
The two "depends on" bugs are regressions from this patch. So they should be uplifted with this one.
Depends on: 836300
Flags: needinfo?(anthony)
Comment on attachment 703969 [details] [diff] [review]
Review pointer to https://github.com/mozilla-b2g/gaia/pull/7686

Let's make our friends happy.
Attachment #703969 - Flags: approval-gaia-v1?(21) → approval-gaia-v1+
v1-train: e721caf0bb039be62d11cf13029d669042bb2626
(In reply to John Ford [:jhford] from comment #30)
> v1-train: e721caf0bb039be62d11cf13029d669042bb2626

Correction:
v1-train: dfc2b0d9bc828270da10eccfd30ace0c88730fad
As much as it pains me, this change is causing gecko to vomit on itself :( (bug 840752).  I think we should back out on 1.0.1.
In particular, the size of the temporary surfaces goes up to around 10MB.  That will mean that every time we unlock the lock screen, we'll shoot down an app process basically.

Sniffle :(.
Blocks: slim-fast
Whiteboard: visual design uxbranch UX-P1 ptracking → visual design uxbranch UX-P1 ptracking [MemShrink:?]
Whiteboard: visual design uxbranch UX-P1 ptracking [MemShrink:?] → visual design uxbranch UX-P1 ptracking [MemShrink]
Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1
(In reply to Lukas Blakk [:lsblakk] from comment #11)
> Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1

I reverted it on v1-train and v1.0.1 because of a 10mb overhead...
(In reply to Vivien Nicolas (:vingtetun) (:21) (away until march 25th) from comment #35)
> (In reply to Lukas Blakk [:lsblakk] from comment #11)
> > Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1
> 
> I reverted it on v1-train and v1.0.1 because of a 10mb overhead...

Set the flag right based on statement above.
ok, so, what about this bug after all ? Was the 10MB overhead fixed in a follow-up bug ?

Right now, this is in master... so if we have a 10MB overhead it's worth fixing it because we'll eventually use master.

Otherwise I suggest backing it out of master too.
Flags: needinfo?(anthony)
Just found Bug 840752 is the fixing-in-gecko bug.
I still agree that we should back this out of master.  This is an unacceptable memory regression; it should not live on any branch that people might use.
I don't have time to investigate the performance issues at the moment so I'm ok backing this out. I've made a note to back this out tomorrow.
Flags: needinfo?(anthony)
Backed out with https://github.com/mozilla-b2g/gaia/commit/9a5e0e4cf3539e27a71188c63d3812ff8ab486dc

For future reference, some commits and bug that touched the same code as this commit:
- 70b9f46 - bug 848281
- b83581c - bug 830644
- 3f9979f - bug 858118
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I had to do a followup for the revert because it broke the linter tests:
https://github.com/mozilla-b2g/gaia/commit/c82c28f22791972ec64b3d7450a1d4dcb312ae06
So, this patch was producing Bug 861177 on mozilla central; I closed that bug now that we've backed out this one but we should take care to that for the next patch.
Whiteboard: visual design uxbranch UX-P1 ptracking [MemShrink] → visual design uxbranch UX-P1 ptracking
Whiteboard: visual design uxbranch UX-P1 ptracking → visual design uxbranch UX-P1
This is no longer a valid bug because there are no bending anymore.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: