Strange problem with gifsave

[lanyusan]

I am developing an Avalonia based desktop app for generative art using NetVips, which needs to composite images to create gif frames, join the frames to make a multiple page image, then call gifsave to generate the gif.

I have encountered a case where gifsave would generate black and white gif but after I convert the joined image to png buffer than reload it to vips image, gifsave would create colored gif correctly.

                    var pngBuffer = framesJoined.PngsaveBuffer();
                    framesJoined = NetVips.Image.NewFromBuffer(pngBuffer);

It seems the vips image with joined frames might need to be tuned to include certain color or band information for gifsave to work correctly. This problem would be fixed by above code of converting to png then reload to buffer.

I have created a reporduce repo and posted the issue in NetVips but it looks now it is might not be a NetVips issue.

The reproduce repo:

https://github.com/lanyusan/net-vips-avanolia-bug

main branch is the bug branch. fix branch is the workaround branch.

Please run dotnet run under project root and in the app select images folder under project root.

[jcupitt]

Hi @lanyusan,

This is usually a problem with the interpretation tag on an image. You probably have it set to something other than srgb when calling gifsave, so it's being written as a mono image plus three alpha channels. Try adding something like (just a guess, my C# isn't great):

image = image.Copy(interpretation=Vips.Enums.Interpretation.sRGB)

Just before the save.

Current libvips has better rules for deciding whether to write (mono + many alpha) or (rgb + alpha), so you can skip this extra set-interpretation step. Or just run current libvips on all of your systems, of course.

[jcupitt]

I noticed your reproducer has:

                for(var i =0; i < framesCount; i++ )
                {
                    delays.Add(10);

You're adding this twice, so I'd remove this first one. Though it probably doesn't matter.

[jcupitt]

... hmm actually, I'm wrong. I made a version of your code in python:

#!/usr/bin/python3

import sys
import re

import pyvips

pngs = [pyvips.Image.new_from_file(filename)
        for filename in sys.argv[1:] if re.match(".*\.png", filename)]
gifs = [pyvips.Image.new_from_file(filename, n=-1) 
        for filename in sys.argv[1:] if re.match(".*\.gif", filename)]

# number of pages all gifs have in common
n_pages = min([gif.get("n-pages") for gif in gifs])

# split gifs to pages
gifs = [[gif.crop(0, i * gif.get("page-height"), 
                  gif.width, gif.get("page-height"))
         for i in range(n_pages)]
        for gif in gifs]

# composite to a list of frames
frames = []
for i in range(n_pages):
    base_image = pngs[0]
    for_layering = pngs[1:] 

    for gif in gifs:
        for_layering.append(gif[i])

    blended_frame = base_image.composite(for_layering, "over")
    
    frames.append(blended_frame)

# and write as a GIF
image = pyvips.Image.arrayjoin(frames, across=1)
image = image.copy(interpretation="srgb")
image.set_type(pyvips.GValue.gint_type, "loop", 0)
image.set_type(pyvips.GValue.array_int_type, "delay", [10] * n_pages)
image.set_type(pyvips.GValue.gint_type, "page-height", frames[0].height)
print("writing x.gif ...")
image.write_to_file("x.gif")

And it still outputs a mono GIF. I'll investigate a bit more.

[lanyusan]

Thanks for the attention.

[jcupitt]

I think I found the error, for my test images at least.

The final gifsave is inheriting palettes from the input GIFs, and one of those gifs was monochrome. The solution is to set the reoptimise flag to gifsave to force it to compute a new palette for the final image.

This program:

#!/usr/bin/python3

import sys
import re

import pyvips

pngs = [pyvips.Image.new_from_file(filename)
        for filename in sys.argv[2:] if re.match(".*\.png", filename)]
gifs = [pyvips.Image.new_from_file(filename, n=-1)
        for filename in sys.argv[2:] if re.match(".*\.gif", filename)]

# number of pages all gifs have in common
n_pages = min([gif.get("n-pages") for gif in gifs])

# split gifs to pages
gifs = [[gif.crop(0, i * gif.get("page-height"),
                  gif.width, gif.get("page-height"))
         for i in range(n_pages)]
        for gif in gifs]

# composite to a list of frames
frames = []
for i in range(n_pages):
    base_image = pngs[0]
    for_layering = pngs[1:]

    for gif in gifs:
        for_layering.append(gif[i])

    blended_frame = base_image.composite(for_layering, "over")

    frames.append(blended_frame)

# and write as a GIF
image = pyvips.Image.arrayjoin(frames, across=1)
image.set_type(pyvips.GValue.gint_type, "loop", 0)
image.set_type(pyvips.GValue.array_int_type, "delay", [10] * n_pages)
image.set_type(pyvips.GValue.gint_type, "page-height", frames[0].height)
print(f"writing {sys.argv[1]} ...")
image.write_to_file(sys.argv[1])

With these inputs:

Run like this:

$ ./avanolia.py x.gif[reoptimise] ~/pics/brain.gif ~/pics/PNG_transparency_demonstration.png

Makes this:

Which I think is correct.

[jcupitt]

@kleisauke @dloebl I think cases like this mean we should flip the sense of reoptimise for 8.14 -- it's probably too dangerous and confusing to have as a default behaviour.

How about deprecating reoptimise and changing the default behaviour to always recompute the palette. As an option, we add a new flag called perhaps copy_palette (maybe? any better ideas?) which will skip the reoptimisation step and reuse the palette from the metadata, if available.

This is a compatibility break, of course, which is bad :( But probably best done as soon as possible.

What do you think?

[jcupitt]

Alternatively, I suppose we could change the way metadata is copied.

At the moment if there are two palettes on an image we always take the first. Instead, we could remove all palettes if there's ambiguity (probably neither is correct). We could do the same with ICC profiles I guess.

[lovell]

Am I right in thinking this problem occurs only when using image-combination operations like arrayjoin and composite? If so, I'm +1 for implicitly removing all input palette metadata during specific operations rather than change existing API.

[jcupitt]

Yes, it's anything that combines several images into one.

[kleisauke]

I remembered a similar discussion about this, see e.g. #2576 (comment).

I agree, we should not assume that we can reuse the palette when using both GIF as input and output.

For ICC profiles, it's probably not a issue. At worst, it may cause colors to be misinterpreted (e.g., when using an RGB profile with CMYK images, which we already have a sanity-check for), or a slight color distortion (e.g. issue #3150). Disabling that behavior by default could make such issues go unnoticed.

Perhaps we should implement the proposed API mentioned in bleroy/core-imaging-playground#26 (comment)? For example:

$ vips copy dancing-banana.gif[n=-1] x.gif[preserve=palette]

Or, perhaps?:

$ vips copy dancing-banana.gif[n=-1] x.gif[reuse=palette]

(since preserve might be the wrong term for this, the palette can still be changed and is not preserved as-is)

I don't know how much work that will be and if it can still be done in the 8.14 release cycle. It's probably less work than the proposed INDEXED mode (which requires changes in libnsgif).

BTW, for reference, this issue was originally reported at kleisauke/net-vips#185 and deliberately cross-posted at https://github.com/libvips/libvips/discussions/3194. The reporter mentioned that it occurred only in Avalonia (+ did not mention that it used GIF as input), which is why I initially mis-triaged this.

[jcupitt]

OK, it sounds like changing gifsave to not reuse palettes by default is the simplest and safest option.

We could perhaps think about some reenabling palette use by default if we know there's no ambiguity in the future some time.

Thanks!