Skip to content

GIF extents size change #8218

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

Closed
Yay295 opened this issue Jul 9, 2024 · 4 comments · Fixed by #8237
Closed

GIF extents size change #8218

Yay295 opened this issue Jul 9, 2024 · 4 comments · Fixed by #8237
Labels

Comments

@Yay295
Copy link
Contributor

Yay295 commented Jul 9, 2024

The GIF file test_extents() test

Pillow/Tests/test_file_gif.py

Lines 1381 to 1390 in 6a9acfa

def test_extents() -> None:
with Image.open("Tests/images/test_extents.gif") as im:
assert im.size == (100, 100)
# Check that n_frames does not change the size
assert im.n_frames == 2
assert im.size == (100, 100)
im.seek(1)
assert im.size == (150, 150)

tests that an image with two frames, where the second frame is offset from the first, will change the size of the image. The actual frame data is that each frame is 100x100, and the second frame is offset by 50 pixels both right and down. Here's an equivalent image with different colored frames (to make it easier to see them):

grow

created with ImageMagick magick -delay 50 -size 100x100 xc:#FF0000 ( xc:#0000FF -set page +50+50 ) grow.gif

Logically, when overlaying one image onto another with an offset, the size of the image will increase by that offset. However, I have not found any application that actually does this when displaying this GIF. Every application I have tried has kept the size of the first frame, cutting off any pixels from the second frame that extend beyond that. ffplay even gives a warning message: "Image too wide by 50, truncating."

Based on this real-world testing, I think Pillow should probably do the same as everyone else and not change the image size when the GIF frames are different sizes.

It also seems that behind the scenes, the image is not actually 150x150. len(im.getdata()) at the end of that test is 10000, not 22500, and im.getpixel((123,123)) raises an IndexError.

@Yay295
Copy link
Contributor Author

Yay295 commented Jul 9, 2024

Related: #3822 #2383

@radarhere radarhere added the GIF label Jul 9, 2024
@radarhere
Copy link
Member

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

Each image must fit within the boundaries of the Logical Screen, as defined in the Logical Screen Descriptor.

I take the word 'must' to mean that images that don't are invalid. Pillow often tries to be flexible though, so we aim to process this image anyway. Given that the images are going against the spec, I don't think there can be an 'official' answer as to what we should do.

The current solution was suggested in #2383 (comment)

I'd propose that if the extents are outside the existing image, we should increase the canvas size to cover the union of the existing and next frame.

My feeling is that if the choice is between 'passing along more image content to the user' and 'cutting off unexpected content', then showing more image content is better. In the face of ambiguity, I also have a general reluctance to reverse earlier decisions.

If we're talking about images that go against the specification, then having a consistent experience across all decoders sounds like an improbable goal, because, well, it's an image with undefined behaviour.

@Yay295
Copy link
Contributor Author

Yay295 commented Jul 16, 2024

I'd propose that if the extents are outside the existing image, we should increase the canvas size to cover the union of the existing and next frame.

It doesn't seem like this is actually happening though, because as I mentioned at the end

len(im.getdata()) at the end of that test is 10000, not 22500, and im.getpixel((123,123)) raises an IndexError.

The only thing that seems to actually be changed is the reported size.

@radarhere
Copy link
Member

I've created #8237

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