-
Notifications
You must be signed in to change notification settings - Fork 49
Handles OCIO shared view token #1268
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
base: develop
Are you sure you want to change the base?
Handles OCIO shared view token #1268
Conversation
The OCIO config can return a special token "" as the colorspace name for a display view. This commit implements handling for this token, replacing it with the display name if found.
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.
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. |
Issue IdentifiedWe've discovered that this issue is likely shared with ACES 1.2 implementations. The PyOpenColorIO function
Proposed Solution
Current Issue with FallbackThe 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. |
Note that this PR may potentially affect the Houdini fix here: ynput/ayon-houdini#252 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 |
Consolidates color space conversion logic into a dedicated `oiiotool_transcode` function for better flexibility and clarity. This change introduces support for display/view transformations, enhancing the tool's ability to handle complex color management workflows. It also fixes issues with conflicting color space parameters and improves handling of source and target display/view configurations.
Introduces a `deprecated` decorator to mark functions as deprecated, issuing a warning when they are called. The `convert_colorspace` function is marked as deprecated, advising users to switch to `oiiotool_transcode`.
Improves readability by adjusting docstring formatting in the `convert_colorspace` function. This change ensures consistent documentation style and enhances clarity.
thank you @MustafaJafar for testing. I would suggest to test it again. the |
source_display = colorspace_data.get("display") | ||
source_view = colorspace_data.get("view") |
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.
As far as I know - any display/view that's currently collected inside intergration does not represent the SOURCE display/view (as in, the input media does not currently have that embedded) but it represents the display/view that should be applied to match what the user is currently seeing in their DCC.
I'm quite sure that's the case at least inside Maya and Houdini integrations.
If we now start using those as source display/view I'm a bit lost as to where the instance/representation would specify the target display/view based on how the DCC is currently configured.
So if Houdini says display view is X and Y. It means the raw output image... should get that X and Y applied to match what the user would be seeing inside houdini viewports, etc.
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.
You are right, I had forgotten about the case. Will add some changes for the case. But batchdelivery needs to have support for source and target display and view.
) | ||
|
||
|
||
def oiiotool_transcode( |
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.
Hmm, convert_colorspace
actually made more sense to me than oiiotool_transcode
-> there is no information about colorspace in the name and "transcode" is too generic for what the function does.
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.
For what it's worth - the ExtractOIIOTranscode
plug-in has come up a few times to also allow using it more generically, e.g. transcode but NOT color convert at all. Like just, convert to JPEG at 50% size which is trivial to with oiiotool
args.
Like here. But currently that only works if you ALSO make it color convert.
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.
oiiotool is used for more that just colorspace operations. It can be used for repositions too - so the name needs to be more generic.
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.
But this function is 100% colorspace related, you must pass in source and target colorspace otherwise it crashes... So it is not generic transcoding. The other arguments are "optional", so the primary usage of the function is "colorspace related", with possibility of doing other stuff, but not the other way around.
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.
But this function is 100% colorspace related, you must pass in source and target colorspace otherwise it crashes... So it is not generic transcoding. The other arguments are "optional", so the primary usage of the function is "colorspace related", with possibility of doing other stuff, but not the other way around.
Correct - my comment was mostly about the ExtractOIIOTranscode
plug-in behavior. The idea I had was just to make whatever it's calling as code would end up being less colorspace specific.
if target_colorspace: | ||
oiio_cmd.extend(["--colorconvert:subimages=0", | ||
source_colorspace, | ||
target_colorspace]) | ||
if view and display: | ||
oiio_cmd.extend(["--iscolorspace", source_colorspace]) | ||
oiio_cmd.extend(["--ociodisplay:subimages=0", display, view]) | ||
# Case 1: Converting to a named colorspace | ||
if all([source_view, source_display]): | ||
# First convert from source display/view to a role/reference space | ||
# that can be used with colorconvert | ||
# For example, converting to "scene_linear" or an appropriate | ||
# intermediate space | ||
# This is a two-step conversion process since there's no direct | ||
# display/view to colorspace command | ||
# This could be a config parameter or determined from OCIO config | ||
tmp_role_space = "scene_linear" | ||
oiio_cmd.extend([ | ||
"--ociodisplay:inverse=1:subimages=0", | ||
source_display, | ||
source_view, | ||
"--colorconvert:subimages=0", | ||
tmp_role_space, | ||
target_colorspace, | ||
]) | ||
else: | ||
# Standard color space to color space conversion | ||
oiio_cmd.extend([ | ||
"--colorconvert:subimages=0", | ||
source_colorspace, | ||
target_colorspace, | ||
]) | ||
else: # Using display/view target | ||
if all([source_view, source_display]): | ||
if source_display == target_display and source_view == target_view: | ||
# No conversion needed if source and target display/view are | ||
# the same | ||
logger.debug( | ||
"Source and target display/view pairs are identical. " | ||
"No color conversion needed." | ||
) | ||
else: | ||
# Complete display/view pair conversion | ||
# Similar approach: go through a reference space | ||
# This could be configured | ||
tmp_role_space = "scene_linear" | ||
oiio_cmd.extend([ | ||
"--ociodisplay:inverse=1:subimages=0", | ||
source_display, | ||
source_view, | ||
"--ociodisplay:subimages=0", | ||
target_display, | ||
target_view, | ||
]) | ||
else: | ||
# Standard conversion from colorspace to display/view | ||
oiio_cmd.extend([ | ||
"--iscolorspace", | ||
source_colorspace, | ||
"--ociodisplay:subimages=0", | ||
target_display, | ||
target_view, | ||
]) |
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.
This would make the conditions a lot easier to follow
# Source view and display are known
if source_view and source_display:
color_convert_args = []
if target_colorspace:
# Use temporarty role space 'scene_linear'
color_convert_args += ["scene_linear", target_colorspace]
elif source_display != target_display or source_view != target_view:
color_convert_args += [target_display, target_view]
else:
logger.debug(
"Source and target display/view pairs are identical."
" No color conversion needed."
)
if color_convert_args:
oiio_cmd.extend([
"--ociodisplay:inverse=1:subimages=0",
source_display,
source_view,
"--colorconvert:subimages=0",
*color_convert_args
])
elif target_colorspace:
# Standard color space to color space conversion
oiio_cmd.extend([
"--colorconvert:subimages=0",
source_colorspace,
target_colorspace,
])
else:
# Standard conversion from colorspace to display/view
oiio_cmd.extend([
"--iscolorspace",
source_colorspace,
"--ociodisplay:subimages=0",
target_display,
target_view,
])
if not target_colorspace and not all([view, display]): | ||
raise ValueError("Both screen and display must be set.") | ||
# Validate input parameters | ||
if all([target_colorspace, target_view, target_display]): |
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 would prefer and
inbetween instead of all([...])
.
@@ -1403,7 +1403,12 @@ def _get_display_view_colorspace_name(config_path, display, view): | |||
""" | |||
config = _get_ocio_config(config_path) | |||
return config.getDisplayViewColorSpaceName(display, view) | |||
colorspace = config.getDisplayViewColorSpaceName(display, view) | |||
# Special token. See https://opencolorio.readthedocs.io/en/latest/guides/authoring/authoring.html#shared-views # noqa |
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.
# Special token. See https://opencolorio.readthedocs.io/en/latest/guides/authoring/authoring.html#shared-views # noqa | |
# Special token. See https://opencolorio.readthedocs.io/en/latest/guides/authoring/authoring.html#shared-views # noqa |
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
convert_colorspace
function was marked as deprecated and new functionoiiotool_transcode
with improved set of input args were added.Testing notes:
sRGB - Display
but before it was returning<USE_DISPLAY_NAME>
.