Always perform color correction and debanding on nonlinear sRGB values.

[allenwp]

Fixes #107730

Screenshots at 4x magnification (view in a dark environment/surround to see the detail):

Notes to reviewers:

• The intent is for debanding to only and always be applied after any encoding transformation (linear/nonlinear) has been applied and as the final step before quantization to integer values. Please let me know if you can think of or discover a scenario where this may not be the case with this PR!
• Please let me know if you can think of any other ways to improve performance
• My first time working with a lot of this code, so I might be making bad assumptions
• I haven't tested with XR and I'm not sure that debanding is always being applied correctly as the last step before quantization to integer values

My test project
color-correction-debanding-fixed.zip

[allenwp]

@beicause You mentioned an idea about how to better address the color correction bug. Thanks for sharing this! Please feel free to share more details here or open a new PR that builds off this one with me as a co-contributor if you'd like.

[beicause]

Optimizes out one srgb->linear conversion by sampling sRGB texture:

diff --git a/servers/rendering/renderer_rd/renderer_scene_render_rd.cpp b/servers/rendering/renderer_rd/renderer_scene_render_rd.cpp
index 2a037d241e..4d30f625ef 100644
--- a/servers/rendering/renderer_rd/renderer_scene_render_rd.cpp
+++ b/servers/rendering/renderer_rd/renderer_scene_render_rd.cpp
@@ -665,6 +665,7 @@ void RendererSceneRenderRD::_render_buffers_post_process_and_tonemap(const Rende
 		tonemap.use_color_correction = false;
 		tonemap.use_1d_color_correction = false;
 		tonemap.color_correction_texture = texture_storage->texture_rd_get_default(RendererRD::TextureStorage::DEFAULT_RD_TEXTURE_3D_WHITE);
+		tonemap.convert_to_srgb = !texture_storage->render_target_is_using_hdr(render_target);
 
 		if (can_use_effects && p_render_data->environment.is_valid()) {
 			tonemap.use_bcs = environment_get_adjustments_enabled(p_render_data->environment);
@@ -674,15 +675,13 @@ void RendererSceneRenderRD::_render_buffers_post_process_and_tonemap(const Rende
 			if (environment_get_adjustments_enabled(p_render_data->environment) && environment_get_color_correction(p_render_data->environment).is_valid()) {
 				tonemap.use_color_correction = true;
 				tonemap.use_1d_color_correction = environment_get_use_1d_color_correction(p_render_data->environment);
-				tonemap.color_correction_texture = texture_storage->texture_get_rd_texture(environment_get_color_correction(p_render_data->environment));
+				tonemap.color_correction_texture = texture_storage->texture_get_rd_texture(environment_get_color_correction(p_render_data->environment), !tonemap.convert_to_srgb);
 			}
 		}
 
 		tonemap.luminance_multiplier = _render_buffers_get_luminance_multiplier();
 		tonemap.view_count = rb->get_view_count();
 
-		tonemap.convert_to_srgb = !texture_storage->render_target_is_using_hdr(render_target);
-
 		RID dest_fb;
 		if (spatial_upscaler != nullptr || use_smaa) {
 			// If we use a spatial upscaler to upscale or SMAA to antialias we need to write our result into an intermediate buffer.
diff --git a/servers/rendering/renderer_rd/shaders/effects/tonemap.glsl b/servers/rendering/renderer_rd/shaders/effects/tonemap.glsl
index c4ae4cb8b0..84c4a0fca8 100644
--- a/servers/rendering/renderer_rd/shaders/effects/tonemap.glsl
+++ b/servers/rendering/renderer_rd/shaders/effects/tonemap.glsl
@@ -911,9 +911,6 @@ void main() {
 			color.rgb = linear_to_srgb(color.rgb);
 		}
 		color.rgb = apply_color_correction(color.rgb);
-		if (!convert_to_srgb) {
-			color.rgb = srgb_to_linear(color.rgb);
-		}
 	}
 
 	if (bool(params.flags & FLAG_USE_DEBANDING) && convert_to_srgb) {

[allenwp]

Optimizes out one srgb->linear conversion by sampling sRGB texture:
...

This seems to work well -- very clever! Thanks @beicause!

[allenwp]

I've finished work on this, so it's now ready for review. (Sorry about the mistaken "ready for review" I made earlier!)

[beicause]

Debanding is a per-viewport setting. I don't know why it should change to a RendererCompositor setting here.

I think debanding has nothing to do with color space. It can be performed in linear space as long as it's done before writing the results to a lower-precision buffer. See also #87350.

[allenwp]

Hmm, I hadn’t thought about how debanding is exposed to scripting as a property of Viewport. The current state of this PR makes that property do nothing when HDR 2D is enabled and also provides no method for changing the setting at runtime.

I’m not sure exactly what to do about this, so let me know if you have thoughts on how to handle this scenario.

Debanding is a per-viewport setting. I don't know why it should change to a RendererCompositor setting here.

I think debanding has nothing to do with color space. It can be performed in linear space as long as it's done before writing the results to a lower-precision buffer. See also #87350.

I will take a look into #87350 and post on that PR if it exhibits the same issues as seen in #107730. The specific dither function that is used for debanding is designed to be applied to nonlinear encoded float values of the final encoding directly before quantization to an 8 bit buffer. Details about this and the consequences of applying it to an encoding that is different than the one used immediately before quantization (i.e. linear encoding) is described in #107730.

The specific dithering function that Godot uses cannot be used on linear values. If you want to use a debanding technique that works on linear encoded values, you need to use a different function such as the one presented by Timothy Lottes in his GDC talk (See part 3: transfer functions and quantization).

[beicause]

#87350 uses the same debanding algorithm with the viewport debanding. But sky shader uses a different one:

godot/servers/rendering/renderer_rd/shaders/environment/sky.glsl

Lines 139 to 146 in 88b9932

	#ifdef USE_DEBANDING
	// https://www.iryoku.com/next-generation-post-processing-in-call-of-duty-advanced-warfare
	vec3 interleaved_gradient_noise(vec2 pos) {
	const vec3 magic = vec3(0.06711056f, 0.00583715f, 52.9829189f);
	float res = fract(magic.z * fract(dot(pos, magic.xy))) * 2.0 - 1.0;
	return vec3(res, -res, res) / 255.0;
	}
	#endif

I'm not sure if these algorithms works in linear space (dividing by 255.0 seems a bit magical), but it's worth taking a look.

[allenwp]

#87350 uses the same debanding algorithm with the viewport debanding. But sky shader uses a different one:

godot/servers/rendering/renderer_rd/shaders/environment/sky.glsl

Lines 139 to 146 in 88b9932

	#ifdef USE_DEBANDING
	// https://www.iryoku.com/next-generation-post-processing-in-call-of-duty-advanced-warfare
	vec3 interleaved_gradient_noise(vec2 pos) {
	const vec3 magic = vec3(0.06711056f, 0.00583715f, 52.9829189f);
	float res = fract(magic.z * fract(dot(pos, magic.xy))) * 2.0 - 1.0;
	return vec3(res, -res, res) / 255.0;
	}
	#endif

I'm not sure if these algorithms works in linear space (dividing by 255.0 seems a bit magical), but it's worth taking a look.

There's actually a PDF linked from the Godot docs that talks about what floating point encoding your dithering should be applied in with these sorts of dithering effects: https://loopit.dk/banding_in_games.pdf (see page 35 onward where it states "convert first, dither afterwards… dither in whatever space the color will be quantized in."). I think it's easier to just read the dithering code to understand that it needs to be applied to nonlinear encoded values, but if anyone wants some "literature" on the subject, there it is.

I will eventually look into the sky shader's debanding, but I noticed that the dithering is scaled by a luminance_multiplier, which may help reduce the problems associated with applying the dither to a linear encoding (maybe???). So this is probably best addressed in a different issue with a different PR that explores it in more depth.

I also now realize there is quite a bit in the docs that needs updating with this PR as well. I'll continue to update it to address all these cases.

[allenwp]

Debanding is a per-viewport setting. I don't know why it should change to a RendererCompositor setting here.

I've fixed this so that debanding is still a property of the viewport and also always applied as the last step before 8-bit quantization.

Just need to figure out how to add this to XR now...

Edit: Hmm. It seems it would be better to store use_debanding in the RenderTarget alongside of use_hdr...

[allenwp]

OK, I've moved use_debanding to be a part of RenderTarget, just like use_hdr... so... I think this means the debanding will be correctly applied in XR scenarios as well(?) Please let me know if you can find any places where debanding may not be correctly applied or where other transformations may happen between debanding and quantization.