Now moves cursor vertically for sixel newlines even if there is no sixel #825

Utkarsh-khambra · 2022-09-20T17:30:10Z

fixes #822

To test this use this script. Since the script requires cell height to be 20px, you can change font size to be 11 in default config with monospace font family.

christianparpart · 2022-09-20T17:51:47Z

Many thanks!

@Utkarsh-khambra with that, I don't think we do not need the configuration option images.sixel_cursor_conformance (bool) anymore as I was clearly misunderstanding/misreading/missing some of the Sixel documentation back in the days. I think it's save to completely get rid of that config option. ;)

src/terminal/SixelParser.cpp

src/terminal/SixelParser_test.cpp

Utkarsh-khambra · 2022-09-20T19:26:03Z

src/terminal/Screen.cpp

-        moveCursorTo(_topLeft.line + unbox<int>(linesToBeRendered) - 1, _topLeft.column);
+        moveCursorTo(_topLeft.line + unbox<int>(linesToBeRendered), _topLeft.column);


from this test

Test of the text cursor position after a Sixel image is output.
On a genuine VT340, after an image is output, the text cursor position is
set to the same row as the top of the final sixel line (regardless of
whether anything is output on that line). The column position remains
unchanged, i.e. it should align with the left of the image.

@christianparpart this change should place the ansi cursor to last sixel row, right?

that is how i read it. any objections, @jerch?

If that's the case I think this is ready to merge.

I think "top of final sixel line" here means to clip the text cursor into the text row, wherever the first of the 6 pixel lines (last sixel band) ended up above. So its not the last line (sixth pixel line), but the first one determining it. Note that this has a weird consequence, if the last sixel band is not perfectly mapped on text line borders, possibly cutting off lower pixel rows, or not trigger scrolling at the end of the screen.
My impl still uses the last pixel line for this, but I prolly will change that.

Edit: Added another question, whether my assumption about the first pixel line is correct, here: jerch/xterm-addon-image#37 (comment)

So that means you may overwrite pixels in the last sixel line, if you print text just after drawing a sixel image.

Yes thats how I understand it so far. But lets see if it gets confirmed...

The VT340 definitely does overwrite pixels in the last sixel line, but the cases which trigger it happen very rarely as long as a text newline is sent after the sixel image. Here is an image from some of the testing I had done to figure out where the text cursor would be positioned based on line endings (only NL, only GNL, or neither).

First, I should state that it seems like the intention was for Sixel images to not include a GLF at the end, but for there to be a text LF after the image if text or another image is going to be written. As @j4james pointed out to me, this allows one to show a full screen sixel image without causing scrolling.

Second, when only LF is used at the end of the sixel data, 90% of the time there will be no overwrite. In actual practice, the glitch is much rarer than 10%. That is because image heights are often divisible by ten or a power of two or the character cell height, none of which can trigger what I call the VT340 text cursor glitch.

Personally, I think it is a programming mistake in the VT340 firmware since I cannot see anything in the documentation that would tip off an application developer to expect that behaviour. And, there does not appear to be any point to it.

I am not sure how to emulate it, but you can predict it on the VT340. For a sixel image of height $h$, let $a=(h-1)\bmod6$ and $b=(h-1)\bmod20$, then, the text will overlap the image when $a>b$.

@hackerb9 If you want to see some serious overlap, try this:

echo -e '\eP1q#"60;1#1!360~\e\\'

Utkarsh-khambra · 2022-09-22T07:52:34Z

Thanks everybody for helping me in this. I got it to output similar image for this test as well, only discrepancy I see is with horizontal alignment but I am pretty sure this is because contour has 9px wide cells for my testing config.

…xel data

christianparpart

Thanks @Utkarsh-khambra for your work, and thanks @jerch, @hackerb9 and @j4james for your very valuable feedback and the joined discussion over here. :-)

hackerb9 · 2022-09-23T06:45:41Z

Thanks everybody for helping me in this. I got it to output similar image for this test as well, only discrepancy I see is with horizontal alignment but I am pretty sure this is because contour has 9px wide cells for my testing config.

