image addon: overwriting with text does not remove tiles anymore

[jerch]

Since the kitty graphics the image addon misbehaves - text is meant to overwrite existing an image tile for a cell like this:

Text must be able to remove image content for IIP and Sixel.

@anthonykim1 Any idea, why it doesn't work anymore? Is this layer-related? Currently the image is always on top, text does not remove the image information. For the image above - the '###' get written but not shown on the screen.

[jerch]

@anthonykim1

Found the critical change in 21f5156

In addons/addon-image/src/ImageStorage.ts:L439:

            while (
              ++col < cols
              && (line.getBg(col) & BgFlags.HAS_EXTENDED)
              && (e = line._extendedAttrs[col] ?? EMPTY_ATTRS)
              && (e.imageId === imageId)
              && (e.tileId === startTile + count)
            ) {

You removed the conditions to check for the correct tile and put something else there. Was that change done manually or with an AI?

It is important to note, that Sixel and IIP don't live on top or bottom, they live on the same layer as text, thus text has to remove image tiles.

Edit: Nope, that is not yet the critical change.

[jerch]

The critical change indeed in 21f5156, but it a few lines higher:

This is the old for loop:

xterm.js/addons/addon-image/src/ImageStorage.ts

Lines 382 to 420 in f447274

	for (let col = 0; col < cols; ++col) {
	if (line.getBg(col) & BgFlags.HAS_EXTENDED) {
	let e: IExtendedAttrsImage = line._extendedAttrs[col] || EMPTY_ATTRS;
	const imageId = e.imageId;
	if (imageId === undefined || imageId === -1) {
	continue;
	}
	const imgSpec = this._images.get(imageId);
	if (e.tileId !== -1) {
	const startTile = e.tileId;
	const startCol = col;
	let count = 1;
	/**
	* merge tiles to the right into a single draw call, if:
	* - not at end of line
	* - cell has same image id
	* - cell has consecutive tile id
	*/
	while (
	++col < cols
	&& (line.getBg(col) & BgFlags.HAS_EXTENDED)
	&& (e = line._extendedAttrs[col] || EMPTY_ATTRS)
	&& (e.imageId === imageId)
	&& (e.tileId === startTile + count)
	) {
	count++;
	}
	col--;
	if (imgSpec) {
	if (imgSpec.actual) {
	this._renderer.draw(imgSpec, startTile, startCol, row, count);
	}
	} else if (this._opts.showPlaceholder) {
	this._renderer.drawPlaceholder(startCol, row, count);
	}
	this._fullyCleared = false;
	}
	}
	}

At the top of the loop the code funnels down to cells of interest (containing image data):

• check against BgFlags.HAS_EXTENDED --> most cells don't have those --> skip
• check for imageId --> only cells with image tiles have those --> skip most with other eAttrs
• now we have a cell with legit image tile data, search for continuation to the right

Your changed code does not funnel against BgFlags.HAS_EXTENDED anymore:

xterm.js/addons/addon-image/src/ImageStorage.ts

Lines 426 to 440 in ad4a6e8

	for (let col = 0; col < cols; ++col) {
	let e: IExtendedAttrsImage;
	if (line.getBg(col) & BgFlags.HAS_EXTENDED) {
	e = line._extendedAttrs[col] ?? EMPTY_ATTRS;
	} else {
	const maybeImg = line._extendedAttrs[col] as IExtendedAttrsImage | undefined;
	if (!maybeImg || maybeImg.imageId === undefined || maybeImg.imageId === -1) {
	continue;
	}
	e = maybeImg;
	}
	const imageId = e.imageId;
	if (imageId === undefined || imageId === -1) {
	continue;
	}

as it adds a weird else clause and pull the narrowing check out of the BgFlags.HAS_EXTENDED check.

This is wrong for a simple reason - BgFlags.HAS_EXTENDED is authoritative for EAttribs access. Without it you might read stale EAttribs from the buffer from previous cell writes. This is an implementation detail of EAttribs (get lazy evicted from the buffer).

TL;DR - Access EAttribs only if BgFlags.HAS_EXTENDED is set. Always, even for other EAttribs values.

[jerch]

Tested several TEs to confirm the behavior with these commands:

// sixel
img2sixel addons/addon-image/fixture/palette.png && echo -e '\x1b[4;5H#######\x1b[9;1H--------'
// iip
imgcat -l addons/addon-image/fixture/palette.png && echo -e '\x1b[4;5H#######\x1b[9;1H--------'
// kitty
kitty icat addons/addon-image/fixture/palette.png && echo -e '\x1b[4;5H#######\x1b[9;1H--------'

xterm - sixel

mlterm - sixel

wezterm - sixel

wezterm - iip

wezterm - kitty

contour - sixel

kitty - kitty

Observation:
So far all behave the same way for different protocols - text erases graphics. Except kitty itself. The question then would be if we just do it like wezterm and treat it like the other protocol regarding cell eviction or take extra measures to implement the kitty way (but then we have to solve it by proper storing the tiles, we still cannot rely on outdated cell data). To me the kitty way looks rather broken, as it hides new text content.

Note that this is only a limited observation, sadly I cannot iTerm2 or windows terminals.

[jerch]

And in comparison our old vs. new behavior one (all done with sixel):

[PerBothner]

Wouldn't the previous "correct" logic fail if the you overwrite part of an image with text that needs extended attributes (such as funky underlines)? It doesn't seem like you can reliably use HAS_EXTENDED to check if tile data has been overwritten by text.

An idea: extend the fg-color property. Specifically allow non-zero "color values" when the color mode is CM_DEFAULT. The existing code seems to ignore the color value in this case, so it would be possible use a special non-zero color value to indicate an image. When text is printed, it will overwrite this special fg color value automatically - no special handling needed.

"Special color value" codes could potentially have other uses.

[anthonykim1]

Thanks and apologies for late response - there's been alot going on this past few weeks 😅

Any idea, why it doesn't work anymore? Is this layer-related?

Yes @jerch , I think I was to get top-layer kitty placement survive text writes.. and screwed up some things.
Was using Opus to drive through the protocol. You have more expertise on image related things so I'd appreciate your eyes on this.

The question then would be if we just do it like wezterm and treat it like the other protocol regarding cell eviction or take extra measures to implement the kitty way (but then we have to solve it by proper storing the tiles, we still cannot rely on outdated cell data). To me the kitty way looks rather broken, as it hides new text content.

I agree we should do the WezTerm-style behavior for now -> The Kitty-style “image survives over text” behavior would need proper separate placement/tile tracking; we shouldn’t get it by reading stale cell data.

So short term maybe we could restore the HAS_EXTENDED guard and treat Kitty like the other protocols for cell eviction. We can track proper Kitty persistent placements separately if we want to support that later.

An idea: extend the fg-color property

@PerBothner The fg-color sentinel idea is interesting, but that feels like a bigger buffer encoding change? For this regression I think restoring the old invariant is the safer route, and we can revisit a different marker if we need something more robust.

[jerch]

Wouldn't the previous "correct" logic fail if the you overwrite part of an image with text that needs extended attributes (such as funky underlines)? It doesn't seem like you can reliably use HAS_EXTENDED to check if tile data has been overwritten by text.

This works for a different reason - EAttribs are only ref'ed during print action as inputhandler carries this ref around. On an underline change that ref gets copycloned and the new style applied. The EAttrib ref of inputhandler does never hold image data, so the copyclone also does not --> image data vanishes from text writes. It is the reason, why addImage has to copyclone existing EAttrib always for every cell with image data. So in the end the mixture of single cell EAttrib ref vs. shared/borrowed ref in inputhandler makes the overwrites possible. It is quite hacky, but was the only way I found without rewriting the whole attribute logic.

This handling allows for one composition: text + partially transparent image would keep the text visible in the transparent areas. That is the closest I got it to original sixel handling.
At some point we introduced transparent BG. In sixel logic the opposite should be possible too: image data + text with transparent BG. But that composition mode is not possible with the logic above, as text erases image data. In sixel terms this is wrong. There should only be one canvas where everything gets painted to in a sticky stacking way including image data. This would be the correct way to handle true "image is on text layer" behavior, but would need major rewrites on all renderers.

To really solve this we would have to solve the composition issues:

• image on text should work
• text on image should work
• image on image should work
• text on text is not needed

The composition modes create a stacking problem - if they should work as listed, it would suggest that image + image + text or image + text + image should also work. This leads to a stack of drawing directives for a single cell from different data sources, which is a nightmare to deal with and not supported by any of the renderers.

Btw the GIP (good image protocol) tried to address these output composition problems but never took place. If I remember right the conclusion there was to let text always erase image data minus SP (Was it SP? No clue, some sentinel char without repr would retain image data). Mainly to reduce the stacking issue to a manageable output model.

TL;DR: I agree with @anthonykim1 - let's revert this change for now. Then we prolly should look into image + image composition (this is a must for sixel, not sure about the other protocols) and see how to address the missing bits on the data storing side.

[j4james]

Just on the subject of "text on image" (without erasing the image), my plan for this was to use DEC's overstrike mode (?20). If you model the terminal as a single text layer on top of which there is a single image layer (with transparency), then overstrike mode could just render any text directly onto the image layer (without filling the background). This also gives you support for "text on text" for free - not very efficiently, but you may not care if that's not your primary use case.

Kitty of course introduces a whole other level of complexity, because it requires that you have multiple image layers, which can be moved independently of each other and the underlying text. But if you ignore the "v1" kitty protocol, and just support the Unicode placement mode, I think it could possibly be implemented in the same way as the other image protocols (i.e. with just a single image layer). I'm not sure how much of the Kitty protocol you intend to support though.