image icon indicating copy to clipboard operation
image copied to clipboard

Unnecessary `memset()`+`memcpy()` in HDR decoding

Open Shnatsel opened this issue 1 year ago • 2 comments

As of image 0.24.5, the HDR decoding needlessly copies the entire output in memory, acquiring a Vec and then calling copy_from_slice to create another Vec:

https://github.com/image-rs/image/blob/55732d14434cf639f2e79890f43a1d48bfb8617b/src/codecs/hdr/decoder.rs#L156-L171

The following function can directly return the Vec it gets from the underlying decoder:

    /// Read the actual data of the image, and store it in Self::data.
    fn get_image_data(&mut self, buf: &mut [u8]) -> ImageResult<Vec<u8>> {
        assert_eq!(u64::try_from(buf.len()), Ok(self.total_bytes()));
        match self.inner.take() {
            Some(decoder) => {
                let img: Vec<Rgb<u8>> = decoder.read_image_ldr()?;
                Ok(bytemuck::allocation::cast_vec(img))
            }
            None => Err(ImageError::Parameter(ParameterError::from_kind(
                ParameterErrorKind::NoMoreData,
            ))),
        }
    }

..or it could if image's Rgb struct implemented bytemuck::Pod. Right now it doesn't, which causes the above to fail to compile. I understand this would require an unsafe impl since a #[derive] won't work on a generic struct.

Once the Pod implementation is sorted, the HDR decoding can be specialized to return the underlying Vec where possible similar to how it's done for JPEG in #1879

Shnatsel avatar Mar 12 '23 04:03 Shnatsel

There's a lot of low hanging fruit if anyone wants to optimize the HDR decoder.

We should probably also deprecate the weird conversion to 8-bit LDR that it does and have it just return Rgb32F data (given that the whole point of the format is to store images that don't fit into normal 8 bit-per-pixel formats!)

fintelia avatar Mar 12 '23 04:03 fintelia

Note: There was an attempt at #1599 to do this, but there were technical problems (the tests are not correctly triggering a panic).

It seems there are still some unnecessary memcpys going on though.

johannesvollmer avatar May 17 '23 04:05 johannesvollmer