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

OpenColorIO Properties #103

Merged
merged 35 commits into from
Jul 21, 2024
Merged

Conversation

john-paulsmith
Copy link
Contributor

This is a generic version of an extension which has been used internally at Boris FX, implementing #102.

A set of five properties describe the OCIO setup of the host:
kOfxImageClipPropOCIOConfig: the OCIO config.
kOfxImageClipPropOCIOColourspace: the colourspace of the input images
kOfxImageClipPropOCIODisplay: the display transform
kOfxImageClipPropOCIODisplayView: the view
kOfxImageClipPropOCIOLook: the look (optional)

@barretpj
Copy link
Contributor

barretpj commented Dec 6, 2022

A few questions

  • this is all host-side. Is there any desire for a plugin to be able to request its inputs are in a specific colour space (or to negotiate a mutally-supported space with the host), or is this simply not possible due to the general nature of OCIO?
  • what defines the colour space of the output image?
  • what meaning if any do colour space properties have for matte inputs?
  • what meaning if any do the display-related properties have when rendering out and there is therefore no viewer?

@barretpj barretpj mentioned this pull request Dec 6, 2022
17 tasks
@barretpj
Copy link
Contributor

barretpj commented Dec 6, 2022

"clips which are color managed by OCIO" makes a requirement that the host uses OCIO. Hosts which have their own colour management may want to use this OFX mechanism without themselves using OCIO. Maybe rephrase these comments to make the requirement be that the image's colour space (and its subsequent treatment in any viewer) can be defined by OCIO?

@john-paulsmith
Copy link
Contributor Author

Thanks for reviewing this Phil, my comments are below.

  • this is all host-side. Is there any desire for a plugin to be able to request its inputs are in a specific colour space (or to negotiate a mutally-supported space with the host), or is this simply not possible due to the general nature of OCIO?

This is possible but we didn't do that in our internal version. If the host is using OCIO, it could construct a pipeline to convert the pixels into a different working colourspace from the same config as requested by the plug-in. However, if the plug-in is using OCIO itself, then this can be done on the plug-in side if needed, and the plug-in can choose whether to pay the price of the conversion or not.

  • what defines the colour space of the output image?

It's assumed to be in the same colourspace as the input image, but that doesn't make sense for matte or data outputs such as flow vectors. A solution would be to have plug-ins set kOfxImageClipPropOCIOColourspace on output clips, where it could be set to "Raw", or an empty string could signify that it should not be colour managed.

  • what meaning if any do colour space properties have for matte inputs?

The host could not set these properties, or they could choose an OCIO role with raw: yes which causes no colour management to happen.

  • what meaning if any do the display-related properties have when rendering out and there is therefore no viewer?

None.

"clips which are color managed by OCIO" makes a requirement that the host uses OCIO. Hosts which have their own colour management may want to use this OFX mechanism without themselves using OCIO. Maybe rephrase these comments to make the requirement be that the image's colour space (and its subsequent treatment in any viewer) can be defined by OCIO?

I suppose that's possible, but the various strings are defined in a specific OCIO config rather than being global. So a host which did not use OCIO itself would need to maintain (or generate on the fly) an OCIO config that matched its own colour management, so that the plug-in could load it. Actually, this makes sense for something like ACES where there is a standard OCIO config available and a host may implement ACES directly. Would that make sense if you were going to use this extension in Baselight?

@barretpj
Copy link
Contributor

barretpj commented Dec 6, 2022

  • what defines the colour space of the output image?

It's assumed to be in the same colourspace as the input image

No, that won't do. Which one if there's more than one input (or more gnarly, in a Transition context going between two images of different spaces)? What if we're in a Generator context (with no inputs)?

A solution would be to have plug-ins set kOfxImageClipPropOCIOColourspace on output clips, where it could be set to "Raw", or an empty string could signify that it should not be colour managed.

Either way it has to be documented. The host has to either tell the plugin what space it wants to receive, or be told by the plugin what space it's getting. Otherwise there's no point having any colour info anywhere.

  • what meaning if any do colour space properties have for matte inputs?

The host could not set these properties, or they could choose an OCIO role with raw: yes which causes no colour management to happen.

Either way it has to be documented. What does no-properties-set actually mean? Does it mean the host doesn't know?

  • what meaning if any do the display-related properties have when rendering out and there is therefore no viewer?

None.

OK that needs to be documented too. And if the intention is that these viewer properties are only to be used if the plugin has its own UI and should/must not affect the rendered image, that needs to be documented too.

"clips which are color managed by OCIO" makes a requirement that the host uses OCIO. Hosts which have their own colour management may want to use this OFX mechanism without themselves using OCIO. Maybe rephrase these comments to make the requirement be that the image's colour space (and its subsequent treatment in any viewer) can be defined by OCIO?

I suppose that's possible, but the various strings are defined in a specific OCIO config rather than being global. So a host which did not use OCIO itself would need to maintain (or generate on the fly) an OCIO config that matched its own colour management, so that the plug-in could load it. Actually, this makes sense for something like ACES where there is a standard OCIO config available and a host may implement ACES directly. Would that make sense if you were going to use this extension in Baselight?

Yes that's precisely what I was thinking. If this becomes the way OFX does colour management then we'd have to make up a config file that contains sufficient information to describe the colour spaces that Baselight will be using around the plugin. In which case it will be necessary to document that a plugin should be careful not to assume that the OCIO config is (a) always the same file path or (b) unchanging and never needs to be re-read.

(NB to avoid unexpected behaviour, Baselight currently always passes linearised images to and from OFX, thanks to a discussion with Bruno years back, unless the plugin implements our private extension to negotiate a known colour space)

@barretpj
Copy link
Contributor

barretpj commented Dec 6, 2022

Actually should the viewer properties really be set per-clip (indeed in the current header, per-input-clip)? Surely any viewer is effectively global (and needs to be accessible by a Generator plugin as well).

@john-paulsmith
Copy link
Contributor Author

I have revised the OCIO properties based on our previous discussions, so that they now address a wider range of use cases. Notably:

  • kOfxImageEffectPropSupportsOCIO is provided on the host and plug-in descriptors to negotiate use of OCIO.
  • The config, display, view and look properties have been moved to the effect instance, as these will not change per clip and are now usable in a generator context.
  • kOfxImageClipPropOCIOColourspace is now read/write and plug-ins need to set it on their output clip.
  • kOfxImageClipPropOCIOPreferredColourspace allows a host or plug-in to set the preferred colourspace for a given clip.

@barretpj and others please take a look and let me have your feedback.

@barretpj
Copy link
Contributor

barretpj commented Feb 7, 2023

Looks better now thanks.

I don't know OCIO but I gather all the colour space names are locally-defined in the config file so there's no globally-agreed name for any globally-defined spaces at all, right? So if a plugin wants to say "I want my inputs in a Rec.1886 tone curve with Rec.709 primaries" the plugin needs to find a space with a definition close to this in the config, and use its name?

This means plugins need to be able to access the config before naming any colour spaces on their clips.

However kOfxImageEffectPropOCIOConfig is set on the effect instance, but kOfxImageClipPropOCIOPreferredColourspace says it's set on a clip descriptor (i.e. at clipDefine time) not in a clip instance. Hopefully that's just a typing error on one side or the other?

@john-paulsmith
Copy link
Contributor Author

However kOfxImageEffectPropOCIOConfig is set on the effect instance, but kOfxImageClipPropOCIOPreferredColourspace says it's set on a clip descriptor (i.e. at clipDefine time) not in a clip instance. Hopefully that's just a typing error on one side or the other?

Good point. I could rework kOfxImageClipPropOCIOPreferredColourspace so that it's instead handled during kOfxImageEffectActionGetClipPreferences, which should work for input clips, then move the property to the clip instance so the host can set it on an output clip. Does that make sense, or can you see a better way for the host to specify a preference?

@barretpj
Copy link
Contributor

barretpj commented Feb 7, 2023

Yes I think that makes sense. Can't make it a host property, because in a host that doesn't use OCIO but instead fabricates an OCIO config at runtime, the config could well not be global, it could be per-project for example. So it has to be per-instance.

@fxtech
Copy link
Contributor

fxtech commented Feb 7, 2023

We should add a list of predefined colorspace "roles" that match up with OCIO roles tha provide generic names such as "linear", "srgb", "data".

@SonyDennisAdams
Copy link
Contributor

@fxtech
Copy link
Contributor

fxtech commented Feb 7, 2023

We should probably set up another meeting to hash out the OCIO spaces/roles vs "generic" spaces/roles thing.

@barretpj
Copy link
Contributor

barretpj commented Feb 7, 2023

So my interpretation is that in OCIO a role is a reasonably well-defined (but not guaranteed?) name which OCIO resolves to a real locally-defined OCIO colour space. The actual colour space is not specified, it could (I imagine) be time-specific depending on the upstream effects in the timeline, project-specific, host-specific, or indeed user-preference-specific. The intention is that code can request "scene_linear" and get something that is likely to be some form of scene-linear space. It can then use OCIO to lookup the precise space definition if it wants/needs to - or it can simply believe that it's probably close enough to what that role is supposed to mean.

@revisionfx
Copy link
Contributor

We should probably set up another meeting to hash out the OCIO spaces/roles vs "generic" spaces/roles thing.

Just some notes without discussing the current proposal per se:

Looking at the encoding list here:

https://opencolorio.readthedocs.io/en/latest/guides/authoring/colorspaces.html

scene-linear
display-linear
log
sdr-video
hdr-video
data

Maybe that is fine for first handshake: check host in current context in clip preferences

Examples for a non OCIO user plug-in:

  • if host is log, maybe would ask for scene-linear - if a special plug-in wants to know more use another layer to deal with that - as discussed by Phil, seems this is where a suite comes in?
  • if host is sdr/hdr in some cases would keep material in that space - at base level, sRGB 8 bit, etc fits in that bucket, in many cases happy with just gamma approximation here. I note that a lot of cameras now encode "YUV" with range -0.5 to 1.5 in practice, -0.25 for iPhone .mov for example... The conversion into integer 16 bit is supposed to account for that using black and white point for internal remapping into intermediate internal buffers in float here.. In practice 16b float is usually good for that. There are a few hosts that are 16b only (might remember wrong but I think HS-Art Diamant, Hitfilm might be internally 16b float although promoted to 32b for effects?)...
  • there is a tiny use case which is thumbnails generation etc - (that render path, and other variations of some display window calling a render). Color picking is another special topic.
  • For specialized applications like color matching, is useful to be in as close as possible to what user sees for processing space.. example: doing color match from a DVD to a new film scan for rerelease in 4K, the two inputs are in different color spaces to start. Similarly our texture projection stuff, want the main input in a colorspace and the texture uv as data. And our denoiser has some max difference thing it tries to optimize for that work better in perceptual space... We sometimes internally convert to gamma 2.2 linear images to be closer, but
  • In case where there are values like threshold, will likely want to get linear so changing working space does not change the threshold meaning internally. Thinking of hosts that globally change everything when you switch see in SDR versus HDR... a keyer is an example.
  • If I ask for something different than current host state, would like to return e.g. linear and have the host do the conversion back
  • Need to flag here e.g. "data" versus others to bypass post-transform by host
    Sometimes an option here display "data" versus display "color render"...
    In a 32b linear usually not an issue, host does not remap the color

@SonyDennisAdams
Copy link
Contributor

So my interpretation is that in OCIO a role is a reasonably well-defined (but not guaranteed?) name which OCIO resolves to a real locally-defined OCIO colour space. The actual colour space is not specified, it could (I imagine) be time-specific depending on the upstream effects in the timeline, project-specific, host-specific, or indeed user-preference-specific.

The list of roles is recommended and typical, but not guaranteed. However, I think you'd typically find "scene-linear", for example. Roles are well-specified and fixed in an OCIO configuration. For example, in the OpenColorIO v2 config for ACES (config-aces-reference.ocio), these are the roles and what color space they refer to:

roles:
  aces_interchange: ACES - ACES2065-1
  cie_xyz_d65_interchange: CIE-XYZ-D65
  color_timing: ACES - ACEScct
  compositing_log: ACES - ACEScct
  data: Utility - Raw
  default: ACES - ACES2065-1
  reference: ACES - ACES2065-1
  rendering: ACES - ACEScg
  scene_linear: ACES - ACEScg

@john-paulsmith
Copy link
Contributor Author

john-paulsmith commented Feb 8, 2023

Thankfully, OCIO 2.2 and ACES have already solved this problem for us. Two ACES configs are built in, and there is an API around built-in colourspaces which means that any software using OCIO 2.2 can rely on the names in the more basic of those configs, the ACES CG Config (not to be confused with the ACEScg colourspace).

OCIO 2.2 Release: Converting To or From a Known Color Space

Summary of the available colourspaces in the ACES configs

To support this I would alter kOfxImageEffectPropSupportsOCIO to be a string X 1 with the following options:

kOfxImageEffectOCIOACESCG
kOfxImageEffectOCIOACESStudio
kOfxImageEffectOCIOCustomConfig

This allows host and plug-in to negotiate what strings are allowed in the various colourspace properties. The rules would be:

  1. If neither host nor plug-in set kOfxImageEffectPropSupportsOCIO, there would be no colour management.
  2. If either host or plug-in sets kOfxImageEffectOCIOACESCG, all strings must be from the config ocio://cg-config-v1.0.0_aces-v1.3_ocio-v2.1.
  3. If both host and plug-in set kOfxImageEffectOCIOACESStudio, all strings must be from the config ocio://studio-config-v1.0.0_aces-v1.3_ocio-v2.1.
  4. If both host and plug-in set kOfxImageEffectOCIOCustomConfig, strings may be anything from the chosen config. The host is free to pass in an ocio:// URI as the config to use one of the built-in configs.

Hosts and plug-ins that implement ACES using their own pipeline could now interoperate fully, by including a mapping from OCIO ACES strings to their own equivalent constants. Plug-ins that simply want to get a linear input can set kOfxImageEffectOCIOACESCG then confidently request scene_linear, and if they want to output motion vectors they can use the data role.

If people think this is a good idea, I will write it up.

@SonyDennisAdams
Copy link
Contributor

JP, that seems quite reasonable. I'd add kOfxImageEffectNonOCIOColorManagement (or such) to indicate that the host (or plugin?) supports color space strings, but is not using OCIO (so there is no config which the plugin can read). Or maybe you're handling that in kOfxImageClipPropOCIOConfig by returning an empty string?

@john-paulsmith
Copy link
Contributor Author

Right, I am redoing the strings to be less OCIO-centric and will update the PR.

@revisionfx
Copy link
Contributor

revisionfx commented Feb 8, 2023 via email

@john-paulsmith
Copy link
Contributor Author

Sorry for the commit mess, I will clean this up now.

@john-paulsmith
Copy link
Contributor Author

The latest changes should be OK to review now. In short, I reworked the properties to provide two alternative colour management styles based on the built-in ACES configs from OCIO 2.2, which should be possible to implement without needing to actually use OCIO.

@revisionfx
Copy link
Contributor

Same as ofxGpuRender, should this be a separate header file instead of in imageEffects...?

@john-paulsmith
Copy link
Contributor Author

Same as ofxGpuRender, should this be a separate header file instead of in imageEffects...?

Seems reasonable as it's optional. I'll do that in the next iteration along with whatever other changes are needed.

@MrKepzie
Copy link
Contributor

MrKepzie commented Feb 8, 2023

I agree too that the new built-in ACES configs make it easier to standardize colorspace names.
Just for the sake of reference, here's the ticket I opened a while back regarding the issue of locating "sRGB" in custom configs: AcademySoftwareFoundation/OpenColorIO#1682

@john-paulsmith
Copy link
Contributor Author

I have revised the spec to be more self-contained. The Native colour management style works with a set of colourspaces defined in ofxColourspaceList.h, which was generated by processing the OCIO ACES Studio config using scripts/genColour. Hosts and plug-ins could implement the Native style without using OCIO. Some basic attributes of the colourspaces are included - IsData, IsSceneLinear and Encoding. Implementations which simply want to group colourspaces into scene linear, log and video could use these attributes to do that.

