1042673 - Automatic brightness needs some hysteresis

Status: RESOLVED FIXED

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

Description

[Soledad Penades [:sole] [:spenades]]

Apparently this needs some filtering and smoothing on the Gaia side.

Comment 1

[Anthony Ricaud (:rik)]

The code lives in apps/system/js/screen_manager.js

Comment 2

[Anthony Ricaud (:rik)]

I just switched to a Flame for my dogfooding, this is crazily annoying!

Comment 3

[Soledad Penades [:sole] [:spenades]]

**NO PRESSURE**

Let me finish my talk and I'll put some love on this :D

Comment 4

[Anthony Ricaud (:rik)]

Thinking and talking about this, this might just be a Flame specific issue where we receive the updates too often. Maybe throttling the brightness updates could be enough to fix this. And if that works, we should probably get this in Gecko or lower to be sure that all consumers of that API don't have the issue.

Comment 5

[Soledad Penades [:sole] [:spenades]]

So I'm not sure if just throttling should work, maybe there should also be the interpolation (or hysteresis) so even if there are less updates, the transition between values was smooth and not like 0, 100, 20, ... which is the annoying part! Does it make sense?

Comment 6

[Soledad Penades [:sole] [:spenades]]

Attachment: PR URL

Works way smoother now. Can it be that simple?

Comment 7

[Anthony Ricaud (:rik)]

Comment on attachment 8501691 [details] [review]
PR URL

Redirecting to Tim.

I doubt this is the best solution though. This may improve the dogfooding experience while we do the real fix though.

I feel that we should throttle those updates rather than doing them every 20ms. That could have a good effect on battery life?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8501691 [details] [review]
PR URL

Unfortunately this can't work. We did the same thing in bug 935391 already.

https://github.com/mozilla-b2g/gaia/commit/7ccd4a9acec6dc490bb53f43c05b293cfe017eb4

I believe the real issue is bug 1064683, in which I comment my finding there. The brightness property in the DOM is a float between 0 to 1, but internally to Gonk it's a 8bit value, and actual step of brightness is hardware-dependent. Tweak this number simply delay the time it takes for the screen to brighten/dim and did not create more steps (i.e. smoother experience).

The other idea of my is to make the dimming and brighten asymmetrical, or start with a delay. Right now the curve is a linear transition from start to end, We can make a cubic-bezier curve and use a ease-in like transition. That's bug 819744. We can dup that bug here if you want to implement this.

Comment 9

[Soledad Penades [:sole] [:spenades]]

Ah, I see! It seemed too easy, but sometimes the easier solutions are the right ones :-)

I also think an ease-in transition might be nicer. Let me try that route, and I'll ask you for an r? or ni? when I'm done.

Thanks for the pointers!

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I couldn't help but to think we need to get move transitionBrightness() out of ScreenManager before working on improvements. ScreenManager is too big to take anything more complex.

I have a WIP patch that will make this part of the code into a tiny class called ScreenBrightnessTransition. I will file a bug, and set this bug to be blocked by that one. 

We can then work on changing the transition function. Does that sounds good to you?

(ni? myself so I will remember to do so)

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Bug 1081426 filed.

Comment 12

[Soledad Penades [:sole] [:spenades]]

Hi Tim!

Got the latest code :)

I'm a little bit confused as to whether this can, or not, be fixed in Gaia. Unless I misunderstood, related bug 1064683 seems to say this is something that cannot be fixed in Gaia.

Other than that, I'll give it another go by working on the new screen_brightness_transition.js file :)

Thanks for the work!

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I think it's possible to hide the hardware behavior (bug 1064683) with a saner curve, and we should work on this bug to do that. If you found that's not the case we can WONTFIX this bug.

Comment 15

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

As the title for this bug says, I think the main issue is not about making this smoother, but making the luminosity not move up and down continuously. If it moved up just before, we need a bigger delta to make it move down. And the other way around. That's what an hysteresis is :)

I discussed with sole on IRC and we agreed that I can try this soon. If you haven't heard from me next monday then it means I did nothing ;)

Comment 16

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Attachment: hysteresis-like solution (github PR)