Wow! I did not expect anybody would replicate that behaviour and I am impressed. I do think it is a firmware bug that a text newline is not always sufficient to separate sixels from text. I suggest new terminals, like Contour, may want to make precise bug-for-bug VT340 compatibility available but disabled by default.

By the way, now that you've duplicated the overlap glitch, how is it affected when the size of the font goes up? If I'm correct, anything taller than 10×20 will reduce the percentage of "bad" image sizes that cause overlap.

Utkarsh-khambra · 2022-09-23T13:48:50Z

may want to make precise bug-for-bug VT340 compatibility available but disabled by default.

That's a good suggestion.
Here are some tests that I did

Cell Height	Overlap
15	No
17	No
18	No
23	No
25	No
27	Yes
28	Yes
30	No
31	No
33	No
35	No
36	No
38	No
40	Yes
41	Yes
43	No
44	No
46	No
48	No
49	No
51	No
53	No
54	No
56	No
57	No
58	No
61	No
62	No
64	No

hackerb9 · 2022-09-23T17:57:00Z

may want to make precise bug-for-bug VT340 compatibility available but disabled by default.

That's a good suggestion. Here are some tests that I did [...]

That looks close, but not what I would have predicted. You're only testing image heights 84 and 96, right?

If we can extrapolate from the VT340 glitch, I would expect the overlap bug to occur when

$$ (h_i-1)\bmod6 > (h_i-1)\bmod h_c $$

where $h_i$ is the height of the image and $h_c$ is the height of the character cell. On a VT340, $h_c$ is fixed at $20$.

With that formula, a character cell of height 23 would have overlap and 28 would not. It's possible my formula is wrong.

But, I wouldn't spend too much time trying to extend this bug to modern terminals. It's not like there is any compatibility to worry about and it'll only cause application developers grief as they have to implement workarounds to not overlap images.

j4james · 2022-09-23T19:43:20Z

The VT340's behavior may not be to your liking, but it's not a bug or a glitch. It actually serves a very useful purpose when you're working with images that you want to align on text boundaries.

For example, if you want to output an image that is exactly one text cell in height, you need to output 4 sixel bands (24px height) with the last 4 pixels being transparent. If you end that with a GNL, the text cursor is exactly below the image.

Similarly if you want an image two text cells in height, it's 7 sixel bands (42px height) with the last 2 pixels transparent. And for an image three text cells in height, it's exactly 6 sixel bands (60px height).

In every case the text cursor ends up exactly below the visible portion of the image, overwriting only the transparent pixels. And if you leave off the final GNL, you can get the text cursor on the bottom row of the image instead.

Example test cases:

echo -en '\eP9;1q#1!800~-!800~-!800~-!800B-\e\\'
echo -en '\eP9;1q#1!800~-!800~-!800~-!800~-!800~-!800~-!800N-\e\\'
echo -en '\eP9;1q#1!800~-!800~-!800~-!800~-!800~-!800~-!800~-!800~-!800~-!800~-\e\\'

Please do not try to "fix" sixel. If you don't like it, and feel the need to change the standard, rather put that effort into a new graphics protocol instead.

hackerb9 · 2022-09-23T21:53:02Z

Please do not try to "fix" sixel. If you don't like it, and feel the need to change the standard, rather put that effort into a new graphics protocol instead.

I respectfully disagree with you, but I also see that one of us is misunderstanding the problem. It's probably me and I want to understand, so let's discus that on a new bug so as to not clutter up this one: hackerb9/vt340test#25.

Utkarsh-khambra requested a review from christianparpart as a code owner September 20, 2022 17:30

github-actions bot added test Unit tests VT: Backend Virtual Terminal Backend (libterminal API) labels Sep 20, 2022

Utkarsh-khambra force-pushed the fix/822 branch 3 times, most recently from 2d533c9 to 93c54cc Compare September 20, 2022 17:49

Utkarsh-khambra force-pushed the fix/822 branch from 93c54cc to 380d4fc Compare September 20, 2022 17:55

github-actions bot added the frontend Contour Terminal Emulator (GUI frontend) label Sep 20, 2022

christianparpart reviewed Sep 20, 2022

View reviewed changes

src/terminal/SixelParser.cpp Outdated Show resolved Hide resolved

