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

pnm: Optimize and improve validation for read_separated_ascii #2421

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

mstoeckl
Copy link
Contributor

@mstoeckl mstoeckl commented Feb 16, 2025

Per commit message:

This function now rejects numbers that start with a leading +, although to match netpbm behavior it does not reject leading zeros. It also avoids allocating on the non-error path, reducing the time needed to read a large plain PPM test image to 50%. As the function no longer keeps the entire token in memory at once, the error messages no longer specify the bad token.

  1. I don't think losing error message specificity here is a big problem, because PNM images are essentially never written by hand so the errors will rarely occur; if parsing does fail, one can easily find non-digit non-whitespace characters in a file with a regexp (e.g. grep -P '[^0-9 \t]')
  2. To create a large "plain" (P3) image to test this with, use e.g. pngtopnm -plain large.png > ~/output.ppm. Depending on the png bit depth this can create both 8-bit and 16-bit PPM files.

This function now rejects numbers that start with a leading +,
although to match netpbm behavior it does not reject leading zeros.
It also avoids allocating on the non-error path, reducing the
time needed to read a large plain PPM test image to 50%. As the
function no longer keeps the entire token in memory at once, the
error messages no longer specify the bad token.
@fintelia fintelia merged commit 10d8b66 into image-rs:main Feb 17, 2025
31 of 32 checks passed
@fintelia
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.

2 participants