Skip to content

Move gamma correction down to bus level#5722

Open
DedeHai wants to merge 3 commits into
wled:mainfrom
DedeHai:gamma_to_buslevel
Open

Move gamma correction down to bus level#5722
DedeHai wants to merge 3 commits into
wled:mainfrom
DedeHai:gamma_to_buslevel

Conversation

@DedeHai

@DedeHai DedeHai commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

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:

  • we can do gamma at full resolution in PWM buses. PWM already uses CIE or "gamma corrected" brightness scaling, color gamma is applied to the full PWM resolution (10-14bits) using floats: expensive on ESP8266 but only done for a handful of channels and therefore acceptable.
  • in digital LEDs we can skip the CCT and white calc shenanigans if c==0 - in PS FX which usually have quite a few black pixels, like PS fiere I saw a 5% FPS improvement.

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:

  • 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

Summary by CodeRabbit

  • New Features

    • Improved color and brightness rendering with consistent gamma behavior across multiple LED output types.
    • Enhanced low-brightness handling to reduce harsh transitions toward black.
  • Bug Fixes

    • Fixed gamma correction so it now follows current settings and properly adapts during realtime playback (including temporary disable/restore behavior).
    • Improved per-pixel processing so zero/black pixels skip unnecessary adjustments, and gamma handling occurs at the correct stage for each output mode.

DedeHai added 2 commits July 7, 2026 23:11
- 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
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR replaces the single gammaCorrectCol flag with applyGamma/gammaCorrectVal, moves gamma handling out of WS2812FX::show(), and applies gamma at bus and brightness update points. Config, settings, and realtime paths now update applyGamma.

Changes

Gamma correction refactor

Layer / File(s) Summary
applyGamma/gammaCorrectVal globals and gate logic
wled00/wled.h, wled00/colors.h, wled00/colors.cpp
Adds applyGamma and gammaCorrectVal globals replacing gammaCorrectCol, and updates Correct, Correct32, and inverseGamma32 to gate on applyGamma.
Pixel loop and brightness gamma removal
wled00/FX_fcn.cpp
Removes in-loop gamma correction from WS2812FX::show() and changes WS2812FX::setBrightness() to remap gamma only when brightness is nonzero.
Bus-level gamma and brightness application
wled00/bus_manager.cpp, wled00/bus_manager.h
Adds BusDigital::setBrightness() with inverse-gamma adjustment, refactors BusDigital::setPixelColor() to gate processing behind c > 0, updates BusPwm and BusHub75Matrix pixel handling, and adds a BusNetwork comment.
Config, settings, and realtime wiring of applyGamma
wled00/cfg.cpp, wled00/set.cpp, wled00/udp.cpp
Sets applyGamma from gammaCorrectCol during config and settings updates, and toggles it in realtime entry/exit handling.

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
Loading

Possibly related PRs

  • wled/WLED#4798: Both PRs change gamma correction and brightness handling across FX_fcn.cpp and bus_manager.cpp/h.
  • wled/WLED#5225: Both PRs modify wled00/cfg.cpp gamma-correction configuration loading.
  • wled/WLED#5343: This PR reshapes the gamma-enable flag used by gamma-related paths.

Suggested labels: enhancement, optimization

Suggested reviewers: blazoncek, willmmiles

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: shifting gamma correction to the bus layer.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
wled00/set.cpp (1)

385-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

LGTM!

The realtime-aware expression correctly gates applyGamma on gammaCorrectCol, realtimeMode, arlsDisableGammaCorrection, and realtimeOverride. The realtimeOverride semantics are sound: when override is active (ONCE/ALWAYS), !realtimeOverride is false, so gamma stays enabled regardless of realtime state.

The expression gammaCorrectCol && !(realtimeMode && arlsDisableGammaCorrection && !realtimeOverride) is duplicated verbatim in udp.cpp line 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:385 and udp.cpp:436 become applyGamma = 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

📥 Commits

Reviewing files that changed from the base of the PR and between bc2c80d and 38772c7.

📒 Files selected for processing (9)
  • wled00/FX_fcn.cpp
  • wled00/bus_manager.cpp
  • wled00/bus_manager.h
  • wled00/cfg.cpp
  • wled00/colors.cpp
  • wled00/colors.h
  • wled00/set.cpp
  • wled00/udp.cpp
  • wled00/wled.h

Comment thread wled00/bus_manager.cpp
Comment thread wled00/bus_manager.cpp Outdated
Comment thread wled00/bus_manager.cpp
Comment thread wled00/bus_manager.cpp Outdated
@DedeHai

DedeHai commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

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.
The second thing I forgot to mention: I also fixed a bug in the code of applying "gamma for brightness" which would switch off the LEDs on low brightness.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 38772c7 and 3839694.

📒 Files selected for processing (1)
  • wled00/bus_manager.cpp

Comment thread wled00/bus_manager.cpp
@DedeHai

DedeHai commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

@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

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@DedeHai Here's a proposed test plan covering the areas you flagged as unverified, plus regression checks on what you've already validated:

1. Digital LEDs — RGB (already validated, include for regression)

  • Sweep brightness 0 → 255 (including 1, 2, and values around gamm8inv(2)) on a solid color (e.g., pure red, white-ish RGB) and confirm no flicker/off-switching at low brightness — this targets the "gamma for brightness" bug fix you mentioned.
  • Verify color hue is visually preserved better than pre-PR at low brightness (the main goal of moving gamma to bus level).
  • Toggle applyGamma off/on via settings (gamma correction disabled) and confirm brightness scaling still behaves sanely (no double-gamma, no black-out).

2. Digital LEDs — White/CCT (untested)

  • Test single-white-channel strips (RGBW) at various brightness levels, confirming white channel scales consistently with RGB channels and doesn't clip/black out early.
  • Test CCT strips (ww/cw) across the CCT slider range at low, mid, and full brightness — verify _pixelCCT handling in WS2812FX::show() (now isolated from gamma logic) still produces correct color temperature output.
  • Specifically check the black-pixel skip path (if (c > 0) guard in BusDigital::setPixelColor) doesn't inadvertently skip CCT/white recalculation when only the white channel is non-zero but RGB is zero — confirm c correctly reflects white-only pixels.

3. PWM buses (untested)

  • Verify smooth dimming curve at low duty cycles (1-10%) for visible steps/flicker using the new powf-based gamma curve in BusPwm::show().
  • Compare against pre-PR PWM brightness curve on the same hardware/channel to confirm no regression in perceived linearity.
  • Test full-brightness and full-off edge cases (duty = 0 and duty = max) for correct behavior.
  • Toggle applyGamma off and confirm PWM falls back to the old linear duty formula correctly.

4. HUB75 matrix (untested)

  • Confirm brightness remains driver-level (not double-scaled by the new bus-level gamma) — check for any washed-out or over-dark output vs. baseline.
  • Verify gamma32(c) application in BusHub75Matrix::setPixelColor doesn't conflict with the driver's own gamma/brightness handling (some HUB75 drivers apply their own gamma table).
  • Test at low and high brightness to confirm no clipping or banding artifacts introduced.

5. Network bus (E1.31/DDP/ArtNet, untested)

  • Since gamma is no longer applied before sending (per your TODO), verify receiving devices/controllers show expected (non-gamma-corrected) color — confirm this is documented/expected behavior for existing receiver setups.
  • Test with a receiver that does its own gamma correction vs. one that doesn't, to confirm the behavior change is acceptable/consistent with intent.
  • Regression-test realtime mode entry/exit (realtimeLock()/exitRealtime()) to confirm applyGamma toggles correctly with arlsDisableGammaCorrection and restores properly after realtime mode ends.

6. Cross-cutting checks

  • Config persistence: verify gammaCorrectVal/gammaCorrectCol/applyGamma settings save/load correctly via cfg.cpp and set.cpp paths.
  • Effects with many black pixels (pixel-heavy effects) to confirm the performance optimization (skipping CCT/white calc when c == 0) doesn't introduce visual artifacts on digital LEDs with white/CCT channels.

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 performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@DedeHai DedeHai changed the title Gamma to buslevel Move gamma correction down to buslevel Jul 9, 2026
@DedeHai DedeHai changed the title Move gamma correction down to buslevel Move gamma correction down to bus level Jul 9, 2026
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.

1 participant