-
Notifications
You must be signed in to change notification settings - Fork 1
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
Perform degamma
and gamma
conversions on user request
#32
base: main
Are you sure you want to change the base?
Conversation
The pointer refactor and additional The alternatives are either preprocessing the input (but this doesn't seem to be too fast either... 😕) or simply documenting that the input and output will be linearized and let the user deal with this (if they need to). |
let params = Parameters { | ||
// Input stb Image is gamma-corrected (i.e. expects to be passed through a CRT with exponent 2.2) | ||
degamma: true, | ||
// Output image is PNG which must be stored with a gamma of 1/2.2 | ||
gamma: true, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both of these can be merged into one parameter.
06f52a4
to
9b795a5
Compare
9b795a5
to
b9341a8
Compare
d79e664
to
35850c1
Compare
Before: Downsample `square_test.png` using ispc_downsampler time: [43.438 ms 43.468 ms 43.500 ms] After: Downsample `square_test.png` using ispc_downsampler time: [29.891 ms 29.922 ms 29.953 ms] change: [-31.246% -31.162% -31.077%] (p = 0.00 < 0.05)
This crate can't assume that the input and output is linear, nor did it correct for that in the `test` example where the output from `stb_image` is clearly nonlinear (it doesn't state this in the "docs", but is visible from not linearizing JPG and PNG inputs and applying a gamma of 1/2.2 when converting HDR to LDR). While we could request users to pre- correct for this and return linear output to them, it is more efficient to do it within the downsampling algorithm that already runs over all the pixels, and (more importantly!) requiring these parameters in the input forces the caller to think about it.
PR #48 seems to have added |
In that sense I do think that this is more so a bug now in that we expose the |
Fixes #25
This crate can't assume that the input and output is linear, nor did it correct for that in the
test
example where the output fromstb_image
is clearly nonlinear (it doesn't state this in the "docs", but is visible from not linearizing JPG and PNG inputs and applying a gamma of 1/2.2 when converting HDR to LDR). While we could request users to pre- correct for this and return linear output to them, it is more efficient to do it within the downsampling algorithm that already runs over all the pixels, and (more importantly!) requiring these parameters in the input forces the caller to think about it.Unfortunately this has a massive performance regression of 150% (±40ms to ±107ms):
Will need to dissect if this is caused by the pointer refactor or purely the extra ALU overhead.