IMG_webp: Incorrect frame composition in animated WebP loading #522
Conversation
The WebP animation loader was not correctly handling frame composition and disposal. Fixed by properly implementing: 1. Frame disposal methods (WEBP_MUX_DISPOSE_BACKGROUND) 2. Initial canvas state based on alpha channel presence 3. Frame blending using SDL_BLENDMODE_NONE for accurate composition 4. Region-specific disposal and updates The loader now correctly: - Uses background color for non-alpha WebPs - Uses transparency for WebPs with alpha channel - Handles frame disposal before applying new frames - Maintains proper frame boundaries during composition This ensures animated WebPs render correctly regardless of their disposal methods and alpha channel configuration.
|
Can you please revert the formatting changes? |
src/IMG_webp.c
Outdated
| if (features.has_alpha) { | ||
| SDL_FillRect(canvas, NULL, SDL_MapRGBA(canvas->format, 0, 0, 0, 0)); | ||
| SDL_FillRect(prevCanvas, NULL, SDL_MapRGBA(prevCanvas->format, 0, 0, 0, 0)); | ||
| } |
There was a problem hiding this comment.
| } | |
| } else { |
src/IMG_webp.c
Outdated
| if (canvas) { | ||
| SDL_FreeSurface(canvas); | ||
| } | ||
| if (anim) { | ||
| IMG_FreeAnimation(anim); | ||
| if (anim->frames) { |
There was a problem hiding this comment.
What did IMG_FreeAnimation() do that we don't want done here?
There was a problem hiding this comment.
I returned the error goto to how it was originally, hopefully fixed up formatting too
src/IMG_webp.c
Outdated
| dst.y = iter.y_offset; | ||
| dst.w = iter.width; | ||
| dst.h = iter.height; | ||
| SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_NONE); |
There was a problem hiding this comment.
How do you handle WEBP_MUX_BLEND?
There was a problem hiding this comment.
oh right, what do you think of this?
```
// Handle blending mode for current frame
if (iter.blend_method == WEBP_MUX_BLEND) {
SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_BLEND);
} else {
// For NO_BLEND, we first clear the region then copy without blending
if (features.has_alpha) {
SDL_FillRect(canvas, &dst, SDL_MapRGBA(canvas->format, 0, 0, 0, 0));
} else {
SDL_FillRect(canvas, &dst, bgcolor);
}
SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_NONE);
}
would be just before blit
There was a problem hiding this comment.
Clearing the region isn't necessary for BLENDMODE_NONE, the entire rectangle is copied from the source.
There was a problem hiding this comment.
We should just be able to use the original code:
if (iter.blend_method == WEBP_MUX_BLEND) {
SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_BLEND);
} else {
SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_NONE);
}
src/IMG_webp.c
Outdated
|
|
||
| // Handle disposal of THIS frame's region before drawing it | ||
| if (iter.dispose_method == WEBP_MUX_DISPOSE_BACKGROUND) { | ||
| SDL_FillRect(canvas, &dst, SDL_MapRGBA(canvas->format, 0, 0, 0, 0)); |
There was a problem hiding this comment.
disposing with the background should use the background color, not black.
src/IMG_webp.c
Outdated
| @@ -347,6 +350,15 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src) | |||
| (bgcolor >> 24) & 0xFF); | |||
| #endif | |||
|
|
|||
| // Initialize both canvases - use bgcolor for non-alpha format, transparency for alpha | |||
There was a problem hiding this comment.
Are you sure we should use transparency for alpha? is bgcolor a transparent pixel in that case?
There was a problem hiding this comment.
I see white in the non-updated areas if not starting with alpha.
Investigation reveals many viewers just ignore background color entirely.
https://developers.google.com/speed/webp/docs/riff_container#animation
Kind of vague.
Some discussion on the matter:
- Properly handles WEBP_MUX_DISPOSE_BACKGROUND with alpha channel status - Correctly applies bgcolor for non-alpha WebPs and transparency for alpha WebPs - Sets appropriate blend modes (BLEND/NONE) based on frame's blend_method - Removes unused prevCanvas and associated operations - Eliminates redundant frame region tracking
This reverts commit b8c0093.
| // Handle disposal and prepare region for new frame | ||
| if (iter.dispose_method == WEBP_MUX_DISPOSE_BACKGROUND || | ||
| iter.blend_method == WEBP_MUX_NO_BLEND) { | ||
| SDL_FillRect(canvas, &dst, bgcolor); |
There was a problem hiding this comment.
| SDL_FillRect(canvas, &dst, bgcolor); | |
| SDL_FillRect(canvas, &dst, bgcolor); |
The current animated WebP loader in SDL_image has incorrect frame composition behavior, leading to visual artifacts in animated WebP files. This is particularly noticeable in WebP animations that use transparency or have overlapping frames with different disposal methods.
some test images here:
https://github.com/inigomonyota/webptestimages