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