feat(color): Refactor writing color space metadata to share logic - #5390
feat(color): Refactor writing color space metadata to share logic#5390brechtvl wants to merge 4 commits into
Conversation
This adds new functions to get colorspace information from ImageSpec and uses them to deduplicate logic. * pvt::is_colorspace_srgb * pvt::get_colorspace_rec709_gamma * pvt::get_colorspace_icc_profile * pvt::get_colorspace_cicp There is existing consistency in that some file formats assume an empty oiio:ColorSpace to mean sRGB, and some don't. This inconsistency is preserved. An improvement is that writing gamma metadata from interop ID now consistently works with display interop IDs too, and not just scene interop IDs. Signed-off-by: Brecht Van Lommel <brecht@blender.org>
|
CC @zachlewis |
|
Did you put it in pvt because you aren't sure about the API and it might change? I feel like pvt is for internals, things that are not part of the public API and therefore don't consider changing them to be breaking ABI. As such, I'd prefer that no pvt namespace be used in any of the format readers/writers (yes, I know there are a few instances I haven't stamped out quite yet) since those in theory should be things that users could have written (and certainly should be allowed to be dynamic plugins, so need to be conscious of ABI). If you can't write a reader/writer without pvt things, that's a signal that there is something missing from the public API. That's a long-winded way of asking if it's possible to put them in the OIIO namespace (well, technically OIIO::v3_1) instead of pvt, and declare them in color.h (or imageio.h, there is a utility section toward the end). Again, curious to hear from @zachlewis about how this fits into the work he's doing and if he has feelings about where they should live. |
Signed-off-by: Brecht Van Lommel <brecht@blender.org>
Signed-off-by: Brecht Van Lommel <brecht@blender.org>
|
Yes, I put them in pvt because I think this API might change further. Though that was probably overly cautious, and I don't really see any problem making them public. Even if the readers and writers end up using a different API, these make some sense as general utility functions to query color space info from The draft PR by @zachlewis does contain a completely different mechanism and APIs for reading and writing. Used only by PNG and EXR still, and looks quite work in progress. It's not clear to me all that can make it into 3.2. I have now moved them to the public API and added Python bindings now. But it would be good to hear what @zachlewis thinks. |
|
Yeah, this is totally fine by me. I have other ideas for how stuff can work under the hood and exposed through the public api, but there's no harm in making these functions public either. I do have a problem with exposing the "rec709_gamma" function, though. I don't like how it works, I don't like what it does, and don't like what it's called. We can have a method for returning the exponent in float form of a color space that uses a pure-power transfer function if that's helpful internally for writers, but I definitely feel that method isn't ready for prime time. (Personally, I think oiio:Gamma could take either one or two floats -- two floats would serve to represent srgb and itu709 piecewise style transforms, one float is just a pure power function -- think about it) Alternatively (or in addition to), what do you guys think about a public get_color_space_info method that returns a struct / dictionary of attributes for a given color space? I feel like that's something we can stick in a public api that gives us some flexibility to maneuver and evolve without breaking the abi? Or am I misunderstanding C++ stuff again? In truth, I think the best way to represent these transfer functions unambiguously in a public api is with strings -- either the "string syntax" for OCIO transforms -- essentially, OCIO yaml oneliners; or with names of named transforms in our builtin config or the OCIO builtin configs. Or with, in most cases, the convention used in interop ids.But that particular function is too contrived and too incorrectly named and too idiosyncratic to warrant exposing to the public, IMHO. |
|
Regarding gamma, we already have I think it would make sense to have a I do feel like that's quite a bit of work beyond this PR, which is mainly trying to deduplicate existing code and make it so If we don't want to have
|
3 participants
Description
This adds new functions to get colorspace information from
ImageSpecand uses them to deduplicate logic.is_colorspace_srgbget_colorspace_rec709_gammaget_colorspace_icc_profileget_colorspace_cicpThere is existing inconsistency in that some file formats assume an empty
oiio:ColorSpaceto mean sRGB, and some don't. This inconsistency is preserved.An improvement is that writing gamma metadata from interop ID now consistently works with display interop IDs too, and not just scene interop IDs.
Ref #4980
Tests
Refactor of existing functionality already covered by tests.
Checklist:
and if I used AI coding assistants, I have an
Assisted-by: TOOL / MODELline in the pull request description above.
behavior.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.