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

Converting from APNG to WebP caused an error about timestamp being a float #6977

Closed
wants to merge 3 commits into from

Conversation

singpolyma
Copy link

@singpolyma singpolyma commented Feb 28, 2023

Helps #7015

Changes proposed in this pull request:

  • cast timestamp to int in WebPImagePlugin so that APNG conversion works instead of crashing

@hugovk
Copy link
Member

hugovk commented Feb 28, 2023

Please can you include a test case that fails without the fix and passes with it?

@radarhere
Copy link
Member

To demonstrate that this problem at least doesn't exist with all APNG files, using https://github.com/python-pillow/Pillow/blob/main/Tests/images/iss634.apng, the following code runs with Pillow 9.4.0 without an error.

from PIL import Image
im = Image.open('iss634.apng')
im.save('out.webp')

@radarhere
Copy link
Member

if you feel uncomfortable writing a test case, just posting a self-contained example here would still be helpful.

@singpolyma
Copy link
Author

This WIP test includes an image from https://signalstickers.com/pack/2ef10d945db55e0712773e18b24febe0 so it may not be properly redistributable as part of your suite, but it reproduces the issue when my patch is not applied

@radarhere
Copy link
Member

Thanks. I see now that I missed the obvious fact that I needed to use save_all in my previous comment.

If I replace your test with

def test_float_duration(tmp_path):
    temp_file = str(tmp_path / "temp.webp")
    with Image.open("Tests/images/iss634.apng") as im:
        assert im.info["duration"] == 70.0

        im.save(temp_file, save_all=True)

    with Image.open(temp_file) as reloaded:
        reloaded.load()
        assert reloaded.info["duration"] == 70

does that sound good to you? It uses one of our existing test images.

@@ -285,7 +285,7 @@ def _save_all(im, fp, filename):
# Append the frame to the animation encoder
enc.add(
frame.tobytes("raw", rawmode),
timestamp,
int(timestamp),
Copy link
Member

@radarhere radarhere Mar 6, 2023

Choose a reason for hiding this comment

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

What do you think about using round instead of int, here and below?

@@ -305,7 +305,7 @@ def _save_all(im, fp, filename):
im.seek(cur_idx)

# Force encoder to flush frames
enc.add(None, timestamp, 0, 0, "", lossless, quality, 0)
enc.add(None, int(timestamp), 0, 0, "", lossless, quality, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enc.add(None, int(timestamp), 0, 0, "", lossless, quality, 0)
enc.add(None, round(timestamp), 0, 0, "", lossless, quality, 0)

@@ -285,7 +285,7 @@ def _save_all(im, fp, filename):
# Append the frame to the animation encoder
enc.add(
frame.tobytes("raw", rawmode),
timestamp,
int(timestamp),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int(timestamp),
round(timestamp),

@radarhere
Copy link
Member

I've created PR #6996 with my two suggestions.

@hugovk
Copy link
Member

hugovk commented Mar 23, 2023

Thanks for the PR, #6996 has been merged instead.

@hugovk hugovk closed this Mar 23, 2023
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

3 participants