Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backlight: support nonlinear brightness #882

Merged
merged 1 commit into from
Oct 23, 2020

Conversation

dancek
Copy link
Contributor

@dancek dancek commented Oct 19, 2020

The raw brightness values are quite useless on some hardware. Add an
option to scale the raw backlight PWM values on a power scale.

Since there is no single correct exponent even in the CIELAB L* function
and the mapping between raw hardware brightness value and relative
luminance is unknown, the implementation allows user to fine-tune the
exponent.

@dancek
Copy link
Contributor Author

dancek commented Oct 19, 2020

Oops! I just realized I didn't update the docs.

@ammgws
Copy link
Collaborator

ammgws commented Oct 19, 2020

Out of interest, what kind of values does your hardware output? Is it the same kind of issue as #642?

In any case the scaling is an interesting idea.

@dancek
Copy link
Contributor Author

dancek commented Oct 19, 2020

#642 is different.

My hardware is a Thinkpad T490, with intel_backlight and a max_brightness of 24242. FWIW the best setting I've found on my hardware is scale_root = 2.7.

I got the idea from https://github.com/arsv/xcubiclight but after reading up a bit realized that hardcoding a choice between 1.0 and 3.0 is not the best solution.

@ammgws
Copy link
Collaborator

ammgws commented Oct 21, 2020

What is the range of values you get for the below files? 0 to max_brightness(24242)? Just trying to understand the behaviours of different hardware..

/sys/class/backlight/xxx/actual_brightness
/sys/class/backlight/xxx/brightness

@dancek
Copy link
Contributor Author

dancek commented Oct 21, 2020

@ammgws yes, the range is 0 to 24242. brightness and actual_brightness tend to have the same value, but there are discontinuities:

# pwd
/sys/devices/pci0000:00/0000:00:02.0/drm/card0/card0-eDP-1/intel_backlight
# for i in $(seq 24242); do echo "$i" > brightness; diff -yW20 brightness actual_brightness | grep '|'; sleep 0.001; done
32    |	31
96    |	95
159   |	160
223   |	224
287   |	288
351   |	350
415   |	414
478   |	479
542   |	543
606   |	607
670   |	669
734   |	733
797   |	798
861   |	862
925   |	926
989   |	988
<snip>
24019 |	24018
24083 |	24082
24146 |	24147
24210 |	24211

I first thought the discontinuities are from polling actual_brightness before the hardware has settled, but no: echo 32 > brightness always results in 31 for actual_brightness.

@ammgws
Copy link
Collaborator

ammgws commented Oct 21, 2020

So IIRC the current code would take your range of 0 to 24242 and scale it into a 0 to 100 percentile range, and when you scroll on the block it would lower or raise the brightness by x percent. So as it is now, you block is usable in terms of being able to go from 0% to 100% brightness. Is this correct so far?

However, you find that the percentage displayed is not necessarily best at representing the brightness level your eyes perceive, so you want to introduce this new scale_root option to solve that?

Sounds reasonable to me, it's kind of like the natural_mapping option for the sound block.

@dancek
Copy link
Contributor Author

dancek commented Oct 22, 2020

@ammgws yes, exactly. The minimum brightness is at value 1, the maximum at value 24242. The midpoint is visually something like 3000, so with the normal linear mapping most of the dynamic range falls in between 1..10%.

What's worse, the 1% value 242 is much, much brighter than the hardware minimum of 1 (with a cubic scale it's around 20% brightness). I can't scroll to 0% but 0 turns the backlight off.

By now I'm not sure if the documentation I've written is very good. And maybe there's a better name than scale_root that I haven't thought of.

@ammgws
Copy link
Collaborator

ammgws commented Oct 22, 2020

Maybe something with "natural" like the other blocks, or maybe something with "nonlinear"?

@dancek
Copy link
Contributor Author

dancek commented Oct 22, 2020

nonlinear_scale_root maybe? I'd like to include root or exponent because that's what the parameter really is.

Another way to do this is to have an enum. I actually first implemented scale = "linear" / scale = "cubic" but I noticed that an exponent of 1/2.6 is much better for me than 1/3.0. And from my understanding this will vary depending on hardware and lighting conditions. Of course an easy-to-understand, easy-to-configure system has its benefits.

@ammgws
Copy link
Collaborator

ammgws commented Oct 22, 2020

@GladOSkar What do you think?

@GladOSkar
Copy link
Contributor

The general idea seems very reasonable (and also applicable to my usage, thx!) to me.

As to the naming for the root parameter i don't have strong preferences. natural in the sound block refers to that specific feature in ALSA so i don't think we should draw a parallel there although with the right settings it could be somewhat fitting as brightness perception is very much logarithmic in humans.

I think nonlinear_scale_root is fine, or since it's technically the root index that is changed scale_root_index but that sounds a bit too abstract. Actually i think something simple like root_scaling might be best.

@ammgws
Copy link
Collaborator

ammgws commented Oct 22, 2020

root_scaling sounds good to me, @dancek how about you?

The raw brightness values are quite useless on some hardware. Add an
option to scale the raw backlight PWM values on a power scale.

Since there is no single correct exponent even in the CIELAB L* function
and the mapping between raw hardware brightness value and relative
luminance is unknown, the implementation allows user to fine-tune the
exponent.
@dancek
Copy link
Contributor Author

dancek commented Oct 23, 2020

Sounds good to me. Here's a rebased version of the commit with root_scaling throughout.

@ammgws
Copy link
Collaborator

ammgws commented Oct 23, 2020

Thanks!

@ammgws ammgws merged commit 4574cea into greshake:master Oct 23, 2020
@GladOSkar
Copy link
Contributor

Just tried it and i love it, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants