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 durations should be preserved from frame info #6259

Closed
raygard opened this issue May 2, 2022 · 1 comment · Fixed by #6265
Closed

GIF durations should be preserved from frame info #6259

raygard opened this issue May 2, 2022 · 1 comment · Fixed by #6265
Labels

Comments

@raygard
Copy link
Contributor

raygard commented May 2, 2022

What did you do?

Copied an animated GIF:

with Image.open('Tazspin.gif') as im:
    im.save(f'new.gif', save_all=True, optimize=True, interlace=0)

What did you expect to happen?

Hoped the copy would be something like the original Tazspin.gif:

Tazspin

What actually happened?

Got this:

new

What are your OS, Python and Pillow versions?

  • OS: Windows 10
  • Python: 3.10.4
  • Pillow: 9.1.0 (slightly modified but you'll get a very similar result with stock 9.1.0)

Discussion:

Duration has been a problem in the past. I think that in the absence of a duration= arg to .save(), the durations ("delay time" in the GIF spec) from the individual frames should be preserved in the output.

Maybe something like this: in _write_multiple_frames() (in GifImagePlugin.py), delete the line

    duration = im.encoderinfo.get("duration", im.info.get("duration"))

Then modify the code that sets im.encoderinfo from im_frame.info:

                for k, v in im_frame.info.items():
                    if k == "transparency" or k == "duration":
                        continue
                    im.encoderinfo.setdefault(k, v)

(Note I am including code changed in PR #6176. I am not making a PR out of this because I don't know how to deal with such conflicts and because I am not sure if this is a good fix. Need input from @radarhere.)

And then before the line if isinstance(duration..., insert:

            if "duration" in im_frame.info:
                encoderinfo.setdefault("duration", im_frame.info["duration"])
            duration = encoderinfo.get("duration")

This seems to give me the desired result:

new_910_fix_d1

I hope it doesn't break anything.

@radarhere
Copy link
Member

It's fine to create conflicting PRs - it just means that whichever one doesn't get merged in first needs to be updated later. And the test suite is what is there to prevent PRs from breaking things.

I've created PR #6265, similar to your suggestion.

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