OpenColorIO icon indicating copy to clipboard operation
OpenColorIO copied to clipboard

Support for OpenEXR to build additional apps

Open remia opened this issue 3 years ago • 6 comments

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.

remia avatar May 03 '22 18:05 remia

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.

remia avatar May 23 '22 17:05 remia

@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.)

doug-walker avatar May 25 '22 21:05 doug-walker

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 avatar May 26 '22 08:05 remia

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

remia avatar Jun 01 '22 20:06 remia

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 avatar Jun 19 '22 19:06 remia

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.

zachlewis avatar Jun 29 '22 15:06 zachlewis

I can update the required checks after this merges.

michdolan avatar Sep 06 '22 13:09 michdolan

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 (https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/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.

remia avatar Sep 08 '22 11:09 remia