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

Support for OpenEXR to build additional apps #1637

Merged

Conversation

remia
Copy link
Collaborator

@remia remia commented May 3, 2022

This PR is a follow up of a work started by @michdolan a while back. It mainly deals with the OpenImageIO / OpenColorIO circular dependency issue for apps (ocioconvert, ociolutimage and ociodisplay) by allowing the use of OpenEXR to build those when OIIO is not found.

Here is a quick overview of the content:

  • Replace oiiohelpers by imageioapphelpers, this library expose the class ImageIO which uses the private implementation idiom to support either OpenEXR or OIIO backend depending on build config,
  • Remove support for Half (OpenEXR 2) from the library,
  • Uses and requires OpenEXR and Imath latest versions (3.1.5),
  • FindOpenEXR.cmake script to allow in source build, currently doesn't support looking for the library by other mean than the find_package(... CONFIG) mode as the minimal requested version install a config script,
  • Fix a const issue in CPUProcessor::apply

There is still some more testing to be done but I prefer to open this early in case there are any decision made above that needs more discussion or revision.

Copy link
Collaborator

@doug-walker doug-walker left a comment

Choose a reason for hiding this comment

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

This adds some welcome flexibility, thanks Remi!

share/cmake/modules/FindOpenEXR.cmake Outdated Show resolved Hide resolved
docs/quick_start/for_devs.rst Show resolved Hide resolved
docs/guides/contributing/repository_structure.rst Outdated Show resolved Hide resolved
src/apps/ocioconvert/main.cpp Outdated Show resolved Hide resolved
@remia remia force-pushed the feature/ociolutimage_no_oiio branch from 4e10997 to 69a607c Compare May 20, 2022 19:13
@remia
Copy link
Collaborator Author

remia commented May 23, 2022

Here are some notes on the current behaviour of the ImageIO class, feedback welcome:

  • Process and write back only the first R,G,B,[A] channels,
  • OpenEXR display/data window are not altered during pixel processing,
  • There is a slight difference in behaviour between OIIO and EXR backend. OIIO will preserve the input bitdepth when writing the file back and EXR will use the processing bitdepth (eg. FLOAT for ocioconvert --gpu),
  • ocioconvert --croptofull and --ch only supported for OIIO backend. My tests on main seem show that the former is broken on some images and the latter have no effect. We may consider removing this functionality and pointing users to oiiotool.

The goal was to keep everything simple and not write a comprehensive image conversion tool.

@doug-walker
Copy link
Collaborator

@remia , we should ask at the TSC, but I agree it is reasonable to remove the croptofull/ch functionality and point users to oiiotool for that.

Regarding the output bit-depth, I think your ImageIO class has the necessary functionality, to request a bit-depth. But we will need to integrate this PR with PR #1648, since in general the output bit-depth may need to be different from the input bit-depth. (Minor point, but I don't think it is strictly accurate that OIIO will preserve the input depth since the user could have an EXR coming in and a JPG going out. But I don't think it matters for the purposes of this PR.)

@remia
Copy link
Collaborator Author

remia commented May 26, 2022

Thanks @doug-walker, I'll add an item to the agenda and look into adding a user parameter to control the output bitdepth on write.

@remia
Copy link
Collaborator Author

remia commented Jun 1, 2022

As discussed during last TSC meeting, tagging @lgritz and @Shootfast as you were both active during the discussion around this issue, any feedback from you (and anyone else obviously) would be greatly appreciated! Thanks

share/cmake/modules/FindExtPackages.cmake Outdated Show resolved Hide resolved
share/cmake/modules/FindExtPackages.cmake Outdated Show resolved Hide resolved
share/cmake/modules/FindExtPackages.cmake Outdated Show resolved Hide resolved
@remia remia force-pushed the feature/ociolutimage_no_oiio branch from 3de3a33 to 3643e34 Compare June 7, 2022 19:47
Signed-off-by: Rémi Achard <[email protected]>
@remia remia force-pushed the feature/ociolutimage_no_oiio branch from 3643e34 to eef0a9a Compare June 8, 2022 18:14
@remia remia marked this pull request as draft June 8, 2022 19:21
@remia remia force-pushed the feature/ociolutimage_no_oiio branch from 626f9a6 to e5f5516 Compare June 19, 2022 17:25
@remia
Copy link
Collaborator Author

remia commented Jun 19, 2022

I think the PR is now ready again for reviews, apologies for the force push 2 weeks ago, I realised later it will probably have broke the reviewers progress on GH.

In terms of changes, the output bitdepth handling on the CPU path of ocioconvert and the build breaks with OpenImageIO have been hopefully fixed. From now on, OCIO will only accept an OIIO version that has been compiled with OpenEXR / Imath 3, I also had to update the minimal requested version to 2.2.14 to be able to find this information. If this is accepted, we will also need @michdolan to update the required checks list on GH due to a naming change (include whether OIIO is used for apps or not).

The analysis builds are currently passing but not fully used due to some missing tags on Imath release branch, limited to 3.1.3 whereas OpenEXR goes to 3.1.5 which create conflicts. I'm trying to get an Imath maintainer to fix that at the moment.

@remia remia marked this pull request as ready for review June 19, 2022 19:25
@zachlewis
Copy link
Collaborator

I just wanted to say, I had an opportunity to refactor our facility "OpenColorIO_tools" package to test this PR, and it feels so refreshing to be able to statically build all the command line tools without the OIIO business. And the OpenEXR build goes as smoothly as the rest of our fancy buildy stuff.

Thanks again Remi -- this makes maintenance so much easier.

Copy link
Collaborator

@michdolan michdolan left a comment

Choose a reason for hiding this comment

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

Awesome work @remia !

@michdolan
Copy link
Collaborator

I can update the required checks after this merges.

@remia
Copy link
Collaborator Author

remia commented Sep 8, 2022

Thanks for the review @michdolan! Could you merge the PR when you have time and update the required checks name at the same time?

FYI there seem to be errors in the analysis builds, one on macOS which seem to track down to using LLVM14 with the current OSL (AcademySoftwareFoundation/OpenShadingLanguage#1492) and the other being a doc building error on Linux which I don't really understand at the moment. Can be looked at at a later time anyway.

@michdolan michdolan merged commit 8e04da7 into AcademySoftwareFoundation:main Sep 19, 2022
@remia remia deleted the feature/ociolutimage_no_oiio branch September 22, 2022 20:11
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.

6 participants