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

Corrected version number when saving GIFs #1379

Closed
wants to merge 1 commit into from

Conversation

radarhere
Copy link
Member

Reported in #1355. GifImagePlugin has the output version number fixed a 87a, but may use features that are only valid for 89a.

http://www.w3.org/Graphics/GIF/spec-gif89a.txt
"The specification given here defines version 89a, which is an extension of version 87a."

The features that require 89a are -

  • Graphic Control Extension
  • Comment Extension (unused by Pillow)
  • Plain Text Extension (unused by Pillow)
  • Application Extension

"An encoder should use the earliest possible version number that includes all the blocks used in the Data Stream."

I think that this sounded reasonable in 1990, but today, both versions of GIF would be supported on a given platform. My suggestion is - if 89a features are used, then 89a. Otherwise, if the input has a version, use that. Otherwise, 87a.

This PR makes that change.

@radarhere radarhere force-pushed the gifversion branch 5 times, most recently from 78f00bc to 9f77b6e Compare August 20, 2015 11:14
break
else:
if im.info.get("version") == "89a":
version = b"89a"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got test coverage drop report, I can suggest to move this to separate unit and add simple unit test for it. But yeah, I know that project's layout is not very friendly to create new functions. Also please let me know if you need my help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just added a specific test. You're welcome to help out if you want to, but I will get it working sooner or later.

@radarhere
Copy link
Member Author

Combined into #1384

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 this pull request may close these issues.

None yet

2 participants