Skip to content

IMG_webp: Incorrect frame composition in animated WebP loading #522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: SDL2
Choose a base branch
from
Open

IMG_webp: Incorrect frame composition in animated WebP loading #522

Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7ace470
IMG_webp: Fix animated WebP frame composition
inigomonyota Feb 13, 2025
09b4899
formatting
inigomonyota Feb 13, 2025
374730d
Revert "formatting"
inigomonyota Feb 14, 2025
2638cb2
Revert "IMG_webp: Fix animated WebP frame composition"
inigomonyota Feb 14, 2025
312ae53
formatting fixup
inigomonyota Feb 14, 2025
b105e30
formatting fixup #2
inigomonyota Feb 14, 2025
a27e2ed
formatting #3
inigomonyota Feb 14, 2025
387b5d5
formatting #4
inigomonyota Feb 14, 2025
8f6c6a0
formatting #4
inigomonyota Feb 14, 2025
abeb7bf
formatting #6
inigomonyota Feb 14, 2025
f8d4896
- Combines disposal and blend preparation into a single operation
inigomonyota Feb 14, 2025
ed1bda3
- Use bgcolor for non alpha
inigomonyota Feb 14, 2025
b8c0093
formatting
inigomonyota Feb 14, 2025
bfbbc2c
Revert "formatting"
inigomonyota Feb 14, 2025
609cd35
formatting
inigomonyota Feb 14, 2025
683d653
formatting
inigomonyota Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions src/IMG_webp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,8 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
WebPData wd;
uint32_t bgcolor;
SDL_Surface *canvas = NULL;
SDL_Surface *prevCanvas = NULL;
SDL_Rect prevRect = { 0 };
WebPMuxAnimDispose dispose_method = WEBP_MUX_DISPOSE_BACKGROUND;

if (!src) {
Expand Down Expand Up @@ -327,7 +329,8 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
}

canvas = SDL_CreateRGBSurfaceWithFormat(0, anim->w, anim->h, 0, features.has_alpha ? SDL_PIXELFORMAT_RGBA32 : SDL_PIXELFORMAT_RGBX32);
if (!canvas) {
prevCanvas = SDL_CreateRGBSurfaceWithFormat(0, anim->w, anim->h, 0, features.has_alpha ? SDL_PIXELFORMAT_RGBA32 : SDL_PIXELFORMAT_RGBX32);
if (!canvas || !prevCanvas) {
goto error;
}

Expand All @@ -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
Copy link
Collaborator

@slouken slouken Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we should use transparency for alpha? is bgcolor a transparent pixel in that case?

Copy link
Author

@inigomonyota inigomonyota Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

SixLabors/ImageSharp#2547

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));
} else {
SDL_FillRect(canvas, NULL, bgcolor);
SDL_FillRect(prevCanvas, NULL, bgcolor);
}

SDL_zero(iter);
if (lib.WebPDemuxGetFrame(demuxer, 1, &iter)) {
do {
Expand All @@ -357,8 +369,15 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
continue;
}

if (dispose_method == WEBP_MUX_DISPOSE_BACKGROUND) {
SDL_FillRect(canvas, NULL, bgcolor);
// Set up destination rect for current frame
dst.x = iter.x_offset;
dst.y = iter.y_offset;
dst.w = iter.width;
dst.h = iter.height;

// 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));
Copy link
Collaborator

@slouken slouken Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disposing with the background should use the background color, not black.

}

curr = SDL_CreateRGBSurfaceWithFormat(0, iter.width, iter.height, 0, SDL_PIXELFORMAT_RGBA32);
Expand All @@ -376,27 +395,25 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
goto error;
}

if (iter.blend_method == WEBP_MUX_BLEND) {
SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_BLEND);
} else {
SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_NONE);
}
dst.x = iter.x_offset;
dst.y = iter.y_offset;
dst.w = iter.width;
dst.h = iter.height;
SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_NONE);
Copy link
Collaborator

@slouken slouken Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you handle WEBP_MUX_BLEND?

Copy link
Author

@inigomonyota inigomonyota Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@slouken slouken Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearing the region isn't necessary for BLENDMODE_NONE, the entire rectangle is copied from the source.

Copy link
Collaborator

@slouken slouken Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
            }

SDL_BlitSurface(curr, NULL, canvas, &dst);
SDL_FreeSurface(curr);

// Store complete frame state
anim->frames[frame_idx] = SDL_DuplicateSurface(canvas);
anim->delays[frame_idx] = iter.duration;

// Save current frame region and disposal method for next iteration
prevRect = dst;
dispose_method = iter.dispose_method;

} while (lib.WebPDemuxNextFrame(&iter));

lib.WebPDemuxReleaseIterator(&iter);
}

SDL_FreeSurface(prevCanvas);

SDL_FreeSurface(canvas);

lib.WebPDemuxDelete(demuxer);
Expand All @@ -406,6 +423,9 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
return anim;

error:
if (prevCanvas) {
SDL_FreeSurface(prevCanvas);
}
if (canvas) {
SDL_FreeSurface(canvas);
}
Expand Down
Loading