Skip to content

Fix incorrectly transposed matrix for displayp3 colorspaces#1960

Merged
jstone-lucasfilm merged 1 commit intoAcademySoftwareFoundation:mainfrom
ld-kerley:fix_displayp3_colorspace_mtx
Aug 7, 2024
Merged

Fix incorrectly transposed matrix for displayp3 colorspaces#1960
jstone-lucasfilm merged 1 commit intoAcademySoftwareFoundation:mainfrom
ld-kerley:fix_displayp3_colorspace_mtx

Conversation

@ld-kerley
Copy link
Copy Markdown
Contributor

When originally authored the matrix was authored transposed. Further internal testing revealed this to manifest as an undesired shift towards red.

Before

mtlx_p3_to_rec709_before

After

mtlx_p3_to_rec709_fixed

@jstone-lucasfilm
Copy link
Copy Markdown
Member

Great catch, thanks @ld-kerley! I'm CC'ing @doug-walker and @carolalynn on this PR, to make sure we're remaining aligned with the upcoming NanoColor project.

@dgovil
Copy link
Copy Markdown
Contributor

dgovil commented Aug 6, 2024

Also just cc'ing @meshula about this too.

@doug-walker
Copy link
Copy Markdown

doug-walker commented Aug 7, 2024

@jstone-lucasfilm , it looks like this PR should get MaterialX much closer to NanoColor.

Here is what NanoColor gives for that conversion, based on the draft config that implements the ASWF Color Interop Forum color spaces and evaluating the result at single precision:
1.224940, -0.04205696, -0.01963755,
-0.2249402, 1.042057, -0.07863604,
0.0, 0.0, 1.098274

So it's fairly well aligned with this PR (enough to normally be below visual tolerances) but there are typically some small differences in matrices such as this based on the details of how they are calculated. For OCIO we use the chromaticity coordinates as the definitive parameters and calculate the matrix values to the reference space at double precision. But then since neither lin_displayp3_scene or lin_rec709_scene are the reference space, there would be some slight loss involved when composing the two matrices (although this is done at double precision too).

The direct double precision result from chromaticities, without compositional loss, is the following (which would be closest to the "true" value):
1.2249401763e+00, -4.2056954710e-02, -1.9637554590e-02,
-2.2494017628e-01, 1.0420569547e+00, -7.8636045551e-02,
0.0, 0.0, 1.0982736001e+00

@jstone-lucasfilm
Copy link
Copy Markdown
Member

Thanks @doug-walker, and in this context, I'd have no objections to moving forward with the matrix transpose in this pull request. Further in the future, we're planning to import our color transform graphs directly from NanoColor, at which point any differences in precision assumptions should be resolved.

Copy link
Copy Markdown
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 looks good to me, thanks @ld-kerley!

@jstone-lucasfilm jstone-lucasfilm merged commit a20bef0 into AcademySoftwareFoundation:main Aug 7, 2024
@meshula
Copy link
Copy Markdown

meshula commented Aug 8, 2024

Since @dgovil summoned me, the matrix Doug wrote out matches the results of the nanocolor implementation currently underpinning GfColor in OpenUSD.

jstone-lucasfilm pushed a commit that referenced this pull request Nov 5, 2024
Backporting #1960 to legacy 1.38 branch.

This feels like a pretty easy bug fix backport that would be to everyones benefit.
@ld-kerley ld-kerley deleted the fix_displayp3_colorspace_mtx branch November 21, 2025 17:21
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.

5 participants