AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.76k stars 434 forks source link

Regression: Excessive precision loss in NVIDIA Cg shaders #1884

Open christianaguilera-foundry opened 10 months ago

christianaguilera-foundry commented 10 months ago

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 commented 10 months ago

In https://github.com/christianaguilera-foundry/OpenColorIO/commit/195a7711c00be3c8b8da06b650b251a94b115493, 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, https://github.com/christianaguilera-foundry/OpenColorIO/commit/b2626d034683b441f94c68d412b5f782180784d1 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?