From cd70b9d73083db910486dbff57cff8e7fcbabce9 Mon Sep 17 00:00:00 2001 From: Sergey Sharybin Date: Fri, 9 May 2025 23:20:35 +0200 Subject: [PATCH] Fix #138637: EXR Sequencer preview is broken The issue was caused by the shader always applying exposure even if it is 1. When the input image had negative values this lead to nan values to the input of the shader which converts to the display space. Solved by only doing to-scene-linear transform in the shader which does this. Caused by 18110744a2 Pull Request: https://projects.blender.org/blender/blender/pulls/138666 --- .../opencolorio/OCIO_gpu_shader_binder.hh | 5 +- .../fallback/fallback_gpu_shader_binder.cc | 4 +- .../opencolorio/intern/gpu_shader_binder.cc | 10 ++- .../libocio/libocio_gpu_shader_binder.cc | 80 ++++++++++--------- .../libocio/libocio_gpu_shader_binder.hh | 5 +- .../gpu_shader_display_transform_frag.glsl | 6 ++ 6 files changed, 66 insertions(+), 44 deletions(-) diff --git a/source/blender/imbuf/opencolorio/OCIO_gpu_shader_binder.hh b/source/blender/imbuf/opencolorio/OCIO_gpu_shader_binder.hh index f64885a8810..c02a200120b 100644 --- a/source/blender/imbuf/opencolorio/OCIO_gpu_shader_binder.hh +++ b/source/blender/imbuf/opencolorio/OCIO_gpu_shader_binder.hh @@ -17,6 +17,8 @@ #include #include +#include "BLI_array.hh" +#include "BLI_span.hh" #include "BLI_string_ref.hh" struct CurveMapping; @@ -104,7 +106,8 @@ class GPUShaderBinder { * Returns true if the shader was successfully created. */ static bool create_gpu_shader(internal::GPUDisplayShader &display_shader, - StringRefNull fragment_source); + StringRefNull fragment_source, + Span> additional_defines); }; } // namespace blender::ocio diff --git a/source/blender/imbuf/opencolorio/intern/fallback/fallback_gpu_shader_binder.cc b/source/blender/imbuf/opencolorio/intern/fallback/fallback_gpu_shader_binder.cc index 5af57f7f199..f0665e3f86e 100644 --- a/source/blender/imbuf/opencolorio/intern/fallback/fallback_gpu_shader_binder.cc +++ b/source/blender/imbuf/opencolorio/intern/fallback/fallback_gpu_shader_binder.cc @@ -78,7 +78,7 @@ void FallbackGPUShaderBinder::construct_display_shader( { const std::string fragment_source = generate_display_fragment_source(display_shader); - if (!create_gpu_shader(display_shader, fragment_source)) { + if (!create_gpu_shader(display_shader, fragment_source, {})) { display_shader.is_valid = false; return; } @@ -93,7 +93,7 @@ void FallbackGPUShaderBinder::construct_scene_linear_shader( const std::string fragment_source = generate_scene_linear_fragment_source(display_shader); - if (!create_gpu_shader(display_shader, fragment_source)) { + if (!create_gpu_shader(display_shader, fragment_source, {{"USE_TO_SCENE_LINEAR_ONLY", ""}})) { display_shader.is_valid = false; } } diff --git a/source/blender/imbuf/opencolorio/intern/gpu_shader_binder.cc b/source/blender/imbuf/opencolorio/intern/gpu_shader_binder.cc index b374961267f..880ca1d4979 100644 --- a/source/blender/imbuf/opencolorio/intern/gpu_shader_binder.cc +++ b/source/blender/imbuf/opencolorio/intern/gpu_shader_binder.cc @@ -444,8 +444,10 @@ void GPUShaderBinder::unbind() const immUnbindProgram(); } -bool GPUShaderBinder::create_gpu_shader(internal::GPUDisplayShader &display_shader, - StringRefNull fragment_source) +bool GPUShaderBinder::create_gpu_shader( + internal::GPUDisplayShader &display_shader, + StringRefNull fragment_source, + const Span> additional_defines) { using namespace blender::gpu::shader; @@ -454,6 +456,10 @@ bool GPUShaderBinder::create_gpu_shader(internal::GPUDisplayShader &display_shad ShaderCreateInfo info("OCIO_Display"); + for (const auto &additional_define : additional_defines) { + info.define(additional_define[0], additional_define[1]); + } + /* Work around OpenColorIO not supporting latest GLSL yet. */ info.define("texture1D", "texture"); info.define("texture2D", "texture"); diff --git a/source/blender/imbuf/opencolorio/intern/libocio/libocio_gpu_shader_binder.cc b/source/blender/imbuf/opencolorio/intern/libocio/libocio_gpu_shader_binder.cc index eb6e0d509b7..0a760941660 100644 --- a/source/blender/imbuf/opencolorio/intern/libocio/libocio_gpu_shader_binder.cc +++ b/source/blender/imbuf/opencolorio/intern/libocio/libocio_gpu_shader_binder.cc @@ -39,11 +39,6 @@ static ConstProcessorRcPtr create_to_display_processor( return create_ocio_display_processor(config, display_parameters); } -static ConstProcessorRcPtr create_noop_processor(const ConstConfigRcPtr &ocio_config) -{ - return create_ocio_processor(ocio_config, ROLE_SCENE_LINEAR, ROLE_SCENE_LINEAR); -} - static bool add_gpu_uniform(internal::GPUTextures &textures, const GpuShaderDescRcPtr &shader_desc, const int index) @@ -168,39 +163,50 @@ static bool create_gpu_textures(internal::GPUTextures &textures, void LibOCIOGPUShaderBinder::construct_shader_for_processors( internal::GPUDisplayShader &display_shader, - ConstProcessorRcPtr &processor_to_scene_linear, - ConstProcessorRcPtr processor_to_display) const + const ConstProcessorRcPtr &processor_to_scene_linear, + const ConstProcessorRcPtr &processor_to_display, + const Span> additional_defines) const { - GpuShaderDescRcPtr shaderdesc_to_scene_linear = GpuShaderDesc::CreateShaderDesc(); - shaderdesc_to_scene_linear->setLanguage(GPU_LANGUAGE_GLSL_1_3); - shaderdesc_to_scene_linear->setFunctionName("OCIO_to_scene_linear"); - shaderdesc_to_scene_linear->setResourcePrefix("to_scene"); - processor_to_scene_linear->getDefaultGPUProcessor()->extractGpuShaderInfo( - shaderdesc_to_scene_linear); - shaderdesc_to_scene_linear->finalize(); + std::string fragment_source; - GpuShaderDescRcPtr shaderdesc_to_display = GpuShaderDesc::CreateShaderDesc(); - shaderdesc_to_display->setLanguage(GPU_LANGUAGE_GLSL_1_3); - shaderdesc_to_display->setFunctionName("OCIO_to_display"); - shaderdesc_to_display->setResourcePrefix("to_display"); - processor_to_display->getDefaultGPUProcessor()->extractGpuShaderInfo(shaderdesc_to_display); - shaderdesc_to_display->finalize(); + GpuShaderDescRcPtr shaderdesc_to_scene_linear; + if (processor_to_scene_linear) { + shaderdesc_to_scene_linear = GpuShaderDesc::CreateShaderDesc(); + shaderdesc_to_scene_linear->setLanguage(GPU_LANGUAGE_GLSL_1_3); + shaderdesc_to_scene_linear->setFunctionName("OCIO_to_scene_linear"); + shaderdesc_to_scene_linear->setResourcePrefix("to_scene"); + processor_to_scene_linear->getDefaultGPUProcessor()->extractGpuShaderInfo( + shaderdesc_to_scene_linear); + shaderdesc_to_scene_linear->finalize(); - /* Create GPU textures. */ - if (!create_gpu_textures(display_shader.textures, shaderdesc_to_scene_linear) || - !create_gpu_textures(display_shader.textures, shaderdesc_to_display)) - { - display_shader.is_valid = false; - return; + if (!create_gpu_textures(display_shader.textures, shaderdesc_to_scene_linear)) { + display_shader.is_valid = false; + return; + } + + fragment_source += shaderdesc_to_scene_linear->getShaderText(); + fragment_source += "\n"; } - std::string fragment_source; - fragment_source += shaderdesc_to_scene_linear->getShaderText(); - fragment_source += "\n"; - fragment_source += shaderdesc_to_display->getShaderText(); - fragment_source += "\n"; + GpuShaderDescRcPtr shaderdesc_to_display; + if (processor_to_display) { + shaderdesc_to_display = GpuShaderDesc::CreateShaderDesc(); + shaderdesc_to_display->setLanguage(GPU_LANGUAGE_GLSL_1_3); + shaderdesc_to_display->setFunctionName("OCIO_to_display"); + shaderdesc_to_display->setResourcePrefix("to_display"); + processor_to_display->getDefaultGPUProcessor()->extractGpuShaderInfo(shaderdesc_to_display); + shaderdesc_to_display->finalize(); - if (!create_gpu_shader(display_shader, fragment_source)) { + if (!create_gpu_textures(display_shader.textures, shaderdesc_to_display)) { + display_shader.is_valid = false; + return; + } + + fragment_source += shaderdesc_to_display->getShaderText(); + fragment_source += "\n"; + } + + if (!create_gpu_shader(display_shader, fragment_source, additional_defines)) { display_shader.is_valid = false; return; } @@ -223,7 +229,8 @@ void LibOCIOGPUShaderBinder::construct_display_shader( return; } - construct_shader_for_processors(display_shader, processor_to_scene_linear, processor_to_display); + construct_shader_for_processors( + display_shader, processor_to_scene_linear, processor_to_display, {}); } void LibOCIOGPUShaderBinder::construct_scene_linear_shader( @@ -234,14 +241,13 @@ void LibOCIOGPUShaderBinder::construct_scene_linear_shader( ConstProcessorRcPtr processor_to_scene_linear = create_to_scene_linear_processor(ocio_config, display_shader); - ConstProcessorRcPtr processor_to_display = create_noop_processor(ocio_config); - - if (!processor_to_scene_linear || !processor_to_display) { + if (!processor_to_scene_linear) { display_shader.is_valid = false; return; } - construct_shader_for_processors(display_shader, processor_to_scene_linear, processor_to_display); + construct_shader_for_processors( + display_shader, processor_to_scene_linear, nullptr, {{"USE_TO_SCENE_LINEAR_ONLY", ""}}); } } // namespace blender::ocio diff --git a/source/blender/imbuf/opencolorio/intern/libocio/libocio_gpu_shader_binder.hh b/source/blender/imbuf/opencolorio/intern/libocio/libocio_gpu_shader_binder.hh index ed47fa8ac8d..1610c571c2a 100644 --- a/source/blender/imbuf/opencolorio/intern/libocio/libocio_gpu_shader_binder.hh +++ b/source/blender/imbuf/opencolorio/intern/libocio/libocio_gpu_shader_binder.hh @@ -23,8 +23,9 @@ class LibOCIOGPUShaderBinder : public GPUShaderBinder { private: void construct_shader_for_processors( internal::GPUDisplayShader &display_shader, - OCIO_NAMESPACE::ConstProcessorRcPtr &processor_to_scene_linear, - OCIO_NAMESPACE::ConstProcessorRcPtr processor_to_display) const; + const OCIO_NAMESPACE::ConstProcessorRcPtr &processor_to_scene_linear, + const OCIO_NAMESPACE::ConstProcessorRcPtr &processor_to_display, + Span> additional_defines) const; protected: void construct_display_shader(internal::GPUDisplayShader &display_shader) const override; diff --git a/source/blender/imbuf/opencolorio/shaders/gpu_shader_display_transform_frag.glsl b/source/blender/imbuf/opencolorio/shaders/gpu_shader_display_transform_frag.glsl index 9c05533a470..04fb6e259d0 100644 --- a/source/blender/imbuf/opencolorio/shaders/gpu_shader_display_transform_frag.glsl +++ b/source/blender/imbuf/opencolorio/shaders/gpu_shader_display_transform_frag.glsl @@ -204,6 +204,11 @@ float4 OCIO_ProcessColor(float4 col, float4 col_overlay) /* Convert to scene linear (usually a no-op). */ col = OCIO_to_scene_linear(col); + /* Skip the rest of the transformation when the shader is only used to transform the input + * texture to the scene linear space. + * This will simplify the shader code, potentially making it faster. More importantly doing so + * avoids math that might lead to nan values: such as applying exposure on negative values. */ +#ifndef USE_TO_SCENE_LINEAR_ONLY /* Apply exposure and white balance in scene linear. */ col = parameters.scene_linear_matrix * col; @@ -239,6 +244,7 @@ float4 OCIO_ProcessColor(float4 col, float4 col_overlay) uint2 texel = get_pixel_coord(image_texture, texCoord_interp.st); col = apply_dither(col, texel); } +#endif return col; }