Hi @TheEssem,

The libvips loop property is used by eg. webp and others as well, so it doesn't have the usual GIF meaning (there's a hidden field called gif-loop with the bizarre GIF interpretation). It really does mean the total number of iterations.

Instead of a flag, how about automatically setting CGIF_ATTR_NO_LOOP if loop == 1? I suppose we might need to subtract 1 from loop for values > 1.

Ah, I see. Fixed.

I tested it a bit. It looks like the GIF loader needs a matching change :/ I thought it had this logic in already.

Patched nsgif to (hopefully) fix it, let me know if there's a more elegant solution.

I don't think we should patch libnsgif unless there's really no choice. I'll see if there's some way to do it within the libnsgif API.

Oh heh you're right, there's no way to do this and stay in the API.

What do you think, @tlsa? It looks like we need to change the way libnsgif reports loop_max. The cases are:

1. GIF contains no NETSCAPE2.0 block Play the animation once, ie. loop_max == 1.
2. GIF contains a NETSCAPE2.0 block with a loop count of 0 Play the animation forever, ie. loop_max == 0.
3. GIF contains a NETSCAPE2.0 block with a loop count of 1 Play the animation twice, ie. loop_max == 2.

I think 3. is the case that libnsgif is currently missing.

Good spot, thanks @TheEssem, @jcupitt. I'll fix LibNSGIF.

Done upstream:

• Fix handling of loop count value in application extension
• Support AnimExts Looping Application Extension

The libnsgif change has helped a lot, now we need to make it work with cgif 0.2 (the version that's used for testing, since 0.3 isn't out yet).

We'll need a test in libvips/meson.build, if you're able, or I could have a go if you like.

Eg. something like:

cgif_dep = disabler()
if quantisation_package.found()
    cgif_dep = dependency('cgif', version: '>=0.2.0', required: get_option('cgif'))
    if cgif_dep.found()
        libvips_deps += cgif_dep
        cfg_var.set('HAVE_CGIF', '1')
        if cc.compiles('#include <cgif.h>\nint i = CGIF_ATTR_NO_LOOP;', name: 'Has CGIF_ATTR_NO_LOOP', dependencies: cgif_dep)
            cfg_var.set('HAVE_CGIF_ATTR_NO_LOOP', '1')
        endif
    endif
endif

Then you can add an #ifdef HAVE_CGIF_ATTR_NO_LOOP to your C.

How exactly would the HAVE_CGIF_ATTR_NO_LOOP fallback work? Would it just revert to the old behavior?

Yes, I suppose so, unless you can think of something better.

I think you don't need a cc.compiles() here, because it is gated on whether you have 0.3, right?

So you can just check cgif_dep.version().version_compare('>=0.3') to see if it is new enough -- this runs entirely as an in process string version comparison, rather than forking to the compiler and running a compile check.

My code scrap was gated on 0.2+ I think, so you would need to test for CGIF_ATTR_NO_LOOP as well.

The usual principle is to test for features rather than version numbers where possible. Version number tests can fail if features get backported, for example.

Oh, I misread your comment, sorry. Sure, we could use version_compare too, I don't really mind.

OK, tests are passing, let's merge! Thank you for doing this work, @TheEssem.

Version number tests can fail if features get backported, for example.

I don't see any rationale for supporting people who backport API/ABI breakages, since that's really just another way of saying "they have hard forked the project and decided to do their own thing that isn't expected to be compatible".

But, do you have an example of people doing this... I'm sorry, incredibly hostile action by deliberately screwing over users? :)

I think the example you see most often is ImageMagick :( They have a sad history of adding and removing API on minor version number changes, and also backporting. A single scalar isn't enough to represent an IM version number, you need at least (x + iy) hehe. I think they are trying harder with im7 to provide a supported API, so perhaps it's not the issue it was.

You know a lot more than me about this of course, maybe I'm being overly cautious.