APPLE: OCIO 2.1.1 with native Metal support

[creijon]

Description of Change(s)

Add OCIO 2 with GPU path for openGL and Metal.

Thanks to the team at Apple for the work on implementing this.

Fixes Issue(s)

• Full OCIO support

• I have submitted a signed Contributor License Agreement

[remia]

That's a great contribution! As it is removing support of OpenColorIO 1.x, if this is approved, I guess we should also ask for a minimal version 2 when looking for OpenColorIO in CMake find_package() call.

[remia]

Looks cannot be specified in setDisplay(), if we want to keep the look override feature, we can use LegacyViewingPipeline pass it the DisplayTransform and use setLooksOverrideEnabled() / setLooksOverride().

[creijon]

Ah, I see that there is an excellent example of this in the OCIO docs pages. I'll put the change in.

[remia]

Tested on CentOS 7 and it worked nicely. I had to comment out the Metal language part because I was compiling with OCIO 2.0.4, which brings me to raising again the question about VFX Reference platform compatibility. I read not so long ago USD was just in the process of moving to VFX 2020 as the minimum supported platform. This still feature OCIO 1.1.1 which this PR drops support for, this PR actually needs VFX 2022 for Metal support which could easily be downgraded to VFX 2021 by having a conditional path for Metal. But actually supporting VFX 2020 would require re-introducing the conditional path that were present before the changes in this PR. That's something for USD leads to answer I assume.

Another point, unrelated to this PR, I found the new UI control for OCIO display view selection hard to understand, I'm not exactly sure what's going on but it will probably need another look.

[sunyab]

Filed as internal issue #USD-7476

[spiffmon]

Yes, this PR will need to be reworked to *not* drop support for OCIO 1.x, or we will not even be able to build with it internally to Pixar.

…

--spiff

On Mon, Jul 11, 2022 at 10:12 AM Sunya Boonyatera ***@***.***> wrote: Filed as internal issue #USD-7476 — Reply to this email directly, view it on GitHub <#1936 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPOU2ASVICZ2B3BF3AZV2LVTRIXZANCNFSM524GCG7A> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[hodoulp]

Yes, this PR will need to be reworked to not drop support for OCIO 1.x,
or we will not even be able to build with it internally to Pixar.

@spiffmon The OCIO v1 has limited GPU support (i.e. LUT approximation in GPU), no Metal support and OCIO v2 is fully backward compatible. But just out of curiosity, could you explain why using v2 is a blocker for Pixar?

[spiffmon]

@hodoulp , Pixar does not currently have OCIO 2.x installed in our software suite, and we do have code outside of the USD project (that must cohabitate with USD) that uses OCIO. For sure we could (and plan to eventually) update to using 2.x everywhere, and if as you say 2.x is 100% backwards compatible, that should be straightforward. But it is still a reasonable-sized task, and we would not want the acceptance of this PR to be predicated on us doing that work.

Additionally, as already mentioned, we are trying to be good Platform citizens to the degree possible, and build_usd.py currently tracks CY2020 except where absolutely incompatible on specific platforms. We're OK with accepting support for 2.x alongside 1.x in the code and having build_usd.py pull in 2.x only on Mac for the Metal support. We understand that switching fully to 2.x will considerably simplify parts of build_usd.py, but we can't justify it quite yet. We have verified that we cannot build against 1.1.1 with the PR as it stands currently.

[creijon]

@hodoulp , Pixar does not currently have OCIO 2.x installed in our software suite, and we do have code outside of the USD project (that must cohabitate with USD) that uses OCIO. For sure we could (and plan to eventually) update to using 2.x everywhere, and if as you say 2.x is 100% backwards compatible, that should be straightforward. But it is still a reasonable-sized task, and we would not want the acceptance of this PR to be predicated on us doing that work.

Additionally, as already mentioned, we are trying to be good Platform citizens to the degree possible, and build_usd.py currently tracks CY2020 except where absolutely incompatible on specific platforms. We're OK with accepting support for 2.x alongside 1.x in the code and having build_usd.py pull in 2.x only on Mac for the Metal support. We understand that switching fully to 2.x will considerably simplify parts of build_usd.py, but we can't justify it quite yet. We have verified that we cannot build against 1.1.1 with the PR as it stands currently.

Hi Spiff, we'll update the PR to include 1.1.1 support. Is it acceptable to have this code in blocks protected by #ifdef?

[spiffmon]

@jonny-apple , thank you - a smallish number of #ifdefs do seem appropriate here!

[creijon]

@spiffmon , we've re-introduced OCIO 1.1.1. But we need a mechanism to control which version of OCIO is downloaded by build_usd.py. Would an additional parameter into be suitable, or switch based on platform? Currently macOS only supports OCIO 2 so we can select that.

[spiffmon]

Thanks for asking, @jonny-apple ! Though it may eventually make sense to provide a switch in advance of vfxreference adoption, we'd like to be complexity-conservative for now, so would request you predicate the version on MacOS.

Thanks again!

[creijon]

Thanks @spiffmon , I'll make the change.

[hodoulp]

I have a concern about having two different versions depending of the platform (by default, OCIO v1 for Windows and Linux, and OCIO v2 for macOS) because I think it could add confusion to users/devs/maintainers.

The OCIO v2 added numerous new features and greatly improved GPU (and CPU) processing. So, an OCIO v1 configuration file will work great with the OCIO v2 library (i.e. macOS) but the other way around is not true (i.e. an OCIO v2 configuration file does not work with the OCIO v1 library (i.e. Windows & Linux)).

I suggest to have a compilation flag (applicable to all platforms) to select the OCIO version which could be set to take OCIO v1 by default for now i.e. leaving Pixar ecosystem safe.

[creijon]

I have a concern about having two different versions depending of the platform (by default, OCIO v1 for Windows and Linux, and OCIO v2 for macOS) because I think it could add confusion to users/devs/maintainers.

The OCIO v2 added numerous new features and greatly improved GPU (and CPU) processing. So, an OCIO v1 configuration file will work great with the OCIO v2 library (i.e. macOS) but the other way around is not true (i.e. an OCIO v2 configuration file does not work with the OCIO v1 library (i.e. Windows & Linux)).

I suggest to have a compilation flag (applicable to all platforms) to select the OCIO version which could be set to take OCIO v1 by default for now i.e. leaving Pixar ecosystem safe.

Thanks, it sounds like a really good change. I'll put it in.

[creijon]

I’ve updated the branch to handle both OCIO 1.1.0 or 2.1.1 based on the user selection in build_usd.py, with defaults to 1.1.0 on Linux and 2.1.1 elsewhere.

I’ve moved the version-specific code in colorCorrectionTask.cpp into separate functions because the inline blocks of #ifdef made it very hard to understand the flow of the main functions.

Hope this helps,

Jon