feat(iv): OCIO

[antond-weta]

Description

This change adds OCIO functionality to iv.

To enable OCIO, first set the $OCIO environment variable to point with a path to the OCIO config. "ocio://default" should work as well.

If a valid config provided, a new menu gets added under "View/OCIO" with an on/off toggle and submenus to select the image color space and display/view and optional look.

If an image color space, display and view are provided as command line parameters ("image-color-space", "display", "view"), then the OCIO mode is switched on automatically.

If no image color space is provided as command line parameters, the top most color space of the config get pre-selected in the menus.

If no display or view is provided as command line parameters, the default values get pre-selected in the menus.

In OCIO mode the exposure and gamma adjustments are done by OCIO. The exposure is done in scene linear, gamma is still in display space. The exposure adjustments don't match between the OCIO and non-OCIO modes.

When OCIO fails to initialise the pipeline for any reason, an error message is shown in the status bar, and the non-OCIO path gets used.

TODO:

• All channel shuffling is done by the existing shader and happens before the OCIO step. We may want to discuss/change that?
• The initial image color space gets selected from the command line parameter, or the first color space in the config. There is no attempt being made to use the image metadata.
• The exposure/gamma values get baked into the shader, which gets recompiled on every change. We may want to use uniforms instead. - DONE

Tests

I've only tested on Macos so far.

Checklist:

• I have read the contribution guidelines.
• I have updated the documentation, if applicable.
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary).
• If I added or modified a C++ API call, I have also amended the
  corresponding Python bindings (and if altering ImageBufAlgo functions, also
  exposed the new functionality as oiiotool options).
• My code follows the prevailing code style of this project. If I haven't
  already run clang-format before submitting, I definitely will look at the CI
  test that runs clang-format and fix anything that it highlights as being
  nonconforming.

[antond-weta]

@lgritz Hey Larry, it seems not all OCIO distributions contain the oglapphelpers library. Should we copy this class https://github.com/AcademySoftwareFoundation/OpenColorIO/blob/main/src/libutils/oglapphelpers/glsl.h to the OIIO repo? It is their reference implementation of a class managing the GPU resources (shaders/textures/uniforms), it works well enough out of the box, I'm not very keen to reimplement it.

Also, I'm using the GPU-related OCIO classes in iv directly, is that ok, or should I expose the GPU stuff in the wrapper classes instead?

[lgritz]

Can you clarify what you mean by "not all OCIO distributions"? Do you mean not all versions of OCIO that can be found? Or do you mean that even at version 2.3 (say), you won't get that header if you install from certain package managers?

For the problem of some earlier versions not having the header, or simply the fact that OCIO 1.x has different APIs: on this, I'm 100% fine with iv only supporting OCIO color management for sufficiently modern versions. There's no reason for you to do extra work to support prior to, I don't know, let's say OpenColorIO 2.2? 2.1?

If there are certain distributions of even modern OCIO that don't seem to include all the components we need, I think I'm also ok with just not supporting iv color management if the files we need aren't found. Maybe throw it back at OCIO to contact the various distributions and get them to include all the necessary files that comprise the package? I'd prefer to not "vendor" parts of OCIO within the OIIO repo if at all possible.

Or am I misunderstanding the situation?

Also, I'm using the GPU-related OCIO classes in iv directly, is that ok, or should I expose the GPU stuff in the wrapper classes instead?

Definitely feel free to use it directly in the iv internals. I don't think there's a good reason to "wrap" it in the same way that we did the color configs and transformations, which made more sense to abstract so that we could provide fallbacks and insulate OIIO users who needed those concepts from direct exposure to OCIO.

[antond-weta]

Can you clarify what you mean by "not all OCIO distributions"?

I can try, though my knowledge of software distribution systems is quite limited.

OpenColorIO/src/libutils/oglapphelpers/CMakeLists.txt says that the class gets built into a static library named "oglapphelpers". If I build OCIO myself, I get the library installed under /opt/local/lib/libOpenColorIOoglapphelpers.a

