-
Notifications
You must be signed in to change notification settings - Fork 650
feat: Auto convert between oiio:ColorSpace and CICP attributes in I/O #4964
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
Open
brechtvl
wants to merge
13
commits into
AcademySoftwareFoundation:main
Choose a base branch
from
brechtvl:cicp-interop-id
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+412
−20
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
9fdcf0c
feat: Auto convert between oiio:ColorSpace and CICP attributes in I/O
brechtvl dd9aa9c
Fix warning about unused variable
brechtvl ec29653
Use enum class and constexpr, fix spelling
brechtvl b9ea1c9
Change API functions to getCICP and getColorInteropID, keep more logi…
brechtvl 822b06f
Use cspan and snake case
brechtvl 2ed2e06
Update python stubs
lgritz 8cd3c0f
Emergency fix: change sonarqube action
lgritz b554ce2
Merge branch 'main' into cicp-interop-id
brechtvl 6c5f7f2
Take into account OCIO config interop_id for mapping to CICP on write
brechtvl 151e75b
Don't map g22_rec709_display to any CICP code for now
brechtvl 5366f57
Add Python docs for new API functions
brechtvl 086b82f
Update python stubs
brechtvl f94d052
Fix missing logic to use interop_id attribute for CICP
brechtvl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Since CICP is primarily used in video files, I think it would be more appropriate to prefer the display-referred mapping. The one exception might be for CICPTransfer::Linear, but what file formats would contain that?
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.
Yes, as mentioned in the description I think that's reasonable too.
There are some potentially negative consequences:
srgb_rec709_scene. If the presence of CICP would change that tosrgb_rec709_display, some applications integrating OIIO would need to be updated (again) to handle both cases.srgb_p3d65_scenefor textures, then it would help to write CICP to make them open and display correctly in other software. But then opening them again in OIIO they would besrgb_p3d65_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.
Welp, you both make very good points.
I've given this a lot of thought, and I've come up with a potential way forward that I believe addresses Doug's concerns about preferring to interpret CICP values as display-referred color spaces, as well as Brecht's concerns about reading and writing metadata that OIIO can interpret reliably.
Upshot: by default, interpret CICP as display-referred; and leverage other color metadata to convey scene-referred encodings.
#4787 (comment)
This said, I think, if you guys are okay with it, for the sake of the other dependent PRs, we should leave this behavior as-is for the immediate future, and continue with implementing a more robust strategy in a forthcoming PR, after the rest of Brecht's related work has made its way in safely.
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.
Fine by me. We have two gates: one on individual PRs, but another on when we backport a batch of PRs to 3.1. It's ok to noodle with the specifics from PR to PR as long as we have things reasonably settled down for the backporting steps.