-
Notifications
You must be signed in to change notification settings - Fork 78
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
timg is not selecting sixel on Windows Terminal Preview #145
Comments
The way timg currently detects the supported protocol is with an XTVERSION query that returns the terminal name, which is compared against a hardcoded list of terminals. This is not ideal, and misses quite a few existing Sixel terminals. The preferred way to detect Sixel is with a Primary Device Attributes query, which should report a 4 in the extension list on terminals that support Sixel. |
Determining the features is a careful dance, as not all queries to determine features will work and some actually end up messing up some terminals as they can't deal with the escape codes they were given (e.g. the kitty graphics query is known to make some terminals glitch). So only queries that don't interfere are selected for such things. There is one additional property to figure out: sixel terminal implementations seem to behave slightly differently w.r.t. cursor placement after an image had been sent; which requires some manual testing and hardcoding right now (setting the Anyway, while I investigate implementing the fallback solution, let's add your terminal so the hardcoded list.
To determine what kind of sixel workaround we need: if you run an animation
Does it work correctly or is the image 'wandering' up or down ? ... or show multiple images in a grid:
... do they align properly or look like they are arranged in a staircase ? |
Use this as a fallback query if the primary query based on environment variables and terminal name is not conclusive. The DA1 query might tell us if the terminals support the Sixel protocol. Best effort: we will still not know if the newline workaround is needed. While at it, some smallish clean-ups, e.g. low-level memmem() is only used in one location. Issue #145
I've implemented the Primary Device Attributes query to determine Sixel capability. Please try again from head to see if this works for you (please also test if animations and grid work as described before, as that behavior can't be auto-detected AFAIK) |
Nothing prints. (Tested on vanilla bash shell)
The image is pushed to the top of the terminal, flickering between a random colored background and frames.
This works well, but also with a random colored background. This was tested in latest commit.
|
With the latest commit: does it work without |
No, it still uses fallback with Unicode characters. |
Sounds like the terminal is neither implementing the For now, I recommend to set the environment variable |
Thank you. I will make an issue on the WT repository. |
Windows Terminal doesn't support the I have some feedback on your fix though. If I'm reading it correctly, I think the test below will produce false positives on terminals that implement features like the Latin-2 character set (which is identified by the number 42), but which don't support Sixel. Line 293 in dbe0299
And regarding the comment below, that first value identifies the terminal conformance level. technically it could be anything in the range 61 to 65. The example in the linked documentation is 64 because it's identifying a level 4 device. Lines 285 to 288 in dbe0299
So what you have there is technically correct as it is, but it's worth noting that some modern terminal emulators don't use a strictly correct DA response, so you may fail to detect them. Some examples that I've encountered in the past:
I don't think it's a big deal if you choose to ignore those, though, since people can always still use the
This is the result of timg wrapping the libsixel output with an extra Sixel introducer. See here: Line 130 in dbe0299
So the start of the Sixel image sequence actually ends up being There's also an unnecessary extra Line 147 in dbe0299
|
The ambiguity between 4 and 42 was deliberate as I see this as a best-effort fallback solution, and I have also not seen any terminal I played with return that. If it becomes a problem, this should be addressed (only a line more code). The DA responses that don't start with 6 might be more of an issue. Maybe it is a good idea to simply look for If you see something in the sixel implementation that causes issues like the blank image situation, please send a pull request. I don't remember exactly why these code-pieces are in there, it might well be that it was needed when testing on the various terminals when I implemented it. |
I've personally seen a few - the current release version of Windows Terminal being one example - but I've just tested that now, and surprisingly it still works with the latest commit. I'm guessing it's because you can't detect the font size, and without that you won't even attempt to detect sixel. I'm not sure about the other terminals, but I suspect they probably wouldn't report the font size either, so this is likely not as big a deal as I initially thought.
OK. I might give that a go this weekend. |
This can help figuring out details while attempting to detect new terminals. Issues: #145
I already changed the detection so that only 4 not 42 is detected. I've now added information about what happens in the terminal query phase if Output looks something like this (here on my local terminal):
|
Thanks. I only noticed that after I'd sent my message, but I've tested both versions now and can confirm that they both work. And regarding the
And this is what I get on the stable version, Windows Terminal v1.21.3231.0 (which doesn't have sixel):
Note that |
I should also mention that the "default cursor placement" is technically incorrect, so you tend to get text overlapping the bottom of images in Windows Terminal, and the grid layouts produce the staircase pattern you mentioned (you may have already noticed this in the screenshot that gestate-8 posted above). But if that works best for the majority of your users, then I'm not expecting you to fix it. Although a command line override for the cursor placement might be a useful option if you don't already have one. |
I tested again and sixel works well without What does the 'staircase' effect refer to? Does that mean images not being centered in each grid column, making them appear like a staircase? |
Staircase effect would be if the images in a grid would not be all aligned well vertically, but would go one character up with each image in a row. So if that looks great then I think we're good here. @j4james will have a look if the outputs at the beginning and end of the image can actually be removed (which might improve the background color glitch) |
As suggested in #145 (comment) I've now added a way to choose the handling of newlines in sixel terminals. TIMG_SIXEL_NEWLINE_WORKAROUND With that, everything should be covered to fix this issue. |
With |
I think that is a problem of the terminal, not properly reporting the size in pixels of their fonts. I suspect that is something like that, so not much |
The Windows Terminal sixel implementation is designed to be compatible with the original VT340, so it emulates a cell size of 10x20 pixels, regardless of the font used (and that's what it reports to apps querying The source of the problem is likely the algorithm used to determine where the cursor ends up after a sixel image has been output. On a real VT340 - which again is what Windows Terminal emulates - the final cursor position overlaps the bottom of the image (this ensures that sixels can be output on the last row of the display without triggering a scroll). To be more precise, the cursor should be on the text row that intersects the top of the last sixel row. So if the last sixel row spans two text rows, the cursor ends up on the top one (this allows for images inlined with text). Very few modern terminals follow this algorithm correctly, though, and there is a lot of variation in the cursor placement algorithms that they do use. I think I counted around 8 last time I tested, although in practice it's probably not as bad as that for an app that always produces images in a specific format. It also doesn't help that terminals sometimes change the algorithm they're using, so it's a bit of a moving target. In short, it's almost impossible for an app to predicate where the cursor is going to end up for an image of arbitrary size. The fact that you've managed to narrow it down to one of two algorithms that work for the majority of your user base is impressive. But it's never going to work for everyone, all of the time. |
Thanks for your insights! (now I am almost tempted to get an original vt340 from ebay if one shows up :) ).
I am wondering what triggers the issues you see sometimes; it sounds like there might be off-by-one situations in which the terminal places the cursor differently from what timg thinks. Is it with smaller images that don't fill the full screen, but have a particular height ? Or is it with images that are filling the full terminal (e.g. with |
There's a guy who did exactly that a couple of years ago, and he has a repo on github where he documents all the information he has gathered about the terminal. See hackerb9/vt340test. And if you decide to get one of your own, he has some useful notes on what USB serial adapters work best for hooking up the terminal to a PC, and things of that sort (for example, see the flow control page).
I haven't looked at it in much detail, but the one obvious pattern I noticed is when testing with an animated gif and setting the cell height with Bearing in mind that the cell height is always 20 pixels on Windows Terminal, so the working cases are all a multiple 60 pixels, which means they're an exact number of sixel rows. But an image that is say 2 text cells high, would be 40 pixels, which is 6.666 sixel rows, so the last sixel row would end up overlapping with the third text row. Some terminals will then leave the cursor on that third row, but it should really be on the second row. I'm assuming timg expects the former, and that's why it fails on Windows Terminal. |
The calculation for how far to jump up in character cells given an image with a certain pixel height is in cell_height_for_pixels() ... maybe this is not calculating the correct thing ? (and then tested in the myriad of other Sixel terminals...) I am open for pull requests :) |
I could "fix" it so that it's VT340 compatible, and thus should work in Windows Terminal, but there's a good chance it'll then no longer work in multiple Linux terminals. And since I expect you have more Linux users than Windows users, that doesn't seem like a great idea. Maybe there'll eventually come a time when all terminals agree to follow standards, and they'll work in the same way, but until then you kind of need to pick a side. Or as I suggested before, find a way to avoid relying on the cursor placement after an image has been output. |
If windows terminal would implement the Should be fairly straightforward for the terminal to implement, and then |
Most of the issues are addressed here. Leaving this open to refine the automatic application of at least the newlne workaround once The somtimes-scroll issue might then be worked around then as well. |
If the cursor position is not rounded up but rounded down, would the following cure the scrolling issue you observed with some picture heights @j4james ? --- a/src/sixel-canvas.cc
+++ b/src/sixel-canvas.cc
@@ -156,8 +156,7 @@ int SixelCanvas::cell_height_for_pixels(int pixels) const {
// Unlike the other exact pixel canvases where we have to round to the
// next even cell_y_px, here we first need to round to the next even 6
// first.
- return -((round_to_sixel(pixels) + options_.cell_y_px - 1) /
- options_.cell_y_px);
+ return -(round_to_sixel(pixels) / options_.cell_y_px);
}
} // namespace timg |
That's not quite right. You need something more like this. // This is the pixel offset of the final sixel row.
int final_sixel_offset = round_to_sixel(pixels) - 6;
// This is the cell/row offset of the final cursor position.
int final_cell_offset = final_sixel_offset / options_.cell_y_px;
// Calling this the cell height is somewhat misleading, but if you're
// just using this to reverse the cursor movement, it should suffice.
int cell_height = final_cell_offset + 1;
return -cell_height; The above is deliberately verbose to make the reasoning clear, but it could just be: return -((round_to_sixel(pixels) - 6) / options_.cell_y_px + 1); Either way, note that this still requires |
Thanks. So once we can recognize the windows terminal by name ( |
Right now only known to be implemented by development version of Windows Terminal. Until we can detect that this is the terminal we're running, requires to set environment variable `TIMG_SIXEL_FULL_CELL_JUMP=1` Issues: #145
Right now only known to be implemented by development version of Windows Terminal. Until we can detect that this is the terminal we're running, requires to set environment variable `TIMG_SIXEL_FULL_CELL_JUMP=1` Issues: #145
Right now only known to be implemented by development version of Windows Terminal. Until we can detect that this is the terminal we're running, requires to set environment variable `TIMG_SIXEL_FULL_CELL_JUMP=1` Issues: #145
Right now only known to be implemented by development version of Windows Terminal. `TIMG_SIXEL_NEWLINE_WORKAROUND` now is a bitmap that allows for the various newline behaviors to be set * 1 = regular newline/carriage return choice * 2 = full cell jump (this new feature) * 3 = the OR'ed value out of 1 and 2 Until we can detect that this is the terminal we're running, requires to set environment variable; for windows terminal, that is `TIMG_SIXEL_NEWLINE_WORKAROUND=3`. Issues: #145
Right now only known to be implemented by development version of Windows Terminal. `TIMG_SIXEL_NEWLINE_WORKAROUND` now is a bitmap that allows for the various newline behaviors to be set * 1 = regular newline/carriage return choice * 2 = full cell jump (this new feature) * 3 = the OR'ed value out of 1 and 2 Until we can detect that this is the terminal we're running, requires to set environment variable; for windows terminal, that is `TIMG_SIXEL_NEWLINE_WORKAROUND=3`. Issues: #145
Alright, merged the other PR, so now the windows terminal should work with environment variable set to |
Windows Terminal Preview supports sixel images.
timg is not auto-detecting sixel support on WT.
timg -ps
works well.The text was updated successfully, but these errors were encountered: