Change image storage to use new ExtendedAttrs payload field

[PerBothner]

Adds a general-purpose payload hook to ExtendedAttrs. This allows us to get rid of the ExtendedAttrsImage logic, which can easily conflict with other extensions.

The PR avoids looking into BufferLine internals by instead using loadCell and setCell. The calls to loadCell may add slightly more overhead; if that is measurable, we can look into reducing the number of loadCell calls.

This is an implementation of this suggestion.

It might make sense for a future update to remove the urlId field from ExtendedAttrs and instead use the general payload hook.

[PerBothner]

An optional optimization is to replace imageId (which needs to be looked up in the _images map) with a direct reference to IImageSpec:

class ImageTileInfo {
  constructor(
    public imageSpec: IImageSpec | undefined = undefined,
    public tileId = -1) {
  }
}

A further possibility would be to remove the _images map. Since each IImageSpec has an associated Marker, when we need to iterate over all images, we can do that by iterating over markers. This assumes we add a payload field to Marker (as suggested in PR #5853), and that the payload field is used to point back to the IImageSpec.

[PerBothner]

On further thought, I think it might be worthwhile to consider merging the Marker and ExtendedAttrs concepts. As you suggest, we could add a general API to add "custom" data (specified by a key and an arbitrary value) to a cell. Internally it probably make sense to attach the custom data to a range of cells in a line, rather than just a single cell. And the API should make it convenient to apply custom data to a whole range using a single method call.

I believe the changes to Marker in PR #5853 provide a good foundation for this model. That change attaches the Markers directly to a LogicalLine making it efficient to attach and query for custom data for a line. Reflow is handled with no extra work. PR #5853 adds a startColumn to a Marker; we would want to be able to also specify an end column. The PR allows a general payload property on a Marker; we might add a key property. The implementation represents the markers associated with a line as a simple linked list. That representation works well if there are at most a small number of Marker ranges associated with a line, which I think would almost always be the case.

With this change, I think we can gradually get rid of ExtendedAttrs. The HAS_EXTENDED flag would be set if there is marker whose range includes the current cell. We would add marker keys for "image", "link-url", "fancy-underlining", "input-prompt", "input-line", and so on, as needed. As an transitional step, we can keep store the ExtendedAttrs type as a payload (using "extended-attrs" as a key).

However, I do think it makes sense to merge in PR #5853 (which includes the changes in this PR) before going further with the "custom data" route.

[PerBothner]

I merged from upstream and all tests pass. Can we merge this? It is blocking BufferLine/LogicalLine changes. @Tyriar requested one small change (which I believe I've made), and potentially a more general API (which is a bigger project that should be considered in conjunction with other Marker/Decorator changes).

[PerBothner]

Could this be merged? It is a prerequisite for PR #5797.

[PerBothner]

My thinking is it would make sense to review and hopefully merge this PR before trying to review the bigger and more complicated PR #5797. One reason is that PR #5797 CI runs fail until this is merged in.

I don't say this PR is my ideal way of attaching images to lines: We're previously discussed ideas about how to combine the concepts of markers, decorators, and extended-attributes into some more general mechanism. However, I think this PR is a good interim step to reduce some technical debt.

[jerch]

@PerBothner Had no chance yet to look at this, sorry.

In general it would be good, if this rewrite passes:

• all image tests
• it should handle image addon: overwriting with text does not remove tiles anymore #5860 correctly. Note that there is a chance for failing several kitty graphics tests, once that issue is correctly handled again. We have to address that separately after fixing what kitty has damaged.
• for perf testing you can use https://github.com/jerch/sixel-bench:
  • run the python script for different image formats for baseline throughput
  • then compare to this PR
  • if there is a huge difference - go into devtools profiling to spot the slowpokes
  • and so on

For sixel-bench to work correctly in the xterm.js demo you have to increase the WriteBuffer cache to 500 MB (this is due to missing flow control in the demo, just append a zero for the tests)

xterm.js/src/common/input/WriteBuffer.ts

Lines 12 to 20 in 29a7384

	/**
	* Safety watermark to avoid memory exhaustion and browser engine crash on fast data input.
	* Enable flow control to avoid this limit and make sure that your backend correctly
	* propagates this to the underlying pty. (see docs for further instructions)
	* Since this limit is meant as a safety parachute to prevent browser crashs,
	* it is set to a very high number. Typically xterm.js gets unresponsive with
	* a 100 times lower number (>500 kB).
	*/
	DISCARD_WATERMARK = 50000000, // ~50 MB

[PerBothner]

All the image examples in the demo work.

The tests here seem to work.

I run sixel-bench under time. Not a hude difference. Given the variability of runs, difficult to tell if there any real difference.

[jerch]

@PerBothner How are the throughput numbers of sixel-bench compared to master? The script should yield a MB/s number. It is a bit annoying to perf test things currently with devtools due to #6106, I hope to get most of the issues there fixed soon. 🫰

[PerBothner]

How are the throughput numbers of sixel-bench compared to master?

Master, 4 runs each test:

python run.py data/data.sixel
1.99s (59.72 MB/s, 344.93 fps)
1.96s (60.42 MB/s, 348.96 fps)
2.11s (56.23 MB/s, 324.76 fps)
2.2s (54.03 MB/s, 312.07 fps)
python run.py data/data.iip.qoi
2.51s (52.11 MB/s, 272.96 fps)
1.81s (72.24 MB/s, 378.39 fps)
2.07s (63.22 MB/s, 331.14 fps)
1.82s (71.75 MB/s, 375.83 fps)

For with this PR:

python run.py data/data.sixel
2.46s (48.13 MB/s, 277.99 fps)
2.1s (56.43 MB/s, 325.92 fps)
2.39s (49.64 MB/s, 286.7 fps)
1.97s (60.28 MB/s, 348.17 fps)
python run.py data/data.iip.qoi
2.14s (61.02 MB/s, 319.62 fps)
1.99s (65.78 MB/s, 344.58 fps)
2.17s (60.39 MB/s, 316.33 fps)
2.04s (64.14 MB/s, 335.98 fps)

Varibility between runs appears bigger than between forks.

[jerch]

@PerBothner Yeah the varibility is high for those type of tests, due to the long line from app --> pty --> demo-server --> websocket --> browser --> xterm.js --> addon. You get halfway stable numbers only, if your computers is otherwise mostly at rest.

I'd say there is a small decline, but not much. Gonna do my own perf tests before I check the code paths myself. By the way - bufferline.loadCell is quite expensive in master due to extended attr object creation, I also addressed this in #6114.

[PerBothner]

It is plausible the slowdown is caused by using loadCell to access the extended-attributes. I'm going to adding a new getExtended method to BufferLine.

[PerBothner]

sixel-bench results with latest check-in:

master
1.26s (94.39 MB/s, 545.18 fps)
1.32s (89.82 MB/s, 518.78 fps)
1.55s (76.5 MB/s, 441.82 fps)
branch
1.59s (74.74 MB/s, 431.66 fps)
1.24s (95.32 MB/s, 550.53 fps)
1.36s (87.08 MB/s, 502.93 fps)

I think that is basically the same.

[PerBothner]

I believe this is ready to be merged.

[jerch]

@PerBothner I think the higher runtime results from loadCell and the fact, that we now have to do two allocations (workCell.extended.clone() + new ImageTileInfo(imageId, tileId)), while before it was only new ExtendedAttrsImage(0, 0, imageId, tileId) in the fall-through (which is the most likely one to be used).

I think the ExtendedAttrs needs a full revamp with proper registering/unregistering capabilities. While your .payload currently solves that for images, it is still a single extension only.

What we in fact need is the following:

static ExtendedAttrs.register(propery_name: string, default_value: any, isSticky: boolean);

• property_name sets a slot on extended attrs with a default value
• isSticky determines, whether the property is single cell or should be carried forward in input handler (like all SGR extension and urlId are carried forward)
• The property handling reduces to .set(property_name, value) and .get(property_name). How the additional properties are stored internally should not matter from outside.
• The currently hardcoded value need to be transformed into this new interface as well.

Note static pattern above is not right yet, as it would apply the slot registration to all terminals in a context. What I mean by it is - we need a terminal-wide class factory, that respects registered slots or disregards them on a terminal instance in a slick way without additional creation overhead.

Example:

term.buffer.registerExtendedAttribute('urlid', 0, true);
// --> registers a slot urlId on internal data:
{
  urlid: 0,
}
term.buffer.registerExtendedAttribute('imageid', 0, false);
// internal data:
{
  urlid: 0,
  imageid: 0,
}

Now the difference of isSticky causes inputhandler to treat isEmpty and clone different:

public isEmpty(): boolean {
  for (const slot of slots) {
    if (slot.isSticky && this._data[slot] !== default) return false;
  }
  return true;
}
static clone(): ExtendedAttrs {
  const obj = new ExtendedAttrs(/* no ctor params anymore*/);
  for (const slot of slots) {
    obj._data[slot] = slot.isSticky ? this._data[slot] : slot.default;
  }
  ...
  return obj;
}

(Maybe clone needs a better name here, as it is in fact not a clone method anymore but a cloneForNextCells.)

Edit:
I know that the semantics of extended attrs are a bit hard to grasp with that reference-carry-forward handling done by the inputhandler, but in the end it is what saves us tons of additional buffer space.

[jerch]

@PerBothner I am not opposed to merge this as interim fix for the direct accesses. But I still think it does not solve the real extensibility issue of extended attributes as I stubbed above.

--> #6142

[jerch]

@PerBothner Plz have a look at my comment below. Looks like a state change bug to me, but I cannot repro it currently on the buffer.

[jerch]

I think this is wrong, as it does not mutate the extended reference? Note that the original version creates a new ExtendedAttrsImage object here.

The issue with this code - loadCell pulls extended from a blueprint for empty extended values, whose ref never changes. Thus the ImageTileInfo will get written to that?

In general - the dance of loadCell - setCell is not meant be used outside of bufferline, as the reference carry-forward semantic is not encapsulated by those methods.

(Maybe I am overlooking something, because I dont see the sideeffects of it currently...)

[PerBothner]

It seems reasonable to just just delete those two lines, and instead fall through. Do you agree?

[jerch]

yes.

And found the repro in between:

echo -ne '\x1b[4:2mxxxxxxxxx\x1b[D' && imgcat -l addons/addon-image/fixture/palette.png

Issue: the "xxx..." gets overwritten with first image tile due to ref sharing.

[PerBothner]

Change committed and pushed.

[jerch]

Yupp - with this everything is fine 👍

[PerBothner]

I think the ExtendedAttrs needs a full revamp with proper registering/unregistering capabilities. While your .payload currently solves that for images, it is still a single extension only.

It is a general extension hook - but as currently designed/implemented a given cell can only support a single payload. Adding support for a list of payloads would be a relatively minor enhancement. However, I have suggested earlier attaching Markers to LogicalLine, adding start/end-ranges to Markers, and then maybe merging the ExtendedAttr support into Marker. But first we should merge in the LogicalLine change before we can attached Markers to LogicalLine ranges.

I am not opposed to merge this as interim fix for the direct accesses.

That would be appreciated. I agree it is not the perfect/final design, but it gets us in the right direction. Though a simple refinement is a bit more future-proof. Perhaps something like this:

const e = line.getPayload(col, ImageTileInfo.constructor) as ImageTileInfo | undefined;
if (e) { ... }

[jerch]

Markers are a minefield perfwise. But lets move that discussion over to #6142 and get done with this here as interim fix.

(Btw this PR runs slightly faster in my tests with sixel-bench doing less work in _writeToCell)