Closed
Bug 829075
Opened 11 years ago
Closed 11 years ago
Lock screen background shows significant banding
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(blocking-b2g:-, blocking-basecamp:-, b2g18+ wontfix, b2g18-v1.0.1 wontfix)
RESOLVED
WORKSFORME
People
(Reporter: cjones, Assigned: rik)
References
Details
(Whiteboard: visual design uxbranch UX-P1)
Attachments
(1 file)
46 bytes,
patch
|
timdream
:
review+
vingtetun
:
approval-gaia-v1+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Whiteboard: visual design
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → anthony
Updated•11 years ago
|
blocking-basecamp: - → ?
Whiteboard: visual design → visual design, uxbranch, UX-P2
Comment 2•11 years ago
|
||
Sorry, did not mean to renom. Bugzilla inflight collisions weirdness.
Updated•11 years ago
|
blocking-basecamp: ? → -
Assignee | ||
Comment 3•11 years ago
|
||
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?
Assignee | ||
Comment 4•11 years ago
|
||
Also, this should probably be tracked as a tef blocker.
blocking-b2g: --- → tef?
Assignee | ||
Comment 5•11 years ago
|
||
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.
Updated•11 years ago
|
blocking-b2g: tef? → -
tracking-b2g18:
--- → +
Comment 6•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: visual design, uxbranch, UX-P1 → visual design, uxbranch, UX-P, ptracking
Comment 7•11 years ago
|
||
Anthony, I tried out your fix on Otoro. Its a significant positive change, can we apply this to all our wallpapers?
Assignee | ||
Comment 8•11 years ago
|
||
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)
Reporter | ||
Comment 9•11 years ago
|
||
Yes, it's far too late in the v1 game to attempt whole-system 32bpp rendering or dithering bitmaps.
Flags: needinfo?(jones.chris.g)
Assignee | ||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Summary: System background (and lock screen background) show significant banding → Lock screen background shows significant banding
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #703969 -
Flags: review?(timdream)
Comment 12•11 years ago
|
||
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)
Updated•11 years ago
|
Whiteboard: visual design, uxbranch, UX-P, ptracking → visual design uxbranch UX-P1 ptracking
Assignee | ||
Comment 13•11 years ago
|
||
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.
Comment 14•11 years ago
|
||
(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.
Assignee | ||
Comment 15•11 years ago
|
||
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.
Comment 16•11 years ago
|
||
(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?
Comment 17•11 years ago
|
||
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...
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Comment 19•11 years ago
|
||
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?
Assignee | ||
Comment 20•11 years ago
|
||
(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.
Comment 21•11 years ago
|
||
(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.
Comment 22•11 years ago
|
||
> 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.
Comment 23•11 years ago
|
||
(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).
Assignee | ||
Updated•11 years ago
|
Attachment #703969 -
Flags: review?(timdream)
Comment 24•11 years ago
|
||
Comment on attachment 703969 [details] [diff] [review] Review pointer to https://github.com/mozilla-b2g/gaia/pull/7686 Good, thanks!
Attachment #703969 -
Flags: review?(timdream) → review+
Comment 25•11 years ago
|
||
Landed on master: https://github.com/mozilla-b2g/gaia/commit/2dac54c4032cbd00698eb6f6a6c6408cfd16a8fb
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 26•11 years ago
|
||
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)
Comment 27•11 years ago
|
||
(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)
Assignee | ||
Comment 28•11 years ago
|
||
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 29•11 years ago
|
||
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+
Comment 30•11 years ago
|
||
v1-train: e721caf0bb039be62d11cf13029d669042bb2626
status-b2g18:
--- → fixed
Comment 31•11 years ago
|
||
(In reply to John Ford [:jhford] from comment #30) > v1-train: e721caf0bb039be62d11cf13029d669042bb2626 Correction: v1-train: dfc2b0d9bc828270da10eccfd30ace0c88730fad
Reporter | ||
Comment 32•11 years ago
|
||
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.
Reporter | ||
Comment 33•11 years ago
|
||
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:?]
Updated•11 years ago
|
Whiteboard: visual design uxbranch UX-P1 ptracking [MemShrink:?] → visual design uxbranch UX-P1 ptracking [MemShrink]
Comment 34•11 years ago
|
||
Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1
status-b2g18-v1.0.1:
--- → fixed
Comment 35•11 years ago
|
||
(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...
Comment 36•11 years ago
|
||
(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.
Comment 37•11 years ago
|
||
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)
Comment 38•11 years ago
|
||
Just found Bug 840752 is the fixing-in-gecko bug.
Comment 39•11 years ago
|
||
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.
Assignee | ||
Comment 40•11 years ago
|
||
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)
Assignee | ||
Comment 41•11 years ago
|
||
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 → ---
Assignee | ||
Comment 42•11 years ago
|
||
I had to do a followup for the revert because it broke the linter tests: https://github.com/mozilla-b2g/gaia/commit/c82c28f22791972ec64b3d7450a1d4dcb312ae06
Comment 43•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: visual design uxbranch UX-P1 ptracking [MemShrink] → visual design uxbranch UX-P1 ptracking
Updated•11 years ago
|
Whiteboard: visual design uxbranch UX-P1 ptracking → visual design uxbranch UX-P1
Comment 44•11 years ago
|
||
This is no longer a valid bug because there are no bending anymore.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•