Conversation
alalek
left a comment
There was a problem hiding this comment.
Thank you for contribution!
Update the OpenEXR library
Please add information about used version and source code location.
+122,989lines
To be discussed.
3rdparty/openexr/CMakeLists.txt
Outdated
|
|
||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/IlmBaseConfig.h.cmakein" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/IlmBaseConfig.h" @ONLY) | ||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/IlmThreadConfig.h.cmakein" |
There was a problem hiding this comment.
See above
set(ILMBASE_VERSION_MAJOR "2")
set(ILMBASE_VERSION_MINOR "3")
set(ILMBASE_VERSION_PATCH "0")
These versions must be updated.
There was a problem hiding this comment.
Updated to 3.1.0.
For the record: I cloned the updated version from here. Had to do minor changes to avoid compiler warnings and such.
There was a problem hiding this comment.
Had to do minor changes to avoid compiler warnings and such.
We usually don't modify 3rdparty source code.
We prefer to suppress warnings in CMakeLists.txt, see ocv_warnings_disable() calls.
We keep only "must have" patches which should be migrated during upgrade.
Right upgrade flow is the following:
- extract "patch" for modified code of legacy version
- upgrade source code to new version
- re-apply and adapt extracted patch.
3.1.0
Current latest release is v3.1.3: https://github.com/AcademySoftwareFoundation/openexr/releases
There are several bugfixed landed. Probably including some fixed CVEs from here: #21326
There was a problem hiding this comment.
Ok. I took the version from master, and it's from Dec 2021, whereas v3.1.3 is from Oct 2021. I guess they just don't update the patch version number in master.
But I can try to revert to v3.1.3, if that's what you prefer?
There was a problem hiding this comment.
There are too much changes against master (100+ files): AcademySoftwareFoundation/openexr@v3.1.3...master
if we declare 3.1.3 then we should use 3.1.3 code.
Please fetch code from v3.1.3 tag.
There was a problem hiding this comment.
We usually don't modify 3rdparty source code.
I think the most relevant change I had to do was this. It seems to me that the OpenEXR code assumes C++17 here. No problem in my project, but some of the OpenCV CI builds were complaining about this.
There was a problem hiding this comment.
Ok, this simple change is fine, especially as dedicated commit (OpenCV 4.x should work with C++11 compiler)
However, no idea about C++17:
There was a problem hiding this comment.
Actually that one (abs/fabs) wasn't needed after all.
In the end of the day, I was able to avoid all other changes to Imath and OpenEXR code, except for these two, which I have left as separate commits:
We are talking about including the generated lookup tables: I guess these could also be generated as part of the build. |
931557f to
f28e5de
Compare
Solution: specify the namespace explicitly
…se compression method in ImfHeader Solution: tweak attribute names
5c63189 to
aa276ec
Compare
|
As far as I'm concerned, I'm now done with this PR. There's still this patch size warning, but I'm not sure if there is something I should (or can) do about it? |
|
@alalek is there any chance we could talk about having this PR merged? |
|
If yes, then I guess it'd make sense to upgrade to v3.1.5 first. |
|
Hi! Sorry for delayed response. OpenEXR becomes a large third party library. Consider building OpenEXR separately and use it in OpenCV like any other generic external library. OpenEXR 3.0+ is supported through |
|
Ok, fair enough. Thank you for the response. |
|
@reunanen Sure, please extract |
|
Closing in favor of #22790. |
Uh oh!
There was an error while loading. Please reload this page.