-
Notifications
You must be signed in to change notification settings - Fork 358
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
Add color transform AdobeRGB to rec709 #1118
Add color transform AdobeRGB to rec709 #1118
Conversation
This commit adds support for transforming AdobeRGB color to rec709. Supported gamma is 2.2 (actually 563/256 per AdobeRGB convention) and linear.
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.
This proposal looks excellent, thanks @rasmusbonnedal.
Since the default color spaces in MaterialX follow the conventions of the OCIO configuration for ACES 1.2, we should verify that the proposed names lin_adobergb
and g22_adobergb
are the right choices here.
@doug-walker and @carolalynn22, what are your thoughts?
@rasmusbonnedal , Carol and I are involved in the OCIO configs working group and are curious about who is using Adobe RGB these days. We had actually decided to drop support for it from the new ACES configs earlier this year. ACEScg seems to be the dominant wide-gamut alternative to the Rec.709 primaries. (And we kept support for linear P3 primaries, since that lines up better with content generation in the film/tv world.) Where are you seeing Adobe RGB being used for encoding textures? I took a quick look through the PR and have a few comments:
|
Thank you @doug-walker for the great review! The furniture company I consult for is transitioning to MaterialX for everything. A large part of the existing textures are encoded in Adobe RGB. I'm not sure of the origin of those but I think it is a combination of material scanner, photography and Photoshop. The use of Adobe RGB is a legacy from their printing days. The support for Adobe RGB in MaterialX is essential for the transition.
|
I have looked into the matrix coefficients. I had taken the RGB to XYZ matrices from here and multiplied them with Numpy. I put Rec.709 and Adobe RGB coefficients from ACES 1.2 in Numpy and got this:
which is more different compared to both my original and yours, so now I don't know what to think! I'm very interested in how you got yours. Here is my attempt: adobergb_coefficients.py |
Thanks @doug-walker and @rasmusbonnedal. A few follow-up thoughts:
|
@rasmusbonnedal , regarding the difference in the matrix coefficients, I calculated my matrix at double precision starting from the chromaticity coordinates for Adobe RGB and Rec. 709. This is more accurate than starting with a pair of single precision matrices between other color spaces, as in your Gist. If you decide you want to be able to reproduce the matrix I supplied, there is code for that approach in both OpenColorIO and aces-dev on GitHub, although since you're a Python user, it may be easier to use Colour for Python from www.colour-science.org. Here's an example of how that is used. Thank you for providing the extra detail about the motivation for adding this color space. Given that this color space does not seem to be widely used in the VFX industry and is primarily for a single company, this would have been an ideal scenario to use a custom OCIO configuration file. It's very unfortunate that OCIO is not yet supported in MaterialX (IMHO, that would be the correct solution to this problem). By building the color space directly into MaterialX, it may mean that the library needs to support it indefinitely, even long after the client that requested it has moved on to another color space. Unfortunately this PR will also mean that the new ACES CG config which will soon be available in many software applications, and which we had intended to be an ideal OCIO config for use with MaterialX, is no longer suitable. The ACES 1.2 config is also not suitable, so unfortunately MaterialX will need a custom OCIO config in order to emulate the behavior of its default color management system. As we know, there was already a PR to add OCIO support to MaterialX that was not merged. Is the AdobeRGB support needed urgently? I wonder if it is feasible to pursue the OCIO approach first? |
@doug-walker Thank you for explanation! I trust the matrix you provided completely, I just wanted to understand why there was a difference. This feature is something that we already use through a patched MaterialX build. The performance in cpu rendering with OSL with the transform code MaterialX generates is very very good. Before this approach we tried the OCIO support in OpenImageIO and found the performance quite slow, especially scaling on multiple render threads. But that may have been something dumb we did. The only mention of OCIO in MaterialX I can is this slide deck which seems to suggest that ocio will be used to inject code in the generated OSL/GLSL/MDL code. That sounds very nice. Do you or @jstone-lucasfilm know if this is still the plan? I appreciate your point of view wrt to the support burden of another color space, but this is essential for us and if this patch is denied and the full OCIO support is not planned in a reasonable timeframe it would severely impede the use of MaterialX at IKEA. |
At some point, I'd love to hear more details so I can look into why performance did not seem good with the native OSL->OIIO->OCIO support. I suspect it's something silly, like having to do an expensive creation of a new color processor on every point instead of caching it properly. Or maybe something that doesn't constant fold during the runtime optimization, as we would expect. |
@doug-walker To my mind, both of the use cases described above are critical ones that MaterialX needs to support:
For the second use case, our current thinking is that this will require a communication channel for color transform graphs from OpenColorIO to MaterialX. This will allow us to use MaterialX shader generation to produce correct, efficient code for OpenColorIO transforms across a diverse set of shading languages, including the open-source GLSL, ESSL, OSL, and MDL generators, but also proprietary MaterialX shading environments such as Karma CPU/XPU and Unreal Engine. Building this communication channel has been a strong area of discussion for the MaterialX and OpenColorIO projects over the past year, and I'd definitely be interested in picking up that thread of discussion in upcoming months. @rasmusbonnedal My sense is that we should proceed with your new proposal, including the latest matrix updates from Doug Walker, so that your team is able to participate in the asset ecosystem with your existing AdobeRGB materials. In the future, we can work with the OpenColorIO team to make sure there's a corresponding configuration that matches our default set of color spaces, which we can then point to as the authoritative definition of the spaces we support natively. |
Review fixes pushed, I have renamed the almost-gamma-2.2 space to adobergb and changed the matrix to the proper one. |
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.
This proposal looks good to me, thanks @rasmusbonnedal!
This commit adds support for transforming AdobeRGB color to rec709. Supported gamma is 2.2 (actually 563/256 per AdobeRGB convention) and linear.
This commit adds support for transforming AdobeRGB color to rec709. Supported gamma is 2.2 (actually 563/256 per AdobeRGB convention) and linear.