When OCIO 2.2.0 is installed via MacPorts, I only get the main dynamic lib. Also oglapphelpers is not mentioned in any files in /opt/local/lib/cmake/OpenColorIO/*.

On my Fedora machine with OCIO 2.2.1 installed via DNF, I don't get the oglapphelpers lib installed, I can see the headers though.

[lgritz]

To keep the #if pollution to a minimum and improve readability, I think it's probably fine here to just keep all the parameters and data members unconditionally, but only use them if OCIO support is built in and of the sufficient version.

[antond-weta]

Will do!

[lgritz]

I think you've just gone and done the thing I came here to suggest...

Ideally, all OCIO builds -- including pre-built versions in package managed distros -- would both include these headers and libraries, and also the OCIO exported cmake config would have proper targets for them (which apparently it does not -- gack!). So probably the right thing to do for now is to "vendor" it by including just the minimum number of files we need. Hopefully, in a future OCIO version, we can be confident that these components are always include in OCIO itself and we can remove them from our side.

[lgritz]

Also, whatever minimum OCIO version you want to impose for this support is fine with me. We didn't have it before at all, so as far as I'm concerned, if the iv color management only works for the current and future OCIO releases, it's still a solid step forward. You shouldn't do a lot of extra work to try to make it work for arbitrarily old OCIO. Make the cut wherever you feel is the right tradeoff between supported versions and the amount of your time worth investing in it.

[antond-weta]

Hey Larry, I first added (nicked from ociodisplay) and then commented out the looks selection menu, as those seem to be legacy, and not really intended for OCIO v2 workflows? Should I remove the looks completely, and maybe add a "disable looks" checkbox instead?

[lgritz]

Wow, I have no idea. You've just run right past the boundary of my OCIO knowledge.

[doug-walker]

The OCIO UX working group recommendation is NOT to expose optimization settings to end users. It is very rarely needed and just causes confusion for end-users. (I know it is in ociodisplay, we should remove it perhaps, but that is more of a debugging tool.)

[antond-weta]

Thanks Doug, I have removed that.

[doug-walker]

@antond-weta , regarding the looks override menu in ociodisplay, my understanding is that that was there for prototyping configs. In a finished config, the looks are included in the views. So a user would select a view that has the desired look in it. (I don't consider this to be OCIO v2 specific.) The ability to override or add looks to views independent of what the config author did is not something I've seen in other apps besides ociodisplay, so I don't think it would be that useful to include it in iv. But if you do want to copy ociodisplay, the LegacyViewingPipeline class in OpenColorAppHelpers.h allows the looks override functionality.

[antond-weta]

@lgritz this is ready for review, I think.
I have removed the OCIO ref implementation class and re-implemented the missing functionality. The code is a bit cleaner this way.

[lgritz]

I'm truly sorry for the delay. Between being sick with COVID for multiple weeks, then vacation and work travel and then more vacation, I let this sit longer than I would have liked. It was in no way a lack of enthusiasm for this PR.

I tried to build myself to give it a spin, and I get this error:

/Users/lg/code/oiio/oiio.lg/src/iv/ivgl_ocio.cpp:364:57: error: too few arguments to function call, expected 8, have 7
                                  channel, interpolation);
                                                        ^
/usr/local/include/OpenColorIO/OpenColorIO.h:3534:18: note: 'getTexture' declared here
    virtual void getTexture(unsigned index,
                 ^
1 error generated.

This is on MacOS using OpenColorIO 2.3.

[antond-weta]

Thanks @lgritz
That extra parameter was introduced in 2.3.0. I've updated the code. I'm not quite certain of how to do that properly though. Do I need to add conditionals so the code builds against pre-2.3.0 as well?

[lgritz]

Yes, I think just an appropriate #if / #else around that call site (or at least as narrow as you can make it) so that it passes the right parameters for each version of OCIO.

There's no need to support OCIO 1.x for the iv color management, but at the very least, we should support OCIO 2.2 and 2.3. Everything >= 2.0 is even better if that's not a lot more trouble, but if it's a burden, I could live with 2.2 minimum for the iv support.

Hmmm... does this mean that our CI doesn't have any test that combines building iv with an OCIO >= 2.3, so that's why it didn't get caught before? Not all the CI tests bother building iv (because they may not have Qt and/or OpenGL set up), depends on the VM, so maybe the few OCIO 2.3 tests were among those, coincidentally.

[lgritz]

CI is failing on Windows, but I don't think that's related to your changes, since I don't believe our Windows CI case will build iv at all.

[lgritz]

LGTM

I don't think this is the end of color support in iv, but this is a solid step of adding the preliminary support and then we can refine.

Thanks, this is something I've wanted for iv for a decade and never quite got around to.