-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
XTSMGRAPHICS "Read sixel graphics geometry" returns the different value from xterm. #656
Comments
@christianparpart doesnt this just return whatever is defined in the EDIT: yes it does contour/src/contour/TerminalSession.cpp Line 964 in 88b4d96
|
Good morning. I'm not sure about how a real VT340 would respond to this, but due to the nature of the fact, that a VT340 is fairly limited to what a modern VTE is capable of, I can't judge for either side. (EDIT: It would not respond at all, as XTSMGRAPHICS is an extension from xterm). But to my understanding, XTSMGRAPHICS is used to query the limits, that's not necessarily the same as how big a screen (terminal viewport in pixels) is. For that there is another VT sequence, @hackerb9, what's your stance on this? Objectively looking at it, I'd rather change lsix to use XTWINOPS on top of XTSMGRAPHICS to query the screen width as the minimum of both values. |
The problem I'm (personally) having with this code, is, that screen size may be changed due to the user, using |
A couple of points worth mentioning:
So personally, I'd recommend apps just rely on |
Sounds absolutely reasonable. Thanks @j4james ;) |
This is just my personal opinion, XTSMGRAPHICS is an XTerm invention, so other terminals should follow XTerm's current spec. As the following explanation,
Additionally, XTWINOPS (CSI 14 t) should return text area size.
The Applications just only are forced to work around for supporting various terminals, but the terminals should not expect the application's work-around effort and should keep the compatibility for the standard as possible. |
I'm currently (today) busy at work, I can respond later tonight more detailed. But I absolutely agree with you here that apps should not suffice from implementation semantic differences. |
My 2cents on XTSMGRAPHICS: The sequence feels weirdly xterm implementation dependent to me, it mostly reflects restrictions/settings on xterm, that other terminals might not have in the same way. In general I think terminal sequences should stay in the general and portable interface realms, and not surface implementation details at all costs, unless the sequence is marked for private usage only. XTSMGRAPHICS fails in that part imho. Now with xterm things are more tricky - it is treated by many app devs and even terminal devs as the de-facto terminal interface standard these days, which makes it hard to ignore any sequence, xterm comes up with. Thats the reason, why I implemented XTSMGRAPHICS as well, but only parts of it, that could be answered reasonably:
For sixel geometry READ it returns the current viewport size in pixels, for READ_MAX I had to cheat and return the square* of the pixel limit of the sixel decoder (default 4096x4096). The last thing is somewhat unlucky, as it is not really limited by that (an image can happily run up to 32k pixel width, if The impl can be found here: https://github.com/jerch/xterm-addon-image/blob/75deeccd20172e753cf8b38846140462d05b9467/src/ImageAddon.ts#L259 Edit: [*] square root 😸 |
Just to be clear, I agree with everyone that is saying you should match XTerm's current behavior if you're going to implement this sequence. But I'd still recommend applications avoid using it if possible.
I fully agree with this. That's why I've just chosen not to implement it. And if I come across an application or library that depends on it, I've encouraged them to avoid that dependency, and support a reasonable fallback. |
Yes, the main problem is that, as the title says, XTSMGRAPHICS "Read sixel graphics geometry" returns the different value from xterm.
I agree with your way.
I agree with you about implementing applications. |
i can change that to match xterm (as much as I dislike that :D). But regardless of that, we could still kindly ask @hackerb9 to change the query order, first test CSI 14 t, and then go for XTSMGRAPHICS if the first fails, or use the minimum value of both (if both return something). Does this sound plausible? |
Hi! Thanks for the kind request. Possible, but not plausible given my current understanding of the situation. First, some ancient history: If there are deficiencies in XTSMGRAPHICS being used for sixel geometry, I'm the one to blame, not XTerm nor XTerm's maintainer, Mr. Dickey. While that extension existed before for changing the pixel depth, I'm the one who proposed using it to also query the sixel geometry and worked with Mr. Dickey to get it working properly in XTerm. The problem at that time, and that still exists with dtterm window ops, like CSI14t, is that window size (or geometry in the old lingo) has been overloaded to mean different things. IIRC, the dtterm escape sequences were meant to be able to position the terminal on the screen, so the "size" originally included the blank border around the text area and even the width of the titlebar and other user interface widgets. While it is a happy accident if CSI14t returns the correct sixel geometry, it is not incorrect for a terminal to do otherwise. Even excluding UI widgets, the dtterm window size may not match the renderable sixel area: a terminal might, for example, choose to always use an 800x480 sixel geometry, no matter what the window size is. On the other hand, if a terminal has implemented XTSMGRAPHICS in a way that it returns something other than the sixel screen size, then it is in error. (And, yes, XTerm itself was in error originally, but that has been fixed since 2019.) So, I think using dtterm's size for sixel geometry is a mistake. But, as always, I'm fallible: Please let me know if I got something wrong. |
I hate to be the one defending the |
Indeed, CSI t is quite convoluted, and even was subject of several security concerns up to CVEs. The WindowOps sequences kinda lost their primary purpose to tune certain desktop integration aspects long ago (mind you, many things simply dont work anymore, as they have no counterpart on modern DEs). So the shift of meaning is a logical consequence as in any evolutionary system (we are just that atm, as we lack a higher authority giving us red/green lines regarding the terminal interface). The problem we all have with this (at least I find it painful) - there is no strict right or wrong in such a scenario, just a more likely/unlikely, which really sucks in terms of strict correctness (computers still need that, unlikely to change until we have QPU-NPUs). Regarding XTSMGRAPHICS I think you should not blame yourself too hard (@hackerb9), while I dont like it for leaking xterm implementation details, it certainly has several pros:
So I try to see things more opportunistic here - since XTSMGRAPHICS is already there, and ppl are likely to use it (its still xterm, we cannot ignore that until some other TE dethrones it) - lets use it to our advantage (thats the reason why I proposed in another thread to put a "reload default palette" option there). Edit: Ah well, I think I will just do that and load the default palette on SET_DEFAULT (technically not quite correct, as it is only meant to change the register numbers, but my impl currently lacks a single sequence for reloading color values only). |
Sorry if it's wrong. I think, Sixel does not have width and height explicitly. If the application uses only In the case, the result of drawing is undefined (depending on the terminal), but application cannot know the case by the XTSMGRAPHICS is required for the application to know the supported sixel dimension by the terminal in advance. So, the application's work-around is the following order:
|
This is a problem that I think is exclusive to XTerm. And one of the effects of this limitation is that
I believe Alacritty is another terminal that still reports the max geometry for
I've never encountered a sixel implementation that supported My recommendation, if you need the cell size, would be |
Thank you for very interesting comment.
I agree with you too.
I updated to minimum of screen size and graphics size:
Thank you for actual and useful recommendation.
My point is that the result of drawing sixel over the supported dimension by the terminal is undefined (depending on the terminal). When XTerm's spec is respected, should the application like lsix do the following ? 1 get maximum image size by XTSMGRAPHIC Pa=4. |
…be capped at terminal viewport dimensions (#656).
…be capped at terminal viewport dimensions (#656).
…limits [terminal] Fixes `XTSMGRAPHICS` when querying sixel image limits, to be capped at terminal viewport dimensions (#656).
@kumattau writes:
Good question. When that happens — which shouldn't be long now that you've patched Alacritty and Contour is already fixed — the answer is pretty simple:
That's it. The purpose of XTSMGRAPHICS with Pₐ = 1 is to provide an explicit way to read the sixel height and width. |
I've been wondering if this bug could have been averted if I had added the following note in the description of XTSMGRAPHICS in XTerm's ctlseqs: The "graphics geometry" attribute is meant for applications to read the size of the visible screen that is available for use. It is never larger than the terminal window, but may be smaller depending upon the terminal implementation. Would that have helped? If so, I'll see about getting it added to |
@hackerb9 to be hoonest? yes. in my case at least. because I looked at ctlseqs.txt and kind of trusted in what it said, given the completeness of that document and the reputation it had, at least the way I perceived it, I did not feel in need to verify every documentation line with its implementation in the xterm source code. Regardless of that, improving documentation is always welcome. :) |
@christianparpart writes:
Thank you. I'm glad I found something that might help others in the future. Before I contact Mr. Dickey about fixing it, was what I wrote sufficient? Could it be clearer? More succinct?
And, this is where my mea culpa becomes mea maxima culpa. I wrote the relevant part of the documentation and Mr. Dickey implemented it in XTerm. It should have been a big hint to me that there was a problem in my spec when he was initially wrote the code so that XTerm's maximum was returned for both Pₐ = 1 and Pₐ = 4. If you wish to know my confusion, read on, otherwise please simply accept my apologies. The problem was that I was writing as an application developer. For applications, knowing the usable screen size is critical and an obvious deficit in DEC's sixel specification. I didn't see any point in reading the maximum geometry value — what would that even mean for an unsettable attribute? Only today did it dawn on me that, for terminal developers, the maximum must be a much more salient number and clarification is needed.
Nor should you have to. The irony is that if you had checked XTerm's source code before 2019, it would have been wrong. Even more ironic: I had been thinking that the error in the XTerm source code wasn't so important since the documentation was clear. Boy, that'll learn me! 😖 Again, please accept my apologies for the confusion. |
Note that the documentation does actually point out that graphics geometry is not necessarily the same as the window size. It's in the "notes" section below the description of the sequence. But I don't think it helps having that information so far removed from where the graphics sequence is described. Anyway, I'm still of the opinion that this is a bug in XTerm that should be fixed. |
I think it is no good to change XTSMGRAPHICS Pa=1 spec because there are terminals which implement XTerm's spec expectedly. But I am interesting the bug-fixed spec details which @j4james thinks. |
Sorry, I wasn't very clear. I didn't mean the Imagine any other graphics library that had a This is not like a function failing under exceptional conditions, like you've run out of memory, or you were trying to load an image 10 times bigger than the screen itself. This is just an image that is size of the terminal window - there's no reason that shouldn't work under normal conditions. |
I second @j4james here, imho xterm's restriction, that the graphical canvas geometry might be smaller than the visible text area, is a no-go these days. (Though I have an idea, where this comes from, the sixel painting is awfully expensive in xterm, esp. with background_select=1, so this might be an optimization to save some drawing time and memory). |
Sorry, I'm not good at English.
I understood that you say XTSMGRAPHICS spec does not have problem but XTerm implementation has problem. I think xterm implementation is off topic in this issue ticket, but I share my test result of displaying 5000 px width sixel on some terminals.
I think the thing that some terminals limit image size is justified by the following reasons:
I think that xterm's 1000px limit is too small and it is better to relax the limit, but I don't think xterm should not have the limit. |
The points you bring up here, account more towards no graphics support at all - which is perfectly fine for a TE, if its environment has strict resource limits (e.g. embedded stuff, kernel console). Then make it optional, e.g. by a compile switch. But if the switch is on, the TE should imho not restrict the graphics area to a smaller size than the allowed viewport. Machines as of today have enough resources to handle that fairly well, this artificial restriction is just an annoyance to create frictions on app dev and terminal user side for no good reasons anymore. So either support full width, or disable graphics output at all. Thats my stance here. SIXEL itself allows up to 32k width - which is very easy to handle, if you do processing in sixel bands, not the whole thing at once (only 6 * 32k * 4 bytes in RGBA8888). And thats from a +30ys old semi-spec - and we still have some decades to go, until we hit that screen resolution area (some cinema projectors operate up to 64k already, but that's not the first audience for terminal output rendering...). |
contour limits image size to the primary screen size. I think that we should have @jerch issue a new ticket to discuss it because it does not relate to XTSMGRAPHICS. |
I am not kicking anyone out, not at all. @kumattau, you have a point to the actual creation of this ticket though. My suggestion: Just create a discussion in the discussion board in this project, if you don't feel too alienated by the fact that this is a Contour TE repo, I am very willing to offer that space for general TE discussions (any type). :-) |
@christianparpart @j4james @jerch I am sorry if I have offended you. For now, I created the following discussion about image size supported by the terminal: #680 |
Contour Terminal version
0.3.1-unreleased-master-7f13fbb0
Installer source
Github: source code cloned
Operating System
Ubuntu Linux 20.04
Architecture
x86-64
Other Software
No response
Steps to reproduce
Expected Behavior
Returned width and height are smaller than or equal to screen width and height (same as the latest xterm).
Actual Behavior
Returned width and height are always 1920 and 1080 regardless of screen size.
Additional notes
By this problem, https://github.com/hackerb9/lsix can not layout pictures as expected position.
contour
xterm
ref: xterm implementation:
https://github.com/ThomasDickey/xterm-snapshots/blob/834b501a1416d7f14b1c38705083f413aea92eee/charproc.c#L4625
The text was updated successfully, but these errors were encountered: