microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.82k stars 8.33k forks source link

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

Open PhMajerus opened 2 months ago

PhMajerus commented 2 months ago

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.

similar-issues-ai[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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.