-
Notifications
You must be signed in to change notification settings - Fork 649
build: several OpenEXR and OpenJPH build related fixes #4875
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
Conversation
|
I know this one is a bit of a jumble, but I would appreciate somebody taking a look as a priority, because this fixes several potential problems people might run into if they try to build OIIO with the newly released OpenEXR 3.4. |
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.
Pull Request Overview
This PR updates OpenEXR and OpenJPH build configurations with several fixes to support OpenEXR 3.4.0 and resolve compilation warnings. The changes include cleanup of obsolete OpenEXR guards, fixes for H2J2K compression enum usage, and improvements to OpenJPH API integration.
- Bump CI tests to OpenEXR 3.4.0 and clean up obsolete float vector guards
- Fix H2J2K compression enum mixing warnings and OpenJPH type mismatches
- Update test scripts and reference output for htj2k tests with correct file paths
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testsuite/oiiotool-readerror/ref/out.err-alt3-exr33.txt | New reference output file for OpenEXR 3.3 error handling tests |
| testsuite/htj2k/run.py | Fix file paths for DPX test images in htj2k test suite |
| testsuite/htj2k/ref/out.txt | Update reference output with corrected relative paths |
| src/openexr.imageio/exrinput_c.cpp | Remove unnecessary blank line in float vector attribute handling |
| src/openexr.imageio/exrinput.cpp | Remove obsolete OpenEXR float vector guards and fix H2J2K enum usage |
| src/jpeg2000.imageio/jpeg2000output.cpp | Add warning suppression and fix type mismatches in OpenJPH API calls |
| src/jpeg2000.imageio/jpeg2000input.cpp | Add warning suppression, fix type conversions, and improve error handling |
| src/cmake/build_OpenEXR.cmake | Update default OpenEXR build version from 3.2.4 to 3.3.5 |
| src/build-scripts/build_openexr.bash | Update OpenEXR version in build script to 3.3.5 |
| INSTALL.md | Add OpenJPH dependency documentation |
| .github/workflows/ci.yml | Update CI to use OpenEXR 3.4.0 and latest dependency versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ojph::set_error_stream(nullptr); | ||
|
|
||
| try { | ||
| Oiio_Reader_Error_handler error_handler(default_error); |
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.
@richardssam I'm curious about your input here, since you wrote these lines.
The calls to ojph::get_error() and ojph::configure_error()...
If I understand correctly, this is getting the old error handler, installing a new one for us (which underneath will at some point call the old error handler as well), reading the header, and then switching back to the original error handler.
These are not in reference to the particular codestream. So they are global to the library?
Is this potentially not thread-safe, if there are two htj2k files being opened and read from simultaneously?
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.
Perhaps I need a better comment here rather than in the class. What I'm doing here when opening a J2K file is to first try to read the file with OJPH, and if that fails (which would happen if the stream ISNT HTJ2K) then fall back to the regular OpenJPEG file reader. The problem is the default error handler of OJPH will always print an error, which I think is annoying and misleading (since I will be handling the error by going to OpenJPEG).
I'm trying to only catch that error, and if its any other error fall back to the regular reader. This feels like I should be good for the situations where there are two HTJ2K files, since neither of them would even hit this error (unless there is an actual error).
And yes, the error handler is global to the library. I'm fuzzy on the thread-safety, it feels ok to me, but I could be missing something.
LMK if you need me to change something (like the comment).
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 I did understand what you were going for here. You grab the old error handler, you install a new error handler that will bubble up the error properly (and also call the old one for other errors -- which I had to modify slightly to make it not write to stderr!), then after trying to open the file, you restore the original error handler.
My specific question here is: since these are global operations, what happens if an app is using OIIO to open j2k (or h2j2k) files in two separate threads simultaneously, and they are strangely interleaving the saving and restoring of error handlers (or if, even, any of those individual calls are not reentrant?).
It feels to me like maybe there needs to be a mutex around this part, to ensure that two threads aren't messing with the error handler simultaneously or in an interleaved way?
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'm not implying that you need to do this. It's like a 3 line fix and I'm happy to do it (after this PR is merged). But I did want a sanity check on the general understanding.
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.
Adding a mutex makes sense, just to be careful, but I dont understand why you are adding "ojph::set_error_stream(nullptr);" Surely if there is a genuine error, we want it going somewhere? nullptr implies no error output.
Also I perhaps should be adding a "ojph::configure_error(default_error) after reading the header, just to ensure we are back to normal.
LMK if you want me to make a patch (on your patch) for the mutex.
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.
Here's an example of how it's not thread safe:
- Thread 1 grabs the old error handler
- Thread 1 installs our error handler
- Thread 2 tries to grab the old error handler (but it's actually our new one!)
- Thread 2 installs our error handler (redundantly)
- Thread 1 reads from the file
- Thread 1 restores the original error handler
- Thread 2 reads from the file... oops, NOT using the custom error handler!
- Thread 2 tries to restore the original error handler... oops, it's ours it installs!
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.
We sure don't want error messages echoing to the console!
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 that their default handler does a fprintf(stderr...) (which we want to suppress, and now I do), and then does a throw.
So I think there is a second error -- your original code seems to assume that only this one specific error (not H2J2k file) will throw. But all errors will throw, if I understand their docs correctly.
|
Yes, I just wanted to suppress the error print in the case where it’s the HTJ2K error, but for other cases I wanted to at least let it say why its erroring out, why wouldn’t you want to know why its unable to read the file?
If we do want to suppress the errors in all cases then perhaps we don’t need the oiio_reader_error_handler at all.
Sam.
From: Larry Gritz ***@***.***>
Date: Sunday, September 7, 2025 at 7:44 PM
To: AcademySoftwareFoundation/OpenImageIO ***@***.***>
Cc: Richards, Sam ***@***.***>, Comment ***@***.***>
Subject: Re: [AcademySoftwareFoundation/OpenImageIO] build: several OpenEXR and OpenJPH build related fixes (PR #4875)
This Message is From an External Sender
Caution: Do not click links or open attachments unless you recognize the sender and know the content is safe.
@lgritz commented on this pull request.
________________________________
In src/jpeg2000.imageio/jpeg2000input.cpp<#4875 (comment)>:
@@ -480,6 +487,9 @@ Jpeg2000Input::open(const std::string& name, ImageSpec& p_spec)
jph_infile* jphinfile = new jph_infile(ioproxy());
ojph_reader = true;
ojph::message_error* default_error = ojph::get_error();
+ // Disable the default OpenJPH error stream to prevent unwanted error output.
+ // Errors will be handled by the custom error handler (Oiio_Reader_Error_handler) configured below.
+ ojph::set_error_stream(nullptr);
try {
Oiio_Reader_Error_handler error_handler(default_error);
I think that their default handler does a fprintf(stderr...) (which we want to suppress, and now I do), and then does a throw.
So I think there is a second error -- your original code seems to assume that only this one specific error (not H2J2k file) will throw. But all errors will throw, if I understand their docs correctly.
—
Reply to this email directly, view it on GitHub<#4875 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACIXQFSPWX6XJE7ZIXDLUNL3RR4JZAVCNFSM6AAAAACFZ7LE72VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCOJUGU2TGNRQHE>.
You are receiving this because you commented.Message ID: ***@***.***>
|
Yes, I want to know why it can't read the file. But when I say that, I really mean "I want the app using OIIO's APIs to know." And that means it needs to be retrievable from |
|
Let's get this PR approved and merged first, it's necessary to build correctly against OpenEXR 3.4. Then I have an idea for how to straighten out the error business. |
|
Sounds good, happy to make a stab at it.
Sam.
…On Sun, 7 Sept 2025 at 22:41, Larry Gritz ***@***.***> wrote:
*lgritz* left a comment (AcademySoftwareFoundation/OpenImageIO#4875)
<#4875 (comment)>
Let's get this PR approved and merged first, it's necessary to build
correctly against OpenEXR 3.4. Then I have an idea for how to straighten
out the error business.
—
Reply to this email directly, view it on GitHub
<#4875 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2TR2CPAQEPCDFCYM5ELZL3RSRALAVCNFSM6AAAAACFZ7LE72VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTENRUGA4DMNZTG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
This is necessary to avoid problems if somebody tries to build against OpenEXR 3.4. I will merge this tomorrow if nobody has any objections by then. |
Original changes and goal: * Bump our "latest releases" CI tests to OpenEXR 3.4.0 * cleanup: dead guard removal for openexr -- our minimum OpenEXR release supports float vectors, so we don't need any `#if` guards for it anymore. * Fix warnings related to mixing Core and C++ library constants for H2J2K compression enums within a switch statement (only noticed on ARM Linux with Clang 18). But it seems that OpenEXR 3.4's auto-building of OpenJPH (to support exr's new h2j2k compression methods) revealed that our CI wasn't building and testing against OpenJPG as well as we thought. So: * Clean up some openjph API use and type mismatches * Suppress warnings on clang and recent gcc versions had with the OpenJPH headers and our use of them. * Fix our test scripts and reference output for the htj2k tests; I think maybe they weren't running at all before, and nad some incorrect file paths. Finally, * When no OpenEXR is found, update the auto-build version to 3.3.5. I wish I could go all the way to 3.4.0, but the OpenEXR 3.4.0 currently auto-builds OpenJPH in a way that (I think incorrectly) installs the OpenJPH components in ways that can overwrite or break existing installations. Signed-off-by: Larry Gritz <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Larry Gritz <[email protected]>
lowercase "openjph" used by the new OpenJPH exported cmake configs. But those are only generated for OpenJPH >= 0.21.2, so make that our new minimum. (This only affects OIIO 3.1 and higher, so it's safe to do this.) Signed-off-by: Larry Gritz <[email protected]>
|
Amendment: Remove FindOpenJPH.cmake, the nomenclature clashes with the This is necessary because without it, our main and 3.1 builds break |
|
More and more things are breaking without this. Merging. |
…reFoundation#4875) Original changes and goal: * Bump our "latest releases" CI tests to OpenEXR 3.4.0 * cleanup: dead guard removal for openexr -- our minimum OpenEXR release supports float vectors, so we don't need any `#if` guards for it anymore. * Fix warnings related to mixing Core and C++ library constants for H2J2K compression enums within a switch statement (only noticed on ARM Linux with Clang 18). But it seems that OpenEXR 3.4's auto-building of OpenJPH (to support exr's new h2j2k compression methods) revealed that our CI wasn't building and testing against OpenJPG as well as we thought. So: * Clean up some openjph API use and type mismatches * Suppress warnings on clang and recent gcc versions had with the OpenJPH headers and our use of them. * Fix our test scripts and reference output for the htj2k tests; I think maybe they weren't running at all before, and nad some incorrect file paths. Finally, * When no OpenEXR is found, update the auto-build version to 3.3.5. I wish I could go all the way to 3.4.0, but the OpenEXR 3.4.0 currently auto-builds OpenJPH in a way that (I think incorrectly) installs the OpenJPH components in ways that can overwrite or break existing installations. * Remove FindOpenJPH.cmake, the nomenclature clashes with the lowercase "openjph" used by the new OpenJPH exported cmake configs. * But those are only generated for OpenJPH >= 0.21.2, so make that our new minimum. (This only affects OIIO 3.1 and higher, so it's safe to do this.) --------- Signed-off-by: Larry Gritz <[email protected]> Co-authored-by: Copilot <[email protected]>
…reFoundation#4875) Original changes and goal: * Bump our "latest releases" CI tests to OpenEXR 3.4.0 * cleanup: dead guard removal for openexr -- our minimum OpenEXR release supports float vectors, so we don't need any `#if` guards for it anymore. * Fix warnings related to mixing Core and C++ library constants for H2J2K compression enums within a switch statement (only noticed on ARM Linux with Clang 18). But it seems that OpenEXR 3.4's auto-building of OpenJPH (to support exr's new h2j2k compression methods) revealed that our CI wasn't building and testing against OpenJPG as well as we thought. So: * Clean up some openjph API use and type mismatches * Suppress warnings on clang and recent gcc versions had with the OpenJPH headers and our use of them. * Fix our test scripts and reference output for the htj2k tests; I think maybe they weren't running at all before, and nad some incorrect file paths. Finally, * When no OpenEXR is found, update the auto-build version to 3.3.5. I wish I could go all the way to 3.4.0, but the OpenEXR 3.4.0 currently auto-builds OpenJPH in a way that (I think incorrectly) installs the OpenJPH components in ways that can overwrite or break existing installations. * Remove FindOpenJPH.cmake, the nomenclature clashes with the lowercase "openjph" used by the new OpenJPH exported cmake configs. * But those are only generated for OpenJPH >= 0.21.2, so make that our new minimum. (This only affects OIIO 3.1 and higher, so it's safe to do this.) --------- Signed-off-by: Larry Gritz <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Zach Lewis <[email protected]>
…reFoundation#4875) Original changes and goal: * Bump our "latest releases" CI tests to OpenEXR 3.4.0 * cleanup: dead guard removal for openexr -- our minimum OpenEXR release supports float vectors, so we don't need any `#if` guards for it anymore. * Fix warnings related to mixing Core and C++ library constants for H2J2K compression enums within a switch statement (only noticed on ARM Linux with Clang 18). But it seems that OpenEXR 3.4's auto-building of OpenJPH (to support exr's new h2j2k compression methods) revealed that our CI wasn't building and testing against OpenJPG as well as we thought. So: * Clean up some openjph API use and type mismatches * Suppress warnings on clang and recent gcc versions had with the OpenJPH headers and our use of them. * Fix our test scripts and reference output for the htj2k tests; I think maybe they weren't running at all before, and nad some incorrect file paths. Finally, * When no OpenEXR is found, update the auto-build version to 3.3.5. I wish I could go all the way to 3.4.0, but the OpenEXR 3.4.0 currently auto-builds OpenJPH in a way that (I think incorrectly) installs the OpenJPH components in ways that can overwrite or break existing installations. * Remove FindOpenJPH.cmake, the nomenclature clashes with the lowercase "openjph" used by the new OpenJPH exported cmake configs. * But those are only generated for OpenJPH >= 0.21.2, so make that our new minimum. (This only affects OIIO 3.1 and higher, so it's safe to do this.) --------- Signed-off-by: Larry Gritz <[email protected]> Co-authored-by: Copilot <[email protected]>
PR #4875, switched to using openjph's exported cmake config instead of our own FindOpenJPH.cmake, and in the process also renamed OpenJPH -> openjph to follow their convention. But I botched it, still using the old OPENJPH_LIBRARIES (etc.) variables instead of fully switching to the correct targets exported from their config. This PR minimally fixes that error, re-enabling use of openjph. Fixes #4893 Signed-off-by: Larry Gritz <[email protected]>
PR AcademySoftwareFoundation#4875, switched to using openjph's exported cmake config instead of our own FindOpenJPH.cmake, and in the process also renamed OpenJPH -> openjph to follow their convention. But I botched it, still using the old OPENJPH_LIBRARIES (etc.) variables instead of fully switching to the correct targets exported from their config. This PR minimally fixes that error, re-enabling use of openjph. Fixes AcademySoftwareFoundation#4893 Signed-off-by: Larry Gritz <[email protected]>
Original changes and goal:
#ifguards for it anymore.But it seems that OpenEXR 3.4's auto-building of OpenJPH (to support exr's new h2j2k compression methods) revealed that our CI wasn't building and testing against OpenJPG as well as we thought. So:
Finally,
When no OpenEXR is found, update the auto-build version to 3.3.5. I wish I could go all the way to 3.4.0, but the OpenEXR 3.4.0 currently auto-builds OpenJPH in a way that (I think incorrectly) installs the OpenJPH components in ways that can overwrite or break existing installations.
Remove FindOpenJPH.cmake, the nomenclature clashes with the lowercase "openjph" used by the new OpenJPH exported cmake configs.
But those are only generated for OpenJPH >= 0.21.2, so make that our new minimum. (This only affects OIIO 3.1 and higher, so it's safe to do this.)