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 band with solid background color get transparent in some cases #17946

Open
PhMajerus opened this issue Sep 21, 2024 · 7 comments · May be fixed by #18260
Open

Sixel band with solid background color get transparent in some cases #17946

PhMajerus opened this issue Sep 21, 2024 · 7 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.
Milestone

Comments

@PhMajerus
Copy link

PhMajerus commented Sep 21, 2024

Windows Terminal version

1.23.2611.0 (Canary)

Windows build number

10.0.26100.1742 ARM64

Steps to reproduce

This one is probably for @j4james

When displaying a sixel image with solid background (␛P1;1q"… or default ␛Pq"…), bands are properly created filled with the solid background color before pseudo-pixels get decoded into them, only if the band is already in the viewport.
When reaching the end of the viewport, new bands added that require scrolling or extending the buffer are created with a transparent background instead of the solid background:

image
(Solarized Light color scheme to make the background more visible, but happens with every color scheme)

My test file has magenta as an unused color # 0, so the background is using black regardless of the VT palette, and then transparency for the bands scrolling the viewport or extending the buffer.
Edit: Reading through #17887, I guess that's because I didn't set color # 256 and it defaulted to black

Here's the test file: test.zip

Is this the expected behavior to match old terminals and we need to explicitly draw all background pixels instead of relying on background fill, or is this a bug?

Expected Behavior

Background should be reliably solid background color.

Actual Behavior

In some cases, the background is transparent.

@PhMajerus PhMajerus 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 21, 2024
Copy link

We've found some similar issues:

If any of the above are duplicates, please consider closing this issue out and adding additional context in the original issue.

Note: You can give me feedback by 👍 or 👎 this comment.

@j4james
Copy link
Collaborator

j4james commented Sep 21, 2024

The bot is actually correct. This is the same issue as #17887. Quoting myself:

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.

It only really makes sense to use background select if you're working with images that are expected to fit on screen. I think it's mostly for backwards compatibility with terminals that didn't scroll.

@PhMajerus
Copy link
Author

PhMajerus commented Sep 21, 2024

Ok, that's a really unexpected behavior, but I trust you to make it behave as it should for backward compatibility.
Thanks for looking into it.

So I guess we really should avoid that background flag for any sixel image that gets printed in the flow of text, we should either go with transparent background, or set all pixels explicitly to ensure a consistent background?

It means libsixel seems to be doing the worst thing, as it leaves the default background fill mode, but doesn't explicitly set pseudo-pixels for transparent pixels from the source image.

@j4james
Copy link
Collaborator

j4james commented Sep 21, 2024

It makes more sense when you think of sixel as a series of drawing operations. When you give it a command like "1;1;80;60, you're asking it to fill an 80x60 area of the screen, but that's going to be clamped to the available screen size. When you later issue a graphic newline that triggers a scroll, that earlier fill operation has come and gone - it's not going to go back and try and reapply it.

That said, I've raised an issue on the vt340test repo to double check that my understanding is correct.

So I guess we really should avoid that background flag for any sixel image that gets printed in the flow of text

Honestly I would avoid it in general. Because even if you're limiting yourself to images that don't scroll, there's still the issue of what color it's going to fill with, and that's not easy to set in an interoperable way. You're much better off just filling the background manually. It was a useful optimisation for hardware terminals that were likely running at something like 19200 baud, but there's really no need for it now.

It means libsixel seems to be doing the worst thing, as it leaves the default background fill mode, but doesn't explicitly set pseudo-pixels for transparent pixels from the source image.

Yeah - that's terrible - but I think that's clearly a bug. Regardless of how scrolling background fill is handled, you'd expect the transparent areas of an image to actually be transparent.

@j4james
Copy link
Collaborator

j4james commented Sep 22, 2024

@PhMajerus I think I might have some good news for you. @hackerb9 has just done some testing on his VT340 and it seems I was very wrong in my interpretation. It does actually fill the background when it scrolls. Although it's not immediately obvious to me how that works, because it appears to be ignoring the given background height. So it might work similar to how you expected it to, but it's also possible it might end up filling more of the screen than you want. We'll need to do some more testing to figure out the exact behavior.

@PhMajerus
Copy link
Author

PhMajerus commented Sep 23, 2024

Thanks for investigating this @j4james!

To me it makes sense, I'd expect the part before the sixels values to act like a header, defining the background fill mode, dimensions, and colors palette.
Just like the colors definitions don't get lost when extending the buffer while the sixel values are being decoded, the background fill mode and dimensions would also be kept in variables and used for the whole duration of the decoding process.

This also raises the next thing that should probably be tested :
How does it behave if you provide sixels values outside of the specified dimensions?
Do they overflow on the right and bottom into the screen buffer outside of the announced pseudo-pixels size, or do they get clipped to the next character boundary, or get clipped at the exact pseudo-pixels width and height specified in the header?

@j4james
Copy link
Collaborator

j4james commented Sep 23, 2024

I'd expect the part before the sixels values to act like a header

Despite what the documentation says, it doesn't actually work like that. The Raster Attributes command (the bit starting with ") doesn't necessarily appear at the start of the image, and there can be multiple occurrences throughout the sequence which change the aspect ratio and background dimensions.

Similarly with the color palette, those colors can be defined anywhere within the image, and they can also be redefined - that's how we support palette animation.

How does it behave if you provide sixels values outside of the specified dimensions?
Do they overflow on the right and bottom into the screen buffer outside of the announced pseudo-pixels size

Yeah, they overflow. That has been tested, and is actually well documented. Honestly most of this stuff has been well tested.

It's just this scrolling thing that has come as a surprise, because on a real VT340 the graphics background color is identical to the text background color, so you wouldn't even notice the effect of a background fill 99% of the time. That's why I assumed it wouldn't apply when scrolling. If a newly scrolled line is going to be filled with the text background color anyway, why would it need to apply an identical graphics background fill? It's only recently I realised you could get it to scroll with a different text background color by setting DECSCNM.

@j4james j4james self-assigned this Sep 25, 2024
@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 Oct 2, 2024
@carlos-zamora carlos-zamora added this to the Backlog milestone Oct 2, 2024
@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