I tried here something simpler than a full hysteresis solution.
The idea is that we should not change the brightness if the new brightness target is not far enough of the last change.

This should prevent moving up/down around an edge. However we may need to adjust the tentative value I've used.

I couldn't try it yet because my proximity sensor doesn't work on my current flame-kk phone. I'll try a pvt build soon.

Comment 17

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Yeah, pvt builds make the light sensor work again ;)

I tested the patch and I think it improves the situation (it doesn't change all the time), although it's not perfect (it still changed at a moment I didn't expect, and when it does it's for a bigger change). Now that I have the sensor working I can look at the actual values and see how we can improve.

Comment 18

[GitHub Autolander Bot]

Attachment: [PullReq] julienw:1042673-hysteresis-luminosity-control-v2 to mozilla-b2g:master

Comment 19

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Comment on attachment 8532923 [details] [review]
[PullReq] julienw:1042673-hysteresis-luminosity-control-v2 to mozilla-b2g:master

Hey Tim,

this patch works fine for me.

I tried various things before doing this patch. I tried comparing the clampedBrightness value instead of the lux value and the result was still not enjoyable.

If you look at the other patch (https://github.com/mozilla-b2g/gaia/pull/26387), I also added some code to throttle and average the value received from the sensor, but from my test, it didn't improve much compared to this more simple patch.

Last but not least, I found that on flame-kk the received values are lower (about 2.5x lower) than on flame-jb. However the result with the current multiplication value does not look bad so I kept it like this, we can change it in a separate patch if we find it's not accurate.

Tell me what you think!

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8532923 [details] [review]
[PullReq] julienw:1042673-hysteresis-luminosity-control-v2 to mozilla-b2g:master

Nice finding! We were all previously looking at screenBrightness (the output) but we never investigate the input (lux) we are receiving.

However, if you want to touch |autoAdjustBrightness| I strongly recommend factoring out the code before doing anything with it. I am sure you feel the pain when patch ScreenManager unit tests.

Comment 21

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Hey Tim,

can you suggest how to refactor this?

Also to be honest this would be more than I'm willing to do. Moreover ScreenManager is not thbig right now (after your bug 1081426) and the change

Comment 22

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Hey Tim,

can you suggest how to refactor this?

Also to be honest this would be more than I'm willing to do. Moreover ScreenManager is not that big right now (after your bug 1081426) and the change here is quite simple. I didn't feel any pain in unit tests :)

If you can suggest something I can try to do it but still I think the change is small and improves the situation.

Comment 23

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Moreover we need to keep master stable given what we want to do with next version, so I think refactoring is not wise :)

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8532923 [details] [review]
[PullReq] julienw:1042673-hysteresis-luminosity-control-v2 to mozilla-b2g:master

I am convinced.

Comment 25

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Thanks for your understanding ! \o/

Let's see how autolander works ;)

Comment 26

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

NI Kevin, looks like autolander tried to land both PR whereas only one had +autoland and was r+.

Comment 27

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/cf24ef4945a4da10f1e8c18925a16b4d0b4c4262

Comment 30

[Kevin Grandon :kgrandon]

Wow, lots of attachments :)

Thanks for the heads-up. I don't think it tried to land both, but it did try to do some validations on PR# 26387. I suppose we don't need to perform validations if it's not going to land.

I think that's the problem, I've filed bug 1109162 to track this.

Comment 32

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

I found an issue with this patch: when disabling/enabling auto adjust, we don't reset previousLux and as a result we don't set the brightness until it changes to a different enough value.

I'll give a shot at bug 989281 so I will try to fix this there as well.

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Julien Wajsberg [:julienw] from comment #32)
> I found an issue with this patch: when disabling/enabling auto adjust, we
> don't reset previousLux and as a result we don't set the brightness until it
> changes to a different enough value.
> 
> I'll give a shot at bug 989281 so I will try to fix this there as well.

That would be a regression then. I tend to think we should not try to fix a regression and provide a feature in one patch -- that would cause problem and limit our options to maneuver (e.g. backout).

Would you mind file a separate follow-up for that?

Comment 34

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Yep ok, I'll do it. And I already have the patch :)