-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Improve correctness of Sixel background fill #18260
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
base: main
Are you sure you want to change the base?
Conversation
774be8c
to
877bfe7
Compare
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. |
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). |
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 VT330 and VT340 hardware 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 fills the entire height of the new rows, regardless of the
height specified in the raster attributes command.
Validation Steps Performed
Thanks to @al20878 and @hackerb9, we've been able to test on both a
VT330 and a VT340, and I've confirmed that we're matching the behavior
of those terminals as closely as possible. There are some edge cases
where they don't agree with each other, so we can't always match both.
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