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

video/out/vo_sixel: Implement sixel as a output device #8222

Closed
wants to merge 2 commits into from

Conversation

tantei3
Copy link
Contributor

@tantei3 tantei3 commented Nov 1, 2020

Based on the implementation of ffmpeg's sixel backend output written by Hayaki Saito.

mpv --vo=sixel --vo-sixel-width=<width> --vo-sixel-height=<height> --really-quiet <file> works from a sixel enabled terminal, although depending on the terminal, sixel images of higher dimensions might not render (like in the xterm I use, it doesn't show more than 1000 pixels images, so max resolution is 1000x1000).

Video playback is smooth but surrounding the video there is garbage text being printed. This is resolved by using the --really-quiet flag. But currently the last row of the terminal can't be used for the video display.

Need to add logs at error handling points. Would like to have some feedback about this.

I haven't tested mpv fully as I don't think anything else would be broken, but please let me know how I can proceed, thanks!

@avih
Copy link
Member

avih commented Nov 1, 2020

Also would like to know how I can detect the terminal window resize and control the resolution of the output image generated by mpv as well

The tct vo does this, you should look into it.

In general,you should probably just copy vo_tct.c entirely and replace the rendering part with the sixel output - unless somehow it doesn't fit with the sexel code, but I think it should be compatible.

@tantei3 tantei3 changed the title [RFC] video/out/vo_sixel: Implement sixel as a output device video/out/vo_sixel: Implement sixel as a output device Nov 2, 2020
Copy link
Member

@avih avih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

Some style comments, escape sequences should not be hardcoded inside the code, the usefullness of scroll_on_demand is questionable IMHO.

Also, missing docs - you must add them.


static const unsigned int depth = 3;

static void scroll_on_demand(struct vo* vo)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please choose a better name for this function, or at least add a comment on what it does and when.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to save_image_offset_location

new_termios.c_cc[VTIME] = 0;
tcsetattr(STDIN_FILENO, TCSAFLUSH, &new_termios);

/* request cursor position report */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we care where the cursor is? shouldn't vo-sixel just move the cursor to wherever it wants regardless of where it was?

tv.tv_usec = 0;
FD_ZERO(&rfds);
FD_SET(STDIN_FILENO, &rfds);
ret = select(STDIN_FILENO + 1, &rfds, NULL, NULL, &tv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even if we do really need the cursor position, isn't there some ioctl which can retrieve it without using terminal-specific escape sequences? For instance, did you check if this escape sequence works in urxvt (even if it doesn't have sixel, the point is that the sequence is hardcoded).

tcsetattr(STDIN_FILENO, TCSAFLUSH, &new_termios);

/* request cursor position report */
fprintf(sixel_output_file, "\033[6n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever you use an escpe sequence at the code, please define it at the top of the file, similar to ESC_HIDE_CURSOR (fixed string) or ESC_GOTOXY (parametric).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

priv->image_height = params->h;
priv->image_width = params->w;
priv->image_format = params->imgfmt;
scroll_on_demand(vo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we remove the scroll_on_demand(vo) call? maybe we can just get rid of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work without scroll_on_demand, I've modified the function to only store the offset location so that people may be able to use --vo-sixel-offset-top and --vo-sixel-offset-left to set the offset from topleft position. In the future, might be able to use TIOCGWINSZ ioctl to also get the resolution in pixels of the terminal window and choose a better default resolution for the image, but for the starting simple implementation just setting the offsets only and rendering based on user specified dimensions.

if (priv->opts->fixedpal) {
status = prepare_static_palette(vo);
} else {
status = prepare_dynamic_palette(vo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I underdstand correctly that by default (dynamic palette) scene change is checked on every frame? how much CPU overhead does it add compared to fixed palette?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is checked for every frame unless --vo-sixel-fixedpallete=1 is set. I'm not sure how much is the overhead (would be great if you can let me know how to compute that) but I didn't notice significant frame drops.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, measuring performance is not always easy, but for starters you could check the mpv cpu usage with htop or some graphical resource monitor or some such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, this can also add a meaningful overhead at the terminal itself, because other than detecting the change, when it is detected - it needs to upload a new set of colors into the terminal, and that may or may not be expensive at the terminal side.

How good/bad does it look with a fixed palette and dithering? I'm wondering whether fixed+dither should be the default instead of dynamic. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried looking at the libsixel functions, but it doesn't seem to be going through image data while computing the dynamic scene change, or while building static palette either. At higher resolutions both methods seemed to have high framedrops. Although in mlterm at least with static palette images aren't distorted. So might be worth switching the default.

} else {
status = prepare_dynamic_palette(vo);
}
if (SIXEL_FAILED(status)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: no need for braces if it only executes one command.

priv->dither = NULL;
status = sixel_dither_new(&priv->testdither, priv->opts->reqcolors, NULL);

if (SIXEL_FAILED(status)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: remove the braces (and elsewhere too in such cases, if there are any).

static int query_format(struct vo *vo, int format)
{
return format == IMGFMT;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the extra empty line.

Copy link
Member

@avih avih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that's better, thanks. Most of the comments are questions, so please reply before making further changes. Thanks.

xor
Positionally stable arithmetic xor based dither

``--vo-sixel-width=<width>`` ``--vo-sixel-height=<height>``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the size in pixels relate to the size in character cells? can it default to fill the terminal size until the left/right edges or the top/bottom (sans 1 line) touch the edges, while the other edges are centered (similar to tct)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the ioctl TIOCGWINSZ then we should be able to get the dimensions of the terminal window in pixels as well as the character width and height as well. So should be able to calculate the optimal size of the terminal window (height_of_terminal * (num_rows -1 )/num_rows) While for the width we can select the terminal full width generally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to use it directly. Mpv already has terminal_get_size, and you only need to include #include "osdep/terminal.h" - like vo_tct.c does.

If that's the only only missing thing to make the video fit automatically, then maybe we should change the width/height value be in character cells units rather than pixels?

Do you need to figure out how many pixels are in each character? Again, who figures the relation between character cells to pixels, and how? what happens if the terminal changes the font size (and keeping the size in cells) while a video is playing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

terminal_get_size just returns the cell counts like 80x25. That is not enough info to map to pixels. Like the terminal on a 1920x1080 monitor might have different pixel width, height compared to a 4k monitor. It also depends on the font size of the terminal set.
That is why the ioctl call is required to get the pixels of terminal window. Plus on resizing the terminal window, you could make the bigger image fit if you started with a smaller terminal initially.
Also then aspect ratio might have to be preserved along with the terminal pixel dimensions. So it is a bit tricky part.
Therefore currently it is fully user specified which I agree is not good user experience.

if (left <= 0 || top > terminal_width)
left = 1;

/*We want to save the offset row and column
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to save it? You know exactly where the image should start - either given by the offset options or because you calculate it yourself, at which case you don't need to save it by the terminal - you know the values and can just move the cursor yourself to that location, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can replace this function with the GOTOXY sequence. But I think it requires printing more characters instead of just two bytes in \0338 which might be insignificant per frame. The saving of coordinates happens just once per resize function callback so to me from mpv perspective, it should be more efficient to just restore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It indeed completely insignificant compared to the image data.

I think I prefer explicit move of the cursor, because otherwise you can't really tell what the terminal would do, especially if scroll happened between the save and restore. Let's make it explicit.


// Make sure that the user specified top offset
// lies in the range 1 to TERMINAL_HEIGHT
// Otherwise default to the topmost row
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. The allowed offset (horizontal and vertical) should be such that it both starts and ENDs within the terminal area. Why do you test only where it starts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you can start with an initial terminal that is smaller in dimensions, and start playback with only like some parts of video being displayed, and later resize the terminal and have sixel show the other parts to the right and bottom.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would it work if parts of the video end outside the terminal? Does it still work smoothly? no extra scrolling or flickering?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the height exceeds then there is flickering. But if the width exceeds, there is no issue as far as I have observed, and on increasing the window it smoothly displays.


``--vo-sixel-width=<width>`` ``--vo-sixel-height=<height>``
The output video resolution will be set to width and height
These default to 320x240 if not set. The terminal window must
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can the user ensure that the terminal size in pixel is bigger than this? typically the user can tell the size in character cells, but not in pixels, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is difficult to ensure in pixels, but the user can then drag and resize their terminal window to get the optimal window without stuttering. It is just recommended to have bigger window otherwise especially if the height is bigger then there is flickering effects due to sixel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my earlier comment on the subject, but in general - mainly that I don't know if it's possible or easy enough, but I think that if we can change the dimensions to be char-cell-based rather than pixels, then it would be a more natural fit for a terminal output.

mp_sws_scale(priv->sws, priv->frame, &src);

// Copy from mpv to RGB format as required by libsixel
memcpy_pic(priv->buffer, priv->frame->planes[0], priv->opts->width * depth, priv->opts->height,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an mpv image format which is already suitable for libsixel without this additional conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll ask in the mpv-devel group since I don't have extensive knowledge about mpv APIs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's leave it like this for now. We might be able to improve it later.

{
struct priv *priv = vo->priv;

printf(ESC_TERMINATE_DCS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the DCS sequence start? If that's the entire image in a DCS sequence, shouldn't it start and end on each frame?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DCS I think is set by the sixel encode function. My guess is that if the player exits and sixel output is interrupted and then this function for cleanup gets called, it exits gracefully (could be wrong though)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean "my guess"? did you try without it? Do you have a use case where using it makes a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't affect playback, only the cleanup part on exit. So playback will not be affected regardless of whether this line is removed or it exists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't asking about playback. I see it's called during uninit. I'm asking if you know of a use case where it makes a difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested without it, and didn't cause any noticeable issue for me on xterm and mlterm, so I have removed it as per your suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't suggest to remove it. I asked you to understand why it's there, and in what use cases this termination helps with some things.

My guess is that at most it's useful in some abort conditions (crash or killed externally), but at such case I don't even know if uninit would be called.

Anyway, it's OK that you removed it - let's keep it that way, but if you can still figure out why it was there in the first place, it would be best.

{
struct priv *priv = vo->priv;

talloc_free(priv->buffer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that priv->buffer, priv->dither, and priv->testdither are always deallocated together, maybe move this sequence to its own function?

Also, is this called on terminal resize? or mpv video resize for whatever reason?

What happens on terminal resize? I don't think i've seen code which handles it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priv->dither is deallocated during the dynamic palette computation as well. Can club them in a function.

So terminal resize is not handled because mpv doesn't detect it (resize callback is not called). Even the behaviour is the same for tct and caca. We'd have to check each time in draw frame, or maybe periodically once per second or so to detect if terminal size has been changed and run resize accordingly, but not sure if including this check in every draw_image call would be optimal way of doing it. Probably resize is called if zoom or some other keybinding to change the input resolution is triggered (don't know how to trigger this).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better way would be to add keybindings for zoom and shifts horizontal and vertical and probably a reset/optimum_screen keybindng which optimally computes using TIOCGWINSZ ioctl and which calls resize callback function. Let the user then accordingly zoom and move seems better option to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's forget terminal resize for now. Maybe at a later stage we'll be able to handle it with sixel and tct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priv->buffer could possibly be deallocated automatically via talloc (check if the vo already has a talloc context).

@avih
Copy link
Member

avih commented Nov 3, 2020

Overall looks good to me. Let's wait a day or two to see if others have further comments, and then merge.

Meanwhile, you can start thinking about how to automate the size. I think maybe create a similar function to get_terminal_size but which also returns the pixel dimensions, and see if you can get it right. If you figure it out well, then we'll make another PR after this one is merged with the size automation.

Good work.

@tantei3
Copy link
Contributor Author

tantei3 commented Nov 3, 2020

Overall looks good to me. Let's wait a day or two to see if others have further comments, and then merge.

Meanwhile, you can start thinking about how to automate the size. I think maybe create a similar function to get_terminal_size but which also returns the pixel dimensions, and see if you can get it right. If you figure it out well, then we'll make another PR after this one is merged with the size automation.

Good work.

Already on it! I just need to figure out the deal with aspect ratio and the centering parts. I'll wait till this one is merged before submitting PR for the best resolution initialization patch.

    struct priv *priv = vo->priv;
    struct winsize size = {0, 0, 0, 0};
    ioctl(STDOUT_FILENO, TIOCGWINSZ, &size);
    priv->opts->width = size.ws_xpixel;
    int height_y = size.ws_ypixel;
    priv->opts->height = (height_y / size.ws_row) * (size.ws_row - 1) - 2;

This code seems to be working at the moment for scaling to the full terminal, but aspect ratio calculation and centering is left.

Based on the implementation of ffmpeg's sixel backend output written
by Hayaki Saito
https://github.com/saitoha/FFmpeg-SIXEL/blob/sixel/libavdevice/sixel.c

Sixel is a protocol to display graphics in a terminal. This commit
adds support to play videos on a sixel enabled terminal using libsixel.
With --vo=sixel, the output will be in sixel format.

The input frame will be scaled to the user specified resolution
(--vo-sixel-width and --vo-sixel-height) using swscaler and then
encoded using libsixel and output to the terminal. This method
requires high cpu and there are high frame drops for 720p and
higher resolution videos and might require using lesser colors and
have drop in quality.  Docs have all the supported options listed
to fine tune the output quality.

TODO: A few parameters of libsixel such as the sixel_encode_policy
and the SIXEL_XTERM16 variables are hardcoded, might want to
expose them as command line options. Also the initialization
resolution is not automatic and if the user doesn't specify the
dimensions, it picks 320x240 as the default resolution which is not
optimal. So need to automatically pick the best fit resolution for
the current open terminal window size.
@avih
Copy link
Member

avih commented Nov 5, 2020

Thanks. Last commit looks good to me.
@jeeb is that how you wanted the options?

@avih
Copy link
Member

avih commented Nov 7, 2020

Merged in master. Thanks for the patience.

Now do auto size in another PR please :)

@avih avih closed this Nov 7, 2020
@jerch
Copy link

jerch commented Nov 29, 2020

SIXEL - one of the worst image formats we have, and now for video streaming, haha.

However I tested it with my new image addon for xterm.js and it works great with the quiet switch (see xtermjs/xterm.js#2503).

A few notes regarding things discussed above:

  • ioctl for getting terminal pixel dimensions:
    Thats not widely supported yet, only a few terminals update winsize.ws_xpixel/es_ypixel currently. This also might not change soon as some systems dont even have those struct members, and it is not yet officially in POSIX (here is an attempt to spec it out as part of termios: https://austingroupbugs.net/view.php?id=1151).
    Thus it might be a good idea to at least have another failover by requesting the pixel size from the terminal directly with CSI 14 t.
  • About sending the sequence finalizer upon an error:
    Stopping a DCS stream without proper finalization might render the terminal unsable (hard to recover by normal user interaction as keystrokes would just add to the DCS subparser state). If something bad happens and you still got time to clean up things, it is a good idea to send the finalizer, no matter how badly the terminal state is screwed up. This way the terminal would at least fall to GROUND state, where the user can interact normally again. Sending it twice doesnt hurt either, an ECMA conformant terminal would just ignore excessive STs (the finalizer is ST, or in 7bit ESC \).
  • Issues with term size changes
    Listening on SIGWINCH might be a starting point, but it does not cover changes in pixel-per-cell coverage yet, it only reports cols/rows. A workaround might be to re-request the pixels on SIGWINCH. Changes in font scaling cannot be spotted with that, the mentioned proposal above also tries to deal with that.

@avih
Copy link
Member

avih commented Nov 29, 2020

* Thats not widely supported yet, only a few terminals update  `winsize.ws_xpixel/es_ypixel` currently.

That's not my observation. Mind listing the terminals you tested, and note which of them don't support it?

This also might not change soon as some systems dont even have those struct members

On which systems do these members not exist? On such case it's our bug and we should test it at configure.

and it is not yet officially in POSIX (here is an attempt to spec it out as part of termios: https://austingroupbugs.net/view.php?id=1151).

Yes, I'm aware of the posix terminal dimensions thread. I'm not seeing it concluded in the near future, and for now as far as I can tell the ioctl method is supported on many systems.

Thus it might be a good idea to at least have another failover by requesting the pixel size from the terminal directly with CSI 14 t.

Not a bad idea to support other methods, but I think (not 100% sure though) that this sequence is even less supported than the ioctl method, and equally (if not more) non-standard. Regardless though, currently the sixel vo is a write-only, and it might not be easy to integrate two-way communication using sequences, at the very least because input from the terminal (reply for such query sequence) is handled elsewhere - at the kb input handler.

Stopping a DCS stream without proper finalization might render the terminal unsable (hard to recover by normal user interaction as keystrokes would just add to the DCS subparser state). If something bad happens and you still got time to clean up things ...

This is an issue we don't currently have a good solution for. Main case is crash or abort (ctrl-c). As far as the sixel vo itself - it cannot/doesn't quit mid-sequence unless the process is aborted - so it does do the best it can under the circumstances. If you have practical solution for this issue - shoot.

Issues with term size changes ...

Yup, we don't yet have a solution for that either. Initial approach, if implemented would be with SIGWINCH and/or occasionally polling the dimensions (currently we do ioctl, but that does not preclude other methods).

Either way, practical improvements in the form of pull requests are welcome.

@jerch
Copy link

jerch commented Nov 29, 2020

Mind listing the terminals you tested, and note which of them don't support it?

Here we go (only tested on Ubuntu 18 for now):

  • gnome-terminal - no
  • xterm - yes
  • urxvt - yes
  • konsole - no
  • aterm - yes
  • alacritty - yes
  • tmux - no
  • screen - yes

Note that vte based terminals + konsole cover roughly 70-80% of terminal usage on linux, both dont support it. No clue if they have patched it yet upstream.

Cannot test different systems right now, will see if I get a hold of them during the next days. As far as I remember there were issues on Solaris and some BSD flavor. On windows ConPty does not support it, not sure about mintty (cygwin?). Also SSH in general will be problematic (but thats a different story, as we have no sound transport), same with multiplexer (detached or multiheaded).

This is an issue we don't currently have a good solution for. Main case is crash or abort (ctrl-c). As far as the sixel vo itself - it cannot/doesn't quit mid-sequence unless the process is aborted - so it does do the best it can under the circumstances. If you have practical solution for this issue - shoot.

Yeah well, in case of segfaults / kernel pulling the plug for whatever reason not much can be done - the terminal will most likely be trash afterwards. SIGINT/SIGSTP (Ctrl-Z) can be caught, their handlers would be a candidate for sending the finalizer unconditionally before shutting down/pausing.

... SIGWINCH and/or occasionally polling the dimensions ...

Sounds reasonable given what is possible currently.

@avih
Copy link
Member

avih commented Nov 29, 2020

* tmux - no

That's actually not my experience. In fact, I was highly surprised when tmux reported the size in pixels correctly at the following setup:

  • Windows with cygwin running tmux inside st (on cygwin-x).
  • From a pane in tmux ssh into Ubuntu (20.04 LTS).
  • On the ubuntu system run a program which reads this ioctl value.

The reported size in pixels matched the pane content size both when zoomed and when not zoomed. I did not expect that to work at all - but it did, out of the box.

I can also add st to the list of terminals which support the size in pixels via ioctl.

However, you didn't list which of the terminals support the alternative you suggested. Do all of them support it?

@avih
Copy link
Member

avih commented Nov 29, 2020

Cannot test different systems right now, will see if I get a hold of them during the next days. As far as I remember there were issues on Solaris and some BSD flavor. On windows ConPty does not support it, not sure about mintty (cygwin?). Also SSH in general will be problematic (but thats a different story, as we have no sound transport), same with multiplexer (detached or multiheaded).

Here are my test resuts for the following program:

#include <stdio.h>
#include <termios.h>
#include <sys/ioctl.h>

int main(int argc, char **argv) {
    struct winsize ws;
    if (ioctl(2, TIOCGWINSZ, &ws) < 0) {
        fprintf(stderr, "tsize: ioctl failed\n");
        return 1;
    }
    printf("cells:%dx%d  pixels:%dx%d  ratios(cell px):%.3fx%.3f \n",
           ws.ws_col, ws.ws_row, ws.ws_xpixel, ws.ws_ypixel,
           (double)ws.ws_xpixel/ws.ws_col, (double)ws.ws_ypixel/ws.ws_row);

    return 0;
}

On Linux, OpenBSD, NetBSD, illumos (OnmiOS), Cygwin, macOS (10.13): works fine and report the size in pixels correctly.

On FreeBSD (12.1) the members exist but end up 0, 0 (which mpv identifies as invalid and falls back to other values).

EDIT - correction - also works fine on FreeBSD. Initially I tested with VTE (xfce4-terminal) which reported 0x0 pixels, but with xterm the correct size was reported.

And on Windows ConPTY doesn't support sixel, so for now it's a moot point.

The ioctl report also apparently works fine over ssh - as long as the containing terminal can report the correct size.

So if you actually know of systems where the members don't exist then we do need to test it at configure, but as far as I can tell, at all the systems which I tested not only do they exist - they also work correctly when the terminal reports the values correctly.

@avih
Copy link
Member

avih commented Nov 30, 2020

To summarize, of the terminals which I do know that they support sixel (xterm, mlterm, iTerm2, mintty, some patched version of st) - all of them report a usable size in pixels via ioctl, and this works on all systems I have access to which I listed above.

And in the terminals which do support reporting the size in pixels regardless of sixel, you can add to your list: iTerm2, kitty, st.

It is true that konsole and VTE do not report it, but they're irrelevant because they don't have sixel. My guess is that when they do support sixels, if that ever happens - they'll also report the size correctly.

@philipl
Copy link
Member

philipl commented Nov 30, 2020

Latest vte master has some non trivial sixel support. There are various issues still open around it too.

@avih
Copy link
Member

avih commented Nov 30, 2020

Latest vte master has some non trivial sixel support. There are various issues still open around it too.

My guess is that when they do support sixels, if that ever happens - they'll also report the size correctly.

https://github.com/GNOME/vte/blob/97d2e87fa196e4023bf658bc56701cac27063434/src/pty.cc#L297 :

#ifdef WITH_SIXEL
        size.ws_ypixel = size.ws_row * cell_height_px;
        size.ws_xpixel = size.ws_col * cell_width_px;
#endif
	_vte_debug_print(VTE_DEBUG_PTY,
			"Setting size on fd %d to (%d,%d).\n",
			master, columns, rows);
        auto ret = ioctl(master, TIOCSWINSZ, &size);

So only konsole remains.

@avih
Copy link
Member

avih commented Dec 3, 2020

... as some systems dont even have those struct members ...

@jerch I'd still highly appreciate an example of such system, because this means it's a bug that we don't test this at configure.

However, do note that the module which xterm.js uses - node-pty does make the same assumption - that these members are always available on unix systems. So in fact, on any unix system which node-pty runs it can also report the size in pixels.

The fact that it doesn't do that (it hardcodes reports of 0, 0 pixels) is an issue with node-pty. I do see that you tried to request it - microsoft/node-pty#266 but that seems stalled. IMHO you should ping them or just send a PR to support reporting the size in pixels.

Because so far, other than xterm.js, all the terminals which we mentioned so far which support sixel - also report the size in pixels using ioctl, and of all the terminals which were mentioned regardless of sixel - only konsole doesn't report it.

@jerch
Copy link

jerch commented Dec 3, 2020

@avih Thx for testing out, saves me some time 👍

Yes I rechecked the still alive POSIX systems (up to AIX) - they all have the pixel members in winsize. So it is only ConPty on Windows which cannot report it currently. I was pretty sure that OpenBSD had some issue reporting the values, but cannot remember anymore why as I tested that like 3ys ago. Guess my test was flawed there (as yours worked).

Yes the node-pty issue is stalled, not sure if that will move anytime soon as the whole module is on trial (the way it is built with node-gyp creates tons of compat issues).

@jerch
Copy link

jerch commented Dec 3, 2020

About tmux or a multplexer in general:

Yes they can promote the correct pixel values, as long as they are connected to a final terminal, which provides correct numbers (they would simply read the ioctl on the second PTY). But that gets really awkward for detached session or multihead. Months ago @nicm indicated to try to deal with detached sessions and missing pixel values in ioctl with a fixed pixel cell size of 7x14, not sure about the multihead situation with different ioctl values from all child ptys.

@avih
Copy link
Member

avih commented Dec 3, 2020

But that gets really awkward for detached session or multihead

Yes, it can be awkward under some conditions. However, in such case I doubt that any other method would work as well - if tmux just dosesn't know the value then it dosn't know it - using a different method to report it won't help with that.

I.e. the context in which you mentioned (wrongly) that tmux doesn't report the size in pixels with ioctl was that we should use a different way to query the terminal - but this would not be useful at this case.

And if it reports a made-up value, then it would be the same value wih both report methods.

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.

6 participants