Skip to content
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

GIF files with comments must be GIF89a #6291

Closed
raygard opened this issue May 13, 2022 · 1 comment · Fixed by #6292
Closed

GIF files with comments must be GIF89a #6291

raygard opened this issue May 13, 2022 · 1 comment · Fixed by #6292
Labels

Comments

@raygard
Copy link
Contributor

raygard commented May 13, 2022

What did you do?

Ran the GIF tests in Pillow-9.1.0, looked at output from test_comment_over_255(tmp_path) test.

What did you expect to happen?

The output GIF file should be GIF89a because it contains a comment.

What actually happened?

The file temp.gif has a signature of GIF87a.

What are your OS, Python and Pillow versions?

  • OS: Windows 10
  • Python: 3.10.4
  • Pillow: 9.1.0

For code, see def test_comment_over_255(tmp_path): in Tests\test_file_gif.py.

The generated image file is:

temp

Discussion:

This is due to an obsolete line in _get_global_header(im, info) in GifImagePlugin.py:

                extensionKey == "comment" and not (1 <= len(info[extensionKey]) <= 255)

The len(info[extensionKey]) should be simply tested for 0 to indicate no comment is wanted.
Suggested change is

                extensionKey == "comment" and len(info[extensionKey]) == 0

I have created a pull request #6292 for this.

As a side note, I think the entire feature test for GIF 87a vs. 89a can be changed from:

    version = b"87a"
    for extensionKey in ["transparency", "duration", "loop", "comment"]:
        if info and extensionKey in info:
            if (extensionKey == "duration" and info[extensionKey] == 0) or (
                extensionKey == "comment" and len(info[extensionKey]) == 0
            ):
                continue
            version = b"89a"
            break
    else:
        if im.info.get("version") == b"89a":
            version = b"89a"

to:

    version = b"87a"
    if (im.info.get("version") == b"89a") or info and (
            "transparency" in info or
            "loop" in info or
            ("duration" in info and info["duration"] != 0) or
            ("comment" in info and len(info["comment"]) != 0)):
        version = b"89a"

I think that's correct but would be happy for someone to look it over.

@radarhere
Copy link
Member

https://www.w3.org/Graphics/GIF/spec-gif89a.txt

  1. Comment Extension.
    ...
    b. Required Version. 89a.

You're right, thanks for catching that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants