Feat: Segment masks #324
Feat: Segment masks #324
Conversation
📝 WalkthroughWalkthroughAdds per-segment mask support: segment mask state and lifecycle, filesystem loading/parsing of segmaskN.json, UI controls and JSON (de)serialization, render-time gating for masked pixels (1D/2D), and enumeration/init hooks to register available masks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend UI
participant App as WLED App
participant Seg as Segment
participant FS as Filesystem
participant Render as Renderer
UI->>App: setMask(seg, id)
App->>App: requestJson({seg:{id,mask}})
App->>Seg: deserializeSegment() with "mask"
Seg->>Seg: setMask(maskId)
Seg->>FS: open segmask{maskId}.json
FS-->>Seg: return JSON (w,h,inv,bitarray)
Seg->>Seg: parse, validate, alloc _mask, set _maskValid
Seg-->>App: mask applied (state updated)
App->>UI: update UI state
Note over Render,Seg: During render loop
Render->>Seg: setPixelColorXY(x,y,color)
Seg->>Seg: maskAllowsXY(x,y)?
alt allowed
Seg->>Seg: apply color
else masked
Seg-->>Seg: skip pixel
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX_fcn.cpp (1)
94-103: Mask reload path missing after copy/assignment.Copy ctor/assignment null out
_mask(and_maskValid) but preservemaskId/maskInvertvia memcpy. When rendering,maskAllows()checksif (!_mask || !_maskValid) return true;, so copied segments silently skip mask application even thoughmaskIdremains set. There is no automatic reload path—the effect must manually callsetMask(maskId)after copying, ormaskIdshould be cleared.
🤖 Fix all issues with AI agents
In `@wled00/FX_fcn.cpp`:
- Around line 253-260: _maskValid is only set once when loading the mask and can
become stale if virtual geometry changes; update the validity check by
recomputing it whenever geometry may change (for example at start of setUp() and
startFrame() or immediately before any mask application routine). Locate where
_maskValid is used (e.g., in FX_fcn::_maskValid checks and any applyMask/paint
routines) and replace the single-time assignment with a runtime check that
compares _maskW/_maskH to calc_virtualWidth()/calc_virtualHeight() (or reassign
_maskValid at the start of setUp()/startFrame()) so the mask validity reflects
current virtual geometry. Ensure no behavioral change besides recomputing
validity.
- Around line 150-176: The clearMask()/setMask() flow can free or replace _mask
while maskAllows() may be reading it on the other core; to fix, serialize the
pointer swap/free with the renderer by acquiring the same mutex used by the
render loop (e.g., segmentMux) around the critical section in Segment::clearMask
and Segment::setMask (before freeing or assigning _mask and updating
_maskLen/_maskW/_maskH/_maskValid/maskId) or alternatively wait for the strip to
be idle using the existing idle/wait API before changing the buffer; locate and
protect the critical sections in Segment::clearMask, Segment::setMask and
anywhere maskAllows reads _mask to prevent use-after-free.
🧹 Nitpick comments (2)
wled00/json.cpp (1)
364-372: Prefer an explicit clear path whenmaskis 0.
This makes intent obvious and avoids any unintended file load ifsetMask(0)isn’t a no-op.♻️ Proposed tweak
if (elem.containsKey("mask")) { // WLEDMM segment mask id int maskVal = elem["mask"] | 0; if (maskVal < 0) maskVal = 0; uint8_t maskId = constrain(maskVal, 0, WLED_MAX_SEGMASKS-1); - if (maskId != seg.maskId || (maskId != 0 && !seg.hasMask())) seg.setMask(maskId); + if (maskId == 0) { + if (seg.hasMask()) seg.clearMask(); + } else if (maskId != seg.maskId || !seg.hasMask()) { + seg.setMask(maskId); + } } if (elem.containsKey("minv")) { // WLEDMM segment mask invert seg.maskInvert = elem["minv"] | seg.maskInvert; }wled00/FX.h (1)
725-741: Consider adding[[gnu::hot]]attribute for hot-path optimization.The mask accessor logic is correct:
- Fail-open design (returns
truewhen mask is absent/invalid) prevents black pixels on error- Bounds checking prevents buffer overread
- Bit-packing/unpacking is efficient and correct (LSB-first)
Since
maskAllows()andmaskAllowsXY()are called for every pixel during rendering, consider adding the[[gnu::hot]]attribute (likeprogress()at line 692 andcurrentBri()at line 700) to hint the compiler for optimization.♻️ Suggested optimization
- inline bool hasMask(void) const { return _mask != nullptr; } // WLEDMM - inline bool maskAllows(uint16_t i) const { // WLEDMM + [[gnu::hot]] inline bool hasMask(void) const { return _mask != nullptr; } // WLEDMM + [[gnu::hot]] inline bool maskAllows(uint16_t i) const { // WLEDMM if (!_mask || !_maskValid) return true; if (size_t(i) >= _maskLen) return false; // WLEDMM: bit-packed mask (LSB-first): byte = i>>3, bit = i&7 bool bit = (_mask[i >> 3] >> (i & 7)) & 0x01; return maskInvert ? !bit : bit; } - inline bool maskAllowsXY(int x, int y) const { // WLEDMM + [[gnu::hot]] inline bool maskAllowsXY(int x, int y) const { // WLEDMM if (!_mask || !_maskValid) return true; if (x < 0 || y < 0) return false; size_t idx = size_t(x) + (size_t(y) * _maskW); if (idx >= _maskLen) return false; // WLEDMM: row-major (x + y*w), bit-packed mask (LSB-first in each byte) bool bit = (_mask[idx >> 3] >> (idx & 7)) & 0x01; return maskInvert ? !bit : bit; }
…with lambda; Remove gnu::hot flags on inline function definitions
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX_fcn.cpp (1)
94-103: Reset mask ID and invert flag when clearing mask buffer on segment copy.After
memcpy, both the copy constructor and copy assignment operator null_maskand reset mask buffer state, but leavemaskIdandmaskInvertfrom the source. This causes the copied segment to report a mask ID that has no actual buffer applied. Reset these to matchclearMask()behavior:Fix for both locations
_maskW = 0; // WLEDMM _maskH = 0; // WLEDMM _maskValid = false; // WLEDMM + maskId = 0; // WLEDMM + maskInvert = false; // WLEDMM
…in copy constructor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/FX_fcn.cpp`:
- Around line 171-176: Segment::setMask currently calls clearMask() but proceeds
even if clearMask failed to release the existing mask, which can leak the old
buffer; after calling clearMask() in Segment::setMask, check the _mask member
and abort (return false) if it is still non-null to avoid overwriting/leaking
the previous buffer—use the existing Segment::clearMask() call, then immediately
guard with a check on _mask before continuing (also preserve the existing id==0
behavior).
| bool Segment::setMask(uint8_t id) { // WLEDMM | ||
| clearMask(); | ||
| if (id >= WLED_MAX_SEGMASKS) { | ||
| return false; | ||
| } | ||
| if (id == 0) return true; |
There was a problem hiding this comment.
Abort mask load if clearMask() couldn't clear.
If clearMask() fails to acquire segmentMux, _mask can remain non-null; setMask() can then overwrite it, leaking the old buffer. Add a guard to bail out when _mask is still set.
🛠️ Suggested guard
bool Segment::setMask(uint8_t id) { // WLEDMM
clearMask();
+ if (_mask) {
+ DEBUG_PRINTLN(F("Segment::setMask: clearMask failed, aborting."));
+ return false;
+ }
if (id >= WLED_MAX_SEGMASKS) {
return false;
}🤖 Prompt for AI Agents
In `@wled00/FX_fcn.cpp` around lines 171 - 176, Segment::setMask currently calls
clearMask() but proceeds even if clearMask failed to release the existing mask,
which can leak the old buffer; after calling clearMask() in Segment::setMask,
check the _mask member and abort (return false) if it is still non-null to avoid
overwriting/leaking the previous buffer—use the existing Segment::clearMask()
call, then immediately guard with a check on _mask before continuing (also
preserve the existing id==0 behavior).
1 participant
Overview
I'm designing a fancy sign for a craft fair booth with an engraved sheet of acrylic in front. In order to properly light it, I would like to define segments for various shapes in the engraved sheet such that I can apply different effects to each area. Currently, support for this is limited to ledmaps (only one across all segments), jMaps (1D -> 2D translation only), and grouping (only works for specific designs)
This PR adds support for clipping masks per segment, defined in virtual LED space. This enables overlapping segments to render distinct effects/palettes while preserving normal segment geometry and mappings.
Summary of changes
How it works
Segmask.json
Masks are defined by 1/0 values in a list similar to ledmapN.json files:
{"w":16,"h":32,"mask":[0,0,0,1,1,1,0,0,0,0,...,0,1,1],"inv":false}Other Notes
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.