Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.hdr images decoded as LDR. #1936

Closed
pema99 opened this issue May 29, 2023 · 3 comments
Closed

.hdr images decoded as LDR. #1936

pema99 opened this issue May 29, 2023 · 3 comments

Comments

@pema99
Copy link
Contributor

pema99 commented May 29, 2023

In my renderer, I was using the following code to load radiance HDR image (.hdr extension), but noticed that all values were getting clamped to [0; 1] range:

pub fn load_dynamic_image(path: &str) -> Option<DynamicImage> {
    image::io::Reader::open(path).ok()?.decode().ok()
}

I was able to fix it by adding a special case:

pub fn load_dynamic_image(path: &str) -> Option<DynamicImage> {
    // Image crate does not by default decode .hdr images as HDR for some reason
    if path.ends_with(".hdr") {
        let hdr_decoder = image::codecs::hdr::HdrDecoder::new(std::io::BufReader::new(
            std::fs::File::open(&path).unwrap(),
        )).ok()?;
        let width = hdr_decoder.metadata().width;
        let height = hdr_decoder.metadata().height;
        let buffer = hdr_decoder.read_image_hdr().ok()?;
        return Some(DynamicImage::ImageRgb32F(image::ImageBuffer::from_vec(
            width,
            height,
            buffer.into_iter().flat_map(|c| vec![c[0], c[1], c[2]]).collect(),
        )?));
    }

    image::io::Reader::open(path).ok()?.decode().ok()
}

I noticed it seems to be caused by this line HdrAdapter::read_image_data:

fn read_image_data(&mut self, buf: &mut [u8]) -> ImageResult<()> {
        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()?; // <-- clamp to LDR
                ...

I guess the simple fix would just be to swap the call to read_image_hdr instead. I'm mainly opening this bug report to verify if this is intentional or indeed a bug. It seems like too obvious of a mistake to me.

Expected

.hdr image is decoded as HDR values, nothing is clamped.

Actual behaviour

.hdr image is decoded such that all pixels above 1.0 are clamped to 1.0.

Reproduction steps

Run the code from the first snippet, passing a path to an .hdr image. Read the pixel values and observe all will be clamped. Attaching the image file I used to test.
https://dl.polyhaven.org/file/ph-assets/HDRIs/hdr/2k/clarens_midday_2k.hdr

@fintelia
Copy link
Contributor

This is a bug, but one that existing code may be unintentionally relying on. I think the right approach is probably to change when we release 0.25.0. Honestly, I'm not really sure why we support LDR decoding of .hdr images at all. Might make sense just to deprecate it and leave only the HDR decoding logic.

@pema99
Copy link
Contributor Author

pema99 commented May 30, 2023

I submitted a patch, will 0.25 be the next release?

@fintelia
Copy link
Contributor

fintelia commented Jun 7, 2023

We'll see. There may be a 0.24.7 release first

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 a pull request may close this issue.

2 participants