imgui-rs / imgui-rs

Rust bindings for Dear ImGui
Apache License 2.0
2.66k stars 362 forks source link

Reconsider sRGB handling in `imgui-glow-renderer` #548

Open thomcc opened 3 years ago

thomcc commented 3 years ago

imgui-glow-renderer goes through a lot of effort for proper sRGB handling, and has no setting to disable or relax it, even if it's paying a very steep performance price.

Specifically, this issue is mainly concerned about the output_srgb=true case, where we don't have a sRGB framebuffer to work with.

Is the work worth it?

Concretely, I'm pretty sure that there are three places where the imgui renderer's perform color operations which are only correct in a color space with a linear transfer function.

So, let's imagine that we did none of it, and determine which cases would be wrong. That is, assume we:

  1. Aren't drawing to an sRGB framebuffer.
  2. Aren't using sRGB textures
  3. Aren't converting vertex colors from sRGB -> to linear
  4. Aren't converting doing any linear -> sRGB conversion on output

The following things would be different:

  1. The alpha blending implicitly by the GPU when outputting a fragment with partial transparency.
    • fromLinear(over(toLinear(a), toLinear(b))) != over(a, b)
  2. The gradient from vertex colors, if there are adjacent vertices with different colors.
    • fromLinear(lerp(toLinear(a), toLinear(b), t)) != lerp(a, b, t)
  3. The result of the fragment_color * tex_color
    • fromLinear(toLinear(a) * toLinear(b)) != a * b

Subjective judging

  1. (alpha blending) This one seems bad, since we render with a lot of partial transparency from fonts...

    What's more, it's probably also the most obvious instance of "this is describing physical operation (transparency) which is no longer modeled correctly". So, sound's bad...

    Actually, you've fallen for my ruse! This is a bug the existing renderer has too (if output_srgb=true), since the GPU only gets sRGB values but has to do alpha blending between them.

    There's no[^1] fix for this unless rendering to an sRGB framebuffer.

[^1]: In theory we could render as linear to a texture, and then render that to the final output, with a shader that performs linear -> sRGB... But this is So Not Worth It. Also, the intermediate texture for the linear output generally will need to be a float texture. (The reason that everything uses sRGB is that linear just doesn't work well for 8 bits)

  1. (vertex gradients) This is pretty rare to start out with, as most of the time imgui spits out triangles that all use the same color.

    The thing is it's a bit hard for me to say for sure that this matters — the outcome of not doing this correctly is just that the gradient won't accurately matches the physical behavior of light, which... isn't clearly a desirable property of GUI widgets.

  2. (frag_color * tex_color) This kind of math is pretty clearly is always wrong to do with sRGB textures, so pretty cut and dry.

    Actually... there's an important cases where the result here is correct either way: if fragment_color is white, or tex_color is white, then it doesn't end up mattering (since toLinear(1.0) == fromLinear(1.0) == 1.0)

    So, do how often do we fall into that case?

    • Everything drawn using the font texture will, since that texture is entirely white with different alpha values.
    • Custom images (Image widget for example) almost always will, unless the user sets a custom tint color.

Subjective Verdict

So, for sure all of these matter, but most matter pretty rarely, or even never. Conversely, there is a very real cost to what we do now, in terms of performance, power, and even memory.

Not to put too fine a point on it — the linear->sRGB op we do in a fragment shader seems awful to do every fragment, given how much overdraw imgui has, especially for integrated GPUs.

It's a very odd tradeoff to make by default without any way of allowing opt-out, especially given that imgui is largely a debug UI.

(Also, not for nothing — I think there's something to be said for being compatible with the existing renderers where possible)

THAT BEING SAID.

This all changes a lot when the user is drawing to a sRGB framebuffer. In that case:

  1. being gamma-correct is way cheaper (far less per-fragment work).
  2. the consequence of doing nothing becomes very bad. (it will look very clearly like shit for them)

Solution?

I'd probably be inclined to say the profounding "meh"-ness aspects outweighs all benefits and we should:

  1. Do everything in linear as we do now if they're using an sRGB framebuffer (or need linear output for another reason).
  2. Otherwise, ignore all of this "srgb" nonsense, and behave the way the existing renderers do.

An alternative might be taking an option for speed/accuracy, or something, but after writing that all up, I don't think it's worth it — we should just .

CC @toyboot4e

dbr commented 3 years ago

I don't think I understand this properly as I don't know a lot about OpenGL and how and where it handles colorspace conversions, but more than a decade spent doing visual-effects compositing has ingrained in me that gamma-correct compositing is very important :sweat_smile:

I did find the default behaviour of the imgui-glow-renderer produced drastically better looking fonts, compared to the default imgui-glium-renderer. Depends a bit on the font and colours, but with dark text on light background there was a very noticeable improvement (e.g the "Myriad Pro Light" actually looked "light", not kind of chunky and a bit jagged). imgui's default look of light text on dark background didn't improve as drastically, but still seemed a little better

thomcc commented 3 years ago

Was this when using an sRGB framebuffer? That case is fine (and actually correct, unlike the glium renderer) and not what should change.

When not using an sRGB framebuffer it attempts to implement it by hand, but, well, it’s actually not possible to do so correctly (without rendering to a temp buffer first, anyway) — you can fix a few of the issues but the actual compositing happens in the wrong colorspace still.

So for that case it should change, IMO.

dbr commented 3 years ago

Ahhh I see, I had misunderstood the "this issue is mainly concerned about" sentence! Yes this was using an sRGB framebuffer - if that case keeps working the same, and the other cases become simpler, then that seems perfectly reasonable to me

jmaargh commented 2 years ago

Hmm, so to summarise your suggestion is just to get rid of the shader functions srgb_to_linear and linear_to_srgb in all cases and be done with it?

FWIW there's a lot of discussion on this upstream issue, which is where I discovered that imgui's vertex colours are sRGB (making the conversion to linear in the vertex shader the "correct" thing to do, though you're right triangle gradients aren't much of a thing in most imgui use).

First, a general counterpoint: do we have any numbers on the actual scale of this performance penalty in the real world?

Thinking about it again, I definitely think the AutoRenderer shouldn't have defaulted to output_srgb=true, rather we should just assume the default case is for people to use sRGB framebuffers if they're rendering to the screen. When using full-fat OpenGL I don't think there's ever a good reason to use output_srgb=true, and coupled with the srgb->linear transformation in the vertex shader everything should be both correct and performant.

If we make that change, the only issue that remains is with GLES, because that's the only case where you're drawing to a screen and can't set the default framebuffer to sRGB. This seems to be a general problem with GLES and I can only assume that the "correct" workaround is for people to render to a texture (either an sRGB format texture, or then use a postprocessing shader to convert to sRGB when outputting to screen) otherwise, as you correctly point out, alpha blending of fragments is wrong. If this is correct, then they'd also never use output_srgb=true.

Which leaves us with: "what's the point in output_srgb=true?" It's left as a crutch for people who are "incorrectly" rendering non-srgb framebuffers to the screen (in either GL/ES cases) to get "more correct" colours at the expense of wrong alpha blending. Niche, sure, but seems worth leaving in for the handful of lines it takes.


tl;dr: I suggest that: