Skip to content

docs(encoder): correct write_image panic condition for ExtendedColorType#2982

Merged
197g merged 2 commits into
image-rs:mainfrom
SAY-5:docs/write-image-panic-bits-per-pixel
May 26, 2026
Merged

docs(encoder): correct write_image panic condition for ExtendedColorType#2982
197g merged 2 commits into
image-rs:mainfrom
SAY-5:docs/write-image-panic-bits-per-pixel

Conversation

@SAY-5
Copy link
Copy Markdown
Contributor

@SAY-5 SAY-5 commented May 24, 2026

Fixes #2425.

The # Panics doc on ImageEncoder::write_image and the per-codec encode methods stated Panics if width * height * color_type.bytes_per_pixel() != buf.len(). These functions take an ExtendedColorType, which has no bytes_per_pixel() method (only bits_per_pixel()), and the formula is wrong for sub-byte color types like L1.

This corrects the documented condition to match the actual requirement: the buffer must hold exactly the bytes for the given dimensions and color type, with each row padded to a whole number of bytes (height * ((width * bits_per_pixel + 7) / 8)), which is what ExtendedColorType::buffer_size already computes.

As noted in the issue, the same stale text was copied across several files; updated all six occurrences in io/encoder.rs, codecs/tiff.rs, codecs/jpeg/encoder.rs, codecs/tga/encoder.rs, codecs/webp/encoder.rs, and codecs/bmp/encoder.rs.

cargo doc --no-deps --lib builds with no new warnings.

@RunDevelopment
Copy link
Copy Markdown
Member

Thanks for the PR!

I'm not sure if this is the best approach. It's correct but also duplicates information. For example, if I said, "The as u32 in the code snippet is wrong because it documents an overflow that doesn't exist," then you'd have to change all copies.

I think it would be better to say "Panics if color.buffer_size(width, height) != buf.len()," but buffer_size isn't public. But maybe it should be? Not just for this. Third-party encoder implementations should also find this function useful. What do you think, @197g?

@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 25, 2026

Good point on the duplication risk. If you'd like, I can make buffer_size pub (or pub(crate) -> pub) so the panic condition can just read Panics if color_type.buffer_size(width, height) != buf.len() as u64``. Would you prefer that approach, or a prose-only description without the inline formula?

@197g
Copy link
Copy Markdown
Member

197g commented May 25, 2026

We haven't made it public previously out of concern that the interpretation of it as bits-per-line times height is not sensible for all of them, and that we might silently be incorrect / have unexpected results in some cases. Consider for instance a block layout with a minimum alignment may want to round up to 4 byte boundaries instead of just the byte. (On WebGPU the row alignment would be as high as 256).

However, this implies that the documentation needs more fixing. Making it public with an explicit mention of the computation's rounding mode being to the nearest byte and nothing else would be my preferred way to see this handled. We can always add different calculation methods later (and with the new metadata in the decoding pipeline a decoder may choose this appropriately).

…ge panic

Signed-off-by: say <say.apm35@gmail.com>
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 25, 2026

Made buffer_size pub with explicit byte-rounding docs and updated the panic doc to Panics if buf.len() as u64 != color_type.buffer_size(width, height).

@197g 197g merged commit 6c6ce12 into image-rs:main May 26, 2026
31 checks passed
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.

Panic documentation for ImageEncoder::write_image is outdated

3 participants