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

Load broken images #1428

Merged
merged 3 commits into from
Sep 24, 2015
Merged

Conversation

homm
Copy link
Member

@homm homm commented Sep 15, 2015

This PR revert change from #38. There was a couple of quick fixes and this change. As I know there was no discussion about this decision.

Later @cezarsa try to create the option for loading broken images in #311. He said:

Because I'm not sure about what use cases @d-schmidt had in mind when the size check was added, I've introduced a new flag called IGNORE_DECODING_ERRORS that if True will never raise IOError.

The problem with not opened images was not confirmed and this changes was reverted.

My point

  1. I don't see a big difference between "truncated image" and "image with broken data stream", at least for my tasks. I think what you may want to read original not corrupted image from a file (LOAD_TRUNCATED_IMAGES = False), or you may want to load as many data from an image as you can (LOAD_TRUNCATED_IMAGES = True). How exactly an image is corrupted (is it truncated or has broken datastream) does not matter for almost all tasks.
  2. As I understand Pillow currently loads broken images (not only truncated) if at least first chunk of the image was loaded correctly (n != 0).
  3. Corrupted first chunk does not mean what nothing was loaded. It can be corrupted in the middle or close to the end. I've attached the test image with corrupted first chunk but which reads for more than the half. Unfortunately, I don't know anything about its licence (and I just realized what we probably need to remove some of my previous added test images due to legal purposes :-).
  4. This can be confusing, but I also don't see a big problem even if nothing was loaded at all. This is just a special case of "something was loaded" in opposite to "everything was loaded correctly". I bet most of image viewers will show a black or transparent rectangle instead of some sort of error in this case.

@homm
Copy link
Member Author

homm commented Sep 18, 2015

Any thoughts?

For being a part of 3.0.0, this should be in master for some time so everyone can confirm what this works for she. There only 12 days left until the release.

@wiredfool
Copy link
Member

I don't have a good handle on the implications of this, and I haven't had a chance to dig through in more depth.

@homm
Copy link
Member Author

homm commented Sep 18, 2015

@wiredfool but you can agree or disagree with my arguments :)

@radarhere
Copy link
Member

Side question - which test images need to be replaced? All of the ones that you've added previously, or just a few?

@homm
Copy link
Member Author

homm commented Sep 22, 2015

I've replaced broken image with broken hopper.png copy.

I'll replace other not CC images later in another PR.

@wiredfool
Copy link
Member

So, the net difference here is that some images will return something where they didn't previously. It's limited to when LOAD_TRUNCATED_IMAGES is on, and there's an error in the first block.

The only possible way this could be a regression is if the image had literally nothing returned from the decoder. Previously, we'd toss an error, now they'll get a black image.

I think it's safe.

wiredfool added a commit that referenced this pull request Sep 24, 2015
@wiredfool wiredfool merged commit 388b2da into python-pillow:master Sep 24, 2015
@homm
Copy link
Member Author

homm commented Sep 24, 2015

👍

@homm homm deleted the load-broken-images branch April 28, 2016 00:22
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.

3 participants