#38 4.2.0 ignores EGifSetGifVersion

closed
nobody
5
2012-10-16
2012-10-04
Tony Cook
No

4.2.0 includes a change that tests the provided extensions to check which version stamp is required, but the change broke the lower level API.

Calling EGifSetGifVersion() sets GifVersionPrefix[3-5]:

void
EGifSetGifVersion(const char *Version) {
strncpy(GifVersionPrefix + GIF_VERSION_POS, Version, 3);
}

but does not modify GifVersionPrefix[0], which is what EGifPutScreenDesc() tests to check if GifVersionPrefix has been set:

if (GifVersionPrefix[0] != '\0')
    write_version = GifVersionPrefix;
else if (Private->gif89)
    write_version = GIF89_STAMP;
else
    write_version = GIF87_STAMP;

so EGifSetGifVersion() is effectively ignored.

A related issue is that the gif89 member of the Private structure never seems to be cleared. In my case this resuled in a hard to reproduce bug where sometimes I'd get an 87a header when I'd requested an 89a header, depending on the state of the memory returned by malloc() in EGifOpen().

This second issues also appears to be present in giflib 5 (by code inspection).

Related: https://rt.cpan.org/Ticket/Display.html?id=79679 (the original bug report leading to this one)

Discussion

  • I have done two things to address this.

    First, I have added code to both branches to ckear the gif89 flag where it seems appropriate.

    Second, I have started a 4.2 branch in the repository to include the fix to EGifSetGifVersion().

    Please test to verify that these fixes address your issue. If they do, I'll ship 4.2.1 and 5.0.1 releases.

     
  • Tony Cook
    Tony Cook
    2012-10-08

    Thanks, this fixes the problem for 4.2.

    For 5.0 there's a different problem.,

    The documentation claims:

    "The default GIF version to write is now computed at write time from the types of an image's extension blocks. (Formerly EGifSpew() behaved this way, but the sequential-writing code didn't.) The result of this computation is available through the new function EGifGetGifVersion()."

    This implies that the sequential code automatically calculates the version required based on the extension blocks, which it hasn't see at the time of EGifPutScreenDesc(). I'd assumed some sort of internal buffering but hadn't checked.

    It turns out that it always writes a GIF87a header using the sequential APIs (with the bug fix above), since for sequential calls no extensions are stored at the time EGifPutScreenDesc() is called.

    I suspect what's needed is to provide an EGifSetGifVersion() that's applied to an open GifFile object, or to disable the sequential API.

    My preference would be the first, since I'd have to rewrite my GIF code otherwise.

     
  • I have added EGifSetGifVersion() to the 5.x branch and documented it.

    Please test and verify that this does what you need. If so, I'll issue 5.0.1.

     
  • Tony Cook
    Tony Cook
    2012-10-10

    Thank you, that solves the problem.

     
  • Fixed in 5.0.1, fix confirmed by bug submitter.