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

sixel support #970

Merged
merged 5 commits into from Aug 29, 2022
Merged

sixel support #970

merged 5 commits into from Aug 29, 2022

Conversation

jerch
Copy link
Contributor

@jerch jerch commented Aug 25, 2022

WIP to get sixel support in ttyd.

@tsl0922 This already works as intended, but I only tested the yarn build + make paths (there is a high chance that the other dev setups in package.json do not work yet). For the bundling of the worker file I only found the awkward way to cat the contents with gulp into inline.html. Maybe you have a better idea to solve this. If you are not interested in sixel support, just drop me a note.

Edit/Sidenote: While messing around with sixels I figured that the zmodem impl is a real throughput killer, and I even ran into random flow control issues. No clue if thats zmodems fault as well, I cannot repro it reliable yet. (Just tested it to see fault tolerance with bigger sixels, as flow control issues are more likely with image data...)

@jerch
Copy link
Contributor Author

jerch commented Aug 29, 2022

@tsl0922 Imho this is ready for review.

about the flow control issues:
Those happen very rarely and only under very big data pressure (like pumping several GBs). There seems to an issue hidden in the code, maybe thats easier to track with a separate issue/PR (sixel support itself is not affected by this, it only surfaces if you try to stream video output from mpv's sixel output encoder).

Or lets discuss things here if you think thats a showstopper.

@jerch jerch marked this pull request as ready for review August 29, 2022 08:59
@tsl0922
Copy link
Owner

tsl0922 commented Aug 29, 2022

there is a high chance that the other dev setups in package.json do not work yet
It's not work with yarn run start.

The worker js string can be embeded with webpack, try the following patch:

--- a/html/src/components/terminal/index.tsx
+++ b/html/src/components/terminal/index.tsx
@@ -4,11 +4,14 @@ import { ITerminalOptions, RendererType, Terminal } from 'xterm';
 import { FitAddon } from 'xterm-addon-fit';
 import { WebglAddon } from 'xterm-addon-webgl';
 import { WebLinksAddon } from 'xterm-addon-web-links';
-
+import { ImageAddon } from 'xterm-addon-image';
 import { OverlayAddon } from './overlay';
 import { ZmodemAddon, FlowControl } from '../zmodem';
 
 import 'xterm/css/xterm.css';
+import worker from 'xterm-addon-image/lib/xterm-addon-image-worker';
+
+const imageWorkerUrl = window.URL.createObjectURL(new Blob([worker], { type: 'text/javascript' }));
 
 interface TtydTerminal extends Terminal {
     fit(): void;
@@ -176,6 +179,7 @@ export class Xterm extends Component<Props> {
         terminal.loadAddon(overlayAddon);
         terminal.loadAddon(new WebLinksAddon());
         terminal.loadAddon(this.zmodemAddon);
+        terminal.loadAddon(new ImageAddon(imageWorkerUrl));
 
         terminal.onTitleChange(data => {
             if (data && data !== '' && !this.titleFixed) {
--- a/html/webpack.config.js
+++ b/html/webpack.config.js
@@ -37,6 +37,15 @@ const baseConfig = {
                     'sass-loader',
                 ],
             },
+            {
+                test: /xterm-addon-image-worker/,
+                type: 'asset/inline',
+                generator: {
+                    dataUrl: content => {
+                        return content.toString();
+                    }
+                }
+            },
         ]
     },
     resolve: {

@jerch
Copy link
Contributor Author

jerch commented Aug 29, 2022

@tsl0922 Oh well, this looks much better and less error-prone 👍

@tsl0922 tsl0922 merged commit b8a88f6 into tsl0922:main Aug 29, 2022
@jerch
Copy link
Contributor Author

jerch commented Sep 10, 2022

@rihaq Sorry I have no binary to share, as I already deleted the checked out project. Either ask @tsl0922 to release it soon, or build the project yourself to get the binaries early.

@jerch
Copy link
Contributor Author

jerch commented Sep 10, 2022

@rihaq Yes the gh actions should produce the binaries containing it.

Sorry, there is nothing like a "sixel community", there are just a bunch peeps trying to get a hold of old DEC specs and working around issues, to get something usable in newer terminal emulators. If you are interested in sixel, these might be starting points to dig deeper: https://github.com/hackerb9/vt340test (also contains original DEC docs) or https://github.com/hackerb9/lsix.

@jerch
Copy link
Contributor Author

jerch commented Sep 10, 2022

Might be a browser rounding bug/artefact (cannot repro it here)? Whats your system, screen resolution and browser version?

If you find bugs, plz file an issue in the addon repo, not here in some already closed PR.

@jerch
Copy link
Contributor Author

jerch commented Sep 12, 2022

@rihaq Have created a patch release v0.1.3 which hopefully fixes the line striping (unclear since I cannot repro). Btw the striping is most likely from your browser having a pixelDeviceRatio with fractions (high res display, or you have some zoom level activated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants