Skip to content
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.

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

Add support for animated WebP images #103

Closed
wants to merge 5 commits into from

Conversation

MCJack123
Copy link

@MCJack123 MCJack123 commented Aug 7, 2022

This PR adds support for resizing animated WebP images through the libwebpmux/demux sublibraries. It uses the WebPAnimEncoder/WebPAnimDecoder APIs to implement new Encoder/Decoder classes instead of relying on OpenCV, similar to the giflib codec.

This does add an additional dependency on those two libraries, but since they're included with the already required libwebp library (just disabled by default), this shouldn't cause much more hassle beyond rebuilding dependencies.

I've done some testing with animated WebPs I had on hand, as well as making sure that non-animated WebPs go through the normal OpenCV route, but it's probably a good idea to run it with other files as well (I only happen to have lossless files at the moment).

I'm not quite sure what happened with the png headers - I guess they were turned into links when I updated the dependencies? I can revert those changes if necessary, but I expect they'll be reverted the moment someone else updates them again.

Closes #97

{
WebPAnimEncoderAdd(enc, NULL, timestamp, NULL);
WebPData webp_data;
WebPDataInit(&webp_data);
Copy link
Contributor

Choose a reason for hiding this comment

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

does libwebp not allow us to provide our own buffer? i'm guessing it wants to be able to resize it. i think like above we probably would have to fork or bring parts of src/mux/anim_encode.c over to avoid the allocations.

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately it does not support custom buffers, but it doesn't need to resize the buffer, so I think this is just because it prefers to calculate the required size internally. It would be simple to patch it to respect the buffer passed in (ignoring the const qualifier :/), and have it complain about too little memory if the buffer's smaller than expected; saving the actual size back into the WebPData struct.

Also, if maximum memory allocation is your concern, libwebpmux does allow compiling with a MALLOC_LIMIT constant that will stop allocation past the specified limit.

webpmux.go Outdated Show resolved Hide resolved
if (!res)
return -1;
cv::Mat img(cv::Size(m->cols, m->rows), CV_8UC4, buf);
img.copyTo(*m);
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like opencv uses WebPDecodeBGRInto to avoid extra alloc and copy here. i looked at animated webp code and it doesn't seem to really support that :( probably our best option would be to copy a lot of src/demux/anim_decode.c into here and reuse the buffers we have to avoid the allocations/copies

@brian-armstrong-discord
Copy link
Contributor

Disregard above comments, I realize now that this will be a very different beast than GIFs because of the VP8 content. I think in that respect, animated WEBPs look more like video transcodes. In light of that, I'm happy to bring this functionality to lilliput, but I'd like to gate it behind some kind of feature toggle. Most likely I would guess Discord won't enable this for a while since we are pretty careful about limiting how much work is done per asset in our resizer. However, I'd be happy to have the functionality provided to lilliput! Thank you again for sending the PR.

I think then the remaining blockers would just be to add a toggle similar to gifMaxFrameDimension to allow us to toggle animated webp decoding on or off, and to split the build deps changes into a new PR. Once I have the build deps script change, we can merge it and run our CI job that will check in all the actual deps changes.

@brian-armstrong-discord
Copy link
Contributor

@Zipdox Discord already supports .mov and .mp4 video containers. These would be your best bet for uploading video.

@MCJack123
Copy link
Author

@brian-armstrong-discord:

I realize now that this will be a very different beast than GIFs because of the VP8 content. I think in that respect, animated WEBPs look more like video transcodes.

AFAICT, WebP doesn't use any sort of true delta frames, unlike plain VP8 or other video files - it just stores keyframes with "dirty rects" to avoid redrawing the same regions. Google appears to pose it as a list of frames that just use the VP8 bitstream format & basic frame compression methods, but it lacks the delta coding and other advanced stuff usually applied to video. (I might be off base with this one though - I don't have much experience with video codecs.)

Discord already supports .mov and .mp4 video containers. These would be your best bet for uploading video.

The point of using WebP is to replace what GIF does now: small, auto-playing animations without audio that can also be viewed as images. GIF is annoying because the files are much larger compared to other formats, while MP4/MOV don't quite fit the use case as they are often larger, may store audio, and (critically) are not auto-playing.

I prefer using WebP in my app over MP4/MOV/WebM because a) it's not hard to encode at all, b) it's well-supported by basically every browser (minus Discord), and c) it fits the niche that GIF has while being much smaller.

There's been plenty of discussion about this on the Feedback forums and Reddit, though I'm sure you all have browsed through all that a few times.


I'm happy to add any changes you may want, though - I'm currently starting classes & juggling some other software stuff, so I don't have a whole lot of time to work on this right now, but I can work on it when I get the chance.

@HybridEidolon
Copy link

I realize now that this will be a very different beast than GIFs because of the VP8 content. ... Most likely I would guess Discord won't enable this for a while since we are pretty careful about limiting how much work is done per asset in our resizer. ...

A stopgap for Discord might be to have a multi-layer approach, checking the content of the WEBP RIFF and only transcoding WEBPs that do not contain VP8 or VP8X chunks. Lossless chunk decompression would still be unbounded, but that's a problem you would already see when scaling PNG images.

@MCJack123
Copy link
Author

I got a bit of time to implement the WebP animation enable flag. To avoid the loading symbol that's plagued Discord for years, I've made it so that at the very least the first frame of the animation is resized as if it were a single-frame file. This shouldn't put much more of a burden on the server, as it already handles static WebP through the OpenCV code. This will likely still cause confusion in users when their animated files are suddenly static, but it's far better than the infinite loading wheel today.

@muckelba
Copy link

muckelba commented Nov 15, 2022

Any Updates on this? I'd love to use webp for emotes for example

@jet082
Copy link

jet082 commented Dec 3, 2022

Seconding that this would be amazing to see merged!

@solusipse
Copy link

webp is becoming the standard, any input on this awesome contribution?

@discord discord locked as off-topic and limited conversation to collaborators Dec 10, 2022
@skidder
Copy link
Contributor

skidder commented Sep 27, 2024

Closing this out since we've added support for animated WebP through a separate set of changes. I appreciate the contribution!

@skidder skidder closed this Sep 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants