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

Added support for decoding plain PPM formats #5242

Merged
merged 22 commits into from Jun 14, 2022

Conversation

Piolie
Copy link
Contributor

@Piolie Piolie commented Feb 2, 2021

Resolves #860
Resolves #4999
Builds upon @gofr's PR draft #5009.

Changes proposed in this pull request:

  • only adds decode support, not encode support;
  • includes several tests;
  • does error handling;
  • is also affected by PGM and PPM maximum grey/color value is ignored #5008. It would be easy to fix this. However, it would make the results of the raw and plain formats inconsistent, so I decided to leave it as is;
  • in plain PBM the inter-token whitespace is optional, so the decoder differs from the one for the other formats;
  • there is also a hack to handle the fact that the 32-bit PGM extension expects signed ints, unlike the 8- and 16-bit ones, which are unsigned.

I made the changes on top of my other, still open, PR (#5121).

gofr's PR could potentially read the whole file at once (if there are no LFs) which for very large files could take up lots of memory. To avoid this, I tried to read one char at a time and although the implementation was easy, the performance was poor. I finally opted to read 1 MB blocks and process each one instead. The code ended up being more complex,* so there's that. On the bright side, it could easily be adapted not to _pull_fd, if it's any use.

*Strictly speaking, the PPM format doesn't allow comments in the raster section. However, these are commonly used. Ignoring them even when they span several blocks or split tokens in half was the hardest part.

@radarhere radarhere mentioned this pull request Feb 2, 2021
@Piolie Piolie changed the title Plain ppm Add support for decoding plain PPM formats Feb 2, 2021
src/PIL/PpmImagePlugin.py Outdated Show resolved Hide resolved
src/PIL/PpmImagePlugin.py Outdated Show resolved Hide resolved
src/PIL/PpmImagePlugin.py Outdated Show resolved Hide resolved
@djbuijs
Copy link

djbuijs commented Jun 3, 2022

I'm looking for this feature as well. What's holding up the merge? I'd be happy to help bring this over the finish-line if there's anything left to do.

@radarhere
Copy link
Member

After #6242, it has become apparent that the hopper_32bit had a maximum value outside of the 0-65535 range. So I've removed those test images.

@radarhere radarhere force-pushed the plainPPM branch 3 times, most recently from 9b5d3aa to c6646f7 Compare June 13, 2022 01:36
@radarhere
Copy link
Member

@djbuijs I've now resolved the merge conflict, and added support for arbitrary maxval in plain formats to continue the work of #6119. Those were the issues that I was conscious of. What's left is just reviewing it to make sure that everything is correct and there's no straightforward improvements to be made.

@radarhere radarhere force-pushed the plainPPM branch 2 times, most recently from 0f7e75e to da43130 Compare June 13, 2022 10:33
@radarhere radarhere merged commit aad3af4 into python-pillow:main Jun 14, 2022
@radarhere radarhere changed the title Add support for decoding plain PPM formats Added support for decoding plain PPM formats Jun 14, 2022
@djbuijs
Copy link

djbuijs commented Jun 14, 2022

Thank you @radarhere!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants