Add support for Sixel images in conhost by j4james · Pull Request #17421 · microsoft/terminal

9 min read Original article ↗

Conversation

j4james

lhecker

Comment on lines 226 to 231

@erwin erwin mentioned this pull request

Jul 2, 2024

github-merge-queue bot pushed a commit that referenced this pull request

Jul 2, 2024
This PR add supports for two query sequences that are used to determine
the pixel size of a character cell:

* `CSI 16 t` reports the pixel size of a character cell directly.
* `CSI 14 t` reports the pixel size of the text area, and when divided
  by the character size of the text area, you can get the character cell
  size indirectly (this method predates the introduction of `CSI 16 t`).

These queries are used by Sixel applications that want to fit an image
within specific text boundaries, so need to know how many cells would be
covered by a particular pixel size, or vice versa. Our implementation of
Sixel uses a virtual cell size that is always 10x20 (in order to emulate
the VT340 more accurately), so these queries shouldn't really be needed,
but some applications will fail to work without them.

## References and Relevant Issues

Sixel support was added to conhost in PR #17421.

## Validation Steps Performed

I've added some unit tests to verify that these queries are producing
the expected responses, and I've manually tested on [XtermDOOM] (which
uses `CSI 16 t`), and the [Notcurses] library (which uses `CSI 14 t`).

[XtermDOOM]: https://gitlab.com/AutumnMeowMeow/xtermdoom
[Notcurses]: https://github.com/dankamongmen/notcurses

## PR Checklist
- [x] Tests added/passed

DHowett pushed a commit that referenced this pull request

Aug 15, 2024
When we have a series of image slices of differing widths, which also
don't align with the cell boundaries, we can get rounding errors in the
scaling which makes the different slices appear misaligned.

This PR fixes the issue by removing the 4 pixel width alignment that was
enforced in the `ImageSlice` class, since that's not actually necessary
when the pixels themselves are already 4 bytes in size. And without
that, the widths should be correctly aligned with the cell boundaries.

## References and Relevant Issues

The initial Sixel implementation was added in PR #17421.

## Validation Steps Performed

I've confirmed that this fixes the rendering glitches reported in
#17711, and all my existing Sixel tests still work as expected.

Closes #17711

DHowett pushed a commit that referenced this pull request

Feb 3, 2025
When Sixel images are rendered, they're automatically scaled to match
the 10x20 cell size of the original hardware terminals. If this requires
the image to be scaled down, the default GDI stretching mode can produce
ugly visual artefacts, particularly for color images. This PR changes
the stretching mode to `COLORONCOLOR`, which looks considerably better,
but without impacting performance.

The initial Sixel implementation was added in PR #17421.

## Validation Steps Performed

I've tested with a number of different images using a small font size to
trigger the downscaling, and I think the results are generally better,
although simple black on white images are still better with the default
mode (i.e. `BLACKONWHITE`), which is understandable.

I've also checked the performance with a variation of the [sixel-bench]
test, and confirmed that the new mode is no worse than the default.

[sixel-bench]: https://github.com/jerch/sixel-bench

DHowett pushed a commit that referenced this pull request

Apr 29, 2025
When Sixel images are rendered, they're automatically scaled to match
the 10x20 cell size of the original hardware terminals. If this requires
the image to be scaled down, the default GDI stretching mode can produce
ugly visual artefacts, particularly for color images. This PR changes
the stretching mode to `COLORONCOLOR`, which looks considerably better,
but without impacting performance.

The initial Sixel implementation was added in PR #17421.

## Validation Steps Performed

I've tested with a number of different images using a small font size to
trigger the downscaling, and I think the results are generally better,
although simple black on white images are still better with the default
mode (i.e. `BLACKONWHITE`), which is understandable.

I've also checked the performance with a variation of the [sixel-bench]
test, and confirmed that the new mode is no worse than the default.

[sixel-bench]: https://github.com/jerch/sixel-bench

(cherry picked from commit b243fb6)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgZ0dHI
Service-Version: 1.22

DHowett pushed a commit that referenced this pull request

Apr 29, 2025
When Sixel images are rendered, they're automatically scaled to match
the 10x20 cell size of the original hardware terminals. If this requires
the image to be scaled down, the default GDI stretching mode can produce
ugly visual artefacts, particularly for color images. This PR changes
the stretching mode to `COLORONCOLOR`, which looks considerably better,
but without impacting performance.

The initial Sixel implementation was added in PR #17421.

## Validation Steps Performed

I've tested with a number of different images using a small font size to
trigger the downscaling, and I think the results are generally better,
although simple black on white images are still better with the default
mode (i.e. `BLACKONWHITE`), which is understandable.

I've also checked the performance with a variation of the [sixel-bench]
test, and confirmed that the new mode is no worse than the default.

[sixel-bench]: https://github.com/jerch/sixel-bench

(cherry picked from commit b243fb6)
Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgZ0dHA
Service-Version: 1.23

carlos-zamora pushed a commit that referenced this pull request

Apr 29, 2025
This PR fixes two cases where image content wasn't correctly erased when
overwritten.

1. When legacy console APIs fill an area of the buffer using a starting
coordinate and a length, the affected area could potentially wrap over
multiple rows, but we were only erasing the overwritten image content on
the first affected row.

2. When copying an area of the buffer with text content over another
area that contained image content, the image in the target area would
sometimes not be erased, because we ignored the `_eraseCells` return
value which indicated that the image slice needed to be removed.

## References and Relevant Issues

The original code was from the Sixel implementation in PR #17421.

## Validation Steps Performed

I've manually verified that these two cases are now working as expected.

## PR Checklist
- [x] Closes #18568

DHowett pushed a commit that referenced this pull request

May 5, 2025
This PR fixes two cases where image content wasn't correctly erased when
overwritten.

1. When legacy console APIs fill an area of the buffer using a starting
coordinate and a length, the affected area could potentially wrap over
multiple rows, but we were only erasing the overwritten image content on
the first affected row.

2. When copying an area of the buffer with text content over another
area that contained image content, the image in the target area would
sometimes not be erased, because we ignored the `_eraseCells` return
value which indicated that the image slice needed to be removed.

## References and Relevant Issues

The original code was from the Sixel implementation in PR #17421.

## Validation Steps Performed

I've manually verified that these two cases are now working as expected.

## PR Checklist
- [x] Closes #18568

(cherry picked from commit 08e76da)
Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgZ3XT4
Service-Version: 1.23

DHowett pushed a commit that referenced this pull request

May 5, 2025
This PR fixes two cases where image content wasn't correctly erased when
overwritten.

1. When legacy console APIs fill an area of the buffer using a starting
coordinate and a length, the affected area could potentially wrap over
multiple rows, but we were only erasing the overwritten image content on
the first affected row.

2. When copying an area of the buffer with text content over another
area that contained image content, the image in the target area would
sometimes not be erased, because we ignored the `_eraseCells` return
value which indicated that the image slice needed to be removed.

## References and Relevant Issues

The original code was from the Sixel implementation in PR #17421.

## Validation Steps Performed

I've manually verified that these two cases are now working as expected.

## PR Checklist
- [x] Closes #18568

(cherry picked from commit 08e76da)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgZ3XUA
Service-Version: 1.22

DHowett pushed a commit that referenced this pull request

Jun 11, 2025
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.

The initial Sixel parser implementation was in PR #17421.

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).

Closes #17887
Closes #17946