All this data is simply preprocessor definitions done in a very simple way and needs supporting code to make it usable. I would encourage this to be contributed by someone who will actually use the native style - all our plug-in and host code uses OCIO.

The mechanism for specifying a preferred colourspace has been extended to allow the plug-in or host to specify an ordered set of preferred colourspaces. This is still a preference though, images could be provided in any colourspace and the receiver of an image must check the relevant property.

@barretpj
Copy link
Contributor

barretpj commented Apr 30, 2023 via email

@john-paulsmith
Copy link
Contributor Author

Maybe nothing specific is needed, let's see if anything becomes obvious during your implementation.

Copy link

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

Thanks for the updates John-Paul! I have a few more comments. My main concern is still around how the simple case will work where the plug-in simply wants to specify a preference for any scene-linear or any perceptually-uniform color space that is convenient for the host.

In addition, I just want to note that now that ACES 2 is out, we are updating the configs on the OCIO side. I think it's fine for OFX to have its own configs to allow OCIO hosts to emulate the native modes, but it may be useful to give some thought to how versioning would work. I guess when OFX 1.6 is released, it could add a second OFX config that would add more color spaces, but the original OFX config would continue to be present for compatibility with older plug-ins?

role_docs = {
'ofx_sdr': 'Any display-referred SDR video such as Rec. 709.',
'ofx_hdr': 'Any display-referred HDR video such as Rec. 2020 HLG or PQ.',
'ofx_log': 'Any colourspace with a log transfer function.',

Choose a reason for hiding this comment

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

I recommend changing this to match OCIO's interpretation of log (in other words, it does not include HDR video color spaces, which also use a log encoding):
'Any scene-referred colourspace with a log transfer function.'

Same for compositing_log on line 48.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in af85e80.

When native colour management is in use, a host must set this
property to point to the OCIO config used to define strings for the version of
OFX it was built against. This will allow a plug-in which uses OCIO to work
directly with native styles.

Choose a reason for hiding this comment

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

To avoid confusion about the exact contents of this, you may want to provide the exact string that must be used for the current OFX. I'm guessing that this currently must be set to the following, but would like confirmation:
"openfx-studio-config-v2.1.0_aces-v1.3_ocio-v2.3.ocio"

Given that OCIO-based apps will need to have a copy of this config, it should be clear that people should not be able to swap in their own version of this for one of the native modes, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this comment to include the filename of the config, but also explained that a host could use a different config, provided it included all the same definitions as the standard config. For example they might have a config which is a superset of the native config.

Choose a reason for hiding this comment

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

Interesting, I had misunderstood how you were intending this to work for the two scenarios where either host is OCIO & plug-in is Native, or host is Native & plug-in is OCIO.

What I had assumed was that whichever half was Native would provide an ID string (not a path) which identifies a specific config from an OFX release. It would then be the responsibility of those implementing OCIO in OFX to have as part of their install a config that could respond to those color spaces. The most straight-forward would be to just have a copy of the OFX config(s). But another scenario would be that it has a config which is a superset of the OFX config which adds additional color spaces relevant to the app or the user. In fact, it could use the OCIO merge feature (under development) to merge the OFX config into a config provided by the end-user.

But if I understand what you're saying, it's the Native half that would provide a path to a config it wants the OCIO one to use. I don't like that approach for two reasons. First, it pushes the burden of having an OCIO config onto the Native half of the scenario, which may have minimal understanding of OCIO and would not want to have to supply a config file. Secondly, it limits the flexibility of the OCIO half of the scenario to use a superset config that has additional color spaces that are meaningful to it. You seem to be saying that the Native half would want to specify a config that could have additional color spaces, but that doesn't make sense to me. In other words, if it's in Native mode, there's no way for it to identify any additional color spaces other than what's in the header file.

Or is it that only the host may set this? I agree that if both host & plug-in are in OCIO mode, it should be the host that determines the config, and it should be a path rather than an ID string. But if only one of the two is OCIO and the other is Native, it should be the Native one that calls out the color space set it is using as an ID string (currently only one set is available, but I'm assuming that a new set might be added with every OFX release).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discarded this idea and added separate properties to specify the native mode config. This means the OCIO property is back to just being for OCIO, removing this overloaded functionality and any requirements for non-OCIO hosts to include an OCIO config. Now if either plug-in or host is implemented using OCIO, but operating in native mode, it can get all the information it needs from these separate properties.

These new properties allow for versioning of the colourspace set (I'm using the OCIO term config for this) independently from the API version, using the OCIO config name as the id string.

include/ofxColour.h Outdated Show resolved Hide resolved
include/ofxColour.h Outdated Show resolved Hide resolved
scripts/genColour Outdated Show resolved Hide resolved
include/ofxColour.h Outdated Show resolved Hide resolved
To aid with versioning, the OFX OCIO config has been renamed, now including the OpenFX version number instead of the version number of the upstream OCIO config.

Basic colourspaces have been removed from the reference OCIO config, allowing hosts and plug-ins to freely choose any colourspace with the correct attributes.

Two colourspaces were missing from the core style and have been added.

Various doc improvements to fix typos and make the intended usage clearer.

Signed-off-by: John-Paul Smith <jps@borisfx.com>
@john-paulsmith
Copy link
Contributor Author

@doug-walker thanks very much for your feedback and review. I've made a number of changes in response. Would you mind having a look over the changes and please let me know if you have any remaining concerns?

Regarding versioning, I'm now versioning the OpenFX native OCIO config in a way that should allow multiple versions to be used side-by-side, and have followed what I think is the correct naming scheme as used for the built-in configs. Please let me know if this makes sense.

The normal approach to versioning in OpenFX is to append a version number to the strings. For example if we want to rev the Core style to include new definitions, we would use OfxImageEffectPropColourManagementCoreV2. However, if we adopt that approach, I can't currently see a way for a plug-in to say it supports both V1 and V2. @garyo do you have any suggestions on how to handle this? Also, although the main API will hopefully be stable, the set of colourspaces will change regularly so we do need a way to allow both plug-ins and hosts to support multiple versions side-by-side. We can't version the colourspace names themselves without breaking compatibility with OCIO.

Also @barretpj it would be great to get some feedback from you on the output clip negotiation, to make sure this addresses your concerns.

@barretpj
Copy link
Contributor

Also @barretpj it would be great to get some feedback from you on the output clip negotiation, to make sure this addresses your concerns.

My comments here don't seem to have been addressed? 88f35d0

@john-paulsmith
Copy link
Contributor Author

My comments here don't seem to have been addressed? 88f35d0

@barretpj I don't see any comments on that commit, did you submit the review?

@garyo
Copy link
Contributor

garyo commented Jul 10, 2024

I can't currently see a way for a plug-in to say it supports both V1 and V2

Right -- we have a well-standardized way to rev suites but not props. I think the same approach can work though; a plugin can try to set PropNameV2; if the host accepts it, then they should both use V2. Otherwise the plugin should set PropName and proceed with v1. Alternatively a host can set a host prop HostPropNameV2 and the plugin can try to get that prop, falling back to V1 otherwise.
At least that's my quick thought on this -- problems/thoughts?

include/ofxColour.h Outdated Show resolved Hide resolved
include/ofxColour.h Outdated Show resolved Hide resolved
be called after first gathering the plug-ins preferred input colourspaces
via OfxImageEffectActionGetClipPreferences. The host must set
kOfxImageClipPropColourspace on the output clip to the chosen colourspace,
or the default value of OfxColourspace_Source.
Copy link
Contributor

Choose a reason for hiding this comment

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

The host can't set kOfxImageClipPropColourspace on the output clip, it's asking the plugin to do that. The host must have set kOfxImageClipPropColourspace on each input clip though.

OfxColourspace_Source default will fail on any plugin without an input clip called "Source". While "Source" is commonly used, it isn't mandated, so this behaviour could cause hosts to fail unexpectedly through no fault of the plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed the docs for this, and made the cross-referencing examples more diverse.


@param handle handle to the instance, cast to an \ref OfxImageEffectHandle
@param inArgs has the property
- \ref kOfxImageClipPropPreferredColourspaces the list of preferred colourspaces
Copy link
Contributor

Choose a reason for hiding this comment

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

Preferred by whom? Is this the list of all the spaces the host would prefer for the output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've clarified in the docs that this is the host's list of preferred colourspaces.

@barretpj
Copy link
Contributor

barretpj commented Jul 10, 2024

My comments here don't seem to have been addressed? 88f35d0

@barretpj I don't see any comments on that commit, did you submit the review?

Sorry I'm not used to using GitHub in this way. It shows me my comments, but it really doesn't make it clear that no one else can see them. Hopefully they've come through now?

Copy link

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

Thanks John-Paul, your update addressed most of my concerns. I noticed two minor items I'll flag. I'll reply in the thread to the one major area that I still don't understand.

scripts/genColour Outdated Show resolved Hide resolved
include/ofxColourspaceList.h Outdated Show resolved Hide resolved
Although the API should remain stable, colourspaces will change over time, and it’s likely hosts and plug-ins will support multiple versions of the spec for backwards compatibility. To address this, new properties have been introduced to allow negotiation of the config to use for native mode. This is somewhat similar to the use of built-in configs in OCIO.

Now that the native mode config negotiation is handled separately, the OCIO config property is reserved for use only in the OCIO style.

Hosts are now responsible for selecting the colour management style and config, and must set these in image effect properties.

Anticipating support for multiple versions, ofxColourspaceList.h has been renamed to a versioned name and is referred to as “the config header”. For now, it could be used as a drop-in replacement for ofxColourspaceList.h, but as soon as a second version is added, developers will need a strategy to manage these.

Additionally, the basic colourspaces have been renamed to include whether they are scene or display referred, and a descriptive label has been added.

Signed-off-by: John-Paul Smith <jps@borisfx.com>
@john-paulsmith
Copy link
Contributor Author

I can't currently see a way for a plug-in to say it supports both V1 and V2

Right -- we have a well-standardized way to rev suites but not props. I think the same approach can work though; a plugin can try to set PropNameV2; if the host accepts it, then they should both use V2. Otherwise the plugin should set PropName and proceed with v1. Alternatively a host can set a host prop HostPropNameV2 and the plugin can try to get that prop, falling back to V1 otherwise. At least that's my quick thought on this -- problems/thoughts?

@garyo I went for a different approach, because rev'ing the property names will be awkward for routine updates to the set of supported colourspaces. Instead, I added an id string (based on the OCIO config name) to the header, and a couple of properties to negotiate which set of colourspaces to use. This way, we should only need to rev the property names for actual functionality changes, rather than just to add new colourspaces.

At the moment this is trivial to implement because there's only one version of the native config. I'm envisioning we will add more versions over time, e.g. an ACES v2 config next year. We should keep old configs around for some time, for backwards compatibility. Once we add a second config, the code to use the definitions will need to get more complex. Depending on what language people are developing in, this might be a case of including each config header in a corresponding source file, then adding the names into a config-specific map.

Copy link

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

Thanks for the updates John-Paul! I like what you did to separate out the kOfxImageEffectPropColourManagementAvailableConfigs.

That clarifies for me how the various scenarios will work with one side being OCIO and the other side being in one of the Native modes. In that scenario, the OCIO side should be able to emulate the Core or Full Native mode by using either the provided config or a superset config.

And I think versioning of the Native color space sets should be more robust as well.

The colourspace strings used in the native styles are from ofx-native-1.5.ocio,
which is based on the OCIO ACES Studio built-in config,
studio-config-v2.1.0_aces-v1.3_ocio-v2.3, and stored for OFX purposes in
openfx-native-v1.5_aces-v1.3_ocio-v2.3.h (referred to as the config header).

Choose a reason for hiding this comment

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

Minor: It looks like the name now starts with ofx rather than openfx. And should "ofx-native-1.5.ocio" on line 35 be "the config header"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b3caf78 and be3de0e.


If kOfxImageEffectPropColourManagementStyle for the instance is a native style,
the host must set this property to indicate the native colour management config
the plug-in should be used to look up colourspace strings.

Choose a reason for hiding this comment

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

Wouldn't the plug-in also need to set this property if it wants to use a native mode? If more than one is available at a future point, I assume that the most recent one supported by both sides should be used?

If the native mode identified is just the basic style, then perhaps it could be omitted, for simplicity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plug-in communicates the list of configs it supports via kOfxImageEffectPropColourManagementAvailableConfigs, then the host chooses which config it wants to use and sets that in kOfxImageEffectPropColourManagementConfig. This applies for the whole effect rather than specifically on the input or output side. You'd normally expect the host to choose the most recent one supported by both sides, but that might not always be the case. For example the user might have set up their colour pipeline based on an older version of ACES. This is why it's left for the host to decide.

Also, this can't be omitted in the basic style, because the basic colourspaces are defined in the config header and we might add new ones in future.

The assumption is that OCIO > Full > Core > Basic, so the highest style
supported by both host and plug-in should usually be chosen by the host.
The chosen style must be set using this property on an image effect instance.
*/

Choose a reason for hiding this comment

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

It might be worth clarifying that, even if both sides are in OCIO mode, the Basic color space indicators may still be used to indicate that the other side has the flexibility to provide any color space in the given family.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't thought about that since removing the basic colourspace roles from the OCIO config. I've now permitted the use of basic colourspaces in kOfxImageClipPropPreferredColourspaces but not in kOfxImageClipPropColourspace, on the basis that in OCIO mode it should always be possible to label the clip with the actual colourspace that was used.

If the host has actually added roles or aliases into its config for the basic colourspaces, they would then be legal to pass in kOfxImageClipPropColourspace. I've clarified the docs to cover this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@doug-walker - I have a question about basic scene log just to be sure - is this equivalent to ACEScc curve excluding a particular chromacity (also for negative values when in float) ... straight log 2 where black and white points are nornalized to 0-1?

Choose a reason for hiding this comment

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

@revisionfx , my interpretation of "ofx_scene_log" is that it is a request to exchange any generic camera-log color space, which would include ARRI LogC (any version), Sony S-Log*, RED Log3G10, ACEScct, and etc. I think ACEScc would certainly fall in that group, but it certainly would not have to use that particular curve.

Many, though not all, plug-ins that work on generic video pixels will work on "ofx_scene_log" too, so I would guess that many plug-ins would include this in their list of acceptable inputs and this would give the host some useful flexibility in what it may provide.

john-paulsmith and others added 13 commits July 12, 2024 08:27
It makse sense to allow the use of basic colourspaces in OCIO mode, but in order to allow that, it’s necessary for negotiate the native-mode config even in OCIO mode, to allow for future changes to the set of basic colourspaces.

Also some minor doc improvements.

Signed-off-by: John-Paul Smith <jps@borisfx.com>
Signed-off-by: John-Paul Smith <jps@borisfx.com>
Clarifying the use of basic colourspaces.

Signed-off-by: John-Paul Smith <jps@borisfx.com>
Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
Also remove a bunch of old unused code from the example I copied this
from.

Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
- Fix negative rowbytes
- Add unspecified preferred input space
- Set input preferred space correctly

Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
- Example dir name
- Example source file name

Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
Signed-off-by: Gary Oberbrunner <garyo@darkstarsystems.com>
By default, the latest native mode config will be included. To disable this and manage config versions explicitly, define OFX_NO_DEFAULT_COLORSPACE_HEADER before including ofxColour.h.

Signed-off-by: John-Paul Smith <jps@borisfx.com>
@garyo garyo mentioned this pull request Jul 12, 2024
@john-paulsmith
Copy link
Contributor Author

I have merged @garyo's example plug-in from #162 and added a new feature at his suggestion. By default, #include ofxColour.h will pull in the latest native mode config. Once we have multiple versions, if a developer wants to use an older version or include multiple versions, they can define OFX_NO_DEFAULT_COLORSPACE_HEADER which will disable that feature.

Copy link

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

Thanks John-Paul, your updates have addressed all of my comments.

I started looking at the added plug-in code, but unfortunately I don't have time to review those new files right now.

@garyo
Copy link
Contributor

garyo commented Jul 21, 2024

I'm going to take the above as a positive review, and merge this branch now. I'm sure there will be small additions/fixes before the release, but this will hopefully get us a bit of air time.

@garyo garyo merged commit da3d49d into AcademySoftwareFoundation:main Jul 21, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

10 participants