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

Regression: Excessive precision loss in NVIDIA Cg shaders #1884

Open
christianaguilera-foundry opened this issue Oct 11, 2023 · 1 comment
Open
Labels
Needs Discussion Needs discussion before implmentation, which could result in changes, or a decision not to proceed.

Comments

@christianaguilera-foundry

Certain operations in the shader generation do not account for the use of half in the NVIDIA Cg shading language.

Excerpt of a NVIDIA Cg shader generated with OpenColorIO 1.1.1:

out_pixel.rgb = max(half3(6.10352e-05, 6.10352e-05, 6.10352e-05), half3(1, 1, 1) * out_pixel.rgb + half3(0, 0, 0));
out_pixel.rgb = half3(1.4427, 1.4427, 1.4427) * log(out_pixel.rgb) + half3(0, 0, 0);

Excerpt of a NVIDIA Cg shader generated with OpenColorIO 2.1.3 [using the same OCIO config]:

outColor.rgb = max( half3(0., 0., 0.), outColor.rgb);
outColor.rgb = log2(outColor.rgb);

Note that 6.10352e-05 (GetHalfNormMin()) is used in OCIO 1, whereas 0. is now seen in OCIO 2.

The issue was initially noticed with images that use pure colors (e.g. #FF0000 or #00FF00): the color turns black.

Input Image OpenColorIO 1 OpenColorIO 2
Input Image OpenColorIO 1 OpenColorIO 2

The issue appears to originate in src/OpenColorIO/ops/log/LogOpGPU.cpp, with the unconditional use of std::numeric_limits<float>::min(). A potential solution, that addresses the case exposed above, is:

-    const float minValue = std::numeric_limits<float>::min();
- 
-    GpuShaderText st(shaderCreator->getLanguage());
+    const GpuLanguage lang = shaderCreator->getLanguage();
+ 
+    const float minValue = lang == GPU_LANGUAGE_CG
+                               ? GetHalfNormMin()
+                               : std::numeric_limits<float>::min();
+
+    GpuShaderText st(lang);

This is likely not the only oversight, though; there may be more precision issues of this nature that involve the Cg generator.

@christianaguilera-foundry
Copy link
Author

In christianaguilera-foundry@195a771, one can see the potential fixes that address the problems we have encountered so far, but I'm not confident those are the only issues, or whether the partial, proposed solution is acceptable (in particular, the change in Lut1DOpGPU.cpp to support LUTs with more than 65504 (GetHalfMax()) entries).

As an alternative take, christianaguilera-foundry@b2626d0 contains the changes to replace the use of half in NVIDIA Cg shaders with float. We are slightly more confident about this change, as it's less likely to get wrong. We noticed only this language uses the 16-bit floating type; for the rest of the languages, 32-bit floating types are used.

Thoughts?

@carolalynn carolalynn added the Needs Discussion Needs discussion before implmentation, which could result in changes, or a decision not to proceed. label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion Needs discussion before implmentation, which could result in changes, or a decision not to proceed.
Projects
None yet
Development

No branches or pull requests

2 participants