src/terminal/SixelParser_test.cpp Outdated Show resolved Hide resolved

Utkarsh-khambra force-pushed the fix/822 branch 4 times, most recently from 44c1fb4 to 753566d Compare September 20, 2022 19:01

Utkarsh-khambra commented Sep 20, 2022

View reviewed changes

Utkarsh-khambra force-pushed the fix/822 branch from 753566d to 7cf5865 Compare September 22, 2022 07:43

Utkarsh-khambra added 2 commits September 22, 2022 13:33

Now moves cursor vertically for sixel newlines even if there is no si…

4c43b7f

…xel data

Removes images.sixel_cursor_conformance config option.

d1ef430

Utkarsh-khambra force-pushed the fix/822 branch 2 times, most recently from 08a78db to 2b0c711 Compare September 22, 2022 08:05

Fixed sixel image rendering with different aspect ratios

3dddd08

Utkarsh-khambra force-pushed the fix/822 branch from 2b0c711 to 3dddd08 Compare September 22, 2022 08:11

christianparpart approved these changes Sep 22, 2022

View reviewed changes

christianparpart merged commit aa3b8e0 into master Sep 22, 2022

christianparpart deleted the fix/822 branch September 22, 2022 08:37

hackerb9 mentioned this pull request Sep 23, 2022

VT340's text cursor placement: glitch or not? hackerb9/vt340test#25

Closed

dnkl mentioned this pull request Feb 29, 2024

sixels: last line cut/truncated on terminal emulators with "correct" text cursor placement hpjansson/chafa#192

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Now moves cursor vertically for sixel newlines even if there is no sixel #825

Now moves cursor vertically for sixel newlines even if there is no sixel #825

Utkarsh-khambra commented Sep 20, 2022

christianparpart commented Sep 20, 2022

Utkarsh-khambra Sep 20, 2022

christianparpart Sep 20, 2022

Utkarsh-khambra Sep 21, 2022

jerch Sep 21, 2022 •

edited

Loading

Utkarsh-khambra Sep 21, 2022

jerch Sep 21, 2022

hackerb9 Sep 22, 2022 •

edited

Loading

j4james Sep 22, 2022

Utkarsh-khambra commented Sep 22, 2022 •

edited

Loading

christianparpart left a comment

hackerb9 commented Sep 23, 2022 •

edited

Loading

Utkarsh-khambra commented Sep 23, 2022

hackerb9 commented Sep 23, 2022

j4james commented Sep 23, 2022

hackerb9 commented Sep 23, 2022

		moveCursorTo(_topLeft.line + unbox<int>(linesToBeRendered) - 1, _topLeft.column);
		moveCursorTo(_topLeft.line + unbox<int>(linesToBeRendered), _topLeft.column);

Now moves cursor vertically for sixel newlines even if there is no sixel #825

Now moves cursor vertically for sixel newlines even if there is no sixel #825

Conversation

Utkarsh-khambra commented Sep 20, 2022

christianparpart commented Sep 20, 2022

Utkarsh-khambra Sep 20, 2022

Choose a reason for hiding this comment

christianparpart Sep 20, 2022

Choose a reason for hiding this comment

Utkarsh-khambra Sep 21, 2022

Choose a reason for hiding this comment

jerch Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Utkarsh-khambra Sep 21, 2022

Choose a reason for hiding this comment

jerch Sep 21, 2022

Choose a reason for hiding this comment

hackerb9 Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

j4james Sep 22, 2022

Choose a reason for hiding this comment

Utkarsh-khambra commented Sep 22, 2022 • edited Loading

christianparpart left a comment

Choose a reason for hiding this comment

hackerb9 commented Sep 23, 2022 • edited Loading

Utkarsh-khambra commented Sep 23, 2022

hackerb9 commented Sep 23, 2022

j4james commented Sep 23, 2022

hackerb9 commented Sep 23, 2022

jerch Sep 21, 2022 •

edited

Loading

hackerb9 Sep 22, 2022 •

edited

Loading

Utkarsh-khambra commented Sep 22, 2022 •

edited

Loading

hackerb9 commented Sep 23, 2022 •

edited

Loading