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

Subsampled chroma gives broken results starting with v3.3.0? #1949

Closed
wkjarosz opened this issue Dec 31, 2024 · 6 comments
Closed

Subsampled chroma gives broken results starting with v3.3.0? #1949

wkjarosz opened this issue Dec 31, 2024 · 6 comments

Comments

@wkjarosz
Copy link

I noticed that starting with v3.3.0 (and presumably due to its switch to OpenEXRCore), my code to read subsampled chroma images, that used to work properly in v3.2.4 (first screenshot), now produces incorrect results (second screenshot).

v3 2 4 v3 3 0

The relevant code where I use OpenEXR is here: https://github.com/wkjarosz/hdrview/blob/15b2cbf5d87108529a6da12df8f15e324b668736/src/imageio.cpp#L222-L253

But essentially, all I do is create my own float arrays that can hold the entire contents of a full-resolution channel. I then use Imf::Slice::Make to insert these arrays into an exr framebuffer object. When subsampling = 1, readPixels will fill up the entire array and everything works fine. When subsampling > 1, readPixels will only insert values into a subset of the array elements, after which point I resample to populate the missing pixels. Previously this worked, but now it produces different results.

Is this different packing of pixel values expected now and I just need to update my code, or should this behavior remain the same between v3.2.4 and v3.3.0? Perhaps I have just always been misusing Imf::Slice::Make and it worked on accident before?

Thanks

@lgritz
Copy link
Contributor

lgritz commented Dec 31, 2024

I admit that this is not a helpful or constructive comment at all, but I think it's regrettable mistake that OpenEXR ever had support for subsampled channels.

@kdt3rd
Copy link
Contributor

kdt3rd commented Dec 31, 2024

huh, no, however there is a new unpacking pipeline that tries to deposit unpacked memory with fewer copies than the old code did, however likely has an unexpected / buggy stride condition. I've tried to add tests for all the common cases, but it should not be required to change any of your code. will have a look. would it be possible to send me a copy of that exr file for testing?

@kdt3rd
Copy link
Contributor

kdt3rd commented Jan 1, 2025

@wkjarosz - given a 4:2:0 subsampled image i had lying around, I was able to reproduce a crash by compiling my own copy of hdrview, which I've fixed in the PR where I tagged this issue, and the image subsequently also looks correct. So either I have fixed the issue you ran into, or do indeed need a copy of that exr file to investigate further...

@wkjarosz
Copy link
Author

wkjarosz commented Jan 1, 2025

Thanks @kdt3rd I'll take a look. In the meantime, the image is just the Chromaticities/Rec709_YC.exr image in the OpenEXR test images suite.

@kdt3rd
Copy link
Contributor

kdt3rd commented Jan 1, 2025

haha, should have recognized it - can confirm, the change I've just merged above will fix this issue. Beyond that, I did submit a PR against hdrview as it crashes on exit for me :) Hopefully will see you at siggraph next year so we can catch up!

@wkjarosz
Copy link
Author

wkjarosz commented Jan 1, 2025

That was impressive quick work tracking down and fixing the bug in my code. Thanks for the PR.

I can confirm your merged commit here fixes things on my end too.

(I hope to make it to SIGGRAPH, and would be great to catch up!)

@wkjarosz wkjarosz closed this as completed Jan 1, 2025
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

No branches or pull requests

3 participants