Skip to content

Panic documentation for ImageEncoder::write_image is outdated #2425

@RunDevelopment

Description

@RunDevelopment

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: 9a8591a 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind: documentationAn enhancement that does not change function

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions