-
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?
Changes from all commits
539be6c
dba8d78
08f6b61
4ebf35d
9be9ad5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,8 @@ | |
import tempfile | ||
import subprocess | ||
import platform | ||
import warnings | ||
import functools | ||
from typing import Optional | ||
|
||
import xml.etree.ElementTree | ||
|
@@ -67,6 +69,47 @@ | |
} | ||
|
||
|
||
def deprecated(new_destination): | ||
"""Mark functions as deprecated. | ||
|
||
It will result in a warning being emitted when the function is used. | ||
""" | ||
|
||
func = None | ||
if callable(new_destination): | ||
func = new_destination | ||
new_destination = None | ||
|
||
def _decorator(decorated_func): | ||
if new_destination is None: | ||
warning_message = ( | ||
" Please check content of deprecated function to figure out" | ||
" possible replacement." | ||
) | ||
else: | ||
warning_message = " Please replace your usage with '{}'.".format( | ||
new_destination | ||
) | ||
|
||
@functools.wraps(decorated_func) | ||
def wrapper(*args, **kwargs): | ||
warnings.simplefilter("always", DeprecationWarning) | ||
warnings.warn( | ||
( | ||
"Call to deprecated function '{}'" | ||
"\nFunction was moved or removed.{}" | ||
).format(decorated_func.__name__, warning_message), | ||
category=DeprecationWarning, | ||
stacklevel=4 | ||
) | ||
return decorated_func(*args, **kwargs) | ||
return wrapper | ||
|
||
if func is None: | ||
return _decorator | ||
return _decorator(func) | ||
|
||
|
||
def get_transcode_temp_directory(): | ||
"""Creates temporary folder for transcoding. | ||
|
||
|
@@ -966,6 +1009,8 @@ def convert_ffprobe_fps_to_float(value): | |
return dividend / divisor | ||
|
||
|
||
# --- Deprecated functions --- | ||
@deprecated("oiiotool_transcode") | ||
def convert_colorspace( | ||
input_path, | ||
output_path, | ||
|
@@ -977,7 +1022,62 @@ def convert_colorspace( | |
additional_command_args=None, | ||
logger=None, | ||
): | ||
"""Convert source file from one color space to another. | ||
"""DEPRECATED function use `oiiotool_transcode` instead | ||
|
||
Args: | ||
input_path (str): Path to input file that should be converted. | ||
output_path (str): Path to output file where result will be stored. | ||
config_path (str): Path to OCIO config file. | ||
source_colorspace (str): OCIO valid color space of source files. | ||
target_colorspace (str, optional): OCIO valid target color space. | ||
If filled, 'view' and 'display' must be empty. | ||
view (str, optional): Name for target viewer space (OCIO valid). | ||
Both 'view' and 'display' must be filled | ||
(if not 'target_colorspace'). | ||
display (str, optional): Name for target display-referred | ||
reference space. Both 'view' and 'display' must be filled | ||
(if not 'target_colorspace'). | ||
additional_command_args (list, optional): Additional arguments | ||
for oiiotool (like binary depth for .dpx). | ||
logger (logging.Logger, optional): Logger used for logging. | ||
|
||
Returns: | ||
None: Function returns None. | ||
|
||
Raises: | ||
ValueError: If parameters are misconfigured. | ||
""" | ||
return oiiotool_transcode( | ||
input_path, | ||
output_path, | ||
config_path, | ||
source_colorspace, | ||
target_colorspace=target_colorspace, | ||
target_display=display, | ||
target_view=view, | ||
additional_command_args=additional_command_args, | ||
logger=logger, | ||
) | ||
|
||
|
||
def oiiotool_transcode( | ||
input_path, | ||
output_path, | ||
config_path, | ||
source_colorspace, | ||
source_display=None, | ||
source_view=None, | ||
target_colorspace=None, | ||
target_display=None, | ||
target_view=None, | ||
additional_command_args=None, | ||
logger=None, | ||
): | ||
"""Transcode source file to other with colormanagement. | ||
|
||
Oiiotool also support additional arguments for transcoding. | ||
For more information, see the official documentation: | ||
https://openimageio.readthedocs.io/en/latest/oiiotool.html | ||
|
||
Args: | ||
input_path (str): Path that should be converted. It is expected that | ||
|
@@ -989,17 +1089,26 @@ def convert_colorspace( | |
sequence in 'file.FRAMESTART-FRAMEEND#.ext', `output.1-3#.tif`) | ||
config_path (str): path to OCIO config file | ||
source_colorspace (str): ocio valid color space of source files | ||
source_display (str, optional): name for source display-referred | ||
reference space (ocio valid). If provided, source_view must also be | ||
provided, and source_colorspace will be ignored | ||
source_view (str, optional): name for source viewer space (ocio valid) | ||
If provided, source_display must also be provided, and | ||
source_colorspace will be ignored | ||
target_colorspace (str): ocio valid target color space | ||
if filled, 'view' and 'display' must be empty | ||
view (str): name for viewer space (ocio valid) | ||
both 'view' and 'display' must be filled (if 'target_colorspace') | ||
display (str): name for display-referred reference space (ocio valid) | ||
target_display (str): name for target display-referred reference space | ||
(ocio valid) both 'view' and 'display' must be filled (if | ||
'target_colorspace') | ||
target_view (str): name for target viewer space (ocio valid) | ||
both 'view' and 'display' must be filled (if 'target_colorspace') | ||
additional_command_args (list): arguments for oiiotool (like binary | ||
depth for .dpx) | ||
logger (logging.Logger): Logger used for logging. | ||
|
||
Raises: | ||
ValueError: if misconfigured | ||
|
||
""" | ||
if logger is None: | ||
logger = logging.getLogger(__name__) | ||
|
@@ -1024,23 +1133,95 @@ def convert_colorspace( | |
"--ch", channels_arg | ||
]) | ||
|
||
if all([target_colorspace, view, display]): | ||
raise ValueError("Colorspace and both screen and display" | ||
" cannot be set together." | ||
"Choose colorspace or screen and display") | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer |
||
raise ValueError( | ||
"Colorspace and both screen and display cannot be set together." | ||
"Choose colorspace or screen and display" | ||
) | ||
|
||
if all([source_view, source_display]) and source_colorspace: | ||
logger.warning( | ||
"Both source display/view and source_colorspace provided. " | ||
"Using source display/view pair and ignoring source_colorspace." | ||
) | ||
|
||
if not target_colorspace and not all([target_view, target_display]): | ||
raise ValueError( | ||
"Both screen and display must be set if target_colorspace is not " | ||
"provided." | ||
) | ||
|
||
if ( | ||
(source_view and not source_display) | ||
or (source_display and not source_view) | ||
): | ||
raise ValueError( | ||
"Both source_view and source_display must be provided if using " | ||
"display/view inputs." | ||
) | ||
|
||
if additional_command_args: | ||
oiio_cmd.extend(additional_command_args) | ||
|
||
# Handle the different conversion cases | ||
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, | ||
]) | ||
Comment on lines
1168
to
+1224
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,
]) |
||
|
||
oiio_cmd.extend(["-o", output_path]) | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if colorspace == "<USE_DISPLAY_NAME>": | ||||||
colorspace = display | ||||||
|
||||||
return colorspace | ||||||
|
||||||
|
||||||
def _get_ocio_config_colorspaces(config_path): | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
is_oiio_supported, | ||
) | ||
from ayon_core.lib.transcoding import ( | ||
convert_colorspace, | ||
oiiotool_transcode, | ||
) | ||
|
||
from ayon_core.lib.profiles_filtering import filter_profiles | ||
|
@@ -94,6 +94,8 @@ def process(self, instance): | |
|
||
colorspace_data = repre["colorspaceData"] | ||
source_colorspace = colorspace_data["colorspace"] | ||
source_display = colorspace_data.get("display") | ||
source_view = colorspace_data.get("view") | ||
Comment on lines
+97
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
config_path = colorspace_data.get("config", {}).get("path") | ||
if not config_path or not os.path.exists(config_path): | ||
self.log.warning("Config file doesn't exist, skipping") | ||
|
@@ -124,7 +126,7 @@ def process(self, instance): | |
|
||
transcoding_type = output_def["transcoding_type"] | ||
|
||
target_colorspace = view = display = None | ||
target_colorspace = target_view = target_display = None | ||
# NOTE: we use colorspace_data as the fallback values for | ||
# the target colorspace. | ||
if transcoding_type == "colorspace": | ||
|
@@ -136,18 +138,20 @@ def process(self, instance): | |
colorspace_data.get("colorspace")) | ||
elif transcoding_type == "display_view": | ||
display_view = output_def["display_view"] | ||
view = display_view["view"] or colorspace_data.get("view") | ||
display = ( | ||
target_view = ( | ||
display_view["view"] | ||
or colorspace_data.get("view")) | ||
target_display = ( | ||
display_view["display"] | ||
or colorspace_data.get("display") | ||
) | ||
|
||
# both could be already collected by DCC, | ||
# but could be overwritten when transcoding | ||
if view: | ||
new_repre["colorspaceData"]["view"] = view | ||
if display: | ||
new_repre["colorspaceData"]["display"] = display | ||
if target_view: | ||
new_repre["colorspaceData"]["view"] = target_view | ||
if target_display: | ||
new_repre["colorspaceData"]["display"] = target_display | ||
if target_colorspace: | ||
new_repre["colorspaceData"]["colorspace"] = \ | ||
target_colorspace | ||
|
@@ -166,16 +170,18 @@ def process(self, instance): | |
new_staging_dir, | ||
output_extension) | ||
|
||
convert_colorspace( | ||
input_path, | ||
output_path, | ||
config_path, | ||
source_colorspace, | ||
target_colorspace, | ||
view, | ||
display, | ||
additional_command_args, | ||
self.log | ||
oiiotool_transcode( | ||
input_path=input_path, | ||
output_path=output_path, | ||
config_path=config_path, | ||
source_colorspace=source_colorspace, | ||
target_colorspace=target_colorspace, | ||
target_display=target_display, | ||
target_view=target_view, | ||
source_display=source_display, | ||
source_view=source_view, | ||
additional_command_args=additional_command_args, | ||
logger=self.log | ||
) | ||
|
||
# cleanup temporary transcoded files | ||
|
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 thanoiiotool_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 withoiiotool
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.
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.