Stencil blendingmode and some speed optimisations#4889
Conversation
- better performance at the cost of about 700 bytes of flash
- Stencil overlay adds an FX on top but leaves the backgroundcolor transparent, this is useful for classic text overlays
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenames blending-style macros to TRANSITION_*, introduces a per-pixel segblend fast path and a new "stencil" blend mode, updates UI to send explicit blend-mode values, simplifies JSON deserialization for segment blendMode, and adjusts inline comments and minor whitespace. No public API signatures changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wled00/FX.h (1)
459-459: Document exact bm codes, incl. Stencil (32) and Average (5).Make the mapping explicit to avoid confusion and to reflect the UI: 0=top, 1=bottom, 2=add, 3=subtract, 4=difference, 5=average, 6=multiply, 7=divide, 8=lighten, 9=darken, 10=screen, 11=overlay, 12=hardlight, 13=softlight, 14=dodge, 15=burn, 32=stencil (uses colors[1] as background mask).
- uint8_t blendMode; // segment blending modes: top, bottom, add, subtract, difference, multiply, divide, lighten, darken, screen, overlay, hardlight, softlight, dodge, burn, stencil + uint8_t blendMode; // segment blending modes (bm): 0=top,1=bottom,2=add,3=subtract,4=difference,5=average,6=multiply,7=divide,8=lighten,9=darken,10=screen,11=overlay,12=hardlight,13=softlight,14=dodge,15=burn,32=stencil (bg=colors[1])wled00/data/index.js (1)
2362-2365: Send bm as a number, not a string.Parse to int before sending to avoid accidental string types and keep consistency with other setters.
- var value = gId(`seg${s}bm`).value; - var obj = {"seg": {"id": s, "bm": value}}; + const value = parseInt(gId(`seg${s}bm`).value, 10); + const obj = {"seg": {"id": s, "bm": value}};wled00/json.cpp (1)
305-305: Validate bm to allowed set (0–15, 32).Removing the clamp enables arbitrary values. segblend defaults to “Top”, but guarding here prevents unexpected UI/API inputs from drifting.
- getVal(elem["bm"], seg.blendMode); + if (getVal(elem["bm"], seg.blendMode)) { + if (seg.blendMode != 32 && seg.blendMode > 15) seg.blendMode = 0; // fallback to Top + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
wled00/FX.h(1 hunks)wled00/FX_fcn.cpp(6 hunks)wled00/data/index.js(2 hunks)wled00/json.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/index.js
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/FX.hwled00/json.cppwled00/FX_fcn.cpp
🧠 Learnings (2)
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (2)
wled00/json.cpp (1)
wled00/util.cpp (2)
getVal(66-83)getVal(66-66)
wled00/FX_fcn.cpp (1)
wled00/colors.cpp (4)
color_add(28-61)color_add(28-28)color_blend(11-21)color_blend(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (2)
wled00/data/index.js (1)
828-829: UI option for Stencil (32) looks good.Matches FX_fcn implementation and JSON handling.
wled00/FX_fcn.cpp (1)
1345-1572: Consolidation into segblend + push/mirror logic: solid.The restructure reduces branching and enables Stencil; coordinate transforms are gated and reused cleanly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
1324-1324: Subtract mode green-channel bug fixedThe green channel now subtracts bottom from top like the others. Thanks for addressing the prior issue.
🧹 Nitpick comments (3)
wled00/FX_fcn.cpp (3)
1317-1343: segblend() consolidation: clear and fast pathSingle switch-based routine reduces indirections and keeps hot code tight. Consider forcing placement/inlining for hot path:
- Add IRAM attribute and inline hint to avoid flash stalls in tight per-pixel loops.
-static uint32_t segblend(CRGBW tcol, CRGBW bcol, CRGBW bgcol, uint8_t blendMode) { +IRAM_ATTR static inline uint32_t segblend(CRGBW tcol, CRGBW bcol, CRGBW bgcol, uint8_t blendMode) {
1339-1340: Stencil semantics: exact color match vs. toleranceEquality on 32-bit color is correct for a pure stencil, but can be brittle if upstream effects introduce minor channel drift. If you see unexpected “holes,” consider (optional) channel-tolerance compare.
- case 32: return tcol.color32 == bgcol.color32 ? bcol.color32 : tcol.color32; // Stencil + case 32: { + // Optional tolerance (e.g. 0..2) to avoid miss due to rounding/dithering + constexpr uint8_t tol = 0; + bool isBG = + (abs(int(tC.r)-int(bgcol.r)) <= tol) && + (abs(int(tC.g)-int(bgcol.g)) <= tol) && + (abs(int(tC.b)-int(bgcol.b)) <= tol) && + (abs(int(tC.w)-int(bgcol.w)) <= tol); + return isBG ? bcol.color32 : tcol.color32; + }
1452-1463: Push offsets computed once; clean modulo handlingEfficient offset math and sign handling. Minor nit: make offset vars int to avoid implicit unsigned wrap on negatives before the modulo.
- unsigned offsetX = (blendingStyle == BLEND_STYLE_PUSH_UP || blendingStyle == BLEND_STYLE_PUSH_DOWN) ? 0 : progInv * nCols / 0xFFFFU; - unsigned offsetY = (blendingStyle == BLEND_STYLE_PUSH_LEFT || blendingStyle == BLEND_STYLE_PUSH_RIGHT) ? 0 : progInv * nRows / 0xFFFFU; + int offsetX = (blendingStyle == BLEND_STYLE_PUSH_UP || blendingStyle == BLEND_STYLE_PUSH_DOWN) ? 0 : int(progInv * nCols / 0xFFFFU); + int offsetY = (blendingStyle == BLEND_STYLE_PUSH_LEFT || blendingStyle == BLEND_STYLE_PUSH_RIGHT) ? 0 : int(progInv * nRows / 0xFFFFU);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/FX_fcn.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/FX_fcn.cpp
🧠 Learnings (2)
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (1)
wled00/FX_fcn.cpp (1)
wled00/colors.cpp (4)
color_add(28-61)color_add(28-28)color_blend(11-21)color_blend(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
🔇 Additional comments (8)
wled00/FX_fcn.cpp (8)
371-374: Palette blend clamp looks goodThe explicit 255 cap avoids pathological loops; safe and clear.
1345-1351: Propagating bgColor for stencil-enabled blendingPlumbing secondary color as background for modes that need it is correct; keeps segblend signature minimal.
1425-1447: 2D mirroring writes: order and bounds look correct
- Single write + mirrored writes use segblend then opacity mix; consistent with 1D path.
- CCT mirror updates are guarded and aligned with pixel writes. LGTM.
1455-1456: applyReverse/applyMirror flags gate transformsGood micro-optimization to avoid unnecessary per-pixel conditionals when no reverse/transpose/mirror is active.
1474-1475: Push mapping before sampling old/new segmentsCorrect to apply push before sampling seg/segO so clipped/fade logic sees the shifted canvas consistently.
1492-1495: Reverse/transpose only when neededDeferred transform after sampling avoids double-mapping. Clean and readable.
1518-1531: 1D path parity with 2D: segblend + opacityMirroring, wrapping with offset, and segblend usage match 2D semantics. Consistency reduces edge-case drift. LGTM.
1613-1615: Gamma application guard aligns with realtime overrideSkipping gamma when ARLS requests it is correct. Note: per prior learnings, effect-level gamma shaping elsewhere is intentional—no changes needed here.
|
A few more comments: The use of CRGBW instead of uint32_t reduces flash size and increases speed, although how exactly eludes me: the two aliases tC / bC don't do anything but make code more compact to read and came from a version where I passed uint32_t and then cast that: if instead of aliases the arguments are used directly, codesize increases by 250bytes, no idea why having aliases makes the compiler happy. one more peculiarity: when I did initial tests in a different branch (heap_handling), using "O2" optimization flag on |
IIRC, often big changes in code size are caused by loop unrolling. |
wled00/FX_fcn.cpp
Outdated
| case 0: return tcol.color32; // Top | ||
| case 1: return bcol.color32; // Bottom | ||
| case 2: return color_add(tcol, bcol, false); // Add | ||
| case 3: c.r = _subtract(tC.r, bC.r); c.g = _subtract(tC.g, bC.g); c.b = _subtract(tC.b, bC.b); c.w = _subtract(tC.w, bC.w); break; // Subtract |
There was a problem hiding this comment.
A bit of a nitpick, but I'd recommend using an inline metafunction or a macro to do the expansion for each channel.
There was a problem hiding this comment.
that would clean up the code significantly, I will check how to do that here.
|
here is the new blending mode in action: Black Hole FX with blur enabled over a background: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
wled00/FX_fcn.cpp (9)
1295-1305: Minor naming/usage mismatch: “unused” helpers are compiled in.
_top,_bottom,_addare marked “function currently unused” and aren’t referenced. Consider removing or markingstatic inline constexprbehind a feature flag to save a few bytes.- static uint8_t _top (uint8_t a, uint8_t b) { return a; } // function currently unused - static uint8_t _bottom (uint8_t a, uint8_t b) { return b; } // function currently unused - static uint8_t _add (uint8_t a, uint8_t b) { unsigned t = a + b; return t > 255 ? 255 : t; } // function currently unused +// #define WLED_DEBUG_BLEND_FUNCS 1 +#ifdef WLED_DEBUG_BLEND_FUNCS +static inline constexpr uint8_t _top (uint8_t a, uint8_t) { return a; } +static inline constexpr uint8_t _bottom (uint8_t, uint8_t b) { return b; } +static inline constexpr uint8_t _add (uint8_t a, uint8_t b) { unsigned t = a + b; return t > 255 ? 255 : t; } +#endif
1320-1321: Prefer an inline function over a macro for type-safety.
APPLY_BLENDrelies on outer names (tC,bC,c) and hides types. Aninlinehelper is safer and can still inline well.-#define APPLY_BLEND(op) c.r = op(tC.r, bC.r); c.g = op(tC.g, bC.g); c.b = op(tC.b, bC.b); c.w = op(tC.w, bC.w) +template<typename Op> +static inline void applyBlendOp(CRGBW& out, const CRGBW& a, const CRGBW& b, Op op) { + out.r = op(a.r, b.r); out.g = op(a.g, b.g); out.b = op(a.b, b.b); out.w = op(a.w, b.w); +}Then:
- case 3: APPLY_BLEND(_subtract); break; + case 3: applyBlendOp(c, tC, bC, _subtract); break;
1322-1348: segblend(): semantics and perf look solid; add force-inline and guard CRGBW conversions.
- Good consolidation of blend modes; stencil (32) behaves as described.
- To keep this hot path in IRAM and reduce call overhead, consider
IRAM_ATTRandalways_inline.- If
color_add()only acceptsuint32_t, relying on implicit CRGBW→u32 conversions is brittle across toolchains.-static uint32_t segblend(CRGBW tcol, CRGBW bcol, CRGBW bgcol, uint8_t blendMode) { +static inline __attribute__((always_inline)) IRAM_ATTR uint32_t segblend(CRGBW tcol, CRGBW bcol, CRGBW bgcol, uint8_t blendMode) { CRGBW tC(tcol); CRGBW bC(bcol); CRGBW c; switch (blendMode) { - case 2: return color_add(tcol, bcol, false); // Add + case 2: return color_add(tcol.color32, bcol.color32, false); // Add (explicit)Verification ask: confirm
CRGBWhas implicit conversion where used, or make all calls explicit (.color32) to avoid accidental overload picks.
1342-1346: Stencil equality: consider optional tolerance to handle palette/gamma drift.Exact 32-bit equality can fail if the top color is dithered or preprocessed. If needed later, add an optional “within N per-channel” compare guarded by a flag.
Would you like a tiny helper that compares channels within a tolerance (e.g., <=1) and is compiled out by default?
1430-1453: 2D mirroring path: index math and CCT writes look correct; avoid repeated segblend bottom fetch.You recompute
segblend(c, _pixels[idx*], ...)per mirror target (necessary because bottoms differ), but_pixels[idx*]is read twice per pixel (once for blend input, once again implicitly in color_blend). Micro-opt: read once per idx and reuse.- _pixels[indx] = color_blend(_pixels[indx], segblend(c, _pixels[indx], bgColor, blendMode), o); + uint32_t base = _pixels[indx]; + uint32_t mixed = segblend(c, base, bgColor, blendMode); + _pixels[indx] = color_blend(base, mixed, o);Same pattern for
idxMX/idxMY/idxMM.
1457-1468: Push offsets: clean and branchless; mask-based wrap is fine.Minor readability nit: unify comment (“shift canvas”) once at the switch header.
1554-1563: On/Off transition workaround still relies on global bri/briT; add a brief comment.Future readers may forget why FADE is special-cased vs others. One-line rationale would help.
1299-1319: Divide/Dodge/Burn edge cases are safe.No divide-by-zero due to guards; semantics are consistent with your chosen definitions. Consider
constexprto help constant-fold.
1322-1348: Unit test coverage for bm=32 (stencil) and regressions on bm=3 suggested.Given prior subtract-mode bug, add minimal tests (or a debug scene) to assert:
- Subtract uses bottom-minus-top across all channels.
- Stencil returns bottom when
top==bg, otherwise top.I can provide a small harness that feeds CRGBW tuples through
segblend()and checks results; want me to drop that intest/?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/FX_fcn.cpp(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/FX_fcn.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (1)
wled00/FX_fcn.cpp (1)
wled00/colors.cpp (4)
color_add(28-61)color_add(28-28)color_blend(11-21)color_blend(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (6)
wled00/FX_fcn.cpp (6)
372-375: Good safety clamp on palette blends.Covers overflow in transition palette blending; no behavior concerns.
1350-1391: blendSegment(): clipping window math is correct; single-pixel segments fallback to FADE is a nice edge-case fix.No issues; good use of dw/dh and inverted ranges for OUTSIDE_IN.
1460-1516: Reverse/transpose gating only when needed: good for perf; group expansion bounds are correct.Use of
groupLength()and min(...) avoids overruns. Looks good.
1531-1536: 1D mirroring path mirrors before primary write; offset wrap is correct.No functional concerns; mirrors respect
offsetand segment bounds.
1575-1577: Restore global blendingStyle: good defensive reset.Prevents leakage into overlay passes.
1618-1620: Gamma guard avoids work on black; confirm realtime/gamma combo.Condition
c > 0 && !(realtimeMode && arlsDisableGammaCorrection)is correct. Verify that “pure white with CCT-only segments” still passes as >0 (it does).If you want, I can craft a quick bench to compare FPS with/without the
c > 0guard on typical scenes.
|
changed to draft, this PR needs to wait until WIP on transparency is done. |
blazoncek
left a comment
blazoncek
left a comment
There was a problem hiding this comment.
I've made my own version of "Stencil" blend mode (with three additional modes) without too much modifications (available in my fork).
Unfortunately code disparity from previous merges prohibits me from supplying a patch without too much effort.
Otherwise, I (personally) dislike the fact that Stencil mode uses constant 32 and the way selection of blending modes is implemented. I have not tested this code.
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
bot
added
the
stale
DedeHai
added
keep
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX_fcn.cpp`:
- Around line 1500-1511: Diagonal push modes (BLEND_STYLE_PUSH_TL/TR/BR/BL)
never set pushOffsetX/pushOffsetY so they act like swipes instead of pushes;
update the switch in FX_fcn.cpp (around the switch handling blendingStyle) to
add cases for BLEND_STYLE_PUSH_TL, BLEND_STYLE_PUSH_TR, BLEND_STYLE_PUSH_BL and
BLEND_STYLE_PUSH_BR and set pushOffsetX and pushOffsetY using the existing
offsetX/offsetY logic (e.g., TL => pushOffsetX = -offsetX + nCols; pushOffsetY =
-offsetY + nRows; TR => pushOffsetX = offsetX; pushOffsetY = -offsetY + nRows;
BR => pushOffsetX = offsetX; pushOffsetY = offsetY; BL => pushOffsetX = -offsetX
+ nCols; pushOffsetY = offsetY) so diagonal pushes actually shift the canvas.
- Around line 1380-1399: The stencil blend (case 16) currently treats any
non-zero top pixel as opaque and ignores the segment background; change it to
compare against the segment background color instead by using
topSegment.colors[1] (bgColor) as the transparent key: ensure bgColor is
declared before the segblend lambda (or otherwise captured) and replace the
condition in blendMode case 16 from "t ? t : b" to "t != bgColor ? t : b" so
true stencil transparency uses the segment background rather than black.
In `@wled00/json.cpp`:
- Line 310: The call getVal(elem["bm"], seg.blendMode) writes integer JSON
values directly into seg.blendMode and you must restore the post-read clamp to
prevent out-of-range values from wrapping; after the getVal(...) call add a
bounds check that constrains seg.blendMode to a maximum (use a shared constant
like SEG_BLENDMODE_MAX declared in a header alongside the blend-mode
declarations) so seg.blendMode = min(seg.blendMode, SEG_BLENDMODE_MAX) (or
equivalent clamp).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a6cb62e9-3bba-45b5-a74c-64ae9d1fda35
📒 Files selected for processing (4)
wled00/FX.hwled00/FX_fcn.cppwled00/data/index.jswled00/json.cpp
✅ Files skipped from review due to trivial changes (2)
- wled00/data/index.js
- wled00/FX.h
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
Looks good to me. 3% speedup is a nice result, too :-)
I am not (yet) an expert for the 16.0 rendering logic, but overall your changes make sense to me 👍
|
@coderabbitai re-review all. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
wled00/FX_fcn.cpp
Outdated
| const int length = topSegment.length(); // physical segment length (counts all pixels in 2D segment) | ||
| const int width = topSegment.width(); | ||
| const int height = topSegment.height(); | ||
| const uint32_t bgColor = topSegment.colors[1]; // background color for blend modes that need it (e.g. stencil) |
There was a problem hiding this comment.
TODO: remove unused variable
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
1506-1511:⚠️ Potential issue | 🟠 MajorDiagonal push styles still fall back to swipe behavior.
BLEND_STYLE_PUSH_TL/TR/BR/BLalready get diagonal clip rects above, but this switch never assigns theirpushOffsetX/Y. They still reach Lines 1521-1522 with zero offsets, so the diagonal variants do not actually push the canvas.🛠️ Suggested fix
switch (blendingStyle) { case BLEND_STYLE_PUSH_RIGHT: pushOffsetX = offsetX; break; case BLEND_STYLE_PUSH_LEFT: pushOffsetX = -offsetX + nCols; break; case BLEND_STYLE_PUSH_DOWN: pushOffsetY = offsetY; break; case BLEND_STYLE_PUSH_UP: pushOffsetY = -offsetY + nRows; break; + case BLEND_STYLE_PUSH_TL: pushOffsetX = offsetX; pushOffsetY = offsetY; break; + case BLEND_STYLE_PUSH_TR: pushOffsetX = -offsetX + nCols; pushOffsetY = offsetY; break; + case BLEND_STYLE_PUSH_BR: pushOffsetX = -offsetX + nCols; pushOffsetY = -offsetY + nRows; break; + case BLEND_STYLE_PUSH_BL: pushOffsetX = offsetX; pushOffsetY = -offsetY + nRows; break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_fcn.cpp` around lines 1506 - 1511, The diagonal push variants (BLEND_STYLE_PUSH_TL, BLEND_STYLE_PUSH_TR, BLEND_STYLE_PUSH_BR, BLEND_STYLE_PUSH_BL) are never assigning pushOffsetX/pushOffsetY in the blendingStyle switch, so they fall back to swipe behavior; update the switch in FX_fcn.cpp to set pushOffsetX and pushOffsetY for those four diagonal cases using the same offset logic as the cardinal directions (e.g., TL: pushOffsetX = -offsetX + nCols; pushOffsetY = -offsetY + nRows; TR: pushOffsetX = offsetX; pushOffsetY = -offsetY + nRows; BR: pushOffsetX = offsetX; pushOffsetY = offsetY; BL: pushOffsetX = -offsetX + nCols; pushOffsetY = offsetY) so the later push logic that reads pushOffsetX/pushOffsetY actually shifts the canvas for diagonal styles.
🧹 Nitpick comments (2)
wled00/data/index.js (1)
801-801: Consider replacing the hardcoded stencil mode ID with a named constant.Using a named constant for
16makes future mode-ID changes safer and easier to track.♻️ Proposed refactor
+const BLEND_MODE_STENCIL = 16; ... - `<option value="16" ${inst.bm==16?' selected':''}>Stencil</option>`+ + `<option value="${BLEND_MODE_STENCIL}" ${inst.bm==BLEND_MODE_STENCIL?' selected':''}>Stencil</option>`+Based on learnings: In WLED code reviews, when code is modified or added, look for "magic numbers" and suggest replacing them with defined constants when meaningful constants exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/index.js` at line 801, The option string uses a magic number 16 for the "Stencil" mode; replace this literal with a named constant (e.g., MODE_STENCIL) so mode IDs are maintainable and discoverable. Define the constant in the module or a shared constants file, then update the template that builds the option (the expression checking inst.bm and the option value) to use that constant instead of 16; ensure any other places comparing or emitting mode ID 16 are updated to reference MODE_STENCIL as well.wled00/FX_fcn.cpp (1)
1367-1388: Replace the raw blend-mode ids/count with named constants.
17and16now need to stay aligned across the clamp, the fast-path switch, and the UI values. Giving them names here will make the next blend-mode addition much less fragile.♻️ Suggested cleanup
-#define BLENDMODES 17 // number of blend modes must match "bm" in index.js, all cases must be handled in segblend() @ blendSegment() +constexpr uint8_t BLEND_MODE_STENCIL = 16; +constexpr uint8_t BLEND_MODE_COUNT = 17; // must stay aligned with the UI values ... - const size_t blendMode = topSegment.blendMode < BLENDMODES ? topSegment.blendMode : 0; // default to top if unsupported mode + const size_t blendMode = topSegment.blendMode < BLEND_MODE_COUNT ? topSegment.blendMode : 0; // default to top if unsupported mode ... - case 16: return t ? t : b; // stencil (use top layer if not black, else bottom) + case BLEND_MODE_STENCIL: return t ? t : b; // stencil (use top layer if not black, else bottom)Based on learnings, in WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_fcn.cpp` around lines 1367 - 1388, Replace the magic numbers used for blend-mode counts/ids with named constants: introduce/use a constexpr or `#define` (e.g., BLEND_MODE_COUNT or BLEND_MODE_STENCIL_ID) and replace literal 17 and 16 in the BLENDMODES define, the clamp for topSegment.blendMode, and the fast-path switch cases inside WS2812FX::blendSegment (references: BLENDMODES, blendSegment, blendMode, segblend, and the switch case labeled 16). Ensure the new constants are defined near related blend-mode enums/definitions so all comparisons and comments (clamp fallback and "stencil" case) use the named symbols instead of raw numbers to keep them in sync when adding new modes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX.h`:
- Line 465: The inline comment for the member variable blendMode in FX.h lists
supported segment blending modes but omits "average", causing the comment to
diverge from actual supported modes; update the comment on uint8_t blendMode to
include "average" among the listed modes so the comment accurately reflects
supported blending options (refer to the symbol blendMode in the FX.h
struct/class when making the change).
---
Duplicate comments:
In `@wled00/FX_fcn.cpp`:
- Around line 1506-1511: The diagonal push variants (BLEND_STYLE_PUSH_TL,
BLEND_STYLE_PUSH_TR, BLEND_STYLE_PUSH_BR, BLEND_STYLE_PUSH_BL) are never
assigning pushOffsetX/pushOffsetY in the blendingStyle switch, so they fall back
to swipe behavior; update the switch in FX_fcn.cpp to set pushOffsetX and
pushOffsetY for those four diagonal cases using the same offset logic as the
cardinal directions (e.g., TL: pushOffsetX = -offsetX + nCols; pushOffsetY =
-offsetY + nRows; TR: pushOffsetX = offsetX; pushOffsetY = -offsetY + nRows; BR:
pushOffsetX = offsetX; pushOffsetY = offsetY; BL: pushOffsetX = -offsetX +
nCols; pushOffsetY = offsetY) so the later push logic that reads
pushOffsetX/pushOffsetY actually shifts the canvas for diagonal styles.
---
Nitpick comments:
In `@wled00/data/index.js`:
- Line 801: The option string uses a magic number 16 for the "Stencil" mode;
replace this literal with a named constant (e.g., MODE_STENCIL) so mode IDs are
maintainable and discoverable. Define the constant in the module or a shared
constants file, then update the template that builds the option (the expression
checking inst.bm and the option value) to use that constant instead of 16;
ensure any other places comparing or emitting mode ID 16 are updated to
reference MODE_STENCIL as well.
In `@wled00/FX_fcn.cpp`:
- Around line 1367-1388: Replace the magic numbers used for blend-mode
counts/ids with named constants: introduce/use a constexpr or `#define` (e.g.,
BLEND_MODE_COUNT or BLEND_MODE_STENCIL_ID) and replace literal 17 and 16 in the
BLENDMODES define, the clamp for topSegment.blendMode, and the fast-path switch
cases inside WS2812FX::blendSegment (references: BLENDMODES, blendSegment,
blendMode, segblend, and the switch case labeled 16). Ensure the new constants
are defined near related blend-mode enums/definitions so all comparisons and
comments (clamp fallback and "stencil" case) use the named symbols instead of
raw numbers to keep them in sync when adding new modes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ecdf17c1-7ac0-4dfb-86d9-4ef07d392c5b
📒 Files selected for processing (4)
wled00/FX.hwled00/FX_fcn.cppwled00/data/index.jswled00/json.cpp
|
@softhack007 thanks for reviewing. I am not sure those FPS improvements are still accurate with the latest implementation but I did check the blending function and it is a solid 10% faster - since the rendering "bottle neck" currently is the "pass pixels to the bus" (in my test: rendering FX: 1-2m, rendering two layers to main buffer (this PR) 3ms, rendering main buffer to two buses: 6ms |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
4 participants
as mentioned in #4550 there is no easy way to overlay text with the new blending modes. this addition fixes that.
I also made some slight optimisations to
blendSegment()and found that the compiler is very unpredictable. I assume that is mostly due to pipelining and core register use. In general, replacing bit-field accessed variables with an alias or a pre-computed value is faster, but here it is not always the case. I tried different variations and this was the best performing one. Replacing any other repeated calculations inblendSegment()does not improve performance.while flash size increases by a few hundred bytes, FPS improve by 3-5% depending on FX and layering used.
Summary by CodeRabbit
New Features
Bug Fixes
Notes