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 3 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. |
2 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 consistency 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.