emilk / egui

egui: an easy-to-use immediate mode GUI in Rust that runs on both web and native
https://www.egui.rs/
Apache License 2.0
22.07k stars 1.59k forks source link

Simplify egui wgpu fragment shader, remove redundant color conversions #3168

Open PPakalns opened 1 year ago

PPakalns commented 1 year ago

By default on desktop gpu devices egui_wgpu / wgpu outputs preferred target format wgpu::TextureFormat::Bgra8Unorm.

Then for rendering it renders to this texture in gamma space (srgb ?)

https://github.com/emilk/egui/blob/master/crates/egui-wgpu/src/egui.wgsl#L84C1-L91C2

@fragment
fn fs_main_gamma_framebuffer(in: VertexOutput) -> @location(0) vec4<f32> {
    // We always have an sRGB aware texture at the moment.
    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;
}

As i understand it happens like this:

Similar thing happens for user registered textures. I need to provide view to srgb texture with Bgra8UnormSrgb format so that shader receives values in linear format.

It looks like it causes double conversion from srgb -> rgb -> srgb. Couldn't this be avoided by only storing textures in srgb format and using everywhere Bgra8Unorm texture view format view which doesn't cause conversion, therefore no need to convert from linear to gamma space in fragment shader.

As I understand, texture formats and their views are only for automatic color conversion in shader and doesn't represent the real target texture color format. Is this correct understanding? At least it looks like wgpu preferred target texture is in Bgra8Unorm format but expects textures in gamma (srgb) outputted to it.

Summary:

It looks that we can simplify fragment shader:

Possible problems

One case where this doesn't work is if someone wants to mix textures that are stored in bgr, srgb color spaces. What are the best practices, are these color conversions in shader costly?

PPakalns commented 1 year ago

Good description about gpu color conversion: https://github.com/gfx-rs/wgpu/wiki/Texture-Color-Formats-and-Srgb-conversions

Wumpf commented 1 year ago

arguably there needs to be 4 combination:

This is only that way egui can deal with incoming textures (which are at least right now to be expected to be 8UnormSrgb) in the correct way. This wouldn't give us removing any of the color conversion code but it would allow egui_wgpu to choose the path with the least conversions for textures it created itself.

Any of these conversions is fairly cheap in the grad scheme of things so I wouldn't worry about it too much. One could easily argue that all this would just unnecessarily bloat and complicate things

Btw. the reason why egui prefers non-gamma-aware framebuffers and does blending in gamma space is detailed here https://github.com/emilk/egui/pull/2071