Skip to content

Split PNM ImageFormat into subformats for PBM, PGM, etc.#2841

Open
mstoeckl wants to merge 3 commits into
image-rs:mainfrom
mstoeckl:pnm-encode-split
Open

Split PNM ImageFormat into subformats for PBM, PGM, etc.#2841
mstoeckl wants to merge 3 commits into
image-rs:mainfrom
mstoeckl:pnm-encode-split

Conversation

@mstoeckl
Copy link
Copy Markdown
Contributor

This PR tries to fix #2810.

It splits ImageFormat::Pnm into five different formats, so that each of the NetPBM formats has its own enum entry. This makes it possible for methods like DynamicImage::save to set the exact format based on the file extension, so that saving to a ".pgm" file will only try to save a PGM image.

This behavioral change may surprise anyone who previously was writing Rgba8 images to a ".pnm" file (actually producing PAM); this will now fail because PNM does not support alpha.

Other than this change, I have tried to avoid changing the behavior of the PnmDecoder, so that anyone using it directly will continue to see the same output. The only addition is a with_dynamic_pnm_header() method to allow autoselecting a PNM header.

I've also added some test images that cover all the main netpbm format variations; some of them are also used as reference images for updated PNM encoding tests in tests/save_pnm.rs.

The test images were all made by me, and licensed Apache-2.0 OR MIT OR CC-0. I've checked their references against NetPBM's conversion (using pamdepth 65535 <$in | pamtopng -verbose >$out.png; the test image maxvals are chosen to be exactly convertible to 16 bit without rounding.).

The test images are easier to compare for correctness with
other decoders. Note that some test images have been modified
to have unusual but valid headers and whitespace.
The new ImageFormats ensure that methods like DynamicImage::save
only write image data which is compatible with the file extension.
(Previously PAM image data was written for any path with a NetPBM
extension, producing images that some other programs couldn't read.)

The PnmDecoder continues to accept any NetPBM format.

This commit also fixes a few match expression cases and parameterizes
error messages.
@fintelia
Copy link
Copy Markdown
Contributor

fintelia commented Apr 6, 2026

I'm torn on this because I don't love having so many NetPBM formats, but I also don't see a better approach to fixing the issue

@fintelia
Copy link
Copy Markdown
Contributor

I think there might be an alternative approach to this.

As I understand it, the problem is that some of the encoding methods in this crate take a Path but are implemented in terms of other methods that work with an ImageFormat. But there's nothing stopping us from changing them to internally pass both the desired format and the original path (if known) when constructing an encoder. If we did that, PNM output could properly dispatch to the right variant based on the actual extension and we wouldn't need to change the public API other than adding a note to the relevant docs.

@mstoeckl
Copy link
Copy Markdown
Contributor Author

mstoeckl commented Apr 24, 2026

But there's nothing stopping us from changing them to internally pass both the desired format and the original path (if known) when constructing an encoder. If we did that, PNM output could properly dispatch to the right variant based on the actual extension and we wouldn't need to change the public API other than adding a note to the relevant docs.

While this could work, I would actually consider this a larger break in behavior than splitting the PNM format, because users of the API now need to consider the note and think about whether they need to pass file extension information, even if they do not use PNM specifically. It would also introduce a mismatch between methods like DynamicImage::write_to and DynamicImage::save, where the latter can be used to write PAM files due to extension autodetection and the former can't.

@fintelia
Copy link
Copy Markdown
Contributor

We can make the change in the 1.0 release, so I'm less worried about behavior changes.

There is already the fully general DynamicImage::write_with_encoder so the other methods mostly serve as more convenient short-hands. I'm imagining that we'd keep the public method signatures completely intact and just make sure than any method which takes a path uses the file extension when constructing an encoder.

My main goal is avoiding clearly wrong behavior while avoiding an increase in the overall API surface to handle an edge case.

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.

DynamicImage::save writes PAM into .pgm paths

2 participants