search

ampas / CTL

The Color Transformation Language (CTL)
Other
220 stars 78 forks source link

Exr resolution fix #119

Closed ThomasWilshaw closed 1 year ago

ThomasWilshaw commented 1 year ago

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

michaeldsmith commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/AcademySoftwareFoundation/openexr/issues/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