Automatic brightness changes constantly and erratically based on head shadow

RESOLVED FIXED in Firefox OS master

Status

Firefox OS
Gaia::System::Power Mgmt
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sheeri, Assigned: cyu)

Tracking

({foxfood})

unspecified
FxOS-S6 (04Sep)
ARM
Gonk (Firefox OS)
foxfood
Dependency tree / graph

Firefox Tracking Flags

(b2g-master fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Automatic brightness changes constantly and erratically based on head shadow
I'm actually not sure which component to file this under. Alexandre, do you know the proper component for the auto-screen-dimmer?
Flags: needinfo?(lissyx+mozillians)
I think it's part of the power management code in system app
Component: General → Gaia::System::Power Mgmt
Flags: needinfo?(lissyx+mozillians)
apps/system/js/screen_auto_brightness.js
apps/system/js/screen_brightness_transition.js
apps/system/js/screen_manager.js
QA Whiteboard: [foxfood-triage]
(Assignee)

Comment 4

3 years ago
The problem is not in the system app. Refer to bug 1179354 for an analysis and experiments.
(Assignee)

Comment 5

3 years ago
Created attachment 8649642 [details] [diff] [review]
Implement a stable auto brightness algorithm

I've dogfooded this automatic screen brightness algorithm on my aries and it's much more stable and usable than the current one.
This algorithm has the following features:

1. Works mainly on the 'brightness' domain instead of the 'lux' domain. The values reported by the ambient light sensor is exponential in nature, while we use the range [0, 1] to represent the brightness. So it's better that we store the converted brightness values instead of raw lux values for computation.

2. As a result of 1., the brightness threshold working on the brightness domain works consistently in low and high brightness. The original one use 10 for lux threshold, but 10 makes the current algorithm very sensitive in high brightness and less sensitive in low brightness.

3. Use a running average of brightness instead of a single value to smooth out fluctuating lux values from artificial light sources such as fluorescent lamp.

4. Synthesize brightness values when the light sensor stops spewing out new values in a dark environment. When we enter a dark room the we might sense brightness values like {0.5, 0.4, 0.35, 0.1}. The last values we have is 0.1 and stops at it. We simulate that new values continue coming in as {0.1, 0.1, ...}. This makes it eventually adjust to the final brightness in a dark environment.

5. Implement the adjustment algorithm as tracing a moving target using a state machine. The environment might change as we gradually move to the desired brightness. Instead of unconditionally adjust to the desired value, we treat the desired brightness as a changing value and we continuously try to adjust to it. The closer we are to the target brightness (from the running average), the slower we make the adjustment.

6. Asymmetric adjustment. It's more serious that we fail to see the screen under sunlight than that the screen is too bright in a dark environment (sorry for your poor retina). We make it quicker to adjust to high brightness and slower to decrease the brightness.
Attachment #8649642 - Flags: feedback?(timdream)
(Assignee)

Comment 6

3 years ago
(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #4)
> The problem is not in the system app. Refer to bug 1179354 for an analysis
> and experiments.

A comparison with nexus 5 and iPhone 4s shows that it's the contrary. Aries-L is too bright in a dark environment. Aries-KK that we dogfood has a larger brightness range, which exacerbates the problem with the current auto brightness algorightm.
Assignee: nobody → cyu
Status: NEW → ASSIGNED
Comment on attachment 8649642 [details] [diff] [review]
Implement a stable auto brightness algorithm

Review of attachment 8649642 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with the approach of changing brightness -- so I am f+ this. 
However there are a lot of coding style changes should be improved first -- sorry for putting you to work on these.

Nice work!

::: apps/system/js/screen_auto_brightness.js
@@ +49,2 @@
>     */
>    ScreenAutoBrightness.prototype._state = 0;

Remove this line?

@@ +49,3 @@
>     */
>    ScreenAutoBrightness.prototype._state = 0;
>    ScreenAutoBrightness.prototype._autoAdjustDelay = 750;

Is this still used? If so please rename it to ALL_CAPS.

@@ +49,4 @@
>     */
>    ScreenAutoBrightness.prototype._state = 0;
>    ScreenAutoBrightness.prototype._autoAdjustDelay = 750;
> +  ScreenAutoBrightness.prototype._syntheticBrightnessDelay = 250;

Name constants with ALL_CAPS.

@@ +49,5 @@
>     */
>    ScreenAutoBrightness.prototype._state = 0;
>    ScreenAutoBrightness.prototype._autoAdjustDelay = 750;
> +  ScreenAutoBrightness.prototype._syntheticBrightnessDelay = 250;
> +  ScreenAutoBrightness.prototype._state;

I would still we keep the _state as an ID, and (sorry -- this is not in the original code) list these states as

ScreenAutoBrightness.prototype.STATE_XXXX = n;

here.

Instead of attaching transition method on the property.

@@ +60,5 @@
> +  ScreenAutoBrightness.prototype.STEP_INTERVAL_MS_DOWN = 150;
> +  ScreenAutoBrightness.prototype.STEP_DELTA = 0.005;
> +
> +  ScreenAutoBrightness.prototype.COOL_DOWN_UP = 2000;
> +  ScreenAutoBrightness.prototype.COOL_DOWN_DOWN = 5000;

COOL_DOWN_MS_UP and COOL_DOWN_MS_DOWN?

@@ +66,5 @@
>    /*
>     * id of delay timeout
>     */
>    ScreenAutoBrightness.prototype._delayTimeout = null;
> +  ScreenAutoBrightness.prototype._transitionTimeout = null;

I am sorry, but these should s/Timeout/TimerID/ or s/Timeout/Timer/.

They also should be initialized to undefined, not null.

@@ +81,2 @@
>     */
> +  ScreenAutoBrightness.prototype._previousBrightnessValues = [];

= null; on the prototype and create new array on constructor, to prevent the array being used on multiple instances.

@@ +114,5 @@
> +    this._state = this.stateCoolingDown;
> +  };
> +
> +  ScreenAutoBrightness.prototype.delayedSynthesizeBrightness = function() {
> +    this._delayTimeout = null;

= undefined;

@@ +124,5 @@
> +
> +  ScreenAutoBrightness.prototype.maybeSynthesizeBrightness = function() {
> +    if (this._delayTimeout) {
> +      clearTimeout(this._delayTimeout);
> +      this._delayTimeout = null;

= undefined

@@ +158,4 @@
>      }
> +
> +    // Invoke the state machine
> +    this._state();

switch (this._state) {
  case XXX:
    this.doAutoAdjustOnXXXX();
    break;
}

This naming and pattern IMHO makes the state callbacks and the state more understandable.

@@ +231,5 @@
> +    // Do nothing.
> +  };
> +
> +  /**
> +   * Utility functions.

All these methods mutating _previousBrightnessValues array probably warrant it's own constructor, maybe named |ScreenAutoBrightnessValues|, but it's up to you if you want to make this adjustment.

@@ +250,5 @@
> +  };
> +
> +  // Push a brightness value to the sliding window using the sensed lux value.
> +  ScreenAutoBrightness.prototype.pushLuxValue = function(lux) {
> +    var self = this;

You don't need this. |this| does not change in L258.

@@ +279,5 @@
> +  };
> +
> +
> +  ScreenAutoBrightness.prototype.getAveragePreviousBrightness = function() {
> +    if (this._previousBrightnessValues.lenght == 0) {

length

@@ +314,5 @@
> +      this._previousBrightnessSetTime = Date.now();
> +      this._currentBrightness = Math.min(1.0, Math.max(0.1, brightness));
> +      this.onbrightnesschange(this._currentBrightness);
> +      // Set brightness directly.
> +      navigator.mozPower.screenBrightness = this._currentBrightness;

Please ask ScreenManager.js to do that.

::: apps/system/js/screen_brightness_transition.js
@@ +21,3 @@
>    ScreenBrightnessTransition.prototype.STEP_DELTA = 0.01;
>  
>    ScreenBrightnessTransition.prototype.onsuccess = null;

Let's rename onsuccess to ontransitionfinish to initiate the symmetry of the naming.
Attachment #8649642 - Flags: feedback?(timdream) → feedback+
The description sounds spectacular. Thank you for the attention to this.
Created attachment 8650976 [details] [review]
[gaia] CervantesYu:bug_1179353 > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Attachment #8650976 - Flags: review?(timdream)
Comment on attachment 8650976 [details] [review]
[gaia] CervantesYu:bug_1179353 > mozilla-b2g:master

Code looks alright but you would need unit tests to assert the behavior.
Attachment #8650976 - Flags: review?(timdream) → feedback+
(Assignee)

Comment 11

3 years ago
Comment on attachment 8650976 [details] [review]
[gaia] CervantesYu:bug_1179353 > mozilla-b2g:master

Added test cases to assert the behavior. Request review again.
Attachment #8650976 - Flags: review?(timdream)
Comment on attachment 8650976 [details] [review]
[gaia] CervantesYu:bug_1179353 > mozilla-b2g:master

Nice work!
Attachment #8650976 - Flags: review?(timdream) → review+
(Assignee)

Comment 13

3 years ago
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #12)
> Comment on attachment 8650976 [details] [review]
> [gaia] CervantesYu:bug_1179353 > mozilla-b2g:master
> 
> Nice work!

Thanks for your review.
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/3440b7a506b8f595ebb9ff641775317fcefaa49b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S6 (04Sep)
My original experience no longer occurs in FOTA eng.naoki.20151007.074137 / 0195b184. This is significantly improved. Thanks!
(Assignee)

Updated

3 years ago
Blocks: 1216090
Hi,

I found this piece of code and I wonder if it would have been possible to implement in screen_brightness_transition instead of screen_auto_brightness ? Now we have 2 places that use a timeout to control a nice fading transition...
(Assignee)

Comment 17

2 years ago
(In reply to Julien Wajsberg [:julienw] (PTO -> 2016) from comment #16)
> Hi,
> 
> I found this piece of code and I wonder if it would have been possible to
> implement in screen_brightness_transition instead of screen_auto_brightness
> ? Now we have 2 places that use a timeout to control a nice fading
> transition...

Yes, it would be better if we put brightness control code together, say, screen_brightness_manager.js. This would allow us to gain a better control over screen brightness and screen_manager.js doesn't need to wire together the 2 files that controls brightness with callbacks. I think screen_brightness_transition.js can/should be implemented as new states added to the state machine.
You need to log in before you can comment on or make changes to this bug.