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

Raise an EOFError when seeking too far in PNG #4528

Merged
merged 4 commits into from Apr 6, 2020

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Apr 5, 2020

Resolves #4518

Looking at PngImagePlugin, there's no need for n_frames or is_animated to use property decorators - they can just be regular properties.

You might think this is just a simplification of the code, and that it would have no effect on behaviour. This is not the case - the common _seek_check method changes behaviour. If _n_frames is present, and None, then it doesn't check the upper limit on the frame number, because that would potentially require seeking all the way to the end of the file. Now that there is no _n_frames, it checks the upper limit, and throws an EOFError.

I've added a test to ensure that an EOFError is thrown moving forward.

Testing, I find that this resolves the issue. This is also noted in #4518 (comment).

@hugovk
Copy link
Member

hugovk commented Apr 5, 2020

@pmrowla Please could you have a look at this PR? Thank you!

@pmrowla
Copy link
Contributor

pmrowla commented Apr 5, 2020

This has the same effect as what I suggested in #4519, and it's simpler. Testing w/scikit-image also shows the the original downstream issue would be resolved.

@python-pillow python-pillow deleted a comment from codecov bot Apr 5, 2020
@hugovk
Copy link
Member

hugovk commented Apr 6, 2020

@radarhere Thoughts about the doccheck failure? I checked the branch out locally and cannot reproduce.

I'd restart it, but the restart button is now missing for me on this repo on Travis CI.

@python-pillow python-pillow deleted a comment from codecov bot Apr 6, 2020
@radarhere
Copy link
Member Author

It's not just this branch. It's appeared in others.

Why now?

The last master run - https://travis-ci.org/github/python-pillow/Pillow/jobs/671262347#L4634 - used Sphinx v2.4.4.

The issue would be that then, Sphinx v3 was released - https://github.com/sphinx-doc/sphinx/releases/tag/v3.0.0

And so then this PR uses Sphinx v3.0.0 - https://travis-ci.org/github/python-pillow/Pillow/jobs/671293213#L4559

What is the error?

https://travis-ci.org/github/python-pillow/Pillow/jobs/671293213#L4567-L4573

A series of warnings like

WARNING: duplicate object description of PIL.PngImagePlugin.PngImageFile, other instance in reference/plugins, use :noindex: for one of them

Sphinx has realised that we are documenting PngImageFile twice. You can see this at https://pillow.readthedocs.io/en/latest/reference/plugins.html#module-PIL.PngImagePlugin - once at the start of the module, and once if you scroll down under ChunkStream.

I've pushed a commit removing the first one. See https://pillow-radarhere.readthedocs.io/en/png_seek/reference/plugins.html#module-PIL.PngImagePlugin for what this now looks like.

@hugovk hugovk merged commit e634c4d into python-pillow:master Apr 6, 2020
@hugovk
Copy link
Member

hugovk commented Apr 6, 2020

And I had 2.4.1 locally. Thanks!

@hmaarrfk
Copy link
Contributor

Kinda nudge toward a release since this issue seems to affecting one passionate user for a release

conda-forge/scikit-image-feedstock#56

@hugovk
Copy link
Member

hugovk commented Apr 24, 2020

Thanks for the nudge! We'll do a release this weekend, keep an eye on #4354 (comment).

@radarhere
Copy link
Member Author

Pillow 7.1.2 has now been released with this fix.

@hmaarrfk
Copy link
Contributor

Thanks!

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.

AttributeError: 'NoneType' object has no attribute 'read'
4 participants