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

Improve OpenEXR support #21324

Closed
wants to merge 4 commits into from
Closed

Conversation

reunanen
Copy link
Contributor

@reunanen reunanen commented Dec 23, 2021

  • Update the OpenEXR library
    • This is just manual copy-paste from the upstream OpenEXR library code
  • OpenEXR encoder: add capability to set the DWA compression level from outside
    • Being able to do this was the reason I started updating the OpenEXR library in the first place

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contribution!

Update the OpenEXR library

Please add information about used version and source code location.

+122,989 lines

To be discussed.

modules/imgproc/include/opencv2/imgproc.hpp Outdated Show resolved Hide resolved
@@ -76,6 +76,8 @@ set(OPENEXR_PACKAGE_STRING "\"OpenEXR ${OPENEXR_VERSION}\"" )

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

set(ILMBASE_VERSION_MAJOR "2")
set(ILMBASE_VERSION_MINOR "3")
set(ILMBASE_VERSION_PATCH "0")

These versions must be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@alalek alalek Dec 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • b30a7d7 : compiler error about ambiguous symbol (namespace issue)
    • fixed by specifying the namespace explicitly
  • 6ff6471 : warning about deprecated attribute
    • fixed by updating the attribute names

@reunanen
Copy link
Contributor Author

+122,989 lines

To be discussed.

We are talking about including the generated lookup tables:

I guess these could also be generated as part of the build.

@reunanen reunanen force-pushed the improve-OpenEXR-support branch 2 times, most recently from 931557f to f28e5de Compare December 23, 2021 17:14
@reunanen
Copy link
Contributor Author

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 alalek mentioned this pull request Dec 24, 2021
4 tasks
@reunanen
Copy link
Contributor Author

reunanen commented Nov 7, 2022

@alalek is there any chance we could talk about having this PR merged?

@reunanen
Copy link
Contributor Author

reunanen commented Nov 7, 2022

If yes, then I guess it'd make sense to upgrade to v3.1.5 first.

@alalek
Copy link
Member

alalek commented Nov 7, 2022

Hi! Sorry for delayed response.

OpenEXR becomes a large third party library.
We are going to drop it from 3rdparty source directory (still no roadmap yet).

Consider building OpenEXR separately and use it in OpenCV like any other generic external library. OpenEXR 3.0+ is supported through find_package(OpenEXR), older version through include/libs directories.

@reunanen
Copy link
Contributor Author

reunanen commented Nov 7, 2022

Ok, fair enough. Thank you for the response.

@reunanen
Copy link
Contributor Author

@alalek This commit would still be needed, though, or otherwise we can't pass the DWA compression level:
aa276ec

If I created a separate PR for just that, would you consider merging it?

@alalek
Copy link
Member

alalek commented Nov 10, 2022

@reunanen Sure, please extract modules/imgcodecs changes to separate PR.

@reunanen
Copy link
Contributor Author

Closing in favor of #22790.

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.

None yet

2 participants