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

Fix .hdr decoding #1937

Closed
wants to merge 1 commit into from
Closed

Fix .hdr decoding #1937

wants to merge 1 commit into from

Conversation

pema99
Copy link
Contributor

@pema99 pema99 commented May 29, 2023

Fixes #1936. First time contributor, so unsure if this is the right way to fix, but in my testing it seemed to work and causes .exr and .hdr image files to have the same behavior when decoding.

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.

@fintelia fintelia mentioned this pull request Jun 10, 2023
19 tasks
Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a potentially breaking change and should wait for the next major release, but LGTM

@fintelia fintelia mentioned this pull request Sep 20, 2023
2 tasks
@fintelia fintelia changed the base branch from master to next-version-0.25 February 12, 2024 05:25
@johannesvollmer
Copy link
Contributor

johannesvollmer commented Feb 12, 2024

I think the test fails, because the reference image are u8 values that are gamma corrected, but f32 values from the hdr decoder are linear. Previously, the hdr decoder returned non-linear u8 values, but now it returns linear f32 values. This means the image returned by the hdr decoder is too dark, and therefore comparison with the reference png fails.

  • changed in this pr: hdr decoder now returns linear values instead of non-linear values
  • legacy: test reference png image has non-linear values (probably correct)
  • legacy: test suite doesn't compare f32 with u8 images (easily fixable)

In #1599, the solution was to make the hdr decoder return non-linear f32 values. However, in hindsight, it seems weird, as in my experience, one would always f32 values to be linear, as this generally seems to be the convention in image processing.

In addition, the tests fail because they can't compare the existing u8 png images with the f32 images from the hdr decoder. This means we must adjust the tests. Maybe the best would be to make the tests perform gamma conversion before comparing the images? Maybe DynamicImage::to_f32() should perform gamma conversion?!

Maybe we need to define within the image crate for f32 images whether they are always gamma or linear?

@fintelia
Copy link
Contributor

Merged via #2150

@fintelia fintelia closed this Feb 19, 2024
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 this pull request may close these issues.

.hdr images decoded as LDR.
3 participants