terminal
terminal copied to clipboard
microsoft
Improve correctness of Sixel background fill
Summary of the Pull Request
This is an attempt to improve the correctness of the background fill that the Sixel parser performs when an image is scrolled because it doesn't fit on the screen.
This new behavior may not be exactly correct, but does at least match the VT340 implementation more closely than it did before.
References and Relevant Issues
The initial Sixel parser implementation was in PR #17421.
Detailed Description of the Pull Request / Additional comments
When a Sixel image has the background select parameter set to 0 or 2, it fills the background up to the active raster attribute dimensions prior to writing out any actual pixel data. But this fill operation is clamped at the boundaries of the screen, so if the image doesn't fit, and needs to scroll, we have to perform an additional fill at that point to cover the background of the newly revealed rows (this is something we weren't doing before).
This later fill uses the width of the most recent raster attributes command, which is not necessarily the same as the initial background fill, and the height that it covers doesn't seem to take the raster attributes into account at all. I'm not sure yet how the height is supposed to be determined, but for now we're filling enough of the screen to cover the sixel rows that have been output.
Validation Steps Performed
We've run a couple of tests on a real VT340, and I've confirmed that the new implementation matches the behavior that we were seeing there.
I've also confirmed that the test case in issue #17946 now matches what the OP was expecting, and the test case in #17887 at least works more consistently now when scrolling (this is not what the OP was expecting though).
PR Checklist
- [x] Closes #17887
- [x] Closes #17946
Since I'm not certain this is actually correct, I don't know if it's a good idea to merge at this point in time, but you're welcome to do so if you think it worthwhile.
[...] I don't know if it's a good idea to merge at this point in time, but you're welcome to do so if you think it worthwhile.
We discussed this today: Given that >1 person stumbled over this and that this brings us closer to the VT340 behavior, we felt like this is worth merging. 🙂
I'm converting this back to a draft because al20878 has done some additional testing for me on his VT330, and it seems that this is still not correct. I'm not yet sure what the real solution is, but I'm working on it.
While I would personally be happy for this to be merged, I should warn you that in some cases this change might be seen as a regression. Because if you've got a Sixel image that isn't an exact multiple of the cell size, and the background select parameter is configured to fill, it can end up filling beyond the bottom of the image when it scrolls.
In the screenshot below, I'm using img2sixel to convert an image that occupies four and half rows. In the first case there was no scrolling, so no extraneous background fill, but in the second case you can see it ends up filling an additional half row below the image.
Much of the time you may not notice, because the default fill color is black, and a lot of people will be using a black or dark background. However the fill color can be unpredictable - it depends on the image, and potentially even previously viewed images, so sooner or later you're liable to notice.
Technically I'd consider this a bug in the image conversion tool, and I always encourage apps to set the background select to transparent to avoid this situation, but there will inevitably be some tools that don't do that (e.g. img2sixel is no longer maintained, so it's likely stuck with this behavior forever).
this change might be seen as a regression.
I am willing to take that risk, as I have many times before in the name of correctness. :)
Thank you so much for doing this.
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
you may need a merge from main to get within throwing distance of "can compile"
but it will still fail inexplicably in the x64 build. results from ARM64 will suffice
In the screenshot below, I'm using img2sixel to convert an image that occupies four and half rows. In the first case there was no scrolling, so no extraneous background fill, but in the second case you can see it ends up filling an additional half row below the image.
I'm wondering how this will affect sixel "gif" rendering and TUI's incorporating sixels
changing the output depending on if it needs to scroll or not does not seem correct or am i misunderstanding?
even with transparent background changing the size would cause issues i think atleast..
i know i've found the artifacts of an older version of this pr before but if it's failed now.. i need to setup a local build env to test a bit.
changing the output depending on if it needs to scroll or not does not seem correct or am i misunderstanding?
@trackd I think you may be misunderstanding. The size of the image is completely independent from the background fill. You can think of the fill as a kind of clear screen operation that is performed before the image is drawn. If you try to fill an area larger than the screen size, it's not going to force the terminal to scroll, and it has no effect on the final cursor position.
So you shouldn't see any change in the layout of a page with this PR applied. All you might notice is some pixels being a different color, and that's only when you've got the P2 parameter set to 0 or 2, and the actual image content has forced the terminal to scroll.
That said, it's possible I've made a mistake somewhere, so if you've got a test case that you think is rendering incorrectly, please do share it.
Sorry, there was an audit issue that I missed (method needed to be noexcept). I think it should be fixed now.
FYI, I ran into this after updating to the recent preview release -- gnuplot sixelgd output now has an unwanted black bar at the bottom but didn't in the previous release. I was about to open an issue assuming this was a regression, but I understand now that it's WAI.
@taviso I think all that is needed for gnuplot to fix this is a small tweak to the following line in sixel.c:
https://github.com/gnuplot/gnuplot/blob/fbeb88eadedf927a4d778b41dd118e373f33eacb/term/sixel.c#L619
It should look like this (inserting 9;1 in the middle of \033Pq):
fprintf(out_fp, "\033P9;1q\"1;1;%d;%d\n", map_width, map_height);
@taviso FYI, I've just raised an issue on the gnuplot bug tracker with the suggested patch. I reported it as a transparency bug, because this also fixes the sixelgd transparent option, and that affects a lot more terminals.
Thanks James! And confirmed here as well, gnuplot -e 'set terminal sixelgd scroll' -e 'plot sin(x)' -e 'print ""' | sed 's/\x1BP/&9;1/' does fix the output, and transparent works too (looks better in xterm as well).