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

Exr resolution fix #119

Merged
merged 2 commits into from
Jan 20, 2023
Merged

Conversation

ThomasWilshaw
Copy link
Contributor

@ThomasWilshaw ThomasWilshaw commented Jan 18, 2023

The EXR unit tests showed up an error whereby the width and height had been swapped around. This PR also makes it so we copy the number of channels when reading an exr file, which might not be appropriate. As an aside I wonder if we should add support for passing through arbitary channel number exrs?

Fixes #121

@ThomasWilshaw ThomasWilshaw marked this pull request as ready for review January 18, 2023 18:06
@michaeldsmith
Copy link
Collaborator

thanks for this - can you please also add an issue so the error is easy to track ?

I also have some additional questions:

If the width/height were swapped on input, does that mean they were also swapped on output? I don't think the EXR outputs were wrong before this fix, right?

Is it easy to describe the symptom of the problem that you fixed? In other words, how did you realize there was an issue?

@ThomasWilshaw
Copy link
Contributor Author

ThomasWilshaw commented Jan 19, 2023

Sorry I will do.

The width/height are only swapped on output, so I think outputs must have been worng for a while. Looking at the blame the 32 bit support has been wrong for 9 years at least.

@ThomasWilshaw
Copy link
Contributor Author

To clarify some details I've only just realised, exr32 support was correct unless scale != 0 or 1 but exr16 is always broken as it uses a slightly different implementation.

@ThomasWilshaw
Copy link
Contributor Author

I'm possibly missing something here but could we not simplify the implementation here by combining the exr_write16 function and exr_write32 function together and pass it either Imf::HALF or Imf::FLOAT? It would save having two virtually identical functions and reduce the likelihood of errors like this?

@michaeldsmith
Copy link
Collaborator

michaeldsmith commented Jan 20, 2023

OK I think I know what happened, to fix the valgrind bug #109 I rewrote the exr_write16() modeled after the exr_write32() code as suggested by OpenEXR maintainers here AcademySoftwareFoundation/openexr#1322

So this is why both exr_write16() and exr_write32() are affected now, but previously (before Jan 2023) it was only in exr_write32(), and I think people use 16-bit EXRs much more than 32-bit EXRs which is why it went unnoticed for so long.

I agree that a refactored single exr_write function that handles both 32 and 16 bit output would be desirable and more maintainable.

Let me double check this and then merge it, and then we can separately do the refactoring in another PR

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.

EXR output resolutions are incorrect
2 participants