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

sixel unexpected background fill #17887

Open
JohnMcPMS opened this issue Sep 9, 2024 · 5 comments · May be fixed by #18260
Open

sixel unexpected background fill #17887

JohnMcPMS opened this issue Sep 9, 2024 · 5 comments · May be fixed by #18260
Assignees
Labels
Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.

Comments

@JohnMcPMS
Copy link
Member

Windows Terminal version

1.22.2362.0

Windows build number

10.0.26040.0

Other Software

My own sixel renderer that I just implemented. I don't doubt that it could be something I'm doing, but the symptom doesn't match my read on the sixel stream. This is a link to the function that does all of the sixel stream rendering:
https://github.com/JohnMcPMS/winget-cli/blob/f5f2a9497b561d2b724cbf754d89490b71a391c8/src/AppInstallerCLICore/Sixel.cpp#L156

Steps to reproduce

This file contains a sixel stream that produces the issue: sixel.txt

cat sixel.txt reproduces the issue.

Expected Behavior

Only pixels within the bounds of the sixel would change.

Actual Behavior

When I don't enable transparency in the sixel (send 0 for P2 in the control string), some arbitrary number of cell height rows are filled with an arbitrary color. The conditions only seem to change based on the dimensions of the sixel image, not the color palette that it contains. This feels very much like an issue with reading uninitialized or unowned memory.

image
Contains a selection behind the color band to indicate that it is on the sixel plane.

@JohnMcPMS JohnMcPMS added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 9, 2024
@j4james
Copy link
Collaborator

j4james commented Sep 10, 2024

The short answer is that the background select parameter is best set to 1 (i.e. using transparency), especially when sixel scrolling is enabled, because it's almost certainly not going to work the way you think. It's main purpose was to clear the screen before an image was output.

For the long answer, you need to know some history. Starting with the VT125, there was no background select option, and there was no sixel scrolling. The screen was always cleared when a sixel image was output, and it always started rendering in the top left corner (and was cut off at the bottom of the screen).

The VT240 introduced the background select option, so you now had a choice of whether or not the screen would be cleared before the image was output. But there was still no sixel scrolling, so again the image started at the top left of the screen, and was cut off at the bottom.

With the VT340 we got the ability to choose whether scrolling was enabled or not, and when it is enabled, the image begins rendering at the current cursor position. But that introduces a new twist for the background select: instead of clearing the whole screen, it now only clears from the cursor up to the bottom right edges of the screen.

The VT340 also introduced the raster attribute command (DECGRA), which lets you set the range of the background select, so instead of clearing all the way to the edges, you could specify a maximum width and height that it would fill. But it only really makes sense to use if you're working with an image that is expected to fit on screen.

To understand why, consider what happens if you start rendering the image on the bottom row. The first thing it's going to do is apply the background select, and at that point in time, the maximum height it can fill is that one row. As the image is output and overflows the bottom, the screen will scroll up revealing new blank rows, but those won't have the background select applied to them because it's too late at that point.

In your example above, this is why you're seeing that one row of yellow at the top of your image. If you cleared the screen before outputting the image, that yellow would fill the entire display.

As for the choice of color, that's entry 0 in the color table, which is black by default, but can be altered by the color definitions in your image. The way colors are assigned is they start at entry 1, working their way upwards through the color table, and only overwrite entry 0 when there are no other entries remaining.

So for example, on a VT240, the 4th color you defined would set the background fill color. On a VT340, it would be the 16th color. And on Windows Terminal, which has 256 colors by default, it'll be the 256th color that you define (in your sample file, that's #255;2;90;77;3).

There are a few weird edge cases that I've glossed over, but that's the general idea of how it works. And the bottom line is it's working as intended. If you don't want that yellow bar, set the background select parameter to 1, or include a DECGRA command which matches your image dimensions.

@JohnMcPMS
Copy link
Member Author

@j4james , thank you for the explanation. My reading on transparency made it seem that it would work differently, specifically controlling whether entry 0 would be rendered. From reading the Terminal code it was also obvious that it would fill the background, which I then assumed would be filled based on the sixel output rather than the entire (currently visible at sixel start) screen.

@j4james
Copy link
Collaborator

j4james commented Sep 22, 2024

@JohnMcPMS I just want to follow up here to let you know that my explanation above is wrong. We've just done a test on a VT340 and it turns out that it does actually fill the background when it scrolls. Although it's not immediately obvious how it works, because it appears to be ignoring the background height. So I'm not positive how that would effect your example above, but I'm guessing it might always fill with yellow to bottom of the screen.

@j4james j4james reopened this Sep 22, 2024
@carlos-zamora
Copy link
Member

carlos-zamora commented Sep 25, 2024

Hey @j4james, want us to assign this to you?

Note to self: we should add a new area for image protocols (ideas: Terminal Images, TImages)

@carlos-zamora carlos-zamora added Area-VT Virtual Terminal sequence support Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 25, 2024
@j4james j4james self-assigned this Sep 25, 2024
@j4james
Copy link
Collaborator

j4james commented Sep 25, 2024

@carlos-zamora I've assigned myself now. I've got a possible fix for this already, but I'm waiting for more test results to confirm some of the edge cases.

@j4james j4james linked a pull request Nov 28, 2024 that will close this issue
2 tasks
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants