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
+20 −7
Open

IMG_webp: Incorrect frame composition in animated WebP loading #522

Changes from 2 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
94 changes: 61 additions & 33 deletions src/IMG_webp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ SDL_Surface *IMG_LoadWEBP_RW(SDL_RWops *src)
return NULL;
}

IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
IMG_Animation* IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
{
Sint64 start;
const char *error = NULL;
Expand All @@ -272,7 +272,8 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
WebPData wd;
uint32_t bgcolor;
SDL_Surface *canvas = NULL;
WebPMuxAnimDispose dispose_method = WEBP_MUX_DISPOSE_BACKGROUND;
SDL_Surface *prevCanvas = NULL;
SDL_Rect prevRect = { 0 };

if (!src) {
/* The error message has been set in SDL_RWFromFile */
Expand Down Expand Up @@ -313,7 +314,7 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
goto error;
}

anim = (IMG_Animation *)SDL_calloc(1, sizeof(*anim));
anim = (IMG_Animation*)SDL_calloc(1, sizeof(*anim));
if (!anim) {
goto error;
}
Expand All @@ -327,27 +328,39 @@ 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;
}

/* Background color is BGRA byte order according to the spec */
bgcolor = lib.WebPDemuxGetI(demuxer, WEBP_FF_BACKGROUND_COLOR);
#if SDL_BYTEORDER == SDL_BIG_ENDIAN
bgcolor = SDL_MapRGBA(canvas->format,
(bgcolor >> 8) & 0xFF,
(bgcolor >> 16) & 0xFF,
(bgcolor >> 24) & 0xFF,
(bgcolor >> 0) & 0xFF);
(bgcolor >> 8) & 0xFF,
(bgcolor >> 16) & 0xFF,
(bgcolor >> 24) & 0xFF,
(bgcolor >> 0) & 0xFF);
#else
bgcolor = SDL_MapRGBA(canvas->format,
(bgcolor >> 16) & 0xFF,
(bgcolor >> 8) & 0xFF,
(bgcolor >> 0) & 0xFF,
(bgcolor >> 24) & 0xFF);
(bgcolor >> 16) & 0xFF,
(bgcolor >> 8) & 0xFF,
(bgcolor >> 0) & 0xFF,
(bgcolor >> 24) & 0xFF);
#endif

// Initialize both canvases - use bgcolor for non-alpha format, transparency for alpha
Copy link
Collaborator

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

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

Choose a reason for hiding this comment

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

Suggested change
}
} else {

Copy link
Author

Choose a reason for hiding this comment

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

got it

else {
SDL_FillRect(canvas, NULL, bgcolor);
SDL_FillRect(prevCanvas, NULL, bgcolor);
}

SDL_zero(iter);
WebPMuxAnimDispose dispose_method = WEBP_MUX_DISPOSE_BACKGROUND;
if (lib.WebPDemuxGetFrame(demuxer, 1, &iter)) {
do {
SDL_Surface* curr;
Expand All @@ -357,8 +370,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

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 @@ -367,65 +387,73 @@ IMG_Animation *IMG_LoadWEBPAnimation_RW(SDL_RWops *src)
}

if (!lib.WebPDecodeRGBAInto(iter.fragment.bytes,
iter.fragment.size,
(uint8_t *)curr->pixels,
curr->pitch * curr->h,
curr->pitch)) {
iter.fragment.size,
(uint8_t*)curr->pixels,
curr->pitch * curr->h,
curr->pitch)) {
error = "WebPDecodeRGBAInto() failed";
SDL_FreeSurface(curr);
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;
// Use BLENDMODE_NONE like in your implementation
SDL_SetSurfaceBlendMode(curr, SDL_BLENDMODE_NONE);
Copy link
Collaborator

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

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

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

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

SDL_free(raw_data);

return anim;

error:
if (prevCanvas) {
SDL_FreeSurface(prevCanvas);
}
if (canvas) {
SDL_FreeSurface(canvas);
}
if (anim) {
IMG_FreeAnimation(anim);
if (anim->frames) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What did IMG_FreeAnimation() do that we don't want done here?

Copy link
Author

Choose a reason for hiding this comment

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

I returned the error goto to how it was originally, hopefully fixed up formatting too

for (int i = 0; i < anim->count; ++i) {
if (anim->frames[i]) {
SDL_FreeSurface(anim->frames[i]);
}
}
SDL_free(anim->frames);
}
if (anim->delays) {
SDL_free(anim->delays);
}
SDL_free(anim);
}
if (demuxer) {
lib.WebPDemuxDelete(demuxer);
}
if (raw_data) {
SDL_free(raw_data);
}

if (error) {
IMG_SetError("%s", error);
}
SDL_RWseek(src, start, RW_SEEK_SET);

return NULL;
}

#else
#if _MSC_VER >= 1300
#pragma warning(disable : 4100) /* warning C4100: 'op' : unreferenced formal parameter */
Expand Down
Loading