AcademySoftwareFoundation / OpenImageIO

Reading, writing, and processing images in a wide variety of file formats, using a format-agnostic API, aimed at VFX applications.
https://openimageio.readthedocs.org
Apache License 2.0
1.98k stars 597 forks source link

feat: implement support for OCIO NamedTransforms #4393

Closed zachlewis closed 2 months ago

zachlewis commented 2 months ago

Description

OCIO-2 config authors may define a set of stand-alone "source-agnostic" NamedTransforms. Unlike the colorconvert, ociodisplay, and ociolook IBAs, which explicitly convert from a color space to something else, the ocionamedtransform IBA behaves more like the ociofiletransform IBA, where the transform itself can be applied either in the forwards or inverse direction, and does not take into consideration the input encoding or image state.

Quoting from the OCIO NamedTransform documentation:

Sometimes it is helpful to include one or more transforms in a config that are essentially stand-alone transforms that do not have a fixed relationship to a reference space or a process space. An example would be a “utility curve” transform where the intent is to simply apply a LUT1D without any conversion to a reference space. In these cases, a named_transforms section may be added to the config with one or more named transforms. [...] This feature may be used to emulate older methods of color management that ignored the RGB primaries and simply applied one-dimensional transformations.

Notably, the built-in OCIO-2 Studio config (ocio://studio-config-latest) contains NamedTransforms "sRGB - Curve" and "Rec709 - Curve", which we could implement as drop-in replacements for the existing linear_to_sRGB, sRGB_to_linear, linear_to_Rec709, and Rec709_to_linear functions in color.h (unless those functions are deprecated...).

There are several other applications for NamedTransforms as well. They can be extremely useful as workflow-oriented transforms implemented at various points in facility pipelines (e.g., for applying shot-specific conversions, grades, gamut-compression, etc.); they can be used to provide diagnostic transforms for visualizing "exposure zones", clipped areas, NaNs, etc; for visually "tagging" problematic clips; for applying parts of ACES Metadata Files parsed by OCIO's forthcoming AMF parser; for conversion to non-RGB color models... all sorts of things.

Tests

Tests are forthcoming. I've tested everything locally, but have not yet implemented proper tests in the testsuite.

Checklist:

lgritz commented 2 months ago

This is looking great, very complete, thanks!

You said in the PR comments that this is ABI-breaking, but actually I think it is not, since it only adds new non-virtual functions. I don't think it breaks backwards-compatibility with linkage. (And it seems to pass our CI test that does specifically check for ABI breaks.)

Shall I wait for you to add some kind of test? Or would you rather do that in a follow-up PR? I don't want us to forget about that, but given how thorough you were on this, I don't have any doubt that you'll follow up with the tests, and obviously these additions are new and can't break anything that previously worked.

zachlewis commented 2 months ago

You said in the PR comments that this is ABI-breaking, but actually I think it is not, since it only adds new non-virtual functions. I don't think it breaks backwards-compatibility with linkage. (And it seems to pass our CI test that does specifically check for ABI breaks.)

Ah, that makes sense... I was concerned that changing the signature for the ColorProcCacheKey constructor would break stuff (I'm not super C++ savvy, so I'm very out of my element here). I figured I'd err on the side of caution.

Shall I wait for you to add some kind of test? Or would you rather do that in a follow-up PR?

Whichever you'd prefer. I'm probably going to prioritize getting python wheels stuff together for next week, so it might be a short while before I get the tests in. (I figured, if this were ABI-breaking, it might make sense to get yr eyes on it sooner rather than later). I don't think it'll take me very much time.

On the other hand, I also noticed there were other OCIO features that needed tests; and there's an itty-bitty other PR I'd like to make adding a couple of ociofiletransform enhancements (context key/value support; "cccid" value support); and since testing context-variable stuff for ociofiletransforms requires a bit more setup, I can see dedicating an entire PR just to generating a testbed (ocio config + transforms) and thoroughly testing ColorProcessorHandle-generating IBAs, etc.

How about this -- let me see if I can bang out some NamedTransform-specific tests before the weekend; and if not, I'll circle back to this in a near-future PR, one way or another.

lgritz commented 2 months ago

Sounds good.

lgritz commented 2 months ago

Since the weekend came and went, I'm going to merge what you have so far (it's all new functions, so it can't break previously-working functionality). I trust you to come back shortly and add tests.