image icon indicating copy to clipboard operation
image copied to clipboard

Panic documentation for `ImageEncoder::write_image` is outdated

Open RunDevelopment opened this issue 10 months ago • 1 comments

The condition for when ImageEncoder::write_image is expected to panic is currently documented as follows:

    /// # Panics
    ///
    /// Panics if `width * height * color_type.bytes_per_pixel() != buf.len()`.
    fn write_image(
        self,
        buf: &[u8],
        width: u32,
        height: u32,
        color_type: ExtendedColorType,
    ) -> ImageResult<()>;

This doesn't make sense, because ExtendedColorType doesn't have a bytes_per_pixel() method, only bits_per_pixel. This is only natural as ExtendedColorType supports formats like L1.

Currently, it is unclear how ImageEncoder::write_image behaves when color_type.bits_per_pixel() is not divisible by 8.

(Also, this doc comment has been copied a lot. Search for "Panics if `width * height" and you'll find 7 copies in 6 files. There might be more, I didn't search for long.)

As for how this came to be: 9a8591ac35e89fb6043f31574e3955c3fb798e5f added the documentation and #2142 changed the signature of write_image without updating it.


Somewhat related, it would also be nice if the data layout of ExtendedColorType::L1 was explained. I assume that it stores 8 pixels in one byte, but it's not clear to me (1) how it handles images with width % 8 != 0 and (2) which bit corresponds to which pixel (i.e. is the pixel at x=0,y=0 the LSB or MSB in the first byte of buf?). Same for a lot of the other variants.

RunDevelopment avatar Feb 23 '25 00:02 RunDevelopment

Thanks for raising this issue! The real requirement on write_image is that the provided byte slice length is precisely what's expected given the color type.

We probably need to write out a description somewhere explaining the expected memory layout for pixel data. The general overview is:

  • 8-bit pixel data is arranged sequentially in row-major format without any padding.
  • 16-bit and 32-bit data is expected to be in native endian
  • sub 8-bit pixel data is packed with multiple samples in a single byte. Rows of pixels are padded to byte boundaries. IIRC both PNG and TIFF follow the same convention on the ordering of bits within a byte and I think we do the same. But I'd have to double check.
  • Places that accept byte slices do not require any alignment.

fintelia avatar Mar 01 '25 22:03 fintelia