Aircoookie / WLED

Control WS2812B and many more types of digital RGB LEDs with an ESP8266 or ESP32 over WiFi!
https://kno.wled.ge
MIT License
15.1k stars 3.27k forks source link

Human-eye linear dimming for PWM strips #2767

Closed mcer12 closed 8 months ago

mcer12 commented 2 years ago

PWM dimming does not look linear to human eye. Not much we can do in terms of addressable strips but it should be very simple to add a lookup table for PWM strips, as a optional setting. This would make low brightness dimming tremendously better and smooth!

Enabling this option would:

  1. set PWM resolution to 12bit (should be enough)
  2. add lookup table for 8bit
  3. lowering PWM frequency a bit might be required for this
mcer12 commented 2 years ago

Here's a great article about this with a python script to generate the lookup table. I personally used this for one of my projects and it's really awesome in practice. It's day and night difference. (don't mind the broken ssl certificate) https://jared.geek.nz/2013/feb/linear-led-pwm

Aircoookie commented 2 years ago

Thank you for the idea! Gamma correction with 2.5 gamma is actually really close to the CIE1931 formula (except that the latter gives a higher output at low brightness levels, which might be nice). In any case, it seems beneficial to go with a gamma of 2.5 instead of the current 2.8, as that aligns much closer to perceived brightness. Maybe we should swap the current gamma table for the CIE table entirely, though I'm not sure if that would improve or worsen low brightness hue accuracy.

Regardless, I agree that a PWM resolution of 12 or even just 10 would help a lot with low brightness color accuracy.

Capture

mcer12 commented 2 years ago

@Aircoookie CIE1931 sounds like the proper way to do things. With addressable strips, it doesn't really matter which one we pick because on low brightness they are all useless anyway. From my personal projects, to archieve really smooth 255 step pwm dimming, we need 12bit table, 10bit sounds like it's good enough but it's really not, it's exponential so on brightness below 10 it will still be choppy. I guess it would be a sensible compromise if there were memory constraints but ESP32 and ESP8266 both should handle the calculation without lookup table if that's the case. Excuse me if some of the suggestions are already implemented since I am not familiar with the code yet.

mcer12 commented 2 years ago

@Aircoookie It's worth a note that Philips Hue (and probably others) also use CIE1931 since it's an industry standard. So it should make WLED more accurate and compatible with other lights.

softhack007 commented 2 years ago

Found this recently, where a lookup table is used for achieving LED brightness curves that look linear to the human eye.

Looks easy: https://github.com/Zanduino/SmoothLED_8bit https://github.com/Zanduino/SmoothLED_10bit/wiki/CIE1931-Compensation https://github.com/Zanduino/SmoothLED_10bit

A common misconception is that gamma correction should be used, as it does have a very similar response to the eye. However, gamma correction has nothing to do with how humans perceive light, it is just coincidence that it appears to work. The CIE 1931 lightness formula is what actually describes how we perceive light.

mcer12 commented 2 years ago

@softhack007 I'm sorry but this is totally off topic, it's atmega code, useless with ESP. ESP32 does 12bit pwm natively so we don't need any interrupts, just a lookup table so it's even more simple here. It's possible to use https://github.com/StefanBruens/ESP8266_new_pwm for esp8266 but that would require reimplementation of pwm and it's not clear if it would be compatible with the rest of the WLED code (because of HW interrupts) so to me it makes sense to do this ESP32 exclusive which should be a minor change.

softhack007 commented 2 years ago

@softhack007 I'm sorry but this is totally off topic, it's atmega code, useless with ESP.

Why? Its essentially a lookup-table. That's not specific to atmega or ESP32. My idea was just to use this mapping table for "WLED brightness" -> PWM brightness, and use the resulting number for the PWM driver.

If you think that's off-topic, no problem, just go ahead and don't let me disturb your circles...

Edit: I was thinking about this table, not the rest of the code: https://github.com/Zanduino/SmoothLED_8bit/blob/8fc9b85842597b3af0eb4c3a8ef013306f15a737/src/SmoothLED.h#L77-L91 or https://github.com/Zanduino/SmoothLED_10bit/blob/896d341e34c661b04d212d9e16965525d0f6cd46/src/SmoothLED.h#L88-L150

a small script to generate a table is in the code comments, so it should be simple to make a similar static LUT for 12-bit or other resolutions.

softhack007 commented 2 years ago

@mcer12 wrote

Here's a great article about this with a python script to generate the lookup table. I personally used this for one of my projects and it's really awesome in practice. It's day and night difference. (don't mind the broken ssl certificate) https://jared.geek.nz/2013/feb/linear-led-pwm

Seems i overlooked that you made a very similar proposal a few weeks ago. What you said was also totally off topic, I presume?

mcer12 commented 2 years ago

@softhack007 No actually my issue is not off-topic and please don't be combatitive, it's not very productive. I would love to see how exactly the 8bit lookup table that you shared solves choppiness at low brightness. Because this is already how WLED works (gamma correction), just calculated without lookup table. What I proposed and what we're talking about here is 12bit lookup table with 255 values which results in prefectly smooth 8bit linear dimming even at low brightness.

mcer12 commented 2 years ago

@softhack007 10bit makes it little bit better but doesn't solve the issue whatsoever. All you really have to do is take a look at first few values in the the lookup table (lowest brightness values) to see the issue.

softhack007 commented 2 years ago

Hi @mcer12 ok, this was also what I had in mind. Thanks for explaining.

Personally, I find it not very helpful to get slammed as "off-topic", and then see that the same idea was expressed "on-topic" already. But OK, i'm not angry about that any more. So let's forget it.

mcer12 commented 2 years ago

@softhack007 mate if you had the same idea then why you're sharing a library that doesn't implement that idea? :D The library does exactly what WLED already does. If the library mapped 10bit values to 8bit, then that would have been a differnt conversation. If you took it as slamming then I'm sorry but you were off topic so I don't know what to tell you.

softhack007 commented 2 years ago

Yes I can see your problem that you are facing.

Just to clarify what's needed: is the question a) how to get from "WLED brightness" (8 bit) to "PWM" brightness (eg. 12 bit) b) get from higher resolution future WLED brightness (eg 12bit ) down to PWM 8 bit?

Case b) does not seem very usefull, because the lost resolution (-4bit) combined with any kind d of calibration curve will lead to the same bad results in low brightness ranges as if you came from 8bit. In case a) finding the best PWM resolution that allows to preserve details in the low brightness ranges might be possible. In this case, I can imagine that existing scripts/formulas can be used to make a LUT, either reprenting each input value or use interpolation to make the LUT smaller.

mcer12 commented 2 years ago

@softhack007 you're not explaining it very clearly, I guess that a) is probably correct. We need to generate LUT with 255 values 0-4095. Using 12bit to generate human-eye linear 8bit.

mcer12 commented 2 years ago

@softhack007 ideally you want to come up with LUT that has unique value for every step. 12bit should be enough for that. But again, since WLED uses some kind of gamma formula (as discussed in my original issue), we can use that. I would only use lut if the formula calculation causes stutter but that shouldn't be an issue.

softhack007 commented 2 years ago

@mcer12 just looked at the WLED code below. (But not 100% sure that's the gamma function you refer to).

https://github.com/Aircoookie/WLED/blob/5f4199183c3782b43aca43743c083c3af26cd4ed/wled00/colors.cpp#L323-L357

I think you can't use that. The reason is simple - the function is from 8bit to 8bit. So it has the same problems that you mentioned already - low values get crushed to zero, and i see no way to recover from that. Maybe a modified version of gamma8_cal() and calcGammaTable() can be used to make a table that is uint16_t instead of byte, and utilizes the complete output range of 0...4095.

Also from performance perspective there is no way around using a LUT, because the gamma formula needs powf() which is really slow. Way too slow for realtime computations - considering that you'll have to calculate powf() for each pixel, with >50 updates per pixel per second.

If a new table is needed any way, then it should be calculated using the CIE1931 formula instead of gamma, because the result will be better especially in the low value ranges; see comparison plot posted by @Aircoookie above.

softhack007 commented 2 years ago

Maybe we should swap the current gamma table for the CIE table entirely, though I'm not sure if that would improve or worsen low brightness hue accuracy.

@Aircoookie maybe we could add both options, I.e. a drop-down for color (brightness?) correction "none", "CIE", and "gamma 2.5" "gamma 2.8". At least to get feedback from users, to better understand which calibration curve looks best.

When I see the current gamma table in colours.cpp, I actually get a bit nervous as all values below 28 get mapped to zero. The same table in CIE might still kill the lowest 10 values, but that's already better than losing 28.

mcer12 commented 2 years ago

@softhack007 Yeah I didn't check WLED code if it's actually calculated or using LUT as it doesn't really matter at this point since we're just shooting ideas. Note that at this point I'm not familiar with WLED code whatsoever, will be checking it out during the weekend, maybe I'll poop out a PR. I really want it to be a minor change that keeps compatibility with everything.

Easiest thing to do IMO would be to generate 12bit LUT (CIE) as I said and just divide it by 16 for addressable lights and ESP8266. Esp shouldn't have an issue with such a minor calculation. I think we should just switch to CIE as it's an industry standard.

softhack007 commented 2 years ago

Agreed, especially as integer divide by 16 is fast because the compiler can simply do a right-shift by 4 bits (make sure to use unsigned, because right-shift on signed is undefined behaviour).

Curious to see the PR when it poops out;-)

mcer12 commented 2 years ago

I quickly checked the code and it doesn't look good. I expected that gamma is applied on low level right before pushing the pixel color / analogWrite. But it seems like it's applied on high level and many functions depend on 8bit. I guess it could be possible to pass uncorrected colors to BusPwm class and apply 12bit lookup separately inside there but that seems quite messy. @Aircoookie any ideas?

mcer12 commented 2 years ago

Btw here are CIE1931 lookup tables 8bit and 12bit. As you can see with 12bit all values are unique => smooooth transition! I tried 10bit but as expected, there is some chopinness. I also tried 11bit and it would work just as smooth as 12bit but seems non-standard... and 12bit is probably more future-proof.

const uint8_t cie[256] = {
    0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 
    1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 
    2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 
    3, 4, 4, 4, 4, 4, 4, 4, 5, 5, 
    5, 5, 5, 6, 6, 6, 6, 6, 7, 7, 
    7, 7, 8, 8, 8, 8, 9, 9, 9, 10, 
    10, 10, 10, 11, 11, 11, 12, 12, 12, 13, 
    13, 13, 14, 14, 15, 15, 15, 16, 16, 17, 
    17, 17, 18, 18, 19, 19, 20, 20, 21, 21, 
    22, 22, 23, 23, 24, 24, 25, 25, 26, 27, 
    27, 28, 28, 29, 30, 30, 31, 31, 32, 33, 
    33, 34, 35, 35, 36, 37, 38, 38, 39, 40, 
    41, 41, 42, 43, 44, 44, 45, 46, 47, 48, 
    49, 50, 50, 51, 52, 53, 54, 55, 56, 57, 
    58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 
    68, 69, 70, 71, 72, 74, 75, 76, 77, 78, 
    79, 81, 82, 83, 84, 85, 87, 88, 89, 91, 
    92, 93, 94, 96, 97, 99, 100, 101, 103, 104, 
    106, 107, 109, 110, 111, 113, 115, 116, 118, 119, 
    121, 122, 124, 126, 127, 129, 130, 132, 134, 135, 
    137, 139, 141, 142, 144, 146, 148, 150, 151, 153, 
    155, 157, 159, 161, 163, 165, 167, 169, 170, 172, 
    174, 177, 179, 181, 183, 185, 187, 189, 191, 193, 
    195, 198, 200, 202, 204, 207, 209, 211, 213, 216, 
    218, 220, 223, 225, 227, 230, 232, 235, 237, 240, 
    242, 245, 247, 250, 252, 255
};
static const uint16_t cie1931[256] = {
    0, 2, 4, 5, 7, 9, 11, 13, 15, 16, 
    18, 20, 22, 24, 26, 27, 29, 31, 33, 35, 
    34, 36, 37, 39, 41, 43, 45, 47, 49, 52, 
    54, 56, 59, 61, 64, 67, 69, 72, 75, 78, 
    81, 84, 87, 90, 94, 97, 100, 104, 108, 111, 
    115, 119, 123, 127, 131, 136, 140, 144, 149, 154, 
    158, 163, 168, 173, 178, 183, 189, 194, 200, 205, 
    211, 217, 223, 229, 235, 241, 247, 254, 261, 267, 
    274, 281, 288, 295, 302, 310, 317, 325, 333, 341, 
    349, 357, 365, 373, 382, 391, 399, 408, 417, 426, 
    436, 445, 455, 464, 474, 484, 494, 505, 515, 526, 
    536, 547, 558, 569, 580, 592, 603, 615, 627, 639, 
    651, 663, 676, 689, 701, 714, 727, 741, 754, 768, 
    781, 795, 809, 824, 838, 853, 867, 882, 897, 913, 
    928, 943, 959, 975, 991, 1008, 1024, 1041, 1058, 1075, 
    1092, 1109, 1127, 1144, 1162, 1180, 1199, 1217, 1236, 1255, 
    1274, 1293, 1312, 1332, 1352, 1372, 1392, 1412, 1433, 1454, 
    1475, 1496, 1517, 1539, 1561, 1583, 1605, 1628, 1650, 1673, 
    1696, 1719, 1743, 1767, 1791, 1815, 1839, 1864, 1888, 1913, 
    1939, 1964, 1990, 2016, 2042, 2068, 2095, 2121, 2148, 2176, 
    2203, 2231, 2259, 2287, 2315, 2344, 2373, 2402, 2431, 2461, 
    2491, 2521, 2551, 2581, 2612, 2643, 2675, 2706, 2738, 2770, 
    2802, 2835, 2867, 2900, 2934, 2967, 3001, 3035, 3069, 3104, 
    3138, 3174, 3209, 3244, 3280, 3316, 3353, 3389, 3426, 3463, 
    3501, 3539, 3576, 3615, 3653, 3692, 3731, 3770, 3810, 3850, 
    3890, 3930, 3971, 4012, 4053, 4095
};
softhack007 commented 2 years ago

@mcer12 yes that looks promising. Even the the 8bit version seems to preserve more details in the lowest brightness values, compared to gamma. 👍

Aircoookie commented 2 years ago

I would propose two things:

What do you think?

blazoncek commented 2 years ago

I quickly checked the code and it doesn't look good. I expected that gamma is applied on low level right before pushing the pixel color / analogWrite.

Didn't I say so? 😉

One thing that comes to mind is to move gamma correction entirely to bus manager and make gamma checkboxes strip independent. Then it can be applied per-bus in an independent fashion, possibly using different LUT for different bus type.

This would increase bus configuration complexity, though.

But would also have the benefit of sending uncorrected color/brightness information to virtual LEDs over DDP to have them use different LUT if configured so.

EDIT: Looks like I've done the separation already.

blazoncek commented 2 years ago

Re-checked the code.

We have calcGammaTable() that will fill gamma LUT with new values calculated on the fly (not used anywhere ATM). If you do not mix analog & digital LEDs it could be done by just adjusting this function to account for CIE LUT. Possibly read from JSON file on startup. The only change needed here would be to change byte gammaT[] to uint16_t gammaT[] and add something like uint16_t maxGammaV to tell what is the maximum value in gamma LUT.

Actual brightness for analog is calculated using C * _bri / 255, C being channel value and _bri being strip/global gamma adjusted brightness (in strip.setBrightness()).

So if WS2812FX::_brightness and Bus::_bri are converted to uint16_t there is a possibility to use them in combination with calcGammaTable() to accommodate nicer brightness changes without major disruption of other code.

blazoncek commented 8 months ago

@mcer12 you are invited to test better low brightness PWM handling in 0.15-b1. Available on Discord only ATM.

blazoncek commented 8 months ago

Implemented in c0beb621e26a5aea017fe20f2f501175a1e33474

blazoncek commented 8 months ago

Available in 0.15.0 beta 1.

mcer12 commented 7 months ago

@blazoncek I just tried it and it looks much better. Testing this on ESP32 board. From what I understand from your changes, only ESP32 variants use the LUT, ESP8266 uses the original gamma correction. That said, you seem to be using some other calculation, not CIE1931 because if I generate the same lut with CIE1931 formula, the tables don't match.

EDIT: Just a suggestion, it would be good to add a little note to what calculation is used, so it's not just mystery values :)

blazoncek commented 7 months ago

That said, you seem to be using some other calculation, not CIE1931 because if I generate the same lut with CIE1931 formula, the tables don't match.

EDIT: Just a suggestion, it would be good to add a little note to what calculation is used, so it's not just mystery values :)

// using gamma 1.7 to extrapolate PWM

This was the closest to CIE LUT, with decent performance and not using any RAM.

EDIT: I was looking for a formula to get CIE values but was unable to find one. If you have one, you can make a PR and substitute gamma extrapolation.

mcer12 commented 7 months ago

@blazoncek Sorry for the confusion... I was asking about the LUT as it looks different to what I just generated but now I see you actually used the LUT I shared in this thread.

This is my newly generated lut, using the same script and it should be correct. Not sure why the original is different, maybe it's a rounding issue, no idea.

const uint16_t cie[256] = { 0, 2, 4, 5, 7, 9, 11, 12, 14, 16, 18, 20, 21, 23, 25, 27, 28, 30, 32, 34, 36, 37, 39, 41, 43, 45, 47, 49, 52, 54, 56, 59, 61, 64, 66, 69, 72, 75, 77, 80, 83, 87, 90, 93, 96, 100, 103, 107, 111, 115, 118, 122, 126, 131, 135, 139, 144, 148, 153, 157, 162, 167, 172, 177, 182, 187, 193, 198, 204, 209, 215, 221, 227, 233, 239, 246, 252, 259, 265, 272, 279, 286, 293, 300, 308, 315, 323, 330, 338, 346, 354, 362, 371, 379, 388, 396, 405, 414, 423, 432, 442, 451, 461, 470, 480, 490, 501, 511, 521, 532, 543, 553, 564, 576, 587, 598, 610, 622, 634, 646, 658, 670, 683, 695, 708, 721, 734, 748, 761, 775, 788, 802, 816, 831, 845, 860, 874, 889, 904, 920, 935, 951, 966, 982, 999, 1015, 1031, 1048, 1065, 1082, 1099, 1116, 1134, 1152, 1170, 1188, 1206, 1224, 1243, 1262, 1281, 1300, 1320, 1339, 1359, 1379, 1399, 1420, 1440, 1461, 1482, 1503, 1525, 1546, 1568, 1590, 1612, 1635, 1657, 1680, 1703, 1726, 1750, 1774, 1797, 1822, 1846, 1870, 1895, 1920, 1945, 1971, 1996, 2022, 2048, 2074, 2101, 2128, 2155, 2182, 2209, 2237, 2265, 2293, 2321, 2350, 2378, 2407, 2437, 2466, 2496, 2526, 2556, 2587, 2617, 2648, 2679, 2711, 2743, 2774, 2807, 2839, 2872, 2905, 2938, 2971, 3005, 3039, 3073, 3107, 3142, 3177, 3212, 3248, 3283, 3319, 3356, 3392, 3429, 3466, 3503, 3541, 3578, 3617, 3655, 3694, 3732, 3772, 3811, 3851, 3891, 3931, 3972, 4012, 4054, 4095, };

Here's what you can use, just change resolution: https://gist.github.com/mathiasvr/19ce1d7b6caeab230934080ae1f1380e