image
image copied to clipboard
Unnecessary `memset()`+`memcpy()` in HDR decoding
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
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!)
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.