Skip to content
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

image support #2503

Closed
wants to merge 162 commits into from
Closed

image support #2503

wants to merge 162 commits into from

Conversation

jerch
Copy link
Member

@jerch jerch commented Oct 24, 2019

Made a new apporach towards image support in xterm.js, mainly based on the previous addon. Currently only a playground, will see where we can get with this. Also not high on the priority list, the discussion in the terminal-wg is still ongoing.

What works so far:

  • SIXEL support with some limitations
    • no custom pixel size (always mapping 1:1)
    • fake shared color palette missing later color changes recoloring older pixels - this is prolly not worth to be implemented in the real shared palette sense, as it was a hardware color addressing issue of old devices and is cumbersome to implement as an artificial restriction.
    • no image blending on transparent pixels yet

Planned:

  • custom image handling based on terminal-wg discussion

Since the addon has an outer dependency, do a cd addons/xterm-addon-image && yarn before calling yarn in the repo folder. The addon is active in the demo and can be tested by feeding it some sixels. Fixed due to changes in build scripts.

Some preview:
grafik

Update:
Since the discussion in terminal-wg is stalled, we should not wait for someone to come up with a "one-fits-all" image solution for terminals. Instead I think we should try to get most SIXEL primitives working, and maybe later on parts of the iTerm2 image protocol.

@jerch jerch added type/proposal A proposal that needs some discussion before proceeding work-in-progress Do not merge labels Oct 24, 2019
@jerch
Copy link
Member Author

jerch commented Oct 24, 2019

@Tyriar Not sure how to deal with outer dependencies in addons - I kept the dep in the addon local package.json. Imho it belongs there, but our build scripts do not respect this yet. For CI I fixed it temporarily by adding another script to the pipelines, locally a cd addons/xterm-addon-image && yarn is needed beforehand or yarn will fail.

package.json Outdated Show resolved Hide resolved
azure-pipelines.yml Outdated Show resolved Hide resolved
addons/xterm-addon-image/src/ImageStorage.ts Outdated Show resolved Hide resolved
@PerBothner
Copy link
Contributor

To repeat myself: One you have image support, it is conceptually easy to generalize (at least at the user level) to embedded HTML as an opaque blob, at least at the full-width <div> level. This is useful for help and other documentation.

Of course there are security implications (beyond those of images), so it probably should be an optional addon, with an HTML sanitizer (feel free to base one on DomTerm's).

@jerch
Copy link
Member Author

jerch commented Feb 18, 2020

@PerBothner Well the div-overlay idea would work, but breaks with lots of grid mechanics. We most likely will have such an extension one day (as already indicated for "marker overlays"), but I am not sure if images should use that. The idea for images is still to get it working for any terminal, not only HTML based ones that can easily layer stuff on top.
Also real vector support would be nice (analogous to ReGIS), maybe a stripped down SVG variant would do (SVG-terminal or so). It needs a spec w'o any outer ref stuff to not pull in foreign content on terminal side by accident (which again would be a security nightmare). Well its just a rough idea currently.

@Tyriar Tyriar marked this pull request as draft April 10, 2020 20:53
@jerch jerch mentioned this pull request May 6, 2020
@ghost
Copy link

ghost commented Jun 14, 2020

@jerch

Just wanted to pass along: the freedesktop thread is basically done with everyone exhausted and throwing up their hands. But sixel around the world is doing pretty good right now.

  • DomTerm's sixel is aaaalmost there, and impressively fast.

  • mintty's sixel and iTerm2 are usable now. I reported some artifacts with it that I think are now fixed and will be in the next release.

  • vte's sixel is in active testing, and appears to be solid.

  • alacritty's sixel is starting implementation, and there may be a prototype for testing any day now.

  • I have put some captures at https://jexer.sourceforge.io/sixel.html for testing.

@jerch
Copy link
Member Author

jerch commented Jun 14, 2020

@klamonte Thx for the update 👍

Yeah its somewhat a shame we came to no usable conclusion in the thread. I am not blaming anyone in particular, I for myself was not able to come up with a good solution, so yeah its also partly my own fault.

Ofc I am not done with the topic, still it got somewhat out of my focus. We have a few things to get straight in the corelibs before we can integrate the prototype here further.

DomTerm's sixel is aaaalmost there, and impressively fast.

Glad you like its speediness 😉. JS isnt that bad after all, but gets very C-ish for max perf.

@PerBothner
Copy link
Contributor

I rebult this version on an older laptop running Fedora 33. However, I realize that may not help - it's not old enough ... Even my ancient Fedora 30 laptop was updated recently enough to be running libc 2.29 ...

Maybe it's time you updated your OS?

@jerch
Copy link
Member Author

jerch commented Nov 2, 2021

@PerBothner Yep, that version works for me and gives (with Chrome):

16.5s (7.19 MB/s, 41.52 fps)

Edit:
Btw that -B switch is pure magic, no clue how you do that, very impressive.

@jerch
Copy link
Member Author

jerch commented Nov 2, 2021

@hackerb9 Sweet that worked, giving me:

  • DOM renderer: ~130 fps on intel, ~140 fps on nvidia
  • canvas renderer: ~120 fps on intel, ~155 fps on nvidia
  • webgl renderer: ~105 fps on intel, ~180 fps on nvidia

Btw with full roundtrip measuring for every single frame xtermjs will fall short alot, because we have a much longer "line" than most native TEs, that more or less sit directly on the PTY. Thinking about it, it is somewhat crazy:
Input chain (already shortened):
pty -> nodejs buffer -> websocket -> browser -> xtermjs buffer -> utf8 decoder -> parser -> wasm worker -> JS buffer -> canvas
Output chain:
data event -> browser -> websocket -> nodejs buffer -> pty sink

Edit: Updated numbers with nvidia card.

@PerBothner
Copy link
Contributor

Thanks for trying out DomTerm.

I tried running sixvid. Nyatocat got upto about 200 fps. spring.mp4 didn't print a number (even after -b -e) but seemed a rather slower than it should be.

It does look like increasing buffer size (at least the buffer in the server for reading pty output) might be a good idea. However, I do get occasional exceptions: Failed to construct 'ImageData': The input data length is not equal to (4 * width * height). I don't know (yet) what causes this - data loss, corruption, a logic error, or something else.

@jerch
Copy link
Member Author

jerch commented Nov 2, 2021

However, I do get occasional exceptions: ...

With the new or the old lib version?

(Btw I am running these tests with intel graphics, can give it another go with the nvidia card as well, might help with webgl variant 😸)

@PerBothner
Copy link
Contributor

I get exceptions with the new version.

This is the logic - maybe you can see something wrong:

        let six = window.SixelDecoder;
        six.init();
        six.decode(bytes, 1);
        let w = six.width, h = six.height;
        let next = ...;
        const reuseCanvas = next instanceof Element
              && next.tagName == "CANVAS"
              && next.width >= w && next.height >= h;
        const canvas = reuseCanvas ? next
              : document.createElement("canvas");
        if (! reuseCanvas) {
            canvas.setAttribute("width", w);
            canvas.setAttribute("height", h);
        }
        const ctx = canvas.getContext('2d');
        const idata = new ImageData(new Uint8ClampedArray(six.data32.buffer), w, h);
        ctx.putImageData(idata, 0, 0);

@jerch
Copy link
Member Author

jerch commented Nov 2, 2021

@PerBothner Yepp the intermediate Uint8ClampedArray must be clamped to 4 * width * height. Maybe try this:

new ImageData(new Uint8ClampedArray(buffer, 0, width * height * 4), width, height)

(Reason: The lib returns a borrowed Uint32Array, which might have a bigger buffer underneath from a previous run, then it will not fit into the ImageData without the slice on the .buffer.)
Really should expose a data8 property for direct usage on ImageData (created an issue for that).

@PerBothner
Copy link
Contributor

@jerch Your proposed seems to fix the problem.

"Btw that -B switch is pure magic, no clue how you do that, very impressive."

The domterm command starts (if need be) a server that (among other things it does) is an http+WebSockets server. When a new terminal is requested, the server forks a shell in the normal TE way. It creates a custom URL that points back at itself, and includes in the hash-value a unique number for the shell process. It also forks a front-end/browser, as specified by the -B option, passing it that unique URL on the browser command-line. The browser gets a skeletal HTML file that just loads a bunch of JavaScript from the server, initializes the session, and opens up a WebSockets connection with the server, passing back the shell process number, so the server can associate the connection with the correct process. Process input/output is sent over this WebSockets connection, as are internal server/browser notifications and requests.

There is of course more to it (domterm includes more-or-less the functionality of tmux), but that's the basic idea.

If you're interested in hooking up xterm.js with the domterm server, that is quite reasonable. In fact there is code in the domterm sources to support just that; however I haven't been maintaining it. It should even be possible for the same server to manage at the same time both DomTerm sessions (for rich text, html, images etc) and xterm.js sessions (for higher speed and perhaps some other features), though I haven't tried that.

@jerch
Copy link
Member Author

jerch commented Nov 3, 2021

@PerBothner Ah ic, so DomTerm basically provides its own terminal server infrastructure, that holds the pty connections and uses installed browsers as frontend directly spawned from there. That a very sensible approach and gives you control over things xtermjs kinda lacks as pure frontend component. We get tons of issues regarding proper page embedding, and many integrations get the server part wrong missing properly delegated flow control, encoding handling or securing the websocket transmission (including our demo, but thats only meant to be used for development of xtermjs itself, ppl should not simply c&p that to their production envs).

@PerBothner
Copy link
Contributor

@jerch The current domterm backend is a much-rewritten fork of ttyd, which uses xterm.js. However. with domterm has a lot more bells and whistles.

@hackerb9
Copy link

hackerb9 commented Nov 5, 2021

Out of Memory on unprocessed data? This should not happen with Sixvid. It waits until the terminal has responded to an inquiry, meaning xterm.js has finished processing the sixel data, before sixvid sends the next frame. [....] there is no way sixvid should ever trigger your OOM parachute.

Mea culpa! I just noticed that the main branch of sixvid did not actually behave the way I described. It indeed was simply splatting frames as fast as possible until you ran out of memory. I've merged the changes to fix that.

If you would, please try again with the current version of sixvid. It should not trigger the OOM parachute. I'm curious what the FPS results will be now that they will be accurate.

@jerch
Copy link
Member Author

jerch commented Nov 6, 2021

@hackerb9 Eww, now we talking about "a long line" kicking in - gives me 51 fps on DOM canvas renderer with intel embedded graphics.
(and yes - it does not run into the OOM safety guard anymore)

@jerch jerch requested a review from Tyriar November 7, 2021 14:48
@jerch jerch marked this pull request as draft November 11, 2021 22:48
@jerch
Copy link
Member Author

jerch commented Nov 11, 2021

Converting back to draft to resolve these issues before going live:

  • remove private / shared palette separation, instead always carry entries forward
  • think about a less invasive palette reset than RIS/DECSTR (maybe hook into some XTSMGRAPHICS action)
  • properly define and document default palette as VT340 (lower 16 colors) + ANSI256 (up to 256) + zeroed (up to 4096)
  • remove other default palettes to shrink package size
  • transit to VT340 cursor positioning, remove all others for sixels
  • update tests & docs regarding changes
  • check if worker loop is properly guarded against malicious sixel data

@jerch
Copy link
Member Author

jerch commented Dec 8, 2021

Closing this PR and #3488, as it is unlikely to be added in near future.

State: Beside the low level sixel issues above (see last comment) it is a working condition. It can easily be extended by other image protocols using the same internal image storage handling once there is a good candidate. Also see #3488 for remaining issues for a neater overall integration with xterm.js.

In theory this PR can be used from another repo to build the addon independently. But be aware, that it heavily relies on internal data structures (mainly for perf and non-duplication reasons), thus might create quite a high maintenance overhead to keep it on par with new xterm.js versions, which somewhat qualifies it to keep in main repo. At least it can be used as a source of ideas for similar approaches/extensions of xtermjs.

@jerch jerch closed this Dec 8, 2021
@hackerb9
Copy link

hackerb9 commented Dec 8, 2021

Oh, that's too bad. I was looking forward to using sixel graphics in xtermjs. Will this affect that fork (I forget the name, but it used a terminal server so one could connect from any web browser) that used this sixel implementation?

@jerch
Copy link
Member Author

jerch commented Dec 9, 2021

@hackerb9 Gimme a few days and I will see if an external repo is a viable workaround (depends on how much can be automatized on new releases of the main repo, and whether needed shims grow too big).

Edit: If you mean "DomTerm" with "I forget the name, but it used a terminal server so one could connect from any web browser" - that will not be affected by this, it uses the underlying sixel lib, which is independent, but not this PR code for its final terminal integration.

@hackerb9
Copy link

Yeah, DomTerm was what I was thinking of. Is there a github page for the sixel lib it uses?

@jerch
Copy link
Member Author

jerch commented Dec 11, 2021

@hackerb9 Yes you can find the sixel lib here https://github.com/jerch/node-sixel (disclaimer - I am the author, and it is still WIP. The decoder has some test coverage, and I'd consider the tested features as beta quality. It still lacks true paletted mode as in terminal vs. printer mode, which I am unsure to add at all. The encoder is mostly untested and very slow, more in early alpha state.)

@PerBothner
Copy link
Contributor

The DomTerm master (as of November 3) uses the new node-sixel 0.15 implementation. The minimized source is in hlib/sixel-decode.js.

(The DomTerm-2.9.4 release uses the old sixel implementation. I hope to make a DomTerm-2.9.5 after some more testing and polishing.)

@jerch
Copy link
Member Author

jerch commented Dec 30, 2021

@hackerb9 Here the experimental addon repo: https://github.com/jerch/xterm-addon-image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal A proposal that needs some discussion before proceeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet