Introduce dithering to reduce banding #4497

jwagner · 2024-05-14T17:44:56Z

This PR introduces dithering in the egui_glow and egui_wgpu backends to reduce banding artifacts.

It's based on the approach mentioned in #4493 with the small difference that the amount of noise is scaled down slightly to avoid dithering colors that can be represented exactly. This keeps flat surfaces clean.

Exaggerated dithering to show what is happening:

Subtle dithering as commited.

Closes #4493

murl-digital

LGTM, I tested this with the baseview windowing backend, with is glow based. Taking a screenshot of a gradient and zooming all the way in shows me the dithering's there:

emilk · 2024-05-27T14:27:04Z

crates/egui-wgpu/src/egui.wgsl

@@ -87,5 +114,6 @@ fn fs_main_gamma_framebuffer(in: VertexOutput) -> @location(0) vec4<f32> {
    let tex_linear = textureSample(r_tex_color, r_tex_sampler, in.tex_coord);
    let tex_gamma = gamma_from_linear_rgba(tex_linear);
    let out_color_gamma = in.color * tex_gamma;
-    return out_color_gamma;
+    let out_color_dithered = vec4<f32>(dither_interleaved(out_color_gamma.rgb, 256.0, in.position * r_locals.pixels_per_point), out_color_gamma.a);


Should have the same comments as the code above, and probably a linebreak

emilk · 2024-05-27T14:27:19Z

crates/egui-wgpu/src/egui.wgsl

+    let out_color_linear = linear_from_gamma_rgb(out_color_gamma.rgb);
+    // Dither the float color down to eight bits to reduce banding.
+    // This step is optional for egui backends.
+    let out_color_dithered = dither_interleaved(out_color_linear, 256.0, in.position);


Are you sure we should apply it to the linear color and not the gamma color? 🤔

I agree, this should be done on the gamma color. It's important to know the context of when this fragment shader is used:

egui/crates/egui-wgpu/src/renderer.rs

Line 319 in ff7a383

"fs_main_linear_framebuffer"

it is enabled in the case of using an srgb converting target. That means whatever float value we emit here will be upon write converted to an srgb value. I.e. as-if the output of this method was wrapped in another call of gamma_from_linear_rgba.

Therefore, we have to make sure that all the meaningful value changes happen on the gamma color.

Note that it would be an entirely different story if we're outputing anything that isn't ending up on a wgpu::Surface that is targeting a srgb interpreting monitor/window. Meaning that all of this breaks down for HDR.

I assumed that the output buffer was linear in that case dithering the linear color is what is wanted. If it's converted back to srgb before storage, it isn't optimal. I'll need to double check that.

Just saw the comment be @Wumpf bellow, so you are right, this is off.

crates/egui-wgpu/src/renderer.rs

Wumpf

great addition, however I think there should be an easy way to disable dithering:

this is srgb dithering and doesn't make sense if someone wants to use an HDR target (e.g. WebGPU can also support P3 instead of srgb)
there may be corner cases where a user doesn't actually want to dither everything drawn with egui indiscriminately as the added noise may destroy otherwise sharp corners

Wumpf · 2024-05-27T16:05:24Z

crates/egui-wgpu/src/egui.wgsl

+    let out_color_linear = linear_from_gamma_rgb(out_color_gamma.rgb);
+    // Dither the float color down to eight bits to reduce banding.
+    // This step is optional for egui backends.
+    let out_color_dithered = dither_interleaved(out_color_linear, 256.0, in.position);


I agree, this should be done on the gamma color. It's important to know the context of when this fragment shader is used:

egui/crates/egui-wgpu/src/renderer.rs

Line 319 in ff7a383

"fs_main_linear_framebuffer"

it is enabled in the case of using an srgb converting target. That means whatever float value we emit here will be upon write converted to an srgb value. I.e. as-if the output of this method was wrapped in another call of gamma_from_linear_rgba.

Therefore, we have to make sure that all the meaningful value changes happen on the gamma color.

Note that it would be an entirely different story if we're outputing anything that isn't ending up on a wgpu::Surface that is targeting a srgb interpreting monitor/window. Meaning that all of this breaks down for HDR.

crates/egui-wgpu/src/egui.wgsl

crates/egui-wgpu/src/renderer.rs

jwagner · 2024-05-27T16:23:07Z

@Wumpf @emilk for disabling dithering, would a feature on the crate do the job or should this be a runtime option?

emilk · 2024-05-27T17:24:55Z

I think a runtime feature flag is likely superior, as it is simpler, and lets the same user use egui_wgpu with different options depending on their render target, which may depend on the adapter capabilities.

Checking a bool in the shader should be fine

jwagner · 2024-05-27T22:11:23Z

I'll do a revision of the PR adding an option and fixing the coordinates in wgpu. I'm not certain where to add it yet. Adding it to native / web options in eframe, and then an implementation specific option in the specific backend (WgpuConfiguration for wgpu , EguiGlow::new for glow) seems about right.

If you'd like it to be added another way, just let me know.

murl-digital · 2024-05-28T14:25:46Z

Adding it to native / web options in eframe, and then an implementation specific option in the specific backend (WgpuConfiguration for wgpu , EguiGlow::new for glow) seems about right.

That works for me, and that's probably the best way to do it, unless you want to mess with feature flags to reduce a tiny amount of runtime overhead

jwagner · 2024-05-30T18:56:35Z

I got a first draft of configurable dithering to work but I'm running out of time for today. I changed to state of the PR back to draft for now.

jwagner · 2024-06-01T16:40:40Z

I updated and cleaned up the code and will mark the PR as 'Ready for review' again.

Testing performed

I tested the demo app using both wgpu and glow, with dithering enabled and disabled, on two different systems (Linux and Mac), as well as on the web using Chrome and Firefox. The dithering works as expected in all cases. While the results from wgpu and glow are not identical, they are very close.

There were some minute differences with dithering disabled as well. So I guess producing identical output between different backends isn't a requirement (and likely not a reasonable goal across systems anyways).

In addition to the demo app, I tested the proposed changes with one of my own applications.

Potential Optimizations

egui_glow::Painter::new, RenderState::create and Renderer::new, now all take quite a few arguments. If desired I can change them to take a struct (with a default) instead.

The implementation of the condition is different for wgpu and glow.

The glow implementation makes use of the glsl preprocessor using a define and if, similar to the existing code for SRGB_TEXTURES. This should result in zero shading cost when dithering is disabled.

The wgpu implementation uses a uniform instead. At least as far as I know wgsl doesn't have a preprocessor and I wasn't to keen to introduce some code to process the shader source. Instead it's using a uniform and a condition in the shader as suggested by @emilk. I'm not sure about the optimizations applied in wgpu, let alone it's various backends but this is likely very cheap (coherent branching) but not zero cost in either case.

Lastly it would be possible to make the amount if dithering configurable, for instance for using egui in an application with a bit depth lower than 8 bit. I don't think this is a very common use case though.

Wumpf · 2024-06-02T15:27:55Z

At least as far as I know wgsl doesn't have a preprocessor and I wasn't to keen to introduce some code to process the shader source.

there's no preprocessor, but this is a great usecase for a pipeline-overridable constant! :) There's an example in the wgpu 0.20.0 release notes.
Unfortunately, they got only added in wgpu 0.20 and egui had to back out of 0.20 for the moment. So would be good to leave a todo note towards (and subsequent patch in) #4560
(not a big deal anyways, this kind of static condition is extremely cheap)

jwagner · 2024-06-03T16:10:43Z

there's no preprocessor, but this is a great usecase for a pipeline-overridable constant! :) There's an example in the wgpu 0.20.0 release notes. Unfortunately, they got only added in wgpu 0.20 and egui had to back out of 0.20 for the moment. So would be good to leave a todo note towards (and subsequent patch in) #4560 (not a big deal anyways, this kind of static condition is extremely cheap)

That sounds like a great solution. I'll need to do a bit more digging to figure out what it does in practice. If it's ok, I'll do a follow up on this PR once it is merged.

Wumpf

lgtm except docs!

crates/eframe/src/epi.rs

jwagner · 2024-07-07T08:54:42Z

Sorry for the delayed response. Life happened.

I rebased the branch and applied the suggestions. I'm not sure if the more detailed descriptions should maybe go into the lower level crates (egui_glow, egui-wgpu) rather than eframe since users of those crates are more likely to care about the specifics.

Another consideration is api compatibility. I guess we don't want to promise that dithering true will always dither to 8 bit. So maybe _Currently_ dithering assumes an sRGB output and thus will apply noise to any input value that lies between would be a slightly softer comment.

Both are minor concerns though so I'd be happy to get the change merged as is.

Thank you for your patience @Wumpf @emilk :)

Wumpf

lgtm!

Still torn about having it opt-out instead of opt-in, but I wrote earlier to leave it be, so let's just do that (:

now that wgpu 0.20 is in we could use pipeline constants, but that constant buffer value won't hurt anyone 🤷

Wumpf · 2024-07-08T07:57:03Z

Re api compatibility: I think for the renderers to handle hdr output correctly some other stuff needs to happen so not too concerned, whoever looks into that can also do something about that. Worst case they have to break the variable name to make it more obvious

This PR introduces dithering in the egui_glow and egui_wgpu backends to reduce banding artifacts. It's based on the approach mentioned in emilk#4493 with the small difference that the amount of noise is scaled down slightly to avoid dithering colors that can be represented exactly. This keeps flat surfaces clean. Exaggerated dithering to show what is happening: ![Screenshot from 2024-05-14 19-09-48](https://github.com/emilk/egui/assets/293536/75782b83-9023-4cb2-99f7-a24e15fdefcc) Subtle dithering as commited. ![Screenshot from 2024-05-14 19-13-40](https://github.com/emilk/egui/assets/293536/eb904698-a6ec-494a-952b-447e9a49bfda) Closes emilk#4493

murl-digital approved these changes May 15, 2024

View reviewed changes

emilk reviewed May 27, 2024

View reviewed changes

emilk requested a review from Wumpf May 27, 2024 14:28

Wumpf requested changes May 27, 2024

View reviewed changes

jwagner marked this pull request as draft May 30, 2024 18:54

jwagner force-pushed the dither branch from 1bc5542 to 00a6a8b Compare May 30, 2024 21:25

jwagner marked this pull request as ready for review June 1, 2024 16:40

emilk requested a review from Wumpf June 5, 2024 07:26

Wumpf requested changes Jun 11, 2024

View reviewed changes

crates/eframe/src/epi.rs Outdated Show resolved Hide resolved

crates/eframe/src/epi.rs Outdated Show resolved Hide resolved

emilk added egui_glow Relates to running egui_glow on native egui-wgpu labels Jun 19, 2024

jwagner added 7 commits July 7, 2024 08:36

introduce dithering to reduce banding

434f144

introduce dithering in egui_glow

2728802

Make dithering configurable

026b8d0

use native_options.dithering when creating glow painter

bc94247

Remove incorrect comment

4d27863

Apply dithering to gamma encoded values in fs_main_linear_framebuffer

43a1f1f

Cleanup, Comments, formatting

636b2a0

jwagner force-pushed the dither branch from 62754e1 to 636b2a0 Compare July 7, 2024 08:39

Review feedback

1e9564d

Wumpf self-requested a review July 8, 2024 07:46

Wumpf approved these changes Jul 8, 2024

View reviewed changes

Wumpf merged commit b283b8a into emilk:master Jul 8, 2024
19 checks passed

evan-barentin mentioned this pull request Oct 15, 2024

Update egui to version 0.29 and glow to 0.14 asny/three-d#498

Merged

thatcomputerguy0101 mentioned this pull request Oct 30, 2024

Update egui to 0.29 and glow to 0.14 asny/three-d#505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce dithering to reduce banding #4497

Introduce dithering to reduce banding #4497

jwagner commented May 14, 2024

murl-digital left a comment

emilk May 27, 2024

emilk May 27, 2024

Wumpf May 27, 2024

jwagner May 27, 2024

jwagner May 27, 2024

Wumpf left a comment

Wumpf May 27, 2024

jwagner commented May 27, 2024

emilk commented May 27, 2024

jwagner commented May 27, 2024 •

edited

Loading

murl-digital commented May 28, 2024

jwagner commented May 30, 2024

jwagner commented Jun 1, 2024

Wumpf commented Jun 2, 2024

jwagner commented Jun 3, 2024

Wumpf left a comment

jwagner commented Jul 7, 2024

Wumpf left a comment

Wumpf commented Jul 8, 2024

Introduce dithering to reduce banding #4497

Introduce dithering to reduce banding #4497

Conversation

jwagner commented May 14, 2024

murl-digital left a comment

Choose a reason for hiding this comment

emilk May 27, 2024

Choose a reason for hiding this comment

emilk May 27, 2024

Choose a reason for hiding this comment

Wumpf May 27, 2024

Choose a reason for hiding this comment

jwagner May 27, 2024

Choose a reason for hiding this comment

jwagner May 27, 2024

Choose a reason for hiding this comment

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf May 27, 2024

Choose a reason for hiding this comment

jwagner commented May 27, 2024

emilk commented May 27, 2024

jwagner commented May 27, 2024 • edited Loading

murl-digital commented May 28, 2024

jwagner commented May 30, 2024

jwagner commented Jun 1, 2024

Testing performed

Potential Optimizations

Wumpf commented Jun 2, 2024

jwagner commented Jun 3, 2024

Wumpf left a comment

Choose a reason for hiding this comment

jwagner commented Jul 7, 2024

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf commented Jul 8, 2024

jwagner commented May 27, 2024 •

edited

Loading