Skip to content
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

Conversation

rasmusbonnedal
Copy link
Contributor

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.
@rasmusbonnedal
Copy link
Contributor Author

image

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a 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?

@doug-walker
Copy link

@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:

  1. The "lin_adobergb" name seems good, and agrees with the ACES 1.2 config. In the ACES configs, we tend to not use the gamma prefix if that is already part of the color space. The ACES 1.2 config does not have a gamma-encoded Adobe RGB space, but if it did, I imagine it might have used "adobergb" rather than "g22_adobergb". (Especially since the actual Adobe RGB gamma is slightly different from 2.2.)

  2. The matrix coefficients I get are slightly different from what you have. I get:
    1.39835574e+00, -2.50233861e-16, 2.77555756e-17
    -3.98355744e-01, 1.00000000e+00, -4.29289893e-02
    0.00000000e+00, 0.00000000e+00, 1.04292899e+00

  3. Usually we try to define matrix coefficients like that in one place, rather than repeating them in various places. But I notice the AP1 matrix is repeated in several places too, so I don't want to hold you to a higher standard than what's been done previously.

  4. The way you've implemented this seems different from how the ACEScg support was done. For example, the code is in different locations and does not have both 3 and 4 channel versions. But I'm not an expert on MatX, so Jonathan would be better able to comment on that part.

@rasmusbonnedal
Copy link
Contributor Author

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.
We have been running this patch in our MaterialX material editor for a couple of months and the material technicians have been happy with it.

  1. That makes sense, I'll change to "adobergb" for the gamma encoded variant if you agree @jstone-lucasfilm.

  2. Interesting, I'll look into it.

  3. I agree. Maybe generate the OSL/GLSL/MDL files which contains the matrix from a single source in CMake?

  4. I have implemented it in the style of srgb_texture_to_lin_rec709. The 4 channel version is there, but in another file (mx_g22_adobergb_to_lin_rec709_color4.osl). I agree this differs from the ACEScg implementation. I can change to that if that's preferable.

@rasmusbonnedal
Copy link
Contributor Author

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:

[[ 1.39831632e+00 -3.98320578e-01  5.36237166e-06]
 [-2.23292513e-06  9.99997109e-01  2.99842012e-06]
 [-4.36533233e-06 -4.29402652e-02  1.04294617e+00]]

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

@jstone-lucasfilm
Copy link
Member

jstone-lucasfilm commented Oct 26, 2022

Thanks @doug-walker and @rasmusbonnedal. A few follow-up thoughts:

  1. Agreed on the name change to adobergb for the gamma variant.
  2. This seems worthwhile to resolve, and let's see what Doug says about the differences between your two results.
  3. For now, we still need a separate matrix definition for each language, but in the future we plan to switch over to graph definitions for color transforms, making these definitions completely language-agnostic.
  4. Your current approach looks fine for now, and as noted above we'll be unifying these definitions across languages in the future.

@doug-walker
Copy link

@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?

@rasmusbonnedal
Copy link
Contributor Author

@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.

@lgritz
Copy link
Contributor

lgritz commented Nov 9, 2022

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.

@jstone-lucasfilm
Copy link
Member

@doug-walker To my mind, both of the use cases described above are critical ones that MaterialX needs to support:

  • Native support for the ACES color spaces in shader generation, allowing public material libraries from a wide variety of sources to be rendered accurately, even in applications and web frameworks that can't directly access OpenColorIO.
  • Robust support for custom OpenColorIO configurations, allowing studios to reference custom color spaces in MaterialX content and achieve consistent rendering across environments.

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.

@rasmusbonnedal
Copy link
Contributor Author

Review fixes pushed, I have renamed the almost-gamma-2.2 space to adobergb and changed the matrix to the proper one.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a 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!

@jstone-lucasfilm jstone-lucasfilm merged commit 76edbeb into AcademySoftwareFoundation:main Nov 11, 2022
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
This commit adds support for transforming AdobeRGB color to rec709. Supported gamma is 2.2 (actually 563/256 per AdobeRGB convention) and linear.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants