-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Support Kitty graphics protocol mvp #5619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Imho this should be fixed in node-pty to populate the pixel dimensions where possible (I think there was an issue with conpty not providing it, but for other platforms it can be populated) Another workaround for console apps is to use WinOps sequences like CSI 14t (see here for an example https://github.com/jerch/sixel-puzzle/blob/56129538af70fa4ec9441d1d3553398dc8e66f1f/termlib.py#L95) But of couse this is beyond xterm.js and has to be implemented by the console app itself. |
| const binaryString = atob(base64Data); | ||
| const bytes = new Uint8Array(binaryString.length); | ||
| for (let i = 0; i < binaryString.length; i++) { | ||
| bytes[i] = binaryString.charCodeAt(i); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has very bad runtime and is a nightmare creating memory pressure with huge images.
You might want to check the wasm based base64 decoder I used for the IIP protocol. It works on chunks, thus should be much friendlier to chunked data ingestion.
For basic usage see https://github.com/jerch/xterm-wasm-parts, for chunked usage addons/addon-image/src/IIPHandler.ts holds the needed bits.
| if (format === KittyFormat.PNG) { | ||
| // PNG: create blob and decode | ||
| const blob = new Blob([bytes], { type: 'image/png' }); | ||
| return createImageBitmap(blob); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know if this is still an issue - older Safari versions could not do this and needed a workaround with new Image() instead. (also see IIP handler)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to work on my work mac, safari: Version 26.2 (21623.1.14.11.9) . Will check again with my personal mac at home that should be more outdated!
|
@anthonykim1 I start to wonder, if the kitty graphics handler should be integrated into the image addon. I don't know the requirements of this protocol for the render layering, but maybe things could be reused and unified? It would make the lifecycling easier and would not introduce another image layer. But as I said - i dont know the kitty protocol requirements, so fusing it might be a futile attempt. Thoughts? |
| this._apcHandler = terminal.parser.registerApcHandler(0x47, (data: string) => { | ||
| return this._handleKittyGraphics(data); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since image data tend to be much bigger than 10 MB, the string based handler interface is not a good choice here (the string handler interface is limited to 10 MB of string data). Also the string forth and back conversion and aggregation exhibits really bad runtime here. Better - implement a full chunk based handler class and register an instance of it directly on the parser. This is much more efficient, as the utf32 codepoints emitted by the parser can be handled borrowed without copying them. (See the IIP and Sixel handlers for how to create a proper chunk handler class.)
| // Load test images as base64 | ||
| const BLACK_1X1_BASE64 = readFileSync('./addons/addon-kitty-graphics/fixture/black-1x1.png').toString('base64'); | ||
| const RGB_3X1_BASE64 = readFileSync('./addons/addon-kitty-graphics/fixture/rgb-3x1.png').toString('base64'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also want tests for f=24 and 32
jerch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments from my side...
| // TODO: This atob + charCodeAt loop has bad runtime and creates memory pressure with large | ||
| // images. Consider using the wasm-based base64 decoder from xterm-wasm-parts which also | ||
| // supports chunked data ingestion. See addon-image/src/IIPHandler.ts for chunked usage. | ||
| const binaryString = atob(base64Data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note on this - the wasm based base64 decoder works best, if the decoded payload size is known (it does a sanity check with this). Unfortunately the kitty protocol does not announce the expected byte size, which is bad as the payload can be any size. In JS we have only 2GB at hand, therefore I strongly suggest to cap the payload at a certain size (the image addon does this at 20MB for IIP data payload and 16M pixels by default, both is adjustable)
For the decoder it means, that you prolly should do the base64 decoding in 1 MB shards (1048576 --> 786432) and add those shards manually to the full byte data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anthonykim1 let's move it into addon-image for this, just make sure all the kitty stuff is clearly separated in a separate folder or something
| for (let i = 0; i < pixelCount; i++) { | ||
| data[dstOffset ] = bytes[srcOffset ]; // R | ||
| data[dstOffset + 1] = bytes[srcOffset + 1]; // G | ||
| data[dstOffset + 2] = bytes[srcOffset + 2]; // B | ||
| data[dstOffset + 3] = isRgba ? bytes[srcOffset + 3] : ALPHA_OPAQUE; | ||
| srcOffset += bytesPerPixel; | ||
| dstOffset += BYTES_PER_PIXEL_RGBA; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still subpar - for RGBA data you can simply use the bytes directly, for RGB interleaving with the 4th byte in uint32 blocks might be faster, schematically:
if (format === KittyFormat.RGBA) {
imgData = new ImageData(bytes, width, height);
} else {
const bytes32 = new Uint32Array(BYTES.buffer);
const data32 = new Uint32Array(DATA.buffer);
let dstOffset = 0;
let srcOffset = 0;
// assuming little endian, PIXELS must be 4-aligned
for (let i = 0; i < PIXELS; i += 4) {
const bloc1 = bytes32[srcOffset++];
const bloc2 = bytes32[srcOffset++];
const bloc3 = bytes32[srcOffset++];
data32[dstOffset++] = bloc1 | 0xFF000000;
data32[dstOffset++] = (bloc1 >> 24) | (bloc2 << 8) | 0xFF000000;
data32[dstOffset++] = (bloc2 >> 16) | (bloc3 << 16) | 0xFF000000;
data32[dstOffset++] = (bloc3 >> 8) | 0xFF000000;
}
// handle leftover here...
}This is 28 vs. 7 reads/writes per 4 pixels (memory access is a lot heavier than the bit manipulations).
On a sidenote - this byte interleaving is a typical task where wasm-simd would outperform by far (can do it with 3 instructions - load, swizzle, store).
Edit: Fixing the bit shifts. Timing is 60 ms vs. 10 ms for 4M pixels (speedup 5-6x, or 0.2 GB/s vs. 1.3 GB/s, wasm-simd would reach here >4GB/s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a gist of different versions: https://gist.github.com/jerch/e2f7695b887228d9e703c53af23c0737
Results:
$> node lib/test.wasm.js
original 50.4 ms, 250 MB/s
slightly optimized 23.9 ms, 526 MB/s
4 byte blocks 8.1 ms, 1553 MB/s
wasm simd 2.65 ms, 4748 MB/sThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is crazy.. 50 -> 2.6 🤯🤩🔥
Part of #5592
WIP
Plan:
(Jan 22, 4:00 PM PST) There's a problem currently where kitty kitten +icat wont return image but using send-image file will.
Copilot's research on why normal kitty kitten +icat doesnt work on xterm demo rn:
(Jan 22, 6:30 PM PST)
Things like:
kitty +kitten icat --use-window-size=80,24,1280,768 --transfer-mode=stream ~/Desktop/xterm.js/demo/logo.pngwork atmTODO since image goes on top of text rn 🥶:
TODOs: