-
Notifications
You must be signed in to change notification settings - Fork 2
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 animated gif conversion #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. :)
I took some time to test this out and found a few issues when testing with some common gifs.
Party parrot original: and converted: It looks like this doesn't preserve the background color and uses the "keep" frame disposal flags which causes previous frames to persist. Past that, the repeat extension isn't set which means the image doesn't loop.
(source: https://emoji.slack-edge.com/T03S7UXFC/fiesta-parrot/a4d99d543d7f1b47.gif)
And a test image with partial frames. Original: . Converted: . Aside from the lack of repeat, this one looks generally okay. Unfortunately the image size increases markedly, from 685 bytes to around 5kb. Digging in a bit I think a significant contributor here is the fact that frames are expanded to full size even if they're originally only cover part of the image. If I'm reading the code correctly, I think that's due to a design decision for the high level image crate's gif codec api https://github.com/image-rs/image/blob/787b655d737594a8331d06d099cb59c0305aa997/src/codecs/gif.rs#L113-L117
(image source: https://peterdn.com/post/2020/10/25/using-imagemagick-to-create-a-gif-test-suite/)
FWIW, I did spend some time getting a prototype working using the lower-level gif crate directly so I think this is still doable. I'll follow up on that when time permits.
src/lib.rs
Outdated
@@ -1,9 +1,11 @@ | |||
#[macro_use] | |||
extern crate serde_derive; | |||
use image::codecs::jpeg::JpegDecoder; | |||
use image::gif::{GifDecoder, GifEncoder}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: image::gif
is deprecated so this should probably be image::codecs::gif::...
src/lib.rs
Outdated
_ => frame, | ||
}); | ||
|
||
let mut out: Vec<u8> = Vec::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this should do the same thing
let mut out: Vec<u8> = Vec::new(); | |
let mut out = vec![]; |
@cagerton Could you share your prototype with the lower-level API ? I can contribute if you don't have time to work on it |
|
I was a bit surprised that imagemagick&graphicsmagick dropped the ball here, tbh |
Closing this since gif support has been not been merged. |
This adds support for animated gif 🎉
If the original file & the target are gifs, each frame in the gif will be resized
I didn't manage to do it in along with the other images, but I think it's enough of a special case to warrant a separate function