-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Use sRGB color space for NSWindow on macOS #6602
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the argument for not using macOS' default colorspace? Just that "other terminals don't do it"? That seems rather weak to me considering the default is basically what is "officially" recommended by Apple.
From what I remember with macOS you should manually call stuff like that if you're doing OpenGL and such. For native windows you have the srgb by default(well, the display native one). |
I have been thinking on this a bit for several days and now I strongly believe that most of macOS users will be happier with sRGB and this should also reduce maintenance cost as well since all the macOS users will see consistent colors. (e.g. if some future macOS user complains about color, maintainers can just say "this is correct behavior, please fix your colorscheme") Some macOS users might become unhappier temporarily just because their existing color scheme will look slightly different after this PR but this should be one-off thing since they should tweak the colorscheme if they do not like "actual color" intended by the color scheme author. @chrisduerr, I think there is nothing like Ideally the default colorspace should be specified by terminal 24bit color escape sequence spec but I just could not find anything about this so I think it makes sense to try to follow existing conventions / implementations. Anyway just using "display's native values" does not make sense. The same color code (e.g. #7FFFD4) will be shown differently per display. So each application (or "platform") need to decide "how specific RGB tuple (e.g. #7FFFD4) will be shown on physical display".
As a conclusion, I think every terminal app should make sure to interpret 24bit color as RGB value in sRGB space to be consistent with the rest of the world (both terminal apps and web, most likely also other authoring tools). We could provide some option to use other color space (like Kitty) but I personally prefer not having such configuration since it's "too niche" ("99.9% of people would not actually need the configuration"). Even web browsers do not have such configuration option. Such configuration will break portability of color codes / colorscheme. I personally also think having such option can be just confusing. We should just interpret color codes in sRGB everywhere for both terminal world (and web platform) unless otherwise specified. |
Additional verificationAs an additional verification, I verified the following on MacBook Pro 14 inch, 2021 (it has "good display" with P3 color space support) Colorscheme fidelity
Colorcode fidelityIt's common to use an editor plugin to preview color codes such as nvim-colorizer (this is just an example and there are many similar tools). To verify colorcode fidelity, I used Neovim + nvim-colorizer.lua in this verification along with Google search result of the color code in Safari. I also checked other browsers (Firefox and Google Chrome) and they are the same. Also tried DuckDuckGo for sure and this is also the same as Google Search result. Before this PRBefore this PR, the colors are slightly different (visually recognizable for me) and macOS's color meter shows different values between the one from editor plugin and the search result from Google (confirmed this is not only for Google. Bing and DuckDuckGo behave the same). Among almost everything, only Alacritty is "broken" After this PRAfter this PR, the colors are exactly the same (visually the same and values in color meter are the same), which is expected and natural behavior. Note that this PR also affects colors in alacritty.yml as expected. |
181c64b
to
8d60280
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good after this. I don't think we should preface methods with their target platform, that's what the cfg
does already and it prevents us from adding functions that do the same on different platforms without first renaming all the existing functions.
Oh you also need to add a changelog entry. |
Co-authored-by: Christian Duerr <contact@christianduerr.com>
@chrisduerr, thanks for quick review! It makes sense not to prefix things with platform name such as "macos". I applied your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, looking all good now.
@naruaway what will happen with external monitors which are not SRGB or with P3 monitors? I guess the colors on them won't match anymore? |
@kchibisov I do not have external monitor so I cannot immediately test but my assumption is that basically any modern monitor supports sRGB, otherwise even CSS colors on web pages will not be shown as intended anyway (sRGB is specified in CSS spec). And even for such monitors, I think it's reasonable to assume (haven't tested) the color on Alacritty will match with Kitty and web browsers after this change. What's the expected behavior in your mind? |
I'm just saying that before we were showing the correct colors on the external monitors without this change at all. The color you render doesn't mean that it's the thing you display. It just macOS has some weird logic around that not present on any other system. I'd assume that it'll either be the same or not expected. If it matters alacritty always rendered as sRGB if it matters, since the fonts are already in sRGB and we just use linear meaning that everything stays as srgb. |
Hm are you sure? At least on macOS, I think without this change, I believe it was showing "incorrect" colors when native values were not the same was sRGB. Do we have existing discussions with screenshot or whatever...? Also what's the definition of "correct color" here? My definition in this PR is "the same color as web browsers".
I think what you mean here is that Alacritty has been writing the correct values (sRGB) in OpenGL buffers and I believe that's true. This PR is to change "how macOS window system interprets values in OpenGL buffer". And I am not totally sure about |
@kchibisov Another relevant thing I was not so sure is that #6390 introduced |
Actually to me things are starting to make more sense (but correct me if I am wrong!)
|
Are you testing on external non-apple monitor? I'm pretty sure on average monitor you'd get different colors now.
They just don't mess with the colors. The point here is that your system compositor blends all the windows and exports as textures. macOS uses some weird scheme for that and maybe tries to gamma correct. If you use linear you'll persist your srgb, if it was srgb, that's how any linux compositor works and alacritty has 'correct' colors on them.
It means without srgb frame buffer. Since we don't want it given that we don't need gamma correction to do srgb. That would make sense if we rendered textures that were created on sRGB monitor, but saved as linear, meaning that you need to do gamma correction, which could be achieved automatically by enabling srgb frame buffer with
That's irrelevant, we don't need to set buffer as sRGB. By doing so you'd introduce a conversion from linear to srgb, but we're not using linear but sRGB already. |
Also, from what I remember kitty had issues with 'bad' colors using |
I now do not have external monitor (both Apple and non Apple) but I think they should show different colors after this PR and I think it's expected as long as it will match with the colors in web browsers Just to clarify, my concern is mostly around range of the values, not non linear transformation (gamma correction). For example,
This is EGL buffer, not OpenGL buffer, right? I thought it's needed since otherwise OS cannot know how to map the color point in OpenGL buffer to display native values (e.g. if we have hypothetical display which can show extremely bright green, without specifying sRGB color space, |
Do you mean kovidgoyal/kitty#2249 (comment) ? BTW I want to make sure that we are on the same page, do you agree that "color codes" (alacritty.yml and true color escape codes, used in Vim color scheme for example) should be interpreted as the ones in sRGB by default? My rationale is in #6602 (comment) I found Kitty's maintainer once said ""proper" thing to do is interpret RGB triples as device |
I mean you should show in display native colorspace, so SRGB is not native for non-srgb monitor. As for the escape sequence there's no standard saying how it should be interpreted, though in our rendering mode we assume that it's SRGB. |
Hm ok, so basically you disagree with #6602 (comment)
True... and it's the source of issues here 😢 My suggestion in this PR is "let's use (try to make) sRGB as defacto standard" since it's practically useful. I think showing colors in native display values is not so useful IMHO |
@kchibisov, I was able to experiment with an external monitor (DELL P2421DC) connected from my MacBook Pro. I just used the default color profile for this display, which is "DELL P2421DC". I think this is "external non-apple monitor".
So I think this PR works as expected with non Apple external monitor as well. What I mean by "expected" is, the color has changed from previous but the new color should be "more correct" and I believe most people want to have the same color as web browsers (and other terminals such as Kitty) For other operating system, I am totally not sure but maybe we should open a new issue for Windows / Linux if some people seem to have any issues instead of discussing here (but if you really disagree with #6602 (comment), I think we can revert or we can consider providing this behavior as config option although personally I think it's not good to increase the number of options for this) |
I mean that you can readjust the colors according to the monitor on the fly or provide options to control it? Though, right now it's not possible to readjust. I'd wait until someone complains though. It's not like I care that much about macOS to continue. If there won't be any desire for the old behavior it'll stay the way it's right now. |
Hi @naruaway, just out of curiosity I was playing with the color and notice something interesting on my system. I'm using a 16 inch mbp with xdr display preset. As you can see from the screenshot, when picking the color in web browser (above is safari and below is chrome), the native value does not show the correct sRGB value. This is different from your test result because you can get correct sRGB value from native values in color meter. However, if I choose Display in sRGB, the result is "almost" correct. On built-in display the result is one number off the sRGB value, while on the external DELL display the number is exactly the same. Basically all other application I tried are the same including firefox and Emacs. For alacritty (version 0.11.0 8dbaa9b, I've not tried the latest patched version yet), if I choose Display native values, the color meter will report sRGB value. The Display sRGB value doesn't. The color displayed on alacritty does seem a bit different as you described in the patch though. I don't think the difference behavior in the color picker between yours and mine is caused by any issue related to alacritty. The color behaves as you mentioned but the number doesn't. I just feel it might be useful for you to report this because users with latest system and device might also notice this later when they do testing😄. I'm not sure if it's a bug in the color picker for latest mac model with xdr display or if Apple changed the behavior. (The color picker still shows "Color LCD" profile if you may notice) |
That's likely because your colorspace is P3 and not sRGB. |
Hi @seraphlive, thanks for testing! For the color values. I agree with @kchibisov and the following is false:
In #6602 (comment), I used "Display in sRGB" for Digital Color Meter so it's almost identical (ignoring off-by-one, I am not sure here but does not matter much). So I think all of my experiments and your experiments make perfect sense and there is no inconsistent / weird behavior.
This is also expected for the behavior before merging this PR. Because "the color meter (Dispaly native values) reports sRGB value.", it is shown in incorrect color since your display is not sRGB display. |
I carefully read your tests again and you are right. I confused myself on your second test, because I was focusing too much on the difference between the Google rgb value and your color picker result. I didn't realize in that test you purpose was just to compare Alacritty and the browser.
Yes. I tried switch to a sRGB preset, the numbers become the same between Display native values and Display sRGB value. Glad to find out there is actually no issue at all :) |
This is the same change as kovidgoyal/kitty@368bc91 (also relevant: kovidgoyal/kitty#4686)
I confirmed that now the rendered color is exactly the same as Kitty.
I think it makes sense to make the default color space sRGB since other popular terminals are using sRGB by default including Kitty and iTerm.
Also showing the same color as web browser, which is using sRGB, is beneficial for several apps such as nvim-colorizer.lua while doing web development.
Closes #4939
Verification on MacBook
I tested on MacBook Pro 14 inch, 2021 with pastel
master (2bd26fb)
this PR
<div style="background-color: #398810;">X</div>
)Concerns
Although I am personally happy with this patch since I am familiar with sRGB more than other color space (had no idea about P3 initially), this will be "breaking change" and some of macOS users will be unhappy due to changed colors for their colorscheme although changed one should be more consistent with other terminals. Actually it looks like Kitty made this configurable viamacos_colorspace
option so that people can still choose other color space such as P3 via configuration option.Before merging this patch, we might need more discussions / considerations. (cf. kovidgoyal/kitty#4686)UPDATE: Now I think this change is "correct" and we actually should not have configuration option to switch color space like kitty does (Kitty's
macos_colorspace
option). Terminal apps do not need to be such "rich" in color config. Simply we should just follow common accepted color space both in terminal world and web platform world, which is sRGB. See #6602 (comment)