-
-
Notifications
You must be signed in to change notification settings - Fork 979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SRGB correct linear gamma blending #5423
SRGB correct linear gamma blending #5423
Conversation
Interesting. Sadly I am super busy at the moment so I dont have the time to review this as it deserves. I will get to it when I can. |
On Fri, Aug 26, 2022 at 07:45:00AM -0700, Martin Wernstål wrote:
* Maybe there are some better/more efficient ways to provide the LUT-data to the GPU.
Regarding this point, maybe better to use a VAO. IIRC uniform buffer
sizes are quite limited, depending on GPU driver/model. Or one could
even use a texture.
* How/if this will affect color on MacOS with different `macos_colorspace`. I have not yet tested this.
You can view, comment on, or merge this pull request online at:
Hopefully some macOS users will be willing to try it out and see. Ping
@Luflosi, @page-down
|
Thanks for this improvement. When the text color is white and the background is black, the font thickness finally approaches the rendering of macOS Cocoa App. However, when the background color is light, such as highlighting selected text, the text becomes thin and blurry. Unfortunately I cannot test it on wide gamut hardware devices (e.g. with Apple Display P3-500 nits color profile) at this time. |
I think I am getting closer to finding the issue with why the blending works with light-on-dark but not the reverse; the rendered glyph alpha-channel needs to be converted from linear to sRGB before being applied to the foreground color. I have trouble finding correct information on how to exactly blend for font-rendering with an alpha channel in particular, and this feels a bit odd to use one alpha value in sRGB space while the rest of them are in linear space. Could possibly also be that some settings for glyph rendering needs to be adjusted instead of the shader itself, the The top image is the current state of the PR, the one below converts the sprite texture alpha channel to sRGB before blending: It does seem like the light-on-dark colors get a bit blurred though, so I am not convinced this is the correct solution. |
a3b1781
to
c4ac212
Compare
Technically the blending of the font is gamma is correct in the PR with just the use of the added sRGB lookup-table for colors, premultiplied alpha blending with those colors in linear space, and This is a bit problematic though since many fonts and font-rendering engines make the assumption that the result is not going to be blended in a gamma-correct way. For example Photoshop uses a gamma of 1.42, and Qt5 on X11 uses 1.7, specifically for rendering text to compensate for fonts which look too thin when rendered correctly (while the rest of the rendering is done in a gamma-correct way). For the engines that do blend in linear colorspace (like ClearType and MacOS) they use what is called stem-darkening or glyph dilation, where thinner small font-sizes have increased weight. FreeType does have support for stem-darkening, but not with TrueType fonts, only the CFF and Type 1 drivers have exposed support for it (which can be turned on using environment-variables or system settings if a suitable font is used). All of this means that many fonts will look too thin when using gamma-correct blending alone. It is probably best to follow Photoshop and Qt5 here and make a special adjustment to the text alpha-channel using a gamma curve, and let the user configure this value so adjustments can be done depending on the used font, size, resolution, and rendering engine. The colors and rest of the blending should still be done with sRGB-correct gamma to ensure the luminance is correct. Here is a comparison between MacOS Terminal (on a MacBook Pro 15" 2019 with Monterey), Kitty with this PR and a gamma curve of 1.7 applied to the glyph alpha, and Kitty 0.26.0: The left one is MacOS Terminal and the middle one is Kitty with the modifications, and finally to the right we have Kitty without this PR. Note that this screenshot looks way better and sharper than it does on the actual retina screen, the varying brightness of the text in Kitty 0.26.0 becomes more obvious when it is smaller. Gamma-correct alpha-blending combined with an applied gamma curve on the glyph alpha channel seems to be getting very close to MacOS rendering of the font. @kovidgoyal Would it be suitable to pre-compute the font gamma adjustment to the rendered alpha channel in |
Should be fine as far as I know. It's used only for rendering box I havent had time to read the code yet, but this reminds me that there |
Since I use both LowDPI and HiDPI monitors, I've noticed that when using the same font size, macOS will slightly increase the weight whenever the actual rendered size is too small. And when the macOS Terminal window is moved to HiDPI display, the font will immediately become sharp and clear, without blurring like when kitty The font
It would be nice to add the ability to adjust based on DPI, not just resolution.
However, the font in the middle screenshots appears jagged, and you can notice that at the bottom of For the top and bottom screenshots in the middle, it is like using a bold font when the background color is black. |
Gamma correction for emojis is already implemented through the
Thank you, that font looks to be a much better test-case for this PR. Using the settings from #4485 without It does look to be very close in terms of shape, but this PR + extra gamma ends up looking washed out. The MacOS rendered font looks like some kind of contrast-curve is used instead of a gamma curve since it is very dark despite the thin strokes. After testing some different values with a sigmoidal contrast-curve I have landed on a guess of a slope of 6 and 10% midpoint as a decent approximation (note that the alpha channel 255 means fully colored pixel): And with a scaled resolution: When looking closely at the same with bright text on dark background we can see that the curve probably needs some more tweaking, especially for the native resolution, if it actually is a sigmoidal contrast curve: |
1342129
to
b86feb6
Compare
Just updating to say this is on my radar, I will get to it. |
After not being happy with the result of the contrast function above I have iterated a bit more on this issue. So I took screenshots of both white-on-black and black-on-white with SF Mono 11pt and compared the pixel values: The black on white screenshot data was inverted to try to see if they overlap, which they do not. Testing with different gamma-curves before inverting the black on white could move them a bit closer but I could not find a perfect match which I'm guessing means that the font rendering is not just stem-darkening and 1.0 or 2.0 gamma (which is what Skia uses depending on if font-smoothing is off or on). We can also see that it is saturating a bit in both cases. So instead I applied a reverse srgb gamma curve to the Terminal.app screenshot and then tried to find an approximation function to transform linear alpha values to something which would somewhat match Terminal.app after gamma: As we can see the two colorschemes differ greatly still which makes it impossible to use any kind of transformation on just the font alpha-channel and produce a result which matches Terminal.app in both cases. When using the curve-constants matching black on white approximation we still get some artifacts in white on black in native resolution even though it is greatly improved from the sigmoidal contrast function: |
On Mon, Aug 29, 2022 at 09:53:52AM -0700, Martin Wernstål wrote:
@kovidgoyal Would it be suitable to pre-compute the font gamma adjustment to the rendered alpha channel in `render_alpha_mask` in `fonts.c`? This way we can avoid having to re-compute the `pow(alpha, 1.0 / font_gamma)` in the shader.
Should be fine as far as I know. It's used only for rendering box
drawing chars and normal chars from fonts.
I havent had time to read the code yet, but this reminds me that there
is also rendering of emoji to consider. So you would need to precompute
gamma for those as well if you do this.
|
Apologies for the long delay, I was focusing on getting kitty-tool done. Now that this is merged into master I have the time to review this PR. Could you please resolve the conflicts. |
This patch converts the cell shaders to use linear gamma for all colors, finally converting it back to sRGB at the end of the fragment shader. To improve performance it uses a lookup-table to convert the 256-color input values to the linear representation of their sRGB colors in the vertex shader, and it also has branch-less implementations for linear-sRGB and sRGB-linear conversions.
…plied alpha in cell fragment shader
…sions from shader
Reason for this is that many fonts are made with incorrect blending in mind and will look too thin when blended in linear space. See: * https://blog.johnnovak.net/2016/09/21/what-every-coder-should-know-about-gamma/ * https://www.puredevsoftware.com/blog/2019/01/22/sub-pixel-gamma-correct-font-rendering/
… font alpha channel adjustment
…st when it does nothing
This matches Terminal.app much better, but is still not perfect for both light on dark and dark on light colorschemes.
1872836
to
7ba1820
Compare
Culd you also make a separate PR with some trivial change so thatthe tests are run automatically on thi PR in the future. |
I did a basic review, the code looks mostly fine (however with graphics its the end result that matters), I will post visual results that don't look good to my eyes as I test things. The first thing I noticed is that dark-on-light text looks significantly lighter with the patch. See below, First screenshot is without patch second is with patch. This is on Linux X11 with the Operator Mono font on a 3840x2160 display scaled by a factor of 3 (dpi set to 288). |
I am fairly certain that this change will be liked by some people and hated by others and not noticed at all by many. And we will of course hear far more from the people that hate it. As such it would be nice if there was a setting to revert rendering to what it used to be. Possibly, have a LUT containing just 1/255 uniformly and have a config option that switches between the two and also doesn't use the SRGB colorspace. Or alternatively, have a set of recommended values for the contrast curve and whitepoint settings to get close to previous rendering in different scenarios. Another idea is to have per font contrast curves, using the existing modify_font configuration machinery that override the global settings. We will also need to deprecate the macos_thickenfont config setting and point users to the knobs that control gamma blending instead. |
Another thought is maybe we need different gamma values (aka LUTs) and curves for each platform so that the out of the box rendering is closer to platform defaults. So one for macOS, one for Linux (I dont think X11 and Wayland need different gamma). |
This is the expected result when blending in linear space, it is unfortunate since it both fixes some fonts while making others look worse (or different compared to the system rendering). This article discusses the issue and some of the possible solutions: https://skia.org/docs/dev/design/raster_tragedy/ . According to this their "Gamma Hack" is not suitable for full-pixel anti-aliased fonts, this seems to match my experience when I tried to render fonts with a different gamma-curve in sRGB-linear colorspace.
I will have to go back and experiment some more and see if I can come up with a suitable way to apply an adjustment which is scaled based on the intensity of the destination/target. Seems like this is what Skia does in its "Contrast Hack" to avoid increasing contrast when the target is darker than the source (this is an issue currently with this PR, the contrast curve further thickens light-on-dark text more than what using sRGB-gamma itself already does). This should hopefully be possible to perform in the shader, making it easy to configure (but will add some computational complexity). If that fails to produce results acceptable by most people, a configuration-setting for sRGB on/off + contrast is probably the best bet. Would require a restart of Kitty whenever this setting is changed though (unless there is some way to reinitialize the gl-context?). |
On Mon, Nov 21, 2022 at 07:31:21AM -0800, Martin Wernstål wrote:
> The first thing I noticed is that dark-on-light text looks significantly lighter with the patch. See below, First screenshot is without patch second is with patch.
This is the expected result when blending in linear space, it is unfortunate since it both fixes some fonts while making others look worse (or different compared to the system rendering). This article discusses the issue and some of the possible solutions: https://skia.org/docs/dev/design/raster_tragedy/ . According to this their "Gamma Hack" is not suitable for full-pixel anti-aliased fonts, this seems to match my experience when I tried to render fonts with a different gamma-curve in sRGB-linear colorspace.
Yeah, the problem with font rendering is there is no "right" answer its
all expectations, systems, fonts, eyes, etc. dependent.
> I am fairly certain that this change will be liked by some people and hated by others and not noticed at all by many. And we will of course hear far more from the people that hate it. As such it would be nice if there was a setting to revert rendering to what it used to be. Possibly, have a LUT containing just 1/255 uniformly and have a config option that switches between the two and also doesn't use the SRGB colorspace. Or alternatively, have a set of recommended values for the contrast curve and whitepoint settings to get close to previous rendering in different scenarios.
I will have to go back and experiment some more and see if I can come up with a suitable way to apply an adjustment which is scaled based on the intensity of the destination/target. Seems like this is what Skia does in its "Contrast Hack" to avoid increasing contrast when the target is darker than the source (this is an issue currently with this PR, the contrast curve further thickens light-on-dark text more than what using sRGB-gamma itself already does). This should hopefully be possible to perform in the shader, making it easy to configure (but will add some computational complexity).
Worth a try.
If that fails to produce results acceptable by most people, a configuration-setting for sRGB on/off + contrast is probably the best bet. Would require a restart of Kitty whenever this setting is changed though (unless there is some way to reinitialize the gl-context?).
I'm OK with it requiring a restart, while the OpenGL state can of course
be changed one would have to re-load all the graphics as well I think.
Too much bother for a setting that is not meant to be tweaked often
anyway.
|
Another possible issue is how changing opengl to use SRGB colorspaces interacts with the macos_colorspace setting, is it handled transparently? Does it cause performance issues when the display colorspace is not srgb but instead apple's p3 colorspace? @pagedown since you use macs maybe you can test this? |
From the results, I can't find any difference, it looks like it has been treated correctly.
Even after actually doing more calculations (for the color space), I could not feel the impact on performance. |
OK, that's good to hear. |
Just a tiny update since I have been pretty busy the last few weeks: I have managed to reproduce the non-linear blending result using a shader in sRGB-linear space by looking at luminance of foreground vs background and then solving for a new alpha value. Unfortunately the calculations used to match non-linear blending are not close to any of the "Contrast Hack" calculations used by other renderers, though it should be possible to use a boolean flag to use this calculation instead of a contrast function (hopefully the unused pow and division instructions won't be a performance problem, worst case we can probably switch the shader or OpenGL configuration depending on this configuration value). And I am still in the process of testing some contrast-curves to see what curve and values will get close to Mac/Windows rendering. |
Another update on this: Since these commits were partially made in an experimental fashion, with some undoing work from the previous, I decided to rewrite this PR while also improving it.
WIP branch: https://github.com/m4rw3r/kitty/tree/linear-gamma-blending Should I replace this PR with the commits from the new branch? |
Do whatever you find easier. I am fine with you opening a new PR if |
I've been using this (merged) PR in my own copy for a couple weeks now, and I have no complaints. I think it looks great. |
See #5969 for the new rewrite. |
This pull request is building on top of #3308 to blend colors in linear colorspace. It will improve font rendering and probably color accuracy when transparency is involved.
To avoid somewhat complex calculations in shaders it uses a lookup-table in a uniform for the color-pickers in the vertex-shaders for cells and borders, and the same data is used to provide the colors supplied to the shaders themselves.
GL_FRAMEBUFFER_SRGB
is then used to automate the conversion back to SRGB-space (if we manually converted back from linear at the end of each shader we would have to re-convert existing colors in every shader which would carry a performance penalty, this way we can mix-and match shaders in any order).Some things I'm a bit uncertain about:
macos_colorspace
. I have not yet tested this.