Move gamma correction down to bus level#5722
Conversation
- add new global bool to check if gamma needs to be applied - restrict digital LED brightness for better color preservation - move from gamma then brightness to "restricted" brightness then gamma (except for HUB75 where brightness is at driver level - use full resolution gamma on PWM buses - skip black pixels in busDigital setPixelColor - fix brightness calculation when using gammaCorrectBri
WalkthroughThis PR replaces the single ChangesGamma correction refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WS2812FX
participant BusDigital
participant BusPwm
participant BusHub75Matrix
WS2812FX->>BusDigital: setPixelColor(c)
BusDigital->>BusDigital: if c>0 apply brightness/gamma/CCT/ABL
WS2812FX->>BusDigital: setBrightness(b)
BusDigital->>BusDigital: apply gamma8inv(_bri+1) if applyGamma
WS2812FX->>BusPwm: setPixelColor(c)
BusPwm->>BusPwm: gamma32(c) then CCT/auto-white
BusPwm->>BusPwm: show() computes duty via gamma curve if applyGamma
WS2812FX->>BusHub75Matrix: setPixelColor(c)
BusHub75Matrix->>BusHub75Matrix: gamma32(c) then update buffers
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
wled00/set.cpp (1)
385-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM!
The realtime-aware expression correctly gates
applyGammaongammaCorrectCol,realtimeMode,arlsDisableGammaCorrection, andrealtimeOverride. TherealtimeOverridesemantics are sound: when override is active (ONCE/ALWAYS),!realtimeOverrideis false, so gamma stays enabled regardless of realtime state.The expression
gammaCorrectCol && !(realtimeMode && arlsDisableGammaCorrection && !realtimeOverride)is duplicated verbatim inudp.cppline 436. Consider extracting a small helper to prevent future divergence:♻️ Optional: extract shared helper for applyGamma computation
// In a shared header or wled.h: static inline bool computeApplyGamma() { return gammaCorrectCol && !(realtimeMode && arlsDisableGammaCorrection && !realtimeOverride); }Then both
set.cpp:385andudp.cpp:436becomeapplyGamma = computeApplyGamma();.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/set.cpp` around lines 385 - 386, The `applyGamma` realtime-aware condition is duplicated in both `set.cpp` and the corresponding logic in `udp.cpp`, so extract it into a shared helper to keep the behavior consistent. Add a small function such as `computeApplyGamma()` in a common location used by both call sites, and have both `set.cpp` and `udp.cpp` assign `applyGamma` through that helper so future changes to `gammaCorrectCol`, `realtimeMode`, `arlsDisableGammaCorrection`, or `realtimeOverride` only need to be made once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/bus_manager.cpp`:
- Around line 481-483: The PWM path is applying gamma twice:
`BusPwm::setPixelColor` currently gamma-corrects before storing into `_data[]`,
and `BusPwm::show` later applies `powf(..., gammaCorrectVal)` again. Remove the
early `gamma32` conversion in `setPixelColor` so `_data[]` stays linear, and
keep the gamma step only in `show()` when driving the PWM output.
- Around line 269-273: The BusDigital::setBrightness method is remapping a true
off value through gamma8inv when _bri is 0, which can leave digital buses driven
at a nonzero brightness. Update the brightness handling in
BusDigital::setBrightness so that an input brightness of 0 stays 0 and only
nonzero values are gamma-corrected when applyGamma is enabled, preserving true
off behavior for digital LEDs.
- Around line 276-285: Move the `_valid` check in `BusDigital::setPixelColor` so
it runs before any pixel remapping or color handling, rather than only inside
the `if (c > 0)` block. This ensures invalid buses return early for black writes
as well, preventing `PolyBus::setPixelColor()` from being called with `_busPtr`
and avoiding underflow in the `_reversed` mapping logic. Keep the fix localized
to `BusDigital::setPixelColor` and preserve the existing brightness/gamma flow
after the guard.
- Around line 290-293: The CCT packing in bus_manager.cpp is collapsing most
values because the current arithmetic in the hasCCT() block only preserves bytes
at the extremes. Update the packing logic in the code that builds wwcw for
TYPE_WS2812_WWA so cctCW and cctWW are combined into the intended 16-bit value
without masking or shifting away the low bits; keep the brightness scaling
behavior intact and ensure the resulting RGBW32(...) assignment receives nonzero
white-channel data across the normal range.
---
Nitpick comments:
In `@wled00/set.cpp`:
- Around line 385-386: The `applyGamma` realtime-aware condition is duplicated
in both `set.cpp` and the corresponding logic in `udp.cpp`, so extract it into a
shared helper to keep the behavior consistent. Add a small function such as
`computeApplyGamma()` in a common location used by both call sites, and have
both `set.cpp` and `udp.cpp` assign `applyGamma` through that helper so future
changes to `gammaCorrectCol`, `realtimeMode`, `arlsDisableGammaCorrection`, or
`realtimeOverride` only need to be made once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74c3b597-34ff-46a7-ab8f-b689db35a150
📒 Files selected for processing (9)
wled00/FX_fcn.cppwled00/bus_manager.cppwled00/bus_manager.hwled00/cfg.cppwled00/colors.cppwled00/colors.hwled00/set.cppwled00/udp.cppwled00/wled.h
|
what I am not too sure about is the network bus: it now dow not send gamma corrected colors as it used to, see the TODO, it would be a one liner to add it back. My thought process was to not do it as receivers can decide to apply it or not. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/bus_manager.cpp`:
- Around line 283-293: The color processing order in bus_manager.cpp is wrong:
gamma32() is being applied before autoWhiteCalc() and colorBalanceFromKelvin(),
which changes the RGB result and the derived WW/CW channels. Update the logic in
the c > 0 path of the bus handling code so brightness and color
temperature/white conversion happen first, then apply gamma correction last
before final output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2084cd41-2e55-4f02-8adb-6f99ca3b27b2
📒 Files selected for processing (1)
wled00/bus_manager.cpp
|
@coderabbitai please propose a test plan, I already confirmed this working on digital LEDs but not on PWM, HUB75 or network. I also did not test any digital LEDs with white or CCT |
|
1. Digital LEDs — RGB (already validated, include for regression)
2. Digital LEDs — White/CCT (untested)
3. PWM buses (untested)
4. HUB75 matrix (untested)
5. Network bus (E1.31/DDP/ArtNet, untested)
6. Cross-cutting checks
Let me know if you'd like me to help draft any specific test firmware builds or scripts to automate parts of this (e.g., a debug effect that sweeps brightness/color for visual verification). ✅ Action performedReview finished.
|
Proposal for #5707
The way color dimming is done currently is a bit of a hack: apply gamma to colors, then scale them down using video scale with hue correction results in some color artefacts. The math sais to do it the other way but up to this point that resulted in color loss and very bad low brightness color resolution. I ran a lot of different tests using different gamma corrections, CIE (or sRGB) approaches with linear tails of gamma and all of them were not satisfactory. So I circled back to doing scaling before gamma which results in better colors but has the issue stated above. To fix it, we need to limit brightness which I implemented in this PR. The actual "trick" is to limit brightness such that we retain at least 2 color steps i.e. final_bri = gamm8inv(2) or larger. This is a bit of a breaking change, up for discussion.
The benefit is that gamma can now be moved down to bus level and has two positive side effects:
minor downside: the brightness slider translates to gamma8inv() table so not every brightness step yields a brightness change.
@softhack007 please check if this fits your bill ;)
changes:
Summary by CodeRabbit
New Features
Bug Fixes