fix: GIF disposal method 2 not applied in Image effect (pixel trails)#5564
fix: GIF disposal method 2 not applied in Image effect (pixel trails)#5564
Conversation
Update pinned GifDecoder commit to include fix for GIF disposal method 2 in RAW draw mode. The previous commit silently ignored disposal=2 when AnimatedGIF was used without a framebuffer (WLED Image effect path), causing stale pixels to trail across frames in transparent GIF animations. Depends on: Aircoookie/GifDecoder#1
WalkthroughThe PR updates the ChangesDependency Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platformio.ini`:
- Line 266: The dependency line that switches to the teetow fork
(https://github.com/teetow/GifDecoder.git#80a2912) needs an inline note and an
explicit maintainer-approval marker: add two comment lines immediately above the
dependency showing it's TEMPORARY and referencing the revert target (e.g.
"TEMPORARY: Using teetow fork pending upstream merge of disposal method 2 fix"
and "TODO: Revert to Aircoookie/GifDecoder#80a2912 once PR merges"), and add a
short comment like "REQUIRES-MAINTAINER-APPROVAL" so reviewers know approval is
required before merge; keep the dependency URL unchanged and ensure the comments
are in the same INI section as the existing URL string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 058da1f1-9c5d-4907-b10a-8a6bdf90ccda
📒 Files selected for processing (1)
platformio.ini
| esp32async/AsyncTCP @ 3.4.7 | ||
| bitbank2/AnimatedGIF@^1.4.7 | ||
| https://github.com/Aircoookie/GifDecoder.git#bc3af189b6b1e06946569f6b4287f0b79a860f8e | ||
| https://github.com/teetow/GifDecoder.git#80a2912eb18c5caf9176509669d674fb26a2f7ee |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the GifDecoder fork accessibility and commit existence
echo "=== Checking fork accessibility ==="
curl -f -s -o /dev/null -w "HTTP %{http_code}\n" https://github.com/teetow/GifDecoder
echo "=== Checking commit exists ==="
curl -f -s -o /dev/null -w "HTTP %{http_code}\n" https://github.com/teetow/GifDecoder/commit/80a2912eb18c5caf9176509669d674fb26a2f7ee
echo "=== Checking for API compatibility indicators ==="
# Check if the fork contains the essential callback methods
curl -s https://raw.githubusercontent.com/teetow/GifDecoder/80a2912eb18c5caf9176509669d674fb26a2f7ee/src/GifDecoder.h | grep -E "(setScreenClearCallback|setUpdateScreenCallback|setDrawPixelCallback|startDecoding|getSize|decodeFrame|getFrameDelay_ms)" || echo "Warning: Could not verify API methods"Repository: wled/WLED
Length of output: 592
🌐 Web query:
teetow GifDecoder fork 80a2912 API compatibility screenClearCallback disposal method
💡 Result:
I couldn’t find any authoritative, source-backed evidence for the specific repo/fork you referenced (“teetow GifDecoder fork 80a2912”) or for an API surface containing “screenClearCallback”, nor any commit mention of “80a2912” that is tied to an actual GifDecoder codebase. What I can say from the sources I found: - In common GIF decoder implementations (e.g., Glide’s GifDecoder), the “disposal method” logic is handled internally while composing frames, with disposal types including NONE(1), BACKGROUND(2), PREVIOUS(3), etc. This affects how pixels from previous frames are cleared/restored, but this is not the same as an app-specific “screenClearCallback”. - Some GIF decoders expose a generic “clear” method on a decoder interface (e.g., Glide’s GifDecoder), but none of the sources I found connect that to “screenClearCallback” or to the “teetow” fork/commit you cited. If you can paste the GitHub URL (or the relevant file/class that defines screenClearCallback and dispose/clear behavior) from the “teetow” fork at commit 80a2912, I can re-run the research narrowly and answer precisely: what the callback is, when it’s invoked, and what disposal/cleanup method it corresponds to in that implementation.
Citations:
- 1: https://android.googlesource.com/platform/external/iosched/+/HEAD/third_party/gif_decoder/src/main/java/com/bumptech/glide/gifdecoder/GifDecoder.java?autodive=0%2F
- 2: https://bumptech.github.io/glide/javadocs/4120/com/bumptech/glide/gifdecoder/GifDecoder.html
- 3: https://gist.github.com/devunwired/4479231
This change requires explicit maintainer approval per project guidelines.
The GifDecoder dependency has been switched to a fork (teetow/GifDecoder#80a2912). Per coding guidelines, modifications to platformio.ini require maintainer approval since changes to the global build environment may break GitHub Actions builds.
Technical verification confirms:
- Fork is accessible and the commit exists
- Required API methods are present and compatible with existing usage in
image_loader.cpp(setScreenClearCallback, setUpdateScreenCallback, setDrawPixelCallback, startDecoding, getSize, decodeFrame, getFrameDelay_ms)
However, maintenance risk remains:
- This is documented as temporary pending upstream merge
- If the fork is deleted or becomes unavailable, builds will break
- The temporary nature may be forgotten, leaving long-term dependency on a personal fork
Suggest adding an inline comment documenting the temporary nature and reversion target:
; TEMPORARY: Using teetow fork pending upstream merge of disposal method 2 fix
; TODO: Revert to Aircoookie/GifDecoder#80a2912 once PR merges
Requires maintainer or WLED organization member approval before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platformio.ini` at line 266, The dependency line that switches to the teetow
fork (https://github.com/teetow/GifDecoder.git#80a2912) needs an inline note and
an explicit maintainer-approval marker: add two comment lines immediately above
the dependency showing it's TEMPORARY and referencing the revert target (e.g.
"TEMPORARY: Using teetow fork pending upstream merge of disposal method 2 fix"
and "TODO: Revert to Aircoookie/GifDecoder#80a2912 once PR merges"), and add a
short comment like "REQUIRES-MAINTAINER-APPROVAL" so reviewers know approval is
required before merge; keep the dependency URL unchanged and ensure the comments
are in the same INI section as the existing URL string.
|
Setting this to draft while punishing the robot for pre-emptively creating this PR =) |
softhack007
added
AI
2 participants
Summary
GIF animations using disposal method 2 ("restore to background") show pixel trails in the WLED Image effect. Transparent areas of each new frame are obscured by leftover opaque pixels from the previous frame.
Root cause
AnimatedGIF's disposal=2 handling in DecodeLZW only runs when a framebuffer is allocated (COOKED mode). WLED's GifDecoder wrapper uses RAW mode with no framebuffer, so disposal semantics are silently skipped.
The old inline workaround in GifDecoder_Impl.h GIFDraw() tried to replace transparent pixels with background color per-scanline, but this has wrong semantics (it modifies the current frame instead of clearing before the next frame) and silently no-ops when background index == transparent index (both commonly 0).
Fix
This PR bumps the pinned GifDecoder commit to include a fix that tracks the previous frame disposal method and calls screenClearCallback() at the start of the next frame when disposal=2 was requested.
See: Aircoookie/GifDecoder#1
Note: the GifDecoder PR must merge first; once it does, the pinned URL here should be updated from teetow/GifDecoder to Aircoookie/GifDecoder at the same commit SHA (80a2912).
Testing
Verified on ESP32 32x16 LED matrix: GIF animations with disposal=2 and transparent regions no longer accumulate pixel trails.
Summary by CodeRabbit