-
Notifications
You must be signed in to change notification settings - Fork 121
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
Conversation
A few questions
|
"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? |
Thanks for reviewing this Phil, my comments are below.
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.
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.
The host could not set these properties, or they could choose an OCIO role with
None.
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? |
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)?
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.
Either way it has to be documented. What does no-properties-set actually mean? Does it mean the host doesn't know?
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.
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) |
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). |
41386a4
to
0f5b480
Compare
I have revised the OCIO properties based on our previous discussions, so that they now address a wider range of use cases. Notably:
@barretpj and others please take a look and let me have your feedback. |
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? |
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? |
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. |
We should add a list of predefined colorspace "roles" that match up with OCIO roles tha provide generic names such as "linear", "srgb", "data". |
These are the typical roles on OCIO: https://opencolorio.readthedocs.io/en/latest/guides/authoring/overview.html#config-roles |
We should probably set up another meeting to hash out the OCIO spaces/roles vs "generic" spaces/roles thing. |
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. |
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 Maybe that is fine for first handshake: check host in current context in clip preferences Examples for a non OCIO user plug-in:
|
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:
|
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
This allows host and plug-in to negotiate what strings are allowed in the various colourspace properties. The rules would be:
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 If people think this is a good idea, I will write it up. |
JP, that seems quite reasonable. I'd add |
Right, I am redoing the strings to be less OCIO-centric and will update the PR. |
Good idea, maybe put in comment what it maps to if you are using OCIO
…On 2/8/2023 7:58 AM, John-Paul Smith wrote:
Right, I am redoing the strings to be less OCIO-centric and will
update the PR.
—
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANCVYUBAXUMCA3SNHPSTHDWWOYCLANCNFSM6AAAAAASU3NS3Q>.
You are receiving this because you commented.Message ID:
***@***.***>
|
Sorry for the commit mess, I will clean this up now. |
e5b1da8
to
682beec
Compare
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. |
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. |
I agree too that the new built-in ACES configs make it easier to standardize colorspace names. |
I have revised the spec to be more self-contained. The 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. |
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.
I'll be implementing it in a highly-colour-space-aware but non-OCIO host,
though we don't use the host support library so I'm not sure whether
contributing code based on that experience would be very useful?
… Message ID: ***@***.***
com>
|
Maybe nothing specific is needed, let's see if anything becomes obvious during your implementation. |
There was a problem hiding this 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?
scripts/genColour
Outdated
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.', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in af85e80.
include/ofxColour.h
Outdated
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
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 <[email protected]>
@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 Also @barretpj it would be great to get some feedback from you on the output clip negotiation, to make sure this addresses your concerns. |
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 |
include/ofxColour.h
Outdated
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
include/ofxColour.h
Outdated
|
||
@param handle handle to the instance, cast to an \ref OfxImageEffectHandle | ||
@param inArgs has the property | ||
- \ref kOfxImageClipPropPreferredColourspaces the list of preferred colourspaces |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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? |
There was a problem hiding this 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.
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 <[email protected]>
@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. |
There was a problem hiding this 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.
include/ofxColour.h
Outdated
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). |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include/ofxColour.h
Outdated
|
||
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. | ||
*/ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 <[email protected]>
Signed-off-by: John-Paul Smith <[email protected]>
Clarifying the use of basic colourspaces. Signed-off-by: John-Paul Smith <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
Also remove a bunch of old unused code from the example I copied this from. Signed-off-by: Gary Oberbrunner <[email protected]>
- Fix negative rowbytes - Add unspecified preferred input space - Set input preferred space correctly Signed-off-by: Gary Oberbrunner <[email protected]>
- Example dir name - Example source file name Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
Signed-off-by: Gary Oberbrunner <[email protected]>
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 <[email protected]>
I have merged @garyo's example plug-in from #162 and added a new feature at his suggestion. By default, |
There was a problem hiding this 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.
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. |
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)