-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Support for graphics in the terminal #4763
base: master
Are you sure you want to change the base?
Conversation
d5a1c08
to
f11ce16
Compare
This sounds like it might have a significant performance impact. Have you actually tested the performance of this PR?
That sounds extremely hacky, which I am not a fan of. This will likely just cause an endless heap of issues with things like selection, so it's not something we can just throw in and forget about.
This seems to just pile on a heap of code to support a bunch of image protocols. I don't like the idea of adding thousands of lines to Alacritty for something with so little use. If we support any protocol it should be the simplest one without any performance impact. I see no benefit in supporting multiple formats.
What about support for devices below OpenGL 3.3? This is something we would like to look into for the future so adding more code that requires at least OpenGL 3.3+ does not seem ideal.
I see little reason for adding memoffset/lazy_static usually, but I haven't looked at the code yet. |
Same |
I launched vtebench three times, in the current Running $ base64 ./target/release/alacritty > /tmp/data
$ wc -l /tmp/data
635257 /tmp/data
# graphics branch
$ perf stat -e cycles,instructions,branches,branch-misses ./target/release/alacritty -e cat /tmp/data
Performance counter stats for './target/release/alacritty -e cat /tmp/data':
4,590,935,968 cycles
10,125,002,121 instructions # 2.21 insn per cycle
2,038,270,623 branches
4,651,069 branch-misses # 0.23% of all branches
0.854657772 seconds time elapsed
0.774462000 seconds user
0.466697000 seconds sys
# master branch
$ perf stat -e cycles,instructions,branches,branch-misses ./target/release/alacritty -e cat /tmp/data
Performance counter stats for './target/release/alacritty -e cat /tmp/data':
4,692,460,970 cycles
10,053,624,431 instructions # 2.14 insn per cycle
2,035,973,894 branches
4,795,961 branch-misses # 0.24% of all branches
0.855561156 seconds time elapsed
0.762745000 seconds user
0.493408000 seconds sys
I changed the NBSP character with a
Without comments and tests, the support for the iTerm2 protocol is less than 100 lines of code.
The only performance impact of this protocol is an extra branch in the
The fallback uses |
This issue is now fixed. |
That doesn't justify adding it. If Alacritty supports any graphics protocol, there should be one and it should be the simplest one available. |
I'm also getting the following error:
|
What is the output of |
96 or something like that iirc. |
I uploaded a fix in 387a224 to check if this fixes the problem. If this works, we may need to generate the code of the fragment shader dynamically, in order to use the 32 units if they are available in the hardware. |
@ayosec did you test the iTerm2 protocol with ranger? previewed images are kept on screen and not cleared, so new images are overlayed on top of the old ones, and images are kept in the background when text is rendered in the preview pane. Another issue is that the iterm2 graphics is working with tmux as long as the image file is small, but with large files, it looks like I'm hitting this, which is not an Alacritty issue but Alaciritty is spamming with warning messages like this:
You should be able replicate with:
Note that this issue doesn't happen when running ranger directly without going through a tmux session, and not when running in an ssh session. |
Ranger relies on overwriting images with spaces. The discussion in #910 rejected my first approach to be able to have this functionality. However, if we can change the implementation in this patch to use As a work-around, images can be replaced with the █ character (reversed with
This error happens because the base64 stream is incomplete. Did you use iTerm2 imgcat? It seems that tmux requires specific sequences to send images. Anyways, since iTerm2 will not be accepted, maybe it's not worth it to spend time debugging the issue. |
I can replicate the issue with the linked imgcat script and an image. I won't attach the image here because I'm not sure about its license but I can send it via email.
That's unfortunate, it would have been nice to have it. |
I guess it's better to wait for sixel to support 24-bit colors? |
Sixel isn't likely to change. |
I have updated the patch with the following changes:
|
Can your implementation of sixel convert 24bit colors into 256 colors and display a video efficiently? |
18fd66e
to
0a41635
Compare
can confirm it does right now... it did not when shadow installed the tmux-sixel-git package |
I thought it was reverted because it was "too bloated" for the minimal base? |
It was enabled with 3.4-4 again. The previously added dependency to |
I ran into a curious issue- for some reason on my 1080p display, tmux shows the I'd be happy to run any tests that might help figure out what's going on. |
@prurigro I have an idea of what might be the source of your problem. From what I can make out, tmux uses an Now I have a main branch build of Alacritty in which the I should note, though, that my main branch build is from version 0.13.0-dev, while the sixel branch is on 0.14.0-dev, so it's not necessarily the sixel branch itself that introduced the problem. However, I think the most likely explanation is that we've just messed up the build somehow, like maybe there are libraries that need updating first in order for it work correctly? That might explain why you're seeing the problem on some systems but not others (assuming you built the app separately on each of them). |
@j4james: Your theory sounds pretty promising! Both systems are identically up to date and are using the same build on that note, so I feel like it's unlikely that it has to do with dependencies. Maybe whatever happens to fix it when you resize the window or change the font scale is also happening when alacritty's scale factor is calculated on a hidpi display? I just tested with the display's scale factor on my 4K monitor set to 100% and I'm seeing the same issue as on my 1080p display. I exited tmux+alacritty, flipped the display's scale factor back to 200%, started alacritty+tmux again then tested and it works immediately without resizing. |
@prurigro Yeah, that makes a lot of sense. Whatever the problem is, though, I don't think the sixel branch is to blame. I've just rebuilt Alacritty from the master branch and confirmed that |
That's it! 63bcc1e works, 117719b fails.
I have no idea how |
is it happening only in a special setup? Because we were sending it twice basically, but now only once, so it sounds like a bug in particular software used? |
I'm just using a little python script to test: import sys, struct, fcntl, termios
fmt = struct.pack('HHHH', 0, 0, 0, 0)
result = fcntl.ioctl(sys.stdout.fileno(), termios.TIOCGWINSZ, fmt)
print(str(struct.unpack('HHHH', result))) It's possible there is a bug somewhere there, but it works in XTerm. I should also be clear that it's just the pixel dimensions that are 0 - the row and column count are correct - so maybe the first time you send it the pixel size hasn't been calculated yet? |
Hm, seems like it was. Not sure why it was that way. Could you try diff --git a/alacritty_terminal/src/tty/unix.rs b/alacritty_terminal/src/tty/unix.rs
index 2c06d54f..2b1fdb2a 100644
--- a/alacritty_terminal/src/tty/unix.rs
+++ b/alacritty_terminal/src/tty/unix.rs
@@ -4,7 +4,6 @@ use std::ffi::CStr;
use std::fs::File;
use std::io::{Error, ErrorKind, Read, Result};
use std::mem::MaybeUninit;
-use std::os::fd::OwnedFd;
use std::os::unix::io::{AsRawFd, FromRawFd};
use std::os::unix::net::UnixStream;
use std::os::unix::process::CommandExt;
@@ -38,17 +37,6 @@ macro_rules! die {
}}
}
-/// Get raw fds for master/slave ends of a new PTY.
-fn make_pty(size: Winsize) -> Result<(OwnedFd, OwnedFd)> {
- let mut window_size = size;
- window_size.ws_xpixel = 0;
- window_size.ws_ypixel = 0;
-
- let ends = openpty(None, Some(&window_size))?;
-
- Ok((ends.controller, ends.user))
-}
-
/// Really only needed on BSD, but should be fine elsewhere.
fn set_controlling_terminal(fd: c_int) {
let res = unsafe {
@@ -194,7 +182,8 @@ fn default_shell_command(shell: &str, user: &str) -> Command {
/// Create a new TTY and return a handle to interact with it.
pub fn new(config: &Options, window_size: WindowSize, window_id: u64) -> Result<Pty> {
- let (master, slave) = make_pty(window_size.to_winsize())?;
+ let pty = openpty(None, Some(&window_size.to_winsize()))?;
+ let (master, slave) = (pty.controller, pty.user);
let master_fd = master.as_raw_fd();
let slave_fd = slave.as_raw_fd();
|
@kchibisov Yeah, that fixes it for me. Thank you. |
@kchibisov Both reverting the commit @j4james found and applying your patch seem to fix the issue for me :) I assume the patch is fixing things the correct way so I'll stick with that. Thank you both for digging into this and coming up with a solution! I realize that whether or not this PR will be merged is still up in the air, but at this point I've run out of issues- it actually works better than other terminals I've tried with support in their release builds. I'm also crossing my fingers that ayosec and everyone else involved (thank you all!) will be willing to maintain this soft fork if it doesn't get merged, because the ability to see thumbnails of graphics I'm working with without having to open a file manager every time is incredibly useful. |
The I merged it in this branch. In my tests the issue was fixed after the merge, so everything should be fine now. |
Thank you for mantaining this fork |
I would love image support |
With `$ cargo update vte`.
Hi just wonder is there a plan to add graphic support for the stable release. Is it ongoing or won't? |
I think this is a forever fork, someone will need to mantain it foreva |
This is a massive patch for a mediocre protocol. If you really need images then this patch might be useful, but I don't see it being a reasonable upstream addition. |
about the performance drop, if it would be made into a setting, and disabled by default - would this also hurt it? if not that would be a nice way to support it if needed but still have alacritty as fast as possible in normal install cases |
Opting out of performance is not something that aligns with Alacritty's goals. |
That's unfortunate because image support is a deal breaker for me. Ligatures and image support, which are by and large the most requested features for this terminal as far as I can tell, aren't exactly the most difficult of asks. We aren't asking for an SSH tool or anything. |
Saying anything text-related isn't difficult definitely shows the depths of understanding you have for these issues. Regardless of difficulty, if you're looking for a terminal emulator with every feature under the sun, I'd recommend looking elsewhere. |
I'd agree with you if I had said they aren't difficult, which I didn't. I said they aren't the most difficult features to add. And saying I'm asking for every feature under the sun, when I'm actually asking for two incredibly mediocre features, one of which even basic terminal emulators such as foot or xterm have, is a strawman. I won't argue with you, though. It's your terminal. Have a nice day |
This patch adds support for graphics in the terminal. Graphics can be sent as Sixel images.
New features
Sixel parser
The Sixel parser is based on the SIXEL GRAPHICS EXTENSION chapter in a DEC manual.
The support is complete except for the pixel aspect ratio parameters. According to the manual, a Sixel image can specify that it expects a specific shape for the pixels in the device, but in none of terminals that I checked these parameters had any effect: they always assume a 1:1 ratio. Also, I didn't find any Sixel generator that emits a different ratio. To avoid extra complexity in the parser, it always assume 1:1 when the image is built.
There are two new terminal modes:
SixelScrolling
(80
)If enabled, new graphics are inserted at the cursor position, and they can scroll the grid.
If disabled, new graphics are inserted at top-left, and they are limited by the height of the window.
SixelPrivateColorRegisters
(1070
)If enabled, every Sixel parser has its own color palette.
If disabled, Sixel images can share a color palette.
Initially I didn't plan to support this mode, since it seems to be specific to xterm, but when I was testing applications using Sixel I found that mpv uses it to reuse a palette between video frames. Since mpv is based on libsixel, I guess that this feature could be used by more applications.
Both modes are enabled by default.
The function to convert HLS colors to RGB is a direct port of the implementation of the same function in xterm. I verified that the function emits the same results in all combinations of values 0, 30, 60, 90, and 100 in every color component. Only a few of these combinations were left in the tests to reduce the noise in the code.
To test the parser there are Sixel images generated with 3 different applications. For each one, there is a
.rgba
file in the same directory with the expected RGBA values. The commands to produce these files are inalacritty_terminal/tests/sixel/README.md
.Byte
90
as DCSSixel images using byte
90
as DCS are not supported. DCS can be eitherESC P
or90
, but thevte
crate only recognizesESC P
. I guess that this is because90
can be a continuation byte in a UTF-8 sequence (two most significant bits are10
), so it can be a valid input from users.Xterm has the same limitation. I don't expect that many applications depends on it.
ppmtosixel
is an exception. It uses90
to start the Sixel data, and it has to be replaced if we want to see an image generated by it.It is still interesting to test
ppmtosixel
because it was written in 1991, long before Sixel was added to most (if not all) terminal emulators.