feat: Auto convert between oiio:ColorSpace and CICP attributes in I/O #4964
Conversation
When reading image files with CICP metadata, automatically set the corresponding "oiio:ColorSpace". When writing files that support CICP and no other colorspace metadata can represent "oiio:ColorSpace", automatically write CICP metadata. Setting "oiio:ColorSpace" on read prefers scene referred over display referred color spaces, changing existing behavior as little as possible. The alternative would have been to interpret the presence of CICP metadata as an indication that the image is likely display referred, which might be reasonable too. Also add new ColorConfig set_colorspace_cicp and get_colorspace_cicp API functions to share logic between file formats. Signed-off-by: Brecht Van Lommel <[email protected]>
brechtvl
force-pushed
the
cicp-interop-id
branch
from
dfe9ccd to
9fdcf0c
Compare
|
Still a draft because:
Relevant discussion: #4787 |
Signed-off-by: Brecht Van Lommel <[email protected]>
|
Oh hells yes! Brecht, this is fantastic! I've been slowly working on my own p.o.c. (in python) for managing the relationship between CICP <--> colorInteropID <--> OCIO, and what you've put together aligns pretty closely with what I've been tinkering with. I really appreciate the initiative, this is exactly the direction we should be heading towards! I have a few suggestions + ideas, for your consideration:
Damn, I have more to add, of course, but I've got a bus to catch! |
|
The existing OIIO 3.1 implementation for OpenEXR and PNG is already setting My inclination would be to ensure To me deferring seems like it could be confusing in Though in either the immediate or deferred case, it's not clear to me when to drop colorspace related attributes that make no sense to write into another file (contradictory or not). |
|
This is looking great to me! I agree with what Zach wrote about where we eventually want to get to. But I think what you have here is still a positive incremental step that we should integrate when the two of you think it's ready. |
Signed-off-by: Brecht Van Lommel <[email protected]>
|
To be honest, I don't really have a problem with setting oiio:ColorSpace as early as possible; I just wonder if it's necessary, and if doing so imposes complexity of trying to keep CICP and oiio:ColorSpace attributes in sync. Either way, we need a mechanism for divining valid oiio:ColorSpace values just in time for color-aware IBAs that are otherwise missing and needing the attribute. My thinking is, if a user isn't using a color-aware IBA, it's probably safe to persist existing CICP primaries and trc data for as long as possible. But as soon as a color-aware IBA is encountered, incoming CICP data is potentially invalidated, so it may as well be removed. This would permit users to manually edit CICP metadata (in whole or in part) either before applying color-aware IBAs or just before writing to supported formats, allowing folks to pretty effortlessly preview the effects of modifying existing CICP data without having to worry about whether modifications to CICP metadata reflect the current state of the oiio:ColorSpace attribute, or vice versa. Conversely, once we're working with color-aware IBAs, we only care about keeping the value of oiio:ColorSpace up to date. If we blast away the CICP attribute every time we set oiio:ColorSpace, then when it comes time to setting CICP metadata for format writers, there's no ambiguity in what to do:
Likewise, if we defer the automatic derivation of "oiio:ColorSpace" to the last possible moment, that means that if the value happens to be set, either the user has set it manually, OR it was set by an upstream IBA applied by the user. If the user is only interested in setting or modifying CICP data, I don't think we should create conditions that would potentially compromise the validity of an oiio:ColorSpace attribute initially set on read. On the other hand, we certainly could endeavor to always update oiio:ColorSpace whenever CICP is updated. But given that color-aware IBAs might specify custom configs, I think it makes more sense to couple CICP mutations with corresponding changes to the config-agnostic "colorInteropID" attribute, interpreting the value as necessary for the given IBA / OCIO config. As for the behavior of the colorInteropID attribute itself, I think it should disappear and reappear as necessary, just like the CICP does, as described above. I'm not totally against allowing users to use CIF strings directly as oiio:ColorSpace values -- I think it would be a nice convenience, personally -- but I think it might be a slippery slope into compounded complexity down the road, when we want to use the colorInteropID to convey "namespaced" color spaces, which might require searching for a config, and then searching for a color space in that config, and then either finding an equivalent color space in the active config, or otherwise creating an ad-hoc transform / Processor into the current config. In other words, given the limited scope and use of the "unnamespaced" / reserved set of CIF strings, I think treating them as if they were known OCIO color spaces is more or less fine; but given the other applications of the colorInteropID attribute, we do need to treat it as a first-class citizen. Does this make sense? |
Also, yeah, to echo what Larry says, I have absolutely no problem integrating what you're currently doing in this PR as is, since I very much agree that it's on the right track! |
|
I was thinking that the reader is primarily responsible for setting the attribs that directly come from the file (like "CICP" from these files, or "colorInteropID" for exr, etc.). Optionally, it can also set "oiio:ColorSpace" if it knows for sure what it should be set to, like if the CICP exactly corresponds to a standard interop ID value that will be available in any default config, but this is not necessarily required. Then, we want a global The few functions that really need to know what color space the data is in can call deduce_color_space() before doing any critical color operations. I think that if we can figure out that there is an interopid that corresponds to whatever this image is, then oiio:ColorSpace should always be that. But I don't think it necessarily needs to be restricted to interop id names in cases where something a file supports isn't expressible as an OCIO color space name. In those cases, of course, we wouldn't expect an OCIO-based color transform to be able to deal with it, but we (OIIO) may internally handle other cases. I do agree that oiio:ColorSpace is non-serialized -- it's never written or read from any file, it's just an internal marker for our best inference (based on what was in the file or other info) about what the data is. |
…c in file formats Signed-off-by: Brecht Van Lommel <[email protected]>
|
I changed the public API functions to be Also added Python bindings for those. |
Signed-off-by: Brecht Van Lommel <[email protected]>
|
I realized the ACES configs were already released with display interop IDs. So I guess if the recommendation is final enough for those, then it should be ok for OpenImageIO too. |
|
Yes, the display interop IDs are good to go. We'll also want to support OCIO's internal color space interop IDs as well -- i.e., all the color spaces in ocio://studio-config-latest with interop_id values prefixed with the "ocio:" namespace... something like ~60-65 spaces in total, I think. |
|
Oh, I just realized this PR is limited to CICP <--> ColorSpace stuff. We don't have to worry about the "ocio:" prefixed namespaces here, necessarily, but the mechanism for handling the CIF tokens and "ocio:"-prefixed namespaces is pretty much the same. I'll write about it in another thread. |
|
Needs the stubs file to be updated (you can download it as an artifact from the failed wheel job). |
Signed-off-by: Larry Gritz <[email protected]>
|
I took the liberty of pushing a fix with the missing stub file changes. Unfortunately, something unrelated has happened to the sonar action and lots of jobs are failing. I'm inclined to just wait an hour an poke it again, some kind of temporary outate. |
Signed-off-by: Larry Gritz <[email protected]>
|
It may be good to wait a bit longer to merge this given the |
|
OK, whatever you guys decide here is fine with me. The last several rounds of this discussion has been on esoteric points that are totally over my head. I trust you to make the right decisions, just let me know when you both agree that what we have here is ready to merge. I've already got the Dec 1's scheduled 3.0 and 3.1 releases staged and ready to go, none of this is going to make that deadline, so whatever we merge here is going to get tried out in main for a few weeks before being backported to become a tagged release, so I think the stakes are low even if you merge something that you decide to revise reasonably soon after. |
|
BTW, @brechtvl, the comments in the C++ header form the documentation for the C++ API, but you also added python bindings which still need to be mentioned briefly in pythonbindings.rst |
Signed-off-by: Brecht Van Lommel <[email protected]>
Signed-off-by: Brecht Van Lommel <[email protected]>
This is unlikely meant to be written with Gamma 2.2 color metadata, so keep behavior unchanged until there is decision on what to do. Signed-off-by: Brecht Van Lommel <[email protected]>
The ColorConfig class was not documented yet, this only documents the new methods. Signed-off-by: Brecht Van Lommel <[email protected]>
brechtvl
force-pushed
the
cicp-interop-id
branch
from
41876f6 to
5366f57
Compare
|
Some more changes:
|
Signed-off-by: Brecht Van Lommel <[email protected]>
brechtvl
force-pushed
the
cicp-interop-id
branch
from
6a70bd7 to
086b82f
Compare
I think, in general, throughout the 3.1 release cycle, we should caveat these color management features as "experimental" and subject to change. I think there's a good chance we won't be able to effectively solicit enough meaningful feedback in some areas without a concrete implementation for folks to mess around with; and I know the OpenColorIO TSC is waiting to see what we end up doing here with stuff like CICP before providing more "official" guidance. Also, I expect that we'll make incremental improvements and incorporate additional features along the way that are outside the scope of what we're immediately trying to achieve. So, it would be cool if we could "reserve the right" to adjust behavior between OIIO patch versions. This would also serve to keep the relevant PRs less painful to review and more pleasurable to merge. |
|
Also, I feel it's important to note that what we're currently doing with writing interop_id strings directly to oiio:ColorSpace isn't really a viable solution (or, at least, not without intervention elsewhere). Doing so presupposes that the ColorConfig used with any given IBA already contains ColorSpace with a name or alias that matches that string, and that assumption really only holds when using one of the newer (OCIO-2.5) builtin OCIO configs. Given what I wrote above, given the scope of this PR, and given what we're doing elsewhere, I have no problem with continuing to populate oiio:ColorSpace with CIF token values for now, if that helps move things forward; but this would mean backtracking a little bit in a future PR, where we more appropriately handle the semantics of config-specific ColorSpaces and config-agnostic interop_ids. As part of that effort, we're also going to need to vastly improve our ColorConfig / ColorSpace equivalency testing. We've got a handful of clever tricks going on in our current ColorConfig.equivalent method, but we need something far more robust, something that actually tests for definitional equivalence within and across ColorConfigs; and we need a mechanism for handling cases where the ColorSpace we're referencing doesn't actually exist in the ColorConfig we're using for an IBA. Fortunately, the OCIO API offers some methods to make this fairly trivial. I'll write about these mechanisms in the color-stuff-discussion-thread. |
Signed-off-by: Brecht Van Lommel <[email protected]>
That sounds reasonable. Though I think that when we're talking about behavior, and not API/ABI compatibility that will cause downstream software to not build correctly, we're allowed to "fix bugs" even if it changes behavior by a reasonable amount, which gives us some leeway naturally to say "this behavior might be changing, but it was wrong before, and we're improving it" at a patch release. Nevertheless, if you think this area is likely to have repeated churn and changes in direction (not just monotonically increasing the set of color scenarios we recognize and handle), then we can add a line to future release notes saying that we're expecting extra change. |
Yeah, but in some sense... so what? Isn't the worst that happens that you might ask it to do a color transformation that depends on OCIO, and it's an error because your config doesn't contain the color space that the image says it is? Seems like that caveat has been in effect forever. We do have the option, I suppose, of requiring OCIO 2.5 minimum and rely on the auto-build to make a baked-in static OCIO if that's newer than what the user has installed?
I'm fine with that. |
|
* Yeah, but in some sense... so what? Isn't the worst that happens that you might ask it to do a color transformation that depends on OCIO, and it's an error because your config doesn't contain the color space that the image says it is? Seems like that caveat has been in effect forever.
Ah, but we have the means to make the interop mechanisms *always* work (well, outside of extreme conditions where people are doing inadvisable things with their OCIO configs); and that is what we should absolutely aim for. Use of one of the reserved colorInteropId strings should *always* elicit valid and reliable behavior, regardless of what’s actually in a provided config. Anything less would sort of violate the implicit contract among the ecosystem of software that uses CIF tokens to interoperate.
So, no, I do not think it’s acceptable to merely align internal hardcoded naming conventions to an external reference of what these entities ostensibly represent — these reserved strings *must* always produce interactions with any given config, and behave like first class citizens. It’s easier than it sounds, as I hope to demonstrate.
* We do have the option, I suppose, of requiring OCIO 2.5 minimum and rely on the auto-build to make a baked-in static OCIO if that's newer than what the user has installed?
Shouldn’t be necessary. OCIO 2.5 would make some stuff easier, and in the future, we’ll be able to simplify aspects of our codebase — but for now, OCIO-2.3+ provides everything we need to do what needs doing, with the additional caveat that we’d need to provide a reference “interop identities” config that reconciles interop-id strings with color space definitions that align with what OCIO-2.5 ships with. Once OCIO-2.5 becomes the OIIO minimum, we can additionally simplify.
…________________________________
From: Larry Gritz ***@***.***>
Sent: Monday, December 1, 2025 1:20:33 PM
To: AcademySoftwareFoundation/OpenImageIO ***@***.***>
Cc: Zach Lewis ***@***.***>; Comment ***@***.***>
Subject: Re: [AcademySoftwareFoundation/OpenImageIO] feat: Auto convert between oiio:ColorSpace and CICP attributes in I/O (PR #4964)
[https://avatars.githubusercontent.com/u/504179?s=20&v=4]lgritz left a comment (AcademySoftwareFoundation/OpenImageIO#4964)<#4964 (comment)>
Also, I feel it's important to note that what we're currently doing with writing interop_id strings directly to oiio:ColorSpace isn't really a viable solution (or, at least, not without intervention elsewhere). Doing so presupposes that the ColorConfig used with any given IBA already contains ColorSpace with a name or alias that matches that string, and that assumption really only holds when using one of the newer (OCIO-2.5) builtin OCIO configs.
Yeah, but in some sense... so what? Isn't the worst that happens that you might ask it to do a color transformation that depends on OCIO, and it's an error because your config doesn't contain the color space that the image says it is? Seems like that caveat has been in effect forever.
We do have the option, I suppose, of requiring OCIO 2.5 minimum and rely on the auto-build to make a baked-in static OCIO if that's newer than what the user has installed?
Given what I wrote above, given the scope of this PR, and given what we're doing elsewhere, I have no problem with continuing to populate oiio:ColorSpace with CIF token values for now, if that helps move things forward; but this would mean backtracking a little bit in a future PR, where we more appropriately handle the semantics of config-specific ColorSpaces and config-agnostic interop_ids.
I'm fine with that.
—
Reply to this email directly, view it on GitHub<#4964 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AARAC4CKRSAZYLPTN37XIHL37SBHDAVCNFSM6AAAAACNLYHACGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKOJYGE3DGNZXGU>.
You are receiving this because you commented.Message ID: ***@***.***>
|
|
There are several other PRs in flight now that are drafts because they depend on this being merged first. So I want to put a premium on expeditiously getting this first one acceptable to merge, even if we need to come back and fine tune afterwards. The 3.1 patches for this month are out, so it's fine to repeatedly revise this in main until the end of the month, and even then, we can always just decide to delay backporting any of these color PRs to dev-3.1 until we are fully satisfied with its state. |
|
I submitted two more PRs intended to be wrap up loose ends left by this PR: compatibility with older OCIO configs and handling display color space interop IDs throughout the I/O code. The implementation choices can be debated and changed, it's just my proposal. |
Agreed! Wherever we land in the end, this PR provides a fantastic basis for moving forward. |
| constexpr ColorInteropID color_interop_ids[] = { | ||
| // Scene referred interop IDs first so they are the default in automatic | ||
| // conversion from CICP to interop ID. Some are not display color spaces | ||
| // at all, but can be represented by CICP anyway. |
There was a problem hiding this comment.
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.
Yes, as mentioned in the description I think that's reasonable too.
There are some potentially negative consequences:
- OIIO currently reads sRGB images as
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. - If you use
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. - Other software for painting and editing textures may write CICP (maybe more so in the future if AVIF, JPEG XL gain more adoption).
There was a problem hiding this comment.
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.
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.
we should leave this behavior as-is for the immediate future, and continue with implementing a more robust strategy in a forthcoming PR
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.
For this PR, the tricky decisions are how to write |
|
Understood; we can discuss and make changes to specifically address these
issues in subsequent issues / PRs. For the sake of this PR, I think this is
a fine place to leave things, especially if it helps make things easier to
align with what you’ve done with Blender.
I’m traveling tomorrow, but over the next couple of days, I’ll have a
closer look at the code ASAP — absent anything glaring, I very much expect
to approve.
…On Mon, Dec 1, 2025 at 6:53 PM Brecht Van Lommel ***@***.***> wrote:
*brechtvl* left a comment (AcademySoftwareFoundation/OpenImageIO#4964)
<#4964 (comment)>
There are several other PRs in flight now that are drafts because they
depend on this being merged first. So I want to put a premium on
expeditiously getting this first one acceptable to merge, even if we need
to come back and fine tune afterwards.
For this PR, the tricky decisions are how to write g22_rec709_display and
how to guess scene referred or display referred from CICP. In both cases
I've left the existing behavior unchanged, to avoid approval being blocked
by a decision for those.
—
Reply to this email directly, view it on GitHub
<#4964 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARAC4ADHVPIJHIRBSQU3BD37TIH5AVCNFSM6AAAAACNLYHACGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKOJZGQ4DINZVGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
4 participants
Description
When reading image files with CICP metadata, automatically set the corresponding "oiio:ColorSpace". When writing files that support CICP and no other colorspace metadata can represent "oiio:ColorSpace", automatically write CICP metadata.
Setting "oiio:ColorSpace" on read prefers scene referred over display referred color spaces, changing existing behavior as little as possible. The alternative would have been to interpret the presence of CICP metadata as an indication that the image is likely display referred, which might be reasonable too. Either way it's a guess.
There is no automatic mapping from
g22_rec709_displayto CICP currently to keep behavior unchanged, as this is often not what you want and further discussion is needed to decided on the right behavior.Also add new ColorConfig
get_cicpandget_color_interop_idAPI functions to share logic between file formats.Tests
Tests were updated, the auto detected color space name appears in the output.
Commands like these should also do the right thing automatically.
Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.