-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
cgifsave does not properly handle color palettes when first frame has limited colors #2622
Comments
Hello @TheEssem, The libvips GIF saver computes the palette from the first frame, then recomputes the palette if it sees a change in subsequent frames. The change detector compares the sum of the RGBA pixels, so it's sensitive, but won't trigger for things like an object moving over a transparent background. I tried with 8.12.1 and I see:
To make: So it seems to be working for me. Perhaps you are linking to an older libvips? |
... you can make your test program a little smaller, you probably know, eg.: /* compile with:
* g++ gif.cc `pkg-config vips-cpp --cflags --libs`
*/
#include <iostream>
#include <vips/vips8>
using namespace vips;
int main(int argc, char **argv) {
if (vips_init(argv[0]))
vips_error_exit(NULL);
VSource source = VSource::new_from_descriptor(0);
VImage in =
VImage::new_from_source(source, "", VImage::option()
->set("n", -1)
->set("access", "sequential"))
.colourspace(VIPS_INTERPRETATION_sRGB);
if (!in.has_alpha())
in = in.bandjoin(255);
int page_height = vips_image_get_page_height(in.get_image());
int n_pages = vips_image_get_n_pages(in.get_image());
std::string captionText =
"<span background=\"white\">" + (std::string)argv[1] + "</span>";
int caption_height = page_height / 5;
VImage text =
VImage::text(captionText.c_str(), VImage::option()
->set("rgba", TRUE)
->set("align", VIPS_ALIGN_CENTRE)
->set("font", "futura bold")
->set("width", in.width())
->set("height", caption_height));
VImage caption = ((text == (std::vector<double>) {0, 0, 0, 0}).bandand())
.ifthenelse(255, text)
.gravity(VIPS_COMPASS_DIRECTION_CENTRE,
in.width(), caption_height, VImage::option()
->set("extend", "white"));
std::vector<VImage> gif;
for (int i = 0; i < n_pages; i++) {
VImage gif_frame = in.crop(0, i * page_height, in.width(), page_height);
VImage frame = caption.join(gif_frame,
VIPS_DIRECTION_VERTICAL, VImage::option()
->set("background", 0xffffff)
->set("expand", TRUE));
gif.push_back(frame);
}
VImage result = VImage::arrayjoin(gif, VImage::option()->set("across", 1));
result.set(VIPS_META_PAGE_HEIGHT, page_height + caption.height());
VTarget target = VTarget::new_to_descriptor(1);
result.write_to_target(".gif", target, VImage::option()
->set("dither", 0));
vips_shutdown();
return 0;
}
|
Ran
Just a note about the original code: the padding/sizing is intentional. I intend to use similar code in a production environment and want the text to stay at the same size relative to the image with word wrapping. I'm coming from ImageMagick where I have another (somewhat messy) implementation of this, and I'm trying to replicate the output from it as much as possible: https://github.com/esmBot/esmBot/blob/8b238a23167efd3f452803bd09857e545ad5aff7/natives/caption.cc Also, I forgot to mention this, but it also only seems to happen when the caption is a certain height; when changing |
Sorry, it's still working for me. Perhaps it's a difference in libimagequant, the thing libvips uses to pick the palette? Perhaps you have a version that's not prioritizing the large smooth graduations of colour you have here? I see:
The version that ships with ubuntu 21.10. |
That command results in |
I've found it! Yes, there's an incredibly stupid bug in the change detector. I'll make a patch. |
We were only using the top 25% of the frame for GIF pallette change detection. Thanks TheEssem See: #2622
OK, the fix will be in 8.12.2. I've credited you in the changelog, I hope that's OK. Thanks for reporting this stupid thing, and thanks for pushing for a fix. |
This is a really bad bug, so we'll probably push out 8.12.2 next weekend. |
Thanks for the speedy fix! :) |
Bug report
Describe the bug
As of commit 251e1d1,
cgifsave
seems to behave strangely in regards to color palettes. When re-encoding a GIF file using another one as input, the palette reusing behavior doesn't seem to recognize any of the colors besides the ones on the first frame when the number of colors is very small, even though the colors in the original GIF change quite a bit.To Reproduce
Steps to reproduce the behavior:
cat input.gif | ./giftest "[put text here]" > output.gif
Expected behavior
I expected the colors on each frame of the output GIF to be similar (if not the same) compared to the input GIF.
Actual behavior
The output GIF seems to only use the color palette from the first frame, despite the actual colors changing drastically (aside from any other non-B/W colors that were on the first frame).
Screenshots
Here is an example GIF I made that demonstrates this behavior:
And here is an example output, using an emoji to show that it detects color from the first frame:
Environment
(please complete the following information)
Additional context
Here is the C++ code I'm using:
I'd suggest adding an emoji or using a color font to experiment with the palette.
The text was updated successfully, but these errors were encountered: