GIF Playback #226
GIF Playback #226
Conversation
|
TODO: tidy up lib_deps and device which builds we include this with |
WalkthroughThe changes integrate GIF handling support into the project. The configuration file now includes two new library dependencies and a build flag to enable GIF functionality. The effect system has been updated to include a new image effect mode that renders .gif images from the filesystem, replacing a previous Halloween mode. Additional modifications include function declarations for file operations and the implementation of GIF image loading and playback cleanup within a new file. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant E as ImageEffect
participant S as Segment
participant L as ImageLoader
U->>E: Activate Image Effect
E->>S: Update segment with GIF mode
alt GIF enabled
S->>L: Call openGif(filename)
L->>S: Render frame via renderImageToSegment()
S->>L: Reset segment → call endImagePlayback()
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (4)
wled00/FX.h (1)
189-189: New effect mode ID assigned to “Image.”
The definition#define FX_MODE_IMAGE 53correctly introduces the new image effect mode. If backward compatibility with any removed mode is needed, consider using a different ID or implement a migration path.wled00/FX_fcn.cpp (1)
272-293: Consider adding error handling for GIF cleanup.While the GIF cleanup integration looks good, consider adding error handling to catch and log any potential failures during GIF cleanup operations.
#ifdef WLED_ENABLE_GIF - endImagePlayback(this); + if (!endImagePlayback(this)) { + // Log error or set error flag + DEBUG_PRINTLN(F("Failed to cleanup GIF resources")); + } #endifwled00/FX.cpp (1)
4664-4667: Remove commented debug code.The commented debug print statements should be removed as they are not being used.
- // if (status != 0 && status != 254 && status != 255) { - // Serial.print("GIF renderer return: "); - // Serial.println(status); - // }platformio.ini (1)
264-265: Review of GIF Library Dependencies Addition:
The two new library entries:
•bitbank2/AnimatedGIF@^1.4.7
•https://github.com/Aircoookie/GifDecoder#bc3af18
have been added to the globallib_depssection. This change clearly aligns with the PR’s goal to integrate GIF playback support.
- Compatibility Check: Ensure these library versions are compatible with your other dependencies and target platforms.
- Conditional Inclusion: Given the comment in the PR about tidying up
lib_depsand board-specific builds, consider if these dependencies should be conditionally included only for environments that require GIF support. This might optimize flash size on devices that do not use this feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
platformio.ini(2 hunks)wled00/FX.cpp(2 hunks)wled00/FX.h(1 hunks)wled00/FX_fcn.cpp(1 hunks)wled00/fcn_declare.h(1 hunks)wled00/image_loader.cpp(1 hunks)
🔇 Additional comments (8)
wled00/image_loader.cpp (1)
133-142: Resource cleanup looks correct.
TheendImagePlayback()implementation closes the file, deallocates the decoder, and resets state variables. This helps avoid memory leaks or dangling references.wled00/fcn_declare.h (1)
112-123: Ensure consistent function naming across files.
All newly introduced GIF-related callback prototypes appear consistent with their counterparts inimage_loader.cpp. However, confirm that each callback name exactly matches its definition to prevent linkage errors.wled00/FX_fcn.cpp (2)
290-292: LGTM: GIF cleanup integration looks good.The addition of the GIF cleanup code in
resetIfRequired()is well-placed and properly guarded by theWLED_ENABLE_GIFpreprocessor directive. This ensures proper cleanup of GIF resources when a segment is reset.
272-293:Details
✅ Verification successful
Verify GIF cleanup implementation.
The
resetIfRequired()method handles cleanup of various segment resources. Let's verify that the GIF cleanup is properly implemented in the codebase.
🏁 Script executed:
#!/bin/bash # Description: Check for GIF cleanup implementation # Look for endImagePlayback implementation and related GIF cleanup code # Search for endImagePlayback function definition echo "Searching for endImagePlayback implementation..." rg "endImagePlayback.*\(.*Segment" -A 5 # Search for GIF-related cleanup code echo "Searching for GIF cleanup code..." rg "WLED_ENABLE_GIF.*cleanup" -A 5Length of output: 844
GIF cleanup confirmed.
The call toendImagePlayback(this)inside the#ifdef WLED_ENABLE_GIFblock inwled00/FX_fcn.cppis correctly implemented. The function definition inwled00/image_loader.cppshows that it properly handles resource cleanup by closing any open file and deallocating the GIF decoder. No further changes are required for GIF cleanup.wled00/FX.cpp (3)
9092-9094: LGTM!The Image effect is properly integrated into the effects list with appropriate conditional compilation.
4657-4668:Details
❓ Verification inconclusive
Consider the TODO from PR description.
The PR mentions a TODO about organizing lib_deps and device builds. While this file implements the GIF functionality correctly, ensure that the necessary library dependencies and device configurations are properly managed in the build system.
🏁 Script executed:
#!/bin/bash # Search for library dependencies and build configurations rg -l 'WLED_ENABLE_GIF' platformio.iniLength of output: 53
Action: Verify that build configurations and library dependencies for GIF support are fully organized as discussed.
The GIF functionality in the file is implemented correctly with proper conditional compilation. However, the PR’s TODO regarding the organization of
lib_depsand device builds still needs your attention. While the grep confirms thatplatformio.iniincludes theWLED_ENABLE_GIFflag, please review and ensure that the build system properly incorporates any additional library dependencies and that device configurations are appropriately updated.
4669-4669:Details
✅ Verification successful
Verify the effect data string parameters.
The effect data string includes
sx=128which might limit the matrix size. Please verify:
- Is 128 the maximum supported size?
- What do the "!" parameters represent?
🏁 Script executed:
#!/bin/bash # Search for documentation or usage of effect data string parameters rg -A 5 'sx=' wled00/Length of output: 7424
Re: Verification of effect data string parameters in Image mode
After reviewing the data strings across the codebase, here are the findings:
Matrix Size Parameter (
sx):
The valuesx=128in the Image effect is consistent with the arbitrary sizing used in other effects—values range from 24 up to 255 in different modes. This confirms that128is not a hard upper limit but simply the configured value for this particular effect.Exclamation Mark (
!) Parameters:
The!entries appear as placeholders in the effect data strings. They denote parameters that are either not explicitly set or should fall back to default behavior, as evidenced by their consistent usage across multiple effects.Overall, the current implementation appears intentional, and there is no indication of a limitation imposed by the use of
sx=128or the default markers.platformio.ini (1)
1113-1113: Build Flag for GIF Support:
The flag-D WLED_ENABLE_GIFhas been added in the shared build flags section (build_flags_S), which will conditionally enable GIF-related code in the project.
- Consistency Check: Please verify that all parts of the codebase implementing GIF playback are correctly guarded by this flag so that the feature compiles only when intended.
| uint32_t wait = currentFrameDelay * 2 - seg.speed * currentFrameDelay / 128; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential integer underflow in frame timing.
When seg.speed is very high, seg.speed * currentFrameDelay / 128 could exceed currentFrameDelay * 2. This may cause underflow upon subtracting, resulting in a large unsigned value for wait and stalling updates.
- uint32_t wait = currentFrameDelay * 2 - seg.speed * currentFrameDelay / 128;
+ uint32_t speedDelay = seg.speed * currentFrameDelay / 128;
+ uint32_t baseDelay = currentFrameDelay * 2;
+ uint32_t wait = (speedDelay >= baseDelay) ? 0 : (baseDelay - speedDelay);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint32_t wait = currentFrameDelay * 2 - seg.speed * currentFrameDelay / 128; | |
| uint32_t speedDelay = seg.speed * currentFrameDelay / 128; | |
| uint32_t baseDelay = currentFrameDelay * 2; | |
| uint32_t wait = (speedDelay >= baseDelay) ? 0 : (baseDelay - speedDelay); |
| char lastFilename[34] = "/"; | ||
| GifDecoder<320,320,12,true> decoder; |
There was a problem hiding this comment.
Consider validating lastFilename length before substring operations.
Storing the segment name at lastFilename + 1 and later relying on strlen(lastFilename) - 4 to verify whether it ends with ".gif" may risk out-of-bounds reads if seg.name is too short. A length check would harden this logic against malformed or truncated segment names.
if (strncmp(lastFilename +1, seg.name, 32) != 0) {
+ if (strlen(seg.name) < 4) {
+ gifDecodeFailed = true;
+ return IMAGE_ERROR_UNSUPPORTED_FORMAT;
+ }
strncpy(lastFilename +1, seg.name, 32);
...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| char lastFilename[34] = "/"; | |
| GifDecoder<320,320,12,true> decoder; | |
| if (strncmp(lastFilename + 1, seg.name, 32) != 0) { | |
| if (strlen(seg.name) < 4) { | |
| gifDecodeFailed = true; | |
| return IMAGE_ERROR_UNSUPPORTED_FORMAT; | |
| } | |
| strncpy(lastFilename + 1, seg.name, 32); | |
| // ... rest of the code | |
| } |
|
Please tell me how to upload it .Gif to the file system to try it out? |
|
Go to http://IP/edit |
I just tested this with my HUB75 panels. Target: adafruit_matrixportal_esp32s . I connected 3 32x64 panels. When playing the effect, it's as if only the first frame from the gif is displayed on the panel and that's it.. I'll be watching the debugging information. |
|
Thanks for testing, @byOZN , looks like some changes are needed to make compatible with WLED-MM then |
I commented out the code in the void |
Looks like I'm probably calling that method in the wrong place in Should it be inside both ifs @softhack007 ? |
3 participants
Pull in GIF support from upstream
Summary by CodeRabbit
New Features
Chores