Skip to content

fix(system): round rgb_to_luminance so white maps to 255#1006

Merged
mairas merged 1 commit into
mainfrom
fix/crgb-luminance-rounding
Jun 16, 2026
Merged

fix(system): round rgb_to_luminance so white maps to 255#1006
mairas merged 1 commit into
mainfrom
fix/crgb-luminance-rounding

Conversation

@mairas

@mairas mairas commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Bug

rgb_to_luminance(255, 255, 255) returned 254. The BT.709 weights sum to a hair under 1.0 in floating point, and the cast to uint8_t truncates, so pure white fell just short of 255.

Fix

Round to nearest with std::lround (clean per clang-tidy's bugprone-incorrect-roundings). Verified the red/green/blue assertions are unaffected.

How it was found

On-device execution of test_crgb (test_rgb_to_luminance_white — "Expected 255 Was 254"); compile-only CI can't catch it. Passes on-device after this change.

The luminance weights sum to just under 1.0 in floating point, so the
truncating cast mapped pure white to 254. Round to nearest instead.
@mairas

mairas commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

Automated review

Verdict: Approve. The fix is correct, minimal, and well-targeted. Source-traced all assertions; only white changes (254→255), no regressions.

Verification (source-traced, IEEE-754 double)

  • Bug confirmed. The BT.709 weighted sum for white evaluates to 254.99999999999997 (the nearest representable weights sum to just under 1.0), so the truncating cast yields 254. std::lround rounds it to 255. Confirmed by compiling the exact expression.
  • No collateral change to existing assertions. Truncation vs. lround for the tested inputs: black 0/0, red 54/54, green 182/182, blue 18/18 — identical. All still satisfy the test expectations (EQUAL(0), EQUAL(255), INT_WITHIN(1, …)).
  • green_dominant ordering preserved. Under lround: red(100)=21, green(100)=72 (was 71 under trunc), blue(100)=7. Ordering green>red, green>blue, red>blue holds; the green bump only widens the margin.
  • Overflow/clamp safe. Max input (255,255,255) → exactly 255, fits uint8_t. std::lround returns long; the cast is safe for the 0..255 range this function produces.
  • #include <cmath> correctly added for std::lround.

Notes (non-blocking)

  • The existing test_rgb_to_luminance_white already uses TEST_ASSERT_EQUAL(255, …), which is precisely the assertion that was failing pre-fix and passes post-fix — so the regression guard is already in place. No new test needed.
  • Comment quality is good: it explains why (weights sum just under 1.0, white would map to 254), not what changed. Conforms to the explain-the-why convention.
  • Minimal and localized; no other truncating luminance call sites exist in the file.

@mairas mairas merged commit 34f874f into main Jun 16, 2026
22 checks passed
@mairas mairas deleted the fix/crgb-luminance-rounding branch June 16, 2026 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant