Skip to content

Conversation

@DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Oct 2, 2025

  • if a segment is destroyed or turned inactive, disable the gif player: only one gif player instance can run at a time, if a inactive or destroyed segment uses it, the effect is broken for other segments.
  • copy FX ironically copied the source segment on each call, should use reference not a copy!

Summary by CodeRabbit

  • Bug Fixes
    • Ensures GIF/image playback stops when segments are disabled, resized, or on allocation failure, preventing stuck animations, artifacts, and potential crashes.
    • Corrects segment updates to apply to the active segment (not a temporary copy), improving consistency of effects and drawing behavior across interactions.
    • Improves cleanup when segments are destroyed, ensuring image playback is properly terminated under all supported configurations.

- if a segment is destroyed or turned inactive, disable the gif player: only one gif player instance can run at a time, if a inactive or destroyed segment uses it, the effect is broken for other segments.
- copy FX ironically copied the source segment on each call, should use reference not a copy!
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 2, 2025

Walkthrough

Updates segment handling and cleanup: replaces a local Segment copy with a reference in FX.cpp, and introduces conditional endImagePlayback calls (guarded by WLED_ENABLE_GIF) in the Segment destructor and multiple early-exit/error paths within Segment::setGeometry.

Changes

Cohort / File(s) Change summary
Segment access semantics
wled00/FX.cpp
Switches from copying a Segment to binding a reference (Segment& sourcesegment = strip.getSegment(sourceid);), so subsequent operations act on the original segment.
GIF playback cleanup hooks
wled00/FX.h, wled00/FX_fcn.cpp
Adds endImagePlayback(this) calls under WLED_ENABLE_GIF in the Segment destructor and in Segment::setGeometry early-exit and failure paths (disable, safety checks, allocation failure) before deallocation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Copy Segment FX #4124 — Touches the same segment-access pattern used by mode_copy_segment; closely related to changing Segment copy to reference.
  • Segment fixes #4811 — Modifies Segment geometry and cleanup logic; overlaps with added cleanup in setGeometry and destructor.

Suggested reviewers

  • blazoncek
  • netmindz

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately identifies the two main bugfixes—one for the GIF player handling inactive segments and another for the copy FX reference change—and conveys them concisely in a single sentence without unrelated details.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b73a34 and 359d46c.

📒 Files selected for processing (3)
  • wled00/FX.cpp (1 hunks)
  • wled00/FX.h (1 hunks)
  • wled00/FX_fcn.cpp (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use spaces (2 per level) for all C++ source and header files

Files:

  • wled00/FX_fcn.cpp
  • wled00/FX.h
  • wled00/FX.cpp
🧬 Code graph analysis (2)
wled00/FX_fcn.cpp (1)
wled00/image_loader.cpp (2)
  • endImagePlayback (133-142)
  • endImagePlayback (133-133)
wled00/FX.h (1)
wled00/image_loader.cpp (2)
  • endImagePlayback (133-142)
  • endImagePlayback (133-133)
⏰ 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 (esp8266_2m_compat)
  • GitHub Check: wled_build / Build Environments (nodemcuv2_160)
  • GitHub Check: wled_build / Build Environments (lolin_s2_mini)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
  • GitHub Check: wled_build / Build Environments (esp32_eth)
  • GitHub Check: wled_build / Build Environments (esp32_wrover)
  • GitHub Check: wled_build / Build Environments (esp8266_2m)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
  • GitHub Check: wled_build / Build Environments (usermods)
  • GitHub Check: wled_build / Build Environments (esp32dev)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full)
  • GitHub Check: wled_build / Build Environments (esp32c3dev)
  • GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
  • GitHub Check: wled_build / Build Environments (nodemcuv2)
  • GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
  • GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (6)
wled00/FX.h (1)

628-630: Good call on GIF cleanup.

Ensuring endImagePlayback(this) runs before tearing down segment resources keeps the decoder’s globals from holding onto freed memory. Nice catch.

wled00/FX.cpp (1)

138-139: Switching to a reference fixes copy-FX semantics; verify draw-context toggling

Good change. Using Segment& ensures stateful calls like setDrawDimensions() and getPixelColor*() operate on the real source segment instead of a stale copy. This should resolve the “source re-copied every call” issue and improve performance.

Please double-check:

  • getSegment() indeed returns a stable lvalue reference for the segment lifetime during rendering.
  • The per-pixel setDrawDimensions() swap is balanced: you set it to the source before reading and restore it to SEGMENT before writing. That avoids leaking the “active draw context.” If setDrawDimensions() mutates global state, there’s no leftover state at function exit.

If both are true, LGTM.

wled00/FX_fcn.cpp (4)

451-453: LGTM! GIF playback cleanup on segment disable.

The placement of endImagePlayback(this) before resource deallocation is correct and aligns with the PR objective to stop GIF playback when a segment is turned inactive.


472-474: LGTM! GIF playback cleanup on geometry validation failure.

Correctly stops GIF playback before deallocating resources when segment geometry is invalid.


488-490: LGTM! GIF playback cleanup on allocation failure.

Properly stops GIF playback when pixel buffer allocation fails, maintaining consistency with other cleanup paths.


451-490: Segment destructor includes GIF cleanup. Verified that ~Segment() in wled00/FX.h calls endImagePlayback(this) under #ifdef WLED_ENABLE_GIF.


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 and usage tips.

@DedeHai DedeHai merged commit 3562fa2 into wled:main Oct 2, 2025
20 checks passed
@DedeHai DedeHai deleted the gif_and_copy_bugfixes branch October 2, 2025 18:40
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