Skip to content
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

Add support for subpixel rendering on Linux #1604

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

nightuser
Copy link

Adds optional support for subpixel rendering when using freetype. This behavior is disabled by default and can be enabled in the configuration file.

@nightuser nightuser force-pushed the freetype_subpixel_rendering branch 2 times, most recently from 9d67ca0 to ba6af6e Compare May 9, 2019 20:32
@nightuser
Copy link
Author

Grayscale:
without_subpixel_rendering

Subpixel:
with_subpixel_rendering

@Luflosi
Copy link
Contributor

Luflosi commented May 9, 2019

Very impressive for your first contribution to kitty!

@Luflosi
Copy link
Contributor

Luflosi commented May 10, 2019

I think it would be better to rename linux_use_subpixel_rendering to use_subpixel_rendering or maybe simply subpixel_rendering, so that if we add support for macOS in the future, we don't have to rename the option or have two options that do the same thing just on two different platforms. You could add a sentence to the description of the option, mentioning that it doesn't work on macOS at the moment. I just spent the past couple hours installing a FreeBSD VM and installing kitty and it also works there 🎉, so the linux part of the option name wouldn't be entirely accurate anyways.
What do you think @kovidgoyal?

@Luflosi
Copy link
Contributor

Luflosi commented May 10, 2019

How do you ensure that kitty uses the correct color on the correct side? In my FreeBSD VM the left side of the characters in kitty is blue while the right side is red. In other programs in my VM and outside of my VM the left side is red while the right side is blue. Doing sub-pixel anti-aliasing wrong in this way will make the font look worse than using no sub-pixel anti-aliasing. Which side needs to have which color might depend on the monitor though, idk.

@kovidgoyal
Copy link
Owner

Yes, I think it should be use_subpixel_rendering. As for BGR vs RGB IIRC that comes from the fontconfig configuration on Linux.

@nightuser
Copy link
Author

@Luflosi when rendering using FT_RENDER_MODE_LCD FreeType produces either RGB or BGR depending on the fontconfig configuration. I haven't tested it thoroughly thou. Also, it might be worth adding support for FT_RENDER_MODE_LCD_V which is used on vertical screens. And I'll definitely remove linux_ prefix.

@nightuser
Copy link
Author

I started working on adding support for LCD_V. It requires a bit of work, as the resulted bitmap is thrice the height of the actual image, so rows is no longer reliable to get the height of a ProcessedBitmap instance.

@Luflosi
Copy link
Contributor

Luflosi commented May 10, 2019

I investigated some more and it turns out that kitty correctly uses RGB in my FreeBSD VM. I made a mistake when thinking about which colors should be on which side, sorry for that.

@kovidgoyal
Copy link
Owner

@nightuser I would not bother with LCD_V if I were you. Its a lot of work and is very rare in actual hardware. I think the only time it is used is if the monitor is rotated.

@nightuser
Copy link
Author

@kovidgoyal well, I find out that some additional work needed for regular LCD, as there are some places in the code where we use that bitmap is grayscale. Also, I have a lot of acquaintances who use their monitors in the vertical orientation, myself included.

@kovidgoyal
Copy link
Owner

If it is important to you feel free to implement support for vertical, I
have no objections.

@nightuser
Copy link
Author

Hi!

Sorry, I was a bit busy with my life.

By the way, I implemented the support for vertical alignment and fixed some places, where it was implicitly assumed that the rendering is grayscale. I tested it with four possible combinations (RGB/BGR and horizontal/vertical), and it renders correctly.

@kovidgoyal
Copy link
Owner

No worries, it is going to be some time before I can find the time to
review this.

@Luflosi
Copy link
Contributor

Luflosi commented May 22, 2019

What would it take to add support for macOS?

@miseran
Copy link
Contributor

miseran commented May 24, 2019

I gave this patch a try, but it doesn't seem to work correctly with transparency yet. Around each character, there is a solid rectangle. (On Xorg with compton, using Intel graphics.)

transparency

Really looking forward to this patch though, since most of my displays are low DPI.

@kovidgoyal
Copy link
Owner

What would it take to add support for macOS?

There would need ot be some way to get CoreText to render multi-channel bitmaps. Since Apple has officially decided to remove sub-pixel rendering, this seems unlikely to be possible.

@nightuser
Copy link
Author

nightuser commented May 24, 2019 via email

@nightuser
Copy link
Author

@kovidgoyal but is it possible to use Freetype on macOS (an optional switch maybe).

@kovidgoyal
Copy link
Owner

People on macs complain a lot if text rendering is different from
Apple's which is why I took the trouble of implementing it using
CoreText in the first place. I'm not really in favor of fighting the
platform's official stance. If Apple does not want to support sub-pixel
anti-aliasing, then that is their choice. Without official support it
means there is no global setting for the pixel stripes order RGB vs BGR
or horizontal vs vertical. And then FreeType will behave in subtly
different ways than CoreText leading to more hard to replicate bugs that
I will have to debug.

@nightuser
Copy link
Author

nightuser commented May 26, 2019

Turns out, it's nearly impossible to implement transparency accurately, at least without rewriting much of the rendering (see https://stackoverflow.com/questions/33507617/blending-text-rendered-by-freetype-in-color-and-alpha , for example).

@kovidgoyal could you help me with this short block of code: https://github.com/nightuser/kitty/blob/freetype_subpixel_rendering/kitty/cell_fragment.glsl#L64 (lines 64-69), please? I'm not fluent in OpenGL at all, so I am probably doing something wrong there.

@miseran I added experimental support for transparency. Could you test it out?

@nightuser nightuser force-pushed the freetype_subpixel_rendering branch from 63038ef to d397b8c Compare May 27, 2019 10:50
@nightuser
Copy link
Author

I rebased my branch against current master because I personally use this patch as a daily driver.

@kovidgoyal
Copy link
Owner

kovidgoyal commented May 27, 2019 via email

@nightuser
Copy link
Author

@kovidgoyal Freetype provides alpha channel per color for each pixel separately. I'm blending the foreground and the background using mix: for a non-transparent background it's as simple as mix(background, foreground, text_fg.rgb). In the case of a transparent background, things get a little hairy:

  1. I use background input variable, but I'm not sure whether multiplying by bg_alpha was already done (it seems that it isn't, but I don't really understand why we need to multiply twice).
  2. We should render only the actual character and not the bg around it. Unfortunately, Freetype gives a mask with "transparent" area which usually should be filled with bg color. Cairo library faces a similar problem and they're using the green channel as the alpha channel. So, I've come up with the following lines:
    float alpha = text_fg.g;
    vec3 scaled_mask = mix(vec3(1.0), text_fg.rgb / alpha, bvec3(alpha > 0));
    vec3 blended_fg = mix(background * bg_alpha * bg_alpha, foreground, scaled_mask);
    It somehow works, but I'm worried about the fact that scaled_mask here might become non-normalized (i.e. we can get values bigger than 1). Also, the way scaled_mask is constructed looks kinda ugly.

@kovidgoyal
Copy link
Owner

kovidgoyal commented May 27, 2019 via email

@kovidgoyal
Copy link
Owner

Regarding scaled mask. I dont quite understand the problem. The only
difference between the transparent and opaque bg cases is that the
background has its own alpha, therefore you cannot simply mix foreground
and background colors. Instead you have to do a full per-channle
alpha blend. See the alpha_blend() function for an example. That
function has a single alpha for the under and a single alpha for the
over color. The difference in your case is you have three alphas for the
over color. It should be possible to write a similar function to do full
per-channel alpha blending. This shuld take care of the problem with
freetype transparent pixels, since they get rendered in the background
color automatically, by virtue of the alpha blend.

@nightuser
Copy link
Author

@kovidgoyal the problem arises because at the end we combine bg and fg textures one above the other. So, if these transparent pixels are rendered in bg color (even if it's semi-transparent), we will draw two semi-transparent backgrounds in some places, which results in more-opaque color.

In opaque case, it's okay, as an opaque image over opaque looks fine.

@nightuser
Copy link
Author

@kovidgoyal It seems that it can probably be done with correct glBlendFunc call. I'll look into it later.

@nightuser nightuser force-pushed the freetype_subpixel_rendering branch from 73708a1 to 6c1bb9b Compare June 21, 2019 19:10
@kovidgoyal
Copy link
Owner

@nightuser is this PR ready for review?

@nightuser
Copy link
Author

@kovidgoyal I still need to test it against the latest changes. I'll do it probably today or tomorrow and then it'll hopefully be ready to be reviewed.

@rien333
Copy link

rien333 commented Oct 18, 2019

@nightuser Obviously take all the time you need, but you are still interested in this right?

@nightuser
Copy link
Author

@rien333 I don't have much time at the moment. Everything works fine modulo transparent backgrounds, and I didn't come up with a good solution for it. Since the Autumn semester started and I'm a teacher assistant, I'm teaching some classes and it takes time, obviously. If you want to contribute and help me with a transparent background, you're more than welcome. If you're just interested in the feature, you'll probably have to wait for a bit.

@rien333
Copy link

rien333 commented Oct 18, 2019

All right, interesting to know that the transparency issue what is keeping this in the pipeline. Probably don't have time either, but we'll see. Good luck with your teaching duties.

@sarnikowski
Copy link

Hey @nightuser, is this still something you are pursuing ? I'm very interested in this feature. Thanks in advance !

@nightuser
Copy link
Author

Hi @sarnikowski. Not really, I don't use kitty currently. I might help if anyone wants to work on the feature. I hope I'll look into it in the future, but I have more important things to do right now.

@raduacosma
Copy link

Hello, as far as I can tell the only problem with this PR is when transparency is enabled, please correct me if I am wrong! In this case, would it be possible to merge this PR, off by default, with a comment in the docs that it doesn't work with transparency and that it impacts performance? I consider subpixel rendering to be essential on a large 1080p monitor, which I have, and everything else that I use has subpixel rendering. Without it there is a noticeable difference that makes the experience worse. I personally don't use transparency and I consider that to be more of a stylistic choice, not a usability one. I think that most people with 1080p monitors and lower would like this feature and a lot of people probably do not use transparency for their terminals. I think Kitty is the best terminal feature and performance wise, but I currently cannot use it simply because it does not have subpixel rendering. If this was merged, it would be great for the most likely vast majority of users that do not have 1440p or 4k monitors. Thanks for the great terminal and hopefully this can get merged!

@ThatOneCalculator
Copy link

+1 on this

@Cons-Cat
Copy link

Cons-Cat commented Mar 2, 2021

Alacritty supports subpixel rendering, and it has a similar bug. Imho this issue isn't so problematic that it should prohibit the feature from being included.

@hramrach
Copy link

Yes, I think it should be use_subpixel_rendering. As for BGR vs RGB IIRC that comes from the fontconfig configuration on Linux.

Can't the on/off come from the fontconfig configuration also, at least the default?

@KonstantinDjairo
Copy link

why that PR wasn't merged yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.