cgifsave does not properly handle color palettes when first frame has limited colors

[TheEssem]

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:

1. Compile the C++ code below using the following command:

   g++ `pkg-config --libs --cflags vips-cpp` giftest.cc

2. Input the GIF file in the screenshots section using the following syntax (probably a good idea to test with a long string): cat input.gif | ./giftest "[put text here]" > output.gif
3. The result should be similar to the example output in the screenshots section

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)

• OS: EndeavourOS Linux (x86_64)
• Vips: 8.12.1

Additional context Here is the C++ code I'm using:

#include <iostream>
#include <vips/vips8>

using namespace vips;

int main(int argc, char **argv) {
  if (vips_init(argv[0])) vips_error_exit(NULL);

  VImage in = VImage::new_from_source(VSource::new_from_descriptor(0),
                                      "[n=-1,access=sequential]");

  int width = in.width();

  int size = width / 10;
  char font_string[12 + sizeof(size)];

  std::sprintf(font_string, "futura bold %d", size);

  int textWidth = width - ((width / 25) * 2);

  std::string captionText =
      "<span background=\"white\">" + (std::string)argv[1] + "</span>";

  VImage text =
      VImage::text(captionText.c_str(), VImage::option()
                                            ->set("rgba", TRUE)
                                            ->set("align", VIPS_ALIGN_CENTRE)
                                            ->set("font", font_string)
                                            ->set("width", textWidth));

  std::vector<double> fullAlpha = {0, 0, 0, 0};

  VImage caption =
      text.relational_const(VIPS_OPERATION_RELATIONAL_EQUAL, fullAlpha)
          .bandand()
          .ifthenelse({255, 255, 255, 255}, text)
          .gravity(VIPS_COMPASS_DIRECTION_CENTRE, width, text.height() + size,
                   VImage::option()->set("extend", VIPS_EXTEND_WHITE));

  int pageHeight = in.get_int(VIPS_META_PAGE_HEIGHT);
  std::vector<VImage> gif;

  for (int i; i < (in.height() / pageHeight); i++) {
    VImage cropped = in.crop(0, i * pageHeight, width, pageHeight)
                         .colourspace(VIPS_INTERPRETATION_sRGB);
    gif.push_back(caption.join(
        !cropped.has_alpha() ? cropped.bandjoin(255) : cropped,
        VIPS_DIRECTION_VERTICAL,
        VImage::option()->set("background", 0xffffff)->set("expand", TRUE)));
  }

  VImage result = VImage::arrayjoin(gif, VImage::option()->set("across", 1));
  result.set(VIPS_META_PAGE_HEIGHT, pageHeight + caption.height());

  result.write_to_target(".gif", VTarget::new_to_descriptor(1),
                         VImage::option()->set("dither", 0));

  vips_shutdown();

  return 0;
}

I'd suggest adding an emoji or using a color font to experiment with the palette.

[jcupitt]

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:

vips copy test.gif[n=-1] x.gif[dither=0]

To make:

So it seems to be working for me. Perhaps you are linking to an older libvips?

[jcupitt]

I tried with your nice test prog:

./a.out < ~/pics/test.gif > x.gif "🥶hello🥶"

To make:

What do you see for --vips-config? I see:

$ vips --vips-config
...
quantisation to 8 bit: yes
...
GIF save with cgif: yes

[jcupitt]

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

• text will auto-fit for you if you give both width and height
• you can use operator overloads for things like relational_const
• the C API has safe functions for getting page height and the number of pages, they should probably be wrapped for the C++ API too
• you can often use strings instead of enums, which is convenient (you ought to be able to use string everywhere, really)
• three different ways of saying white is bad, we should fix this :(

[TheEssem]

Ran vips --vips-config, here is my output:

enable debug: no
enable deprecated library components: yes
enable modules: no
use fftw3 for FFT: yes
accelerate loops with orc: yes
ICC profile support with lcms: yes (lcms2)
zlib: yes
text rendering with pangocairo: yes
font file support with fontconfig: yes
RAD load/save: yes
Analyze7 load/save: yes
PPM load/save: yes
GIF load:  yes
EXIF metadata support with libexif: yes
JPEG load/save with libjpeg: yes (pkg-config)
JXL load/save with libjxl: no (dynamic module: no)
JPEG2000 load/save with libopenjp2: yes
PNG load with libspng: no
PNG load/save with libpng: yes (pkg-config libpng >= 1.2.9)
quantisation to 8 bit: yes
TIFF load/save with libtiff: yes (pkg-config libtiff-4)
image pyramid save: yes
HEIC/AVIF load/save with libheif: yes (dynamic module: no)
WebP load/save with libwebp: yes
PDF load with PDFium:  no
PDF load with poppler-glib: yes (dynamic module: no)
SVG load with librsvg-2.0: yes
EXR load with OpenEXR: yes
OpenSlide load: no (dynamic module: no)
Matlab load with matio: no
NIfTI load/save with niftiio: no
FITS load/save with cfitsio: yes
GIF save with cgif: yes
Magick package: MagickCore (dynamic module: no)
Magick API version: magick7
load with libMagickCore: yes
save with libMagickCore: yes

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 caption_height on line 36 of your code to caption_height + (in.width() / 10) (to restore the padding) and using a long text caption, the broken color palettes can be seen. Interestingly, however, I found out something else: when using your code with the patch and long caption described above, it resulted in this: This makes me think that there's probably something overwriting the existing palette, since the first frame did not have any green/yellow colors to my knowledge. It also seems to change more depending on how large the caption part is.

[jcupitt]

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:

$ pkg-config imagequant --modversion
2.12.2

The version that ships with ubuntu 21.10.

[TheEssem]

That command results in 2.17.0 on my system. Another thing to note is that this also seems to happen with builds using Quantizr.

[jcupitt]

I've found it! Yes, there's an incredibly stupid bug in the change detector. I'll make a patch.

[jcupitt]

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.

[jcupitt]

This is a really bad bug, so we'll probably push out 8.12.2 next weekend.

[TheEssem]

Thanks for the speedy fix! :)