809645 - Auto brightness is always too dark

Status: RESOLVED FIXED

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

Description

[Hubert Figuiere [:hub]]

The phone by default automatically set brightness. And it is always too dark IMHO. So I have to disable it.

Comment 1

[Casey Yee]

I agree, the screen is very dim.  The first thing I always do after FRE is change my screen brightness to something reasonable.

Comment 3

[Patryk Adamczyk [:patryk] UX]

We need to fix this, its horrible. Auto dimming should be enabled to reduce blinding people at night (on LCD displays, on OLED displays its more for colour preservation), but since our screen brightness is so poor I suggest the following:
1. Off by default
2. When turned on, its only half as intense (dimmed). Currently it appears to dim the screen all the way down to 0% brightness by default.

BTW this will prevent people from wanting to get this phone. Screen is a huge selling factor, look at our competitors with their beautiful, BRIGHT! screens.

Comment 4

[Patryk Adamczyk [:patryk] UX]

Added UX-P1 as this really diminishes the user experience. You need to go to settings and change an option before you can even use the phone!

Comment 5

[David Scravaglieri [:scravag]]

This depends on the device I guess

Comment 6

[Kevin Grandon :kgrandon]

Appears it's only a one-liner to change the default value: https://github.com/KevinGrandon/gaia/compare/bug_809645_brightness_default

Regarding the second issue:
(In reply to Patryk Adamczyk [:patryk] UX from comment #3)
> 2. When turned on, its only half as intense (dimmed). Currently it appears
> to dim the screen all the way down to 0% brightness by default.

I don't think we can solve this directly. The setting is either turned on or off, and the brightness is adjusted by what the light sensor sees. We may be able to change how we calculate which brightness level to display though.

Comment 7

[David Flanagan [:djf]]

The relevant code is in apps/system/js/screen_manager.js:144

        // This is a rather naive but pretty effective heuristic
        var brightness =
          Math.max(Math.min((evt.value / 1100), this._userBrightness), 0.1);
        if (Math.abs(this._targetBrightness - brightness) > 0.3)
          this.setScreenBrightness(brightness, false);

evt.value is the value that the devicelight sensor gives us.  This is nominally measured in lux and ranges from 0 in a completely dark room to 10240 when held close to bright lightbulb.  So this is a logarithmic scale, and we should probably call Math.log() in the calculation.

Also, I'm suspicious about the 0.3 constant... That seems too big, but I'll have to investigate the brightness adjustment algorithm to be sure.

Comment 8

[David Flanagan [:djf]]

I also don't think that this auto adjustment value should be capped to the user brightness level.  If you set auto brightness in settings, the user brigtness slider goes away, so the user has no idea that their setting might still have an impact.  Certainly we shouldn't cap the brightness to that value.  That's just a bug.

Maybe we could use the user brightness as an adjustment, but even then it doesn't seem right that the user doesn't get to adjust the slider while the auto brightness setting is on.

Comment 9

[David Flanagan [:djf]]

The slider in the settings app allows manual adjustments of brightness between 0.1 and 1.0.  0.1 is comfortable in a completely dark room, so that seems like a reasonable minimum value to use.

While walking around, the value recorded by the light sensor varies substantially depending on what direction you're facing. We don't want to be adjusting the brightness all the time, but I suspect we want to be more sensitive than the 0.3 threshold in the current computation.

Comment 10

[David Flanagan [:djf]]

The current calculation will max out the brightness whenever the light sensor reads more than 1100, which is basically indoor lighting or overcast day values. So if we're too dark with that computation, then there's no way we can be bright enough in full sun....

Actually, I think there's a bug with the 0.3 bit... If the phone ever dims down to 0.75, for example, then if the light sensor tells us to go up to 1.0, we never will because we'll get stuck at 0.75.

That, I think, is the bug.

I think I should do some kind of exponentially weighted average of values from the sensor.  Instead of using the current timeout-based adjustment, I think I should adjust after every reading (about 1 a second) but do a time weighted average so the adjustments aren't too big.

Comment 11

[David Flanagan [:djf]]

The current calculation uses the user's maximum brightness value as a maximum for the auto brightness value as well. Given the way the settings app works, this seems like a serious bug, and I'm going to take it out.  (Its either that, or we need to keep the brightness slider always visible in settings)

On the other hand, it would be nice to make the auto brightness somewhat configurable. What if in power save mode we made the maximum brightness lower? Would that be reasonable?

Patryk, what do you think? Should auto brightness be configurable at all, or just bright, bright, bright?

Comment 12

[David Flanagan [:djf]]

Hmm, I see now that the devicelight event only fires when the ambient light changes. I was thinking it fired every couple of seconds no matter what.  Since it only fires on changes, my weighted time average idea doesn't make sense.  So I'll just use the existing smooth dim/brighten code to adjust.  That actually simplifies things.

Comment 13

[David Flanagan [:djf]]

Attachment: link to patch on github

Tim,

I think you know more about the screen_manager module than anyone. Do you have time for this review?

I think this improves the auto-brightness algorithm, and I think it removes an actual bug that could cause the brightness to get stuck at values like 0.75.

I have not tested this during daylight hours, only in well lit and dimly lit parts of my house at nighttime.

Comment 14

[David Flanagan [:djf]]

Patryk, Josh, Casey: if you can find Tim, you might ask him to demo this patch for you. I think it fixes the bug. But it also makes the brightness algorithm more configurable, so its easy to make tweaks if you're not fully satisfied with it.

Patryk, I'm cancelling my needinfo query for you because, it was out of scope for v1 anyway.

Comment 15

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

\o/ land it land it

Comment 16

[David Flanagan [:djf]]

Comment on attachment 699015 [details]
link to patch on github

Well "\o/ land it" is not quite the same as r+, but it is good enough that I'll request approval-gaia-master to land :-)

Vivien, if this gets a positive review, will you approve it and land it, please?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 809645

User impact if declined: everyone will hate the phone because the screen is too dim, firefox os will flop, and we'll never get another chance to save the mobile web.

Testing completed: I've tested that the screen brightness varies as the ambient light varies.  I've tested that I can sleep and wake the phone.  I haven't tested the interactions with phone calls and such.  I change the location of a clearTimeout() call, but hopefully Tim will review that and think it is okay. 

Risk to taking this patch (and alternatives if risky):

Comment 17

[David Flanagan [:djf]]

Tim in your review note that I moved the clearTimeout() call from one function to another.  And I also changed a targetBrightness to savedBrightness, in what I think was a bug fix. But you might understand that better than I do.

Note that something has regressed recently and if you put the phone to sleep while it is unlocked, it takes a really long time to wake up again.  That is not the result of this patch, though.  I assume someone has filed a bug on that, but haven't searched yet.

Comment 18

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

(In reply to David Flanagan [:djf] from comment #16)
> Comment on attachment 699015 [details]
> link to patch on github
> 
> Well "\o/ land it" is not quite the same as r+, but it is good enough that
> I'll request approval-gaia-master to land :-)
> 
> Vivien, if this gets a positive review, will you approve it and land it,
> please?
> 

I will flash this into a unagi and asking UX for opinion before setting the r+.

Comment 19

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

Comment on attachment 699015 [details]
link to patch on github

Thanks for the patch! I was hesitated to fix this because we might need to tweak it for the final device (given the fact they all come with different screens and light sensors), but I agree with Josh that we could add the config to customize wiki and ask OEMs to tweak it by themselves. :)

I am not sure the reason behind moving from linear to logarithm equation, but it works as desired by turning the overall brightness brighter.

Comment 20

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

https://github.com/mozilla-b2g/gaia/commit/1df210d165f89ff97a8053753eb0660ed989cd68

Comment 21

[Lucas Verney [:Phyks]]

I found on the contrary, on my ZTE Open C (with Firefox OS 1.3) that auto brightness is always too bright.

Maybe it should be better to have a switch to change the mean brightness for the auto brightness feature ?