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

SpiderImagePlugin: raise an error when seeking in a non-stack file #1794

Merged
merged 2 commits into from Apr 4, 2016

Conversation

radarhere
Copy link
Member

Copy of PR #1741, with a test added.

Using ImageSequence.Iterator on a non-stack SPIDER image leads to infinite loop.
EOFError (which stops the iteration) is never raised because when the image isn't
a stack, seek() returns gently without error.

Using ImageSequence.Iterator on a non-stack SPIDER image leads to infinite loop.
EOFError (which stops the iteration) is never raised because when the image isn't a stack,
seek() returns gently without error.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.92% when pulling fb86d69 on radarhere:spiderimageplugin into fd7fa4e on python-pillow:master.

@hugovk
Copy link
Member

hugovk commented Apr 3, 2016

Get an infinite loop as expected on latest Pillow 3.2.0 (Python 2.7 / OS X).

Check completes with this patch.


from PIL import Image, ImageSequence

im = Image.open("images/hopper.spider")
Copy link
Member

Choose a reason for hiding this comment

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

Please can you update this to "Tests/images/" so it can be run from the project root? That'll make it consistent with other checks/tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, done.

@radarhere radarhere force-pushed the spiderimageplugin branch 2 times, most recently from 6f02d25 to deb61a0 Compare April 3, 2016 10:54
@hugovk
Copy link
Member

hugovk commented Apr 3, 2016

Thanks. I forgot earlier: is it possible to unit test this raise too?

https://coveralls.io/builds/5638613/source?filename=PIL%2FSpiderImagePlugin.py#L176

@radarhere
Copy link
Member Author

I thought about that. Then I thought that if such a test failed, it would go into DOS, and that would be bad. I would have thought that was the idea behind separating the test into it's own file.

@wiredfool
Copy link
Member

If we're checking a non-stack file, we can just bail at 2 or more iterations of the same item. We could then incorporate that test into the normal test routine, since it wouldn't be a DOS.

(though, I do appreciate not having a DOS automatically run in the testing suite either way)

@radarhere
Copy link
Member Author

I realised that it is possible to write the additional test without running the risk of DOS. So I've updated the commit.

@wiredfool
Copy link
Member

@radarhere:

We could fold thedos check into the main spider tests like this:

im = Image.open("Tests/images/hopper.spider")
for i, frame in enumerate(ImageSequence.Iterator(im)):
    if i > 1:
        print ("DOS Check Failed")
        # or self.fail("Dos Check failed")
        break

@radarhere
Copy link
Member Author

Thanks for the code. Done. I've removed the original separate dos file, let me know if you think it would be better to have both.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling dabb68a on radarhere:spiderimageplugin into * on python-pillow:master*.

@wiredfool wiredfool merged commit ca5e22b into python-pillow:master Apr 4, 2016
@radarhere radarhere deleted the spiderimageplugin branch April 4, 2016 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants