gifdecoder bugfixes, up to 30% speedup, and minor improvements #279
Conversation
* fixed a bug that caused wrong behavior with segment mirroring
(effects must use virtualHeight() / virtualWidth() instead of height() / width())
* added image blur as an option (second slider)
* added very basic error reporting for users
* up to 25% faster, especially with big animated gifs
* made all local variables "static" (don't pollute global namespace)
* drawPixelCallback: cache calculation that do not depend on x/y position
* reduced memory allocations on boards without PSRAM, to avoid crashes
WalkthroughShort-circuits image mode for non-matrix or very-small segments; updates FX_MODE_IMAGE metadata; refactors GIF/image rendering into file-local statics with PSRAM-aware decoder allocation, per-segment scaling/upscaling, new file I/O callbacks, enhanced error/OOM handling, painting optimizations, and thorough cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant FX as FX::mode_image
participant R as ImageLoader
participant D as GIFDecoder
participant P as draw2DPixelCallback
participant U as updateScreenCallback
FX->>FX: check matrix && max(virtW,virtH) >= 4
alt not matrix or too small
FX-->>FX: return mode_oops()
else
FX->>R: start render for segment
R->>R: set segCols/segRows, compute expandX/expandY
R->>D: openGif(filename) & alloc (PSRAM-aware)
alt alloc fail or invalid file
D-->>R: error / DECODER_ALLOC
R-->>FX: abort playback
else
D->>P: decode -> draw2DPixelCallback(pixel)
P->>P: nearest-neighbor map, bounds check, skip same pixel
P->>R: paint into segment buffer
loop per frame
D->>U: frame complete -> timing
U->>R: optional blur, reset last pos
end
R->>R: cleanup (close file, dealloc, reset state)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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.cpp (1)
142-145: Add WLED_DISABLE_2D guard around Akemi fallback to prevent build breaksCalling mode_2DAkemi_core(false) unguarded will fail with -D WLED_DISABLE_2D, since Akemi isn’t compiled in that config. Restore the guard to match intended behavior.
Apply this diff:
// 2D fallback: akemi in blue - if (SEGMENT.is2D() && (width > 3) && (height > 3)) - return mode_2DAkemi_core(false); + #ifndef WLED_DISABLE_2D + if (SEGMENT.is2D() && (width > 3) && (height > 3)) + return mode_2DAkemi_core(false); + #endifBased on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.cpp(2 hunks)wled00/image_loader.cpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/FX.cppwled00/image_loader.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (2)
wled00/FX.cpp (1)
wled00/FX_fcn.cpp (1)
SEGMENT(737-812)
wled00/image_loader.cpp (1)
wled00/FX_fcn.cpp (4)
i(714-724)i(714-714)i(725-731)i(725-725)
⏰ 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). (20)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
🔇 Additional comments (10)
wled00/FX.cpp (2)
4696-4703: 2D-only gate and small-matrix bail look good
- Enforces matrix-only use and avoids tiny panels; switches to virtualWidth()/virtualHeight() as required by mirroring fix. LGTM.
Minor: confirm the cutoff “< 4” is intentional (Akemi fallback uses >3). If 3×N matrices are supported elsewhere, consider aligning thresholds.
4709-4709: FX string: Blur slider added—confirm mapping and defaults“Image@!,Blur,;;;12;sx=128,ix=0” adds Blur as the second slider. Please verify:
- Which field the renderer reads for blur (Intensity vs customN), and that this label matches it.
- That ix=0 is the desired default (not overly blurring or disabling filtering inadvertently).
If the renderer expects a custom slider, consider relabeling/mapping accordingly (e.g., c2) to avoid user confusion.
wled00/image_loader.cpp (8)
12-20: LGTM! Static declarations reduce namespace pollution.The refactoring to static linkage encapsulates the decoder state properly and the PSRAM-conditional sizing helps prevent OOM crashes on constrained boards.
42-56: LGTM! Good observability and consistent static pattern.The side-effect comment and debug output improve maintainability. The additional static rendering state variables follow the same encapsulation pattern.
62-71: LGTM! Well-designed blur feature with performance optimization.The conditional blur strategy (fast 1D
blurRowsfor low intensity, slower 2Dblurfor high intensity) provides a good balance between visual quality and performance. ThelastX/lastYinvalidation correctly resets the downscaling optimization state.
82-87: LGTM! Loop reordering improves cache locality.The Y-outer/X-inner loop order is optimal for row-major pixel storage, which is typical for LED matrices. This should provide the claimed performance improvement.
108-109: LGTM! Correct fix for segment mirroring.Using
virtualWidth()andvirtualHeight()instead ofwidth()andheight()properly accounts for segment transformations including mirroring. This addresses the bug mentioned in the PR objectives.
116-125: LGTM! Improved error reporting for users.The enhanced error messages with
USER_PRINTFprovide better feedback when GIFs fail to load or have unsupported formats, addressing the "basic error reporting" goal mentioned in the PR.
166-178: LGTM! Robust frame decode error handling and timing compensation.The frame decode error handling (lines 166-170) properly logs failures and returns appropriate error codes. The frame timing compensation (lines 173-174) correctly prevents integer underflow when frames run longer than intended, with the ternary operator providing safe clamping to zero.
134-146: Rewrite issue: decoder.alloc() OOM error handling is unimplemented, despite acknowledged riskThe review comment's analysis is accurate. While the code acknowledges that
decoder.alloc()may throw OOM and crash (line 134), there is no error handling in place. The commented-out check at line 151 (//if (!decoder)...) shows a previous attempt to handle allocation failures, but it was disabled or removed. Currently, onlystartDecoding()errors are caught (lines 138-145);alloc()failures bypass all error handling.Since GifDecoder.h is an external library not present in the repository, the solution depends on whether its
alloc()method supports error recovery (return codes or exceptions). Recommend either:
- Wrapping
decoder.alloc()in try-catch if the library can throw exceptions, or- Investigating why the line 151 check was disabled and restoring appropriate error handling if the library provides error detection.
catch error early before starting the decode/paint loop
... by adding an exception handler that will catch OOM inside decoder.alloc(). not tested yet.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/image_loader.cpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/image_loader.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/image_loader.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/image_loader.cpp
🧬 Code graph analysis (1)
wled00/image_loader.cpp (1)
wled00/FX_fcn.cpp (4)
i(714-724)i(714-714)i(725-731)i(725-725)
⏰ 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). (20)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
if seg.name is empty or shorter than four characters, ``strlen(lastFilename) - 4`` underflows (size_t), so the pointer passed to strcmp lands far before the buffer and triggers undefined behavior. This patch catches too-short segment names and aborts decoding.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/image_loader.cpp (1)
114-119: Complete the extension check fix by using the cached length.Line 115 computes
fnameLenbut then callsstrlen(lastFilename)again in thestrcmp. This is inefficient and defeats the purpose of caching the length.Apply this diff:
size_t fnameLen = strlen(lastFilename); - if ((fnameLen < 4) || strcmp(lastFilename + strlen(lastFilename) - 4, ".gif") != 0) { + if ((fnameLen < 4) || strcmp(lastFilename + fnameLen - 4, ".gif") != 0) { gifDecodeFailed = true;
🧹 Nitpick comments (1)
wled00/image_loader.cpp (1)
135-158: Robust error handling with helpful user messaging.The exception handling for
decoder.alloc()(lines 135-146) and comprehensive error checking fordecoder.startDecoding()(lines 149-156) significantly improve reliability. The specific error codes and user-facing messages align well with the PR's objective of adding basic error reporting.Minor suggestion: Line 143's message "I'm going to shoot myself now" could be more professional:
- USER_PRINTLN("\nGIF decoder out of memory. I'm going to shoot myself now.\n"); + USER_PRINTLN("GIF decoder out of memory\n");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/image_loader.cpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/image_loader.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/image_loader.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/image_loader.cpp
🧬 Code graph analysis (1)
wled00/image_loader.cpp (1)
wled00/FX_fcn.cpp (4)
i(714-724)i(714-714)i(725-731)i(725-725)
⏰ 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). (20)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
🔇 Additional comments (6)
wled00/image_loader.cpp (6)
12-20: Good namespace hygiene with static scoping.Moving these variables to file-local static scope prevents global namespace pollution and aligns with the PR objectives. The PSRAM-aware decoder sizing is a sensible approach to avoid OOM crashes on constrained boards.
50-56: Static rendering state variables support the performance optimizations.These file-local statics cache rendering state and enable the optimizations described in the PR (especially the lastX/lastY pixel-skip optimization and pre-calculated expand factors). The comment explaining why
gifWidthandgifHeightremainuint16_tis helpful.
62-71: Blur functionality implementation looks solid.The two-tier blur approach (fast
blurRowsfor smaller amounts on 2D matrices, slowerblurfor larger amounts) is a good performance trade-off. Invalidating thelastX/lastYcache after blur ensures correctness for the next frame.
72-88: Downscaling and upscaling optimizations are well-implemented.The nearest-neighbor downscaling with the
lastX/lastYskip optimization effectively reduces redundant pixel writes. The unsigned cast at line 77 elegantly handles both negative coordinates and out-of-bounds checks. The loop order change for better cache locality is a nice touch.
108-109: Correct fix for segment mirroring bug.Using
virtualWidth()andvirtualHeight()instead ofwidth()andheight()properly accounts for segment mirroring, directly addressing the bug mentioned in the PR objectives.
172-182: Division by zero guard successfully addresses past review concerns.The dimension validation at lines 174-178 properly protects against division by zero in both the
expandX/expandYcalculations (lines 180-181) and thedrawPixelCallbackdivisions (lines 74-75). The comprehensive error handling with user-facing messages completes the fix from previous review rounds.
* use camelCase for local variables * furure support: rename drawPixelCallback to draw2DPixelCallback
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/image_loader.cpp (2)
115-116: Redundant strlen() call after computing fnameLen.Line 115 computes
fnameLenbut line 116 callsstrlen(lastFilename)again. Use the cached value for efficiency.Apply this diff:
size_t fnameLen = strlen(lastFilename); - if ((fnameLen < 4) || strcmp(lastFilename + strlen(lastFilename) - 4, ".gif") != 0) { + if ((fnameLen < 4) || strcmp(lastFilename + fnameLen - 4, ".gif") != 0) {
136-147: Exception handling is well-structured, but error message may alarm users.The try-catch block correctly handles OOM during allocation. However, the message "I'm going to shoot myself now" (line 144) might unnecessarily alarm users. Consider a more reassuring message like "GIF decoder out of memory. Please try a smaller image or free up memory."
Apply this diff:
- USER_PRINTLN("\nGIF decoder out of memory. I'm going to shoot myself now.\n"); + USER_PRINTLN("GIF decoder out of memory. Please try a smaller image.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/image_loader.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/image_loader.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/image_loader.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/image_loader.cpp
🧬 Code graph analysis (1)
wled00/image_loader.cpp (1)
wled00/FX_fcn.cpp (4)
i(714-724)i(714-714)i(725-731)i(725-725)
⏰ 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: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
🔇 Additional comments (10)
wled00/image_loader.cpp (10)
12-20: PSRAM-aware decoder configuration looks good.The conditional compilation correctly selects smaller decoder parameters for boards without PSRAM, reducing memory pressure and preventing crashes.
26-36: File I/O callbacks are correctly implemented.The callbacks provide a clean interface between the decoder and the file system, delegating to the static
fileobject.
42-48: Enhanced logging in openGif improves debugging.The addition of the filename to debug output helps track file operations.
50-57: Per-segment rendering state properly encapsulated.The static variables track rendering state across frames while avoiding global namespace pollution, aligning with the PR's objectives.
63-72: Blur implementation elegantly uses the intensity slider.The two-tier approach (fast
blurRowsfor light blur on 2D, slowerblurfor heavier amounts) balances performance and quality. ResettinglastX/lastYensures downscaling cache is fresh for the next frame.
73-88: Downscaling optimization and cache locality improvements are effective.The
lastX/lastYcache (line 78) correctly skips redundant pixel writes during downscaling. The Y-outer/X-inner loop order (lines 83-84) improves cache locality for row-major memory layouts. The unsigned cast bounds check (line 77) elegantly handles both negative values and out-of-range coordinates.
108-110: Critical fix: using virtualWidth/virtualHeight correctly handles segment mirroring.This addresses the bug mentioned in the PR objectives. The virtual dimensions account for mirroring and rotation, ensuring correct rendering.
150-158: Comprehensive error handling with appropriate error flag mapping.The code correctly captures decoder errors, logs them, and maps specific error codes (TOO_WIDE, UNSUPPORTED_FEATURE, INVALID_PARAMETER) to memory error flags. Line 158 properly resets previous memory error flags on success.
173-182: Zero-dimension guard resolves division-by-zero risks.The validation at lines 175-179 prevents division by zero in both the
draw2DPixelCallback(lines 75-76) and the upscaling calculations (lines 181-182). The ceiling division pattern correctly ensures proper pixel coverage during upscaling.
185-189: Frame decode error handling is consistent and complete.The error handling properly captures decode failures, sets the failure flag, logs the error code, and returns the appropriate error constant.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/image_loader.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/image_loader.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/image_loader.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/image_loader.cpp
🧬 Code graph analysis (1)
wled00/image_loader.cpp (1)
wled00/FX_fcn.cpp (4)
i(714-724)i(714-714)i(725-731)i(725-725)
⏰ 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). (19)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_V4_S)
If seg.name is at least 32 characters long, strncpy will fill the 32-byte window without appending '\0'. This could lead to sporadic errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/image_loader.cpp (1)
66-69: Clarify the threshold for blur method selection.The magic number
24determines whether to use fastblurRowsor slowerblur. Consider extracting this to a named constant or adding a comment explaining why 24 is the threshold.- if ((blurAmount < 24) && (activeSeg->is2D())) activeSeg->blurRows(activeSeg->intensity >> 1); // some blur - fast + const uint8_t BLUR_THRESHOLD = 24; // threshold between fast row-based blur and full 2D blur + if ((blurAmount < BLUR_THRESHOLD) && (activeSeg->is2D())) activeSeg->blurRows(activeSeg->intensity >> 1); // some blur - fast
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/image_loader.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/image_loader.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/image_loader.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/image_loader.cpp
🧬 Code graph analysis (1)
wled00/image_loader.cpp (1)
wled00/FX_fcn.cpp (4)
i(714-724)i(714-714)i(725-731)i(725-725)
⏰ 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: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
🔇 Additional comments (9)
wled00/image_loader.cpp (9)
12-20: LGTM: Static scope and PSRAM-aware decoder sizing.The conversion to
staticscope prevents namespace pollution, and the conditional decoder template parameters appropriately reduce memory usage on boards without PSRAM.
42-48: LGTM: Debug logging and clear side-effect documentation.The added logging aids troubleshooting, and the comment clearly documents the function's side-effect on the static
filevariable.
50-57: LGTM: Cached rendering state enables the performance optimizations.These static variables support the per-segment scaling and "cached calculations that do not depend on x/y position" mentioned in the PR objectives.
73-88: LGTM: Downscaling logic is correct with guards in place.The division by
gifWidth/gifHeightis now safe thanks to the zero-dimension guard at lines 176-180. The unsigned cast for bounds checking is elegant, thelastX/lastYoptimization reduces redundant writes, and the reordered loops improve cache locality.
108-110: LGTM: Correct fix for segment mirroring bug.Using
virtualWidth()andvirtualHeight()instead ofwidth()andheight()properly accounts for segment mirroring, as noted in the PR objectives.
114-128: LGTM: File validation now safe and user-friendly.The explicit null termination (line 114) and length guard (line 116) address the critical issues from past reviews. The enhanced error messages support the "very basic error reporting for users" mentioned in the PR objectives.
151-159: LGTM: Comprehensive error handling for decoder initialization.The error code capture, specific error-to-flag mapping, and flag reset on success provide good error reporting as mentioned in the PR objectives.
175-183: LGTM: Division by zero guard addresses past critical issues.The zero-dimension validation (lines 176-180) prevents division by zero in both
draw2DPixelCallbackand theexpandX/expandYcalculations, resolving the critical issues from previous reviews.
186-190: LGTM: Frame decode error handling is consistent and thorough.Proper error detection, flag setting, logging, and error code return.
no fun, just rabbits. Lets stay professional.
| gifDecodeFailed = false; | ||
| if (strcmp(lastFilename + strlen(lastFilename) - 4, ".gif") != 0) { | ||
| size_t fnameLen = strlen(lastFilename); | ||
| if ((fnameLen < 4) || strcmp(lastFilename + fnameLen - 4, ".gif") != 0) { // empty segment name, name too short, or name not ending in .gif |
There was a problem hiding this comment.
open ends (will not fix in this PR):
- compare should be case-insensitive - ".GIF", ".Gif", ".gIf" should be accepted.
esp32 does not have "stricmp", so we might need a custom solution, maybe with a utility function that internally usestolower(c)before comparing. - strip leading / trailing blanks from the filename.
gifdecoder bugfixes, up to 30% speedup, and minor improvements - several optimizations for the drawing a single pixel (hot path) - Image mode now runs correctly on 2D/matrix layouts with mirroring. - added an extra slider for Blur intensity; - improved per-segment scaling/upscaling and memory-aware decoder behavior on low-RAM devices. - Robust filename validation, graceful failures for missing/unsupported files or low-memory, better decoding error handling and playback cleanup. - Better user messages open ends (will not fix in this PR): * ".gif" filename compare should be case-insensitive - ".GIF", ".Gif", ".gIf" should be accepted. esp32 does not have "stricmp", so we might need a custom solution, maybe with a utility function that internally uses tolower(c) before comparing. Also strip leading / trailing blanks from the filename.
fixed a bug that caused wrong behavior with segment mirroring
(effects must use virtualHeight() / virtualWidth() instead of height() / width())
added image blur as an option (second slider)
added very basic error reporting for users
up to 25% faster, especially with big, animated gifs
made all local variables "static" (don't pollute global namespace)
drawPixelCallback: cache calculations that do not depend on x/y position
reduced memory allocations on boards without PSRAM, to avoid crashes. This may lead to error messages when trying opening GIFs > 64x64 pixels, however boards without PSRAM are not able to handle more pixels any way.
Summary by CodeRabbit
New Features
Bug Fixes