Handles OCIO shared view token

[jakubjezek001]

Changelog Description

The OCIO config can return a special token "<USE_DISPLAY_NAME>" as the colorspace name for a display view. This commit implements handling for this token, replacing it with the display name if the token is found. This is fixing compatibility for >= Aces 1.3 configs.

Additional info

• I had been doing tests in regards of implementation for new ocio configs https://github.com/ynput/ayon-core/pull/1115. Following issue had been blocking me from farther tests.
• the convert_colorspace function was marked as deprecated and new function oiiotool_transcode with improved set of input args were added.

Testing notes:

1. Use following snippet in any of your DCC or Tray Console

from ayon_core.pipeline.colorspace import get_display_view_colorspace_name

ocio_config_path = "<path to any of new aces1.3 configs from ayon-ocio addon >=1.2.1>"
display = "sRGB - Display"
view = "ACES 1.0 - SDR Video"

resolved_colorspace = get_display_view_colorspace_name(
    ocio_config_path,
    display,
    view,
)

print(resolved_colorspace)

2. This should be printing sRGB - Display but before it was returning <USE_DISPLAY_NAME>.

[jakubjezek001]

This has to be put on hold for while, it seems this is more workaround rather then proper solution. OCIO v2 seems to have some better way to resolve the colorspace from view. The token is actually returned only on shared views. So perhaps we need to first detect what is the version and then get the view colorspace.

[jakubjezek001]

Issue Identified

We've discovered that this issue is likely shared with ACES 1.2 implementations. The PyOpenColorIO function getDisplayViewColorSpaceName has inconsistent return values:

• Sometimes returns a Display space name
• Other times returns the <USE_DISPLAY_NAME> token (when using OCIO v2 shared views)

Proposed Solution

1. Implement proper handling of the <USE_DISPLAY_NAME> token as done in this PR
2. For any code using _get_display_view_colorspace_name that expects a resolved colorspace:
   • Check if the returned value matches the display variable value
   • If matched, set both display and view attributes to the tools that support them (e.g., oiiotool)
   • Otherwise, set the view colorspace directly

Current Issue with Fallback

The current fallback approach is incorrect because it only applies the View_transformation (essentially just RRT) without applying the Output Display Transform (ODT). This results in incomplete color processing.

[BigRoy]

1. Implement proper handling of the <USE_DISPLAY_NAME> token as done in this PR

Note that this PR may potentially affect the Houdini fix here: https://github.com/ynput/ayon-houdini/pull/252 Tagging @MustafaJafar for visibility.

It looks like, if this PR is merged - that the fix implemented there could potentially be removed?

[MustafaJafar]

1. Implement proper handling of the <USE_DISPLAY_NAME> token as done in this PR

Note that this PR may potentially affect the Houdini fix here: ynput/ayon-houdini#252 Tagging @MustafaJafar for visibility.

It looks like, if this PR is merged - that the fix implemented there could potentially be removed?

I gave it a test run and It works. and when merging this PR we can remove https://github.com/ynput/ayon-houdini/blob/ba60bb3416397d7a8b0d638bba0ea94beaf75458/client/ayon_houdini/api/colorspace.py#L66-L67 The mentioned PR also allows the color space validator to pass if display value is used instead of colorspace.

[jakubjezek001]

I gave it a test run and It works. and when merging this PR we can remove ynput/ayon-houdini@ba60bb3/client/ayon_houdini/api/colorspace.py#L66-L67 The mentioned PR also allows the color space validator to pass if display value is used instead of colorspace.

thank you @MustafaJafar for testing. I would suggest to test it again. the convert_colorspace was marked as deprecated but it should be still supported. I needed to solve the problem of additional input args for source display and view.

[jakubjezek001]

https://github.com/ynput/ayon-core/pull/1268#discussion_r2142737987

it seems to me that the playblast is capturing actually source and target colorspace. It is rendering the viewport into what ever the display is set to, and then it is adding the display and view into the colorspaceData. We might consider the colorspace source and target in this case @BigRoy

[jakubjezek001]

cam we test this again please @MustafaJafar @moonyuet

[BigRoy]

#1268 (comment)

it seems to me that the playblast is capturing actually source and target colorspace. It is rendering the viewport into what ever the display is set to, and then it is adding the display and view into the colorspaceData. We might consider the colorspace source and target in this case @BigRoy

A Maya playblast, yes. (I believe we're also doing no OIIO transcode on those because we're not collecting actually any colorspace data on playblasts?)

Maya and Houdini renders, no. They render raw output - we pass the display and view from the DCC to the publish data for ExtractOIIOTranscode to transcode to that display view. In which it's not the source, but the target.

[jakubjezek001]

#1268 (comment) it seems to me that the playblast is capturing actually source and target colorspace. It is rendering the viewport into what ever the display is set to, and then it is adding the display and view into the colorspaceData. We might consider the colorspace source and target in this case @BigRoy

A Maya playblast, yes. (I believe we're also doing no OIIO transcode on those because we're not collecting actually any colorspace data on playblasts?)

Maya and Houdini renders, no. They render raw output - we pass the display and view from the DCC to the publish data for ExtractOIIOTranscode to transcode to that display view. In which it's not the source, but the target.

In that case it is not using the workfow as it should. Colorspace data are defining current state of colorspace in the representation and not future state. The target colorspace display and view in this case should be collected to instance.data and then used later as target display and view. We do use target colorspaces in other dccs in settings - but never they are kept in representation colorspaceData as this is not carrier for downstream target processing.

---

Here is proposed solution. I would rather add OCIO into the name of keys just to make sure it is explicit enought.

repres = instance.data["representations"]
for idx, repre in enumerate(list(repres)):
    # target space, display and view might be defined upstream
    target_colorspace = instance.data.get("targetOCIOColorspace")
    target_display = instance.data.get("targetOCIODisplay")
    target_view = instance.data.get("targetOCIOView")
	...

I am working if we should not make the section in settings optional, since it would be more understandable for users. Current use is following (after 2228037656b8eeaa400fcaab1329c6416dbc050f):

1. check if instance data is having set target colorspace, display, view keys and set them as default value
2. check if settings are having set any value in fields if yes then override the value set in 1.
3. if nothing is set from 1. and 2. then fill the values from repre colorspaceData and use source and target as the same

Curretnly user might feel that any value has to always be added to the colorspace fields since it is not explicit enough to navigate him to not to add anything. We could solve it similarly like it is done in Batch delivery tools or Nuke presets.

The toggle for enabling target colorspace override would be serving the purpose quite well.

[BigRoy]

In that case it is not using the workfow as it should. Colorspace data are defining current state of colorspace in the representation and not future state. The target colorspace display and view in this case should be collected to instance.data and then used later as target display and view. We do use target colorspaces in other dccs in settings - but never they are kept in representation colorspaceData as this is not carrier for downstream target processing.

I think we should start converting this instance.data:

{
	"colorspaceConfig": ocio_path,
	"colorspaceDisplay": display,
	"colorspaceView": view,
}

To:

{
	"colorspaceConfig": ocio_path,
	"colorspaceTargetDisplay": display,
	"colorspaceTargetView": view,
}

And when referring to source in instance.data we should use:

{
	"colorspaceConfig": ocio_path,
	"colorspaceSourceDisplay": display,
	"colorspaceSourceView": view,
}

And then for backwards compatibility

instance.data["colorspaceTargetDisplay"] = instance.data.pop("colorspaceDisplay, None)
instance.data["colorspaceTargetView"] = instance.data.pop("colorspaceView, None)

Or something along those lines? Do you have an example, from before this PR where instance.data["colorspaceDisplay"] is actively used as "source display" currently instead of target display?

Here is proposed solution. I would rather add OCIO into the name of keys just to make sure it is explicit enought.

Agreed - for both source and target.

Current use is following (after 2228037):

1. check if instance data is having set target colorspace, display, view keys and set them as default value
2. check if settings are having set any value in fields if yes then override the value set in 1.
3. if nothing is set from 1. and 2. then fill the values from repre colorspaceData and use source and target as the same

What if "source" is not set? Which is the case for e.g. maya and houdini render publishes? Only target is defined, and there is no 'source' display view because no display view is ever applied to the source? 🗡️ (I've rarely seen a source with display view embedded on purpose; maybe only from comp?)

Curretnly user might feel that any value has to always be added to the colorspace fields since it is not explicit enough to navigate him to not to add anything. We could solve it similarly like it is done in Batch delivery tools or Nuke presets.

It's explained in the tooltips for the attributes. @MustafaJafar worked on that I believe. Your provided screenshot does not make this clearer - the fact that it states "Target" does not elaborate to me that it can be empty. There should just be a label instead of tooltip "That these can be empty" OR an extra Enum option that just states: "Use expected target display/view from host" or whatever if you want to make it very explicit.

[antirotor]

I completely agree with the need for clarification what is the source and what is the target. What I would love to see is that nothing is directly setting/using values like

instance.data["colorspaceConfig"] = colorspace_data["config"]
instance.data["colorspaceDisplay"] = colorspace_data["display"]
instance.data["colorspaceView"] = colorspace_data["view"]

but it should be ideally done using API call like set_target_color_info(instance, data) (screw the names but you get the idea).

[BigRoy]

I completely agree with the need for clarification what is the source and what is the target. What I would love to see is that nothing is directly setting/using values like

instance.data["colorspaceConfig"] = colorspace_data["config"]
instance.data["colorspaceDisplay"] = colorspace_data["display"]
instance.data["colorspaceView"] = colorspace_data["view"]

but it should be ideally done using API call like set_target_color_info(instance, data) (screw the names but you get the idea).

And perhaps even stored on the instance as data classes too? 😎

[antirotor]

And perhaps even stored on the instance as data classes too? 😎

Now you are pushing it too far.... Heresy I say! 🤣

[MustafaJafar]

Could we update the PR description? I guess this PR adds more than just returning the display name instead of <USE_DISPLAY_NAME> if I'm not mistaken.