-
Notifications
You must be signed in to change notification settings - Fork 6
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
images aren't serialized #47
Comments
Unfortunately thats not possible atm and also not easy fixable, as there are several conceptual issues to overcome:
To quick draft a possible solution:
I am not very fond of the current state of the serialize addon, not sure if such an extension is wanted (argued in the past about it, but kinda gave up). Also the proper image encoding protocol remains unclear atm. |
@PerBothner Drafted a quick demo PR to test the idea above (see #48). Ofc this cannot be used until the serialize addon sees major changes. Also the image re-encoding is very rough atm (using IIP for every single cell, which is not how it should be done). |
There is prolly a simpler solution possible - the image addon currently always draws on top of FG/BG content not erasing it. Thus it should be possible to keep most of the serialize addon as it is, but invoke a full line image serializer for every line, and just append its sequence to the current line. This has several advantages:
Will see if that works better... Update: I've implemented the line based serialization with a23a372, which works much better and is also reasonably fast. |
Addendum regarding a proper protocol: It turns out that IIP has all the basics to guarantee a stable cell coverage, if dimensions are set with plain numbers denoting row/col extend, eg. Nonetheless IIP with col/row metrics works good enough atm to get a working solution for serialization running. 😸 Edit: |
The QOI image format might be a good choice for image serialization, as it provides very fast encoding/decoding with okish compression rate. Some early numbers testing QOI decoding speed with the sixel-bench test data:
(Note: The JPEG numbers are flawed, as I did a simple 75% conversion, which reduces data from ~130MB to 17MB for jpeg.) Did not test yet the encoding path, but I expect this to be much faster than any PNG/JPEG encoding. |
A simple way to serialize a canvas is to use |
@PerBothner Well, thats what my playground branch does atm: xterm-addon-image/src/ImageAddon.ts Lines 322 to 329 in a23a372
and the encoding speed is underwhelming - it takes ~70ms for an image of 420x420 pixels. To me this seems not to be viable option to serialize a longer scrollbuffer with possibly multiple images in it, as it will add up to seconds of blocking time pretty fast. Thats where the promise of QOI may take place - it encodes slightly worse than PNG, but at 30-50x faster speed, which would reduce the runtime of 70ms easily to <<10ms. NB: The high runtime is partially artificial from line-wise handling, a single Update: A quick test with a hacky not yet optimized QOI encoder finishes the same image in ~10 ms, thus runs in a third of the time compared to
Also tried to avoid getImageData and read pixel values back from webgl texture, which was slightly better (~4x faster):
Still way to go... |
Haha - stumbled over the next hassle - fast bytearray to string conversion. I really dont get it why JS has no fast const data = ... // uint8 array from encoder
let p = 0;
let s = '';
while (p < data.length) {
s += String.fromCharCode.apply(null, data.subarray(p, p += 65536) as any);
} Also Some interim results:
|
@PerBothner Imho QOI is the right decision, as it is able to serialize things 2.5 - 5x faster than with browser methods (see full roundtrip numbers and explanation here: #48 (comment)). |
@PerBothner |
Does it really matter if serialization is a bit slow? How often do you need to do it? Can you think of any usecase for a terminal where even half a second would be a problem? Of course an extensible interface is valuable even if performance isn't the main motivator. Where I think speed might be more valuable is detecting grapheme clusters, which can be combined with detecting wide characters - see my library. Though detecting clusters while rendering seems sub-optimal - perhaps cluster (and wide character) detection should be done lazily, the first time a line is rendered. Anyway, this is a whole different set of issues, which I have some thought and experience in. |
Well the issue with the serialize addon is simple - it is convoluted for no good reason, so I dont like to alter things there and introduce an interface on top. I would basically end up rewriting it anyway, so a clean start makes things alot easier. The perf bonus is just the brick to get me doing it. @ grapheme clusters |
For grapheme clusters, why do you need to store codepoint attributes, assuming you're using either dom or canvas renderers? The browser takes care of that. You may need to know cluster boundaries but you can detect those at the same time you're detecting double-width characters, using an efficient trie lookup (which would probably be a good candidate for wasm). "If those ever will make it back into xterm.js" If I get to the point of switching DomTerm to use the xterm.js engine by default, I'm assuming it will have to be a fork, since the xterm.js maintainers don't seem to be interested in rich data or variable-width text. |
Yes, I meant that lookup data for the unicode codepoint range to eval incoming chars. I dont quite remember, what I used back then, but it was quite big (I think it was a condensed bitmap for fast lookup). So no, I was not storing additional attributes in the buffer, it was just the segmentation logic, that needed additional lookup data. Btw I think other TEs implementing grapheme support put the segmentation burden early right after the utf-8 decoding before doing Anyway, its kinda offtopic here. |
Off-topic - about grapheme clusters: The table for codepoint attributes used in DomTerm is a Uint32Array of length 12764, initialized from a 4032-character atob string. I consider that acceptable. Basically DomTerm treats a cluster as wide if any of the constituents is wide, or it sees an emoji_presentation_selector or a regional_indicator pair.
That is why terminals need to figure out how to handle variable-width fonts. On my roadmap is a wrapper for input editors (such as GNU readline) where the terminal uses a variable-width font (layout handled by the browser engine), but it pretends to the application that it's all regular fixed-width columns. This would be enabled by some extra shell integration flags without applications having to be otherwise modified. Applications that know they are running under DomTerm can request that a variable-width font be used for output. |
I got xterm.js + xterm-add-image working under DomTerm. However, it doesn't appear a sixel-produced image is saved by the serialize addon.
Specifically, I printed some images (using lsix), and then did a "detach session". When I then did "attach" on the session, the images were gone.
Serialization is important not only for detach+attach, but also when dragging a window to a new top-level window - this also uses serialization on most platforms.
In "regular" DomTerm (not based on xterm.js), an image is represented as a
<img>
element with adata:
URL. When a buffer is serialized as HTML, this automatically handles such images. (FWIW, I'm actively working on improving the sixel handling in regular DomTerm.)The text was updated successfully, but these errors were encountered: