Add inline images for foot terminal

Conversation 20 Commits 8 Files Changed 7

cljoly commented

Foot supports printing images in the terminal. This however relies
on the image being converted to sixels, which are then sent to foot.

This is a proof of concept to convert images referenced in markdown
documents, by calling the external chafa command. Chafa does three
main things:

Download the image, if it is remote (for instance, an HTTPS URL).
Print the image with the maximum size possible, given the size of the
current terminal window. Ideally, we would display the original size
of the underlying image, but that seems difficult over the sixel
medium, because the tool creating the sixel (chafa in our case, but
see below for alternatives) does not know the size of one pixel in
the terminal emulator.
Support a wide variety of formats, even PDF.

With the Kitty protocol, we don’t have to worry about (1), (2) and (3),
because the terminal emulator is in charge of actually displaying the
image.

To move past the proof of concept phase, we could use a library to get
sixels from an image. chafa is also offered as a C library, but it’s
probably easier to stick to a Rust one, like viuer. That means
however that we have to deal with (1) and (2) ourselves. In particular,
missing remote images might be replaced by an empty image, which could
be stored as some constant sixel code in a variable, a bit like what is
currently done for wezterm.

Another option is to keep using the chafa command line. If it is
available in the path, then we expose the image
TerminalCapabilities. This could even improve support of other
terminal emulators, as chafa would detect the capabilities of the
emulator itself, falling back to crafty Unicode to approximate the image
on a terminal emulator that would support neither the kitty protocol nor
sixels.

What are your thoughts on these two options @flausch?

Fix #196

[Foot][1] supports printing images in the terminal. This however relies on the image being converted to sixels, which are then sent to foot. This is a proof of concept to convert images referenced in markdown documents, by calling the external [chafa][2] command. Chafa does three main things: 1. Download the image, if it is remote (for instance, an HTTPS URL). 2. Print the image with the maximum size possible, given the size of the current terminal window. Ideally, we would display the original size of the underlying image, but that seems difficult over the sixel medium, because the tool creating the sixel (chafa in our case, but see below for alternatives) does not know the size of one pixel in the terminal emulator. 3. Support a wide variety of formats, even PDF. With the Kitty protocol, we don’t have to worry about (1), (2) and (3), because the terminal emulator is in charge of actually displaying the image. To move past the proof of concept phase, we could use a library to get sixels from an image. chafa is also offered as a C library, but it’s probably easier to stick to a Rust one, like [viuer][3]. That means however that we have to deal with (1) and (2) ourselves. In particular, missing remote images might be replaced by an empty image, which could be stored as some constant sixel code in a variable, a bit like what is currently done for wezterm. Another option is to keep using the chafa command line. If it is available in the path, then we expose the `image` `TerminalCapabilities`. This could even improve support of other terminal emulators, as chafa would detect the capabilities of the emulator itself, falling back to crafty Unicode to approximate the image on a terminal emulator that would support neither the kitty protocol nor sixels. What are your thoughts on these two options @flausch? [1]: https://codeberg.org/dnkl/foot/ [2]: https://github.com/hpjansson/chafa/ [3]: https://crates.io/crates/viuer Fix #196

swsnr commented

Owner

I'm not sure; I think I'd like to pass this one.

I don't think I'd like to depend on an external tool or library here; we already support the iTerm and Kitty protocols, and the less dependencies the better.

If foot supports only sixel images and has no plans to support any of the other protocols, then at most I think I'd accept a library for RGB to sixel conversion. But printing and formatting for the terminal doesn't seem to be so hard as to warrant a library.

In particular, missing remote images might be replaced by an empty image, which could be stored as some constant sixel code in a variable, a bit like what is
currently done for wezterm.

I'm not sure what you're referring to here: mdcat doesn't display empty images if remote images fail; instead it just leaves the original image link in place.

I'm not sure; I think I'd like to pass this one. I don't think I'd like to depend on an external tool or library here; we already support the iTerm and Kitty protocols, and the less dependencies the better. If foot supports only sixel images and has no plans to support any of the other protocols, then at most I think I'd accept a library for RGB to sixel conversion. But printing and formatting for the terminal doesn't seem to be so hard as to warrant a library. > In particular, missing remote images might be replaced by an empty image, which could be stored as some constant sixel code in a variable, a bit like what is currently done for wezterm. I'm not sure what you're referring to here: mdcat doesn't display empty images if remote images fail; instead it just leaves the original image link in place.

cljoly commented

Poster

If foot supports only sixel images and has no plans to support any of the other protocols, then at most I think I'd accept a library for RGB to sixel conversion.

I believe there is no plan¹ for foot to support any other protocol. I'll look into using the sixel crate to limit external dependencies then.

I'm not sure what you're referring to here: mdcat doesn't display empty images if remote images fail; instead it just leaves the original image link in place.

Right, sorry.

I'm not in the foot team, it’s mostly based on this comment. ↩︎

> If foot supports only sixel images and has no plans to support any of the other protocols, then at most I think I'd accept a library for RGB to sixel conversion. I believe there is no plan[^1] for foot to support any other protocol. I'll look into using the [sixel](https://lib.rs/crates/sixel) crate to limit external dependencies then. [^1]: I'm not in the *foot* team, it’s mostly based on [this comment](https://codeberg.org/dnkl/foot/issues/481#issuecomment-196862). > I'm not sure what you're referring to here: mdcat doesn't display empty images if remote images fail; instead it just leaves the original image link in place. Right, sorry.

swsnr commented

Owner

As far as I understand this ocmment foot does not consider image support a priority, so I think mdcat won't consider image support in foot that important either.

I guess if you'd like good image support the idea is too use a different terminal emulator. Wezterm or kitty come to my mind.

So I think I'd only like to add sixel support if the code uses minimal dependencies and doesn't rely on external tools.

Displaying previews of PDF documents as images is very much out of scope for mdcat 🙂

As far as I understand this ocmment foot does not consider image support a priority, so I think mdcat won't consider image support in foot that important either. I guess if you'd like good image support the idea is too use a different terminal emulator. Wezterm or kitty come to my mind. So I think I'd only like to add sixel support if the code uses minimal dependencies and doesn't rely on external tools. Displaying previews of PDF documents as images is very much out of scope for mdcat 🙂

cljoly commented

Poster

As far as I understand this ocmment foot does not consider image support a priority, so I think mdcat won't consider image support in foot that important either.

Fair enough.

Displaying previews of PDF documents as images is very much out of scope for mdcat 🙂

Agreed, it was just an example of how far chafa went :)

So I think I'd only like to add sixel support if the code uses minimal dependencies and doesn't rely on external tools.

Understood, the sixel crate might be suitable then.

> As far as I understand this ocmment foot does not consider image support a priority, so I think mdcat won't consider image support in foot that important either. Fair enough. > Displaying previews of PDF documents as images is very much out of scope for mdcat 🙂 Agreed, it was just an example of how far chafa went :) > So I think I'd only like to add sixel support if the code uses minimal dependencies and doesn't rely on external tools. Understood, the [sixel crate][sixel] might be suitable then. [sixel]: https://crates.io/crates/sixel

cljoly force-pushed foot-support from 2b267da797 to f531dabb87

cljoly commented

Poster

(just rebased)

cljoly force-pushed foot-support from f531dabb87 to c05aefe52d

swsnr commented

Owner

Thanks. As far as I can see it still uses the external command, though.

Did you have a chance to look at this crate you mentioned?

By the way, it looks as if some code changed formatting; did you run rustfmt?

Also the copyright headers don't fit the license. You're free to submit your contribution under the Apache license of ocurse but please be aware that mdcat itself is distributed under the MPL license.

And finally please run scripts/check to see if your change passes all required tests and checks.

Thanks. As far as I can see it still uses the external command, though. Did you have a chance to look at this crate you mentioned? By the way, it looks as if some code changed formatting; did you run rustfmt? Also the copyright headers don't fit the license. You're free to submit your contribution under the Apache license of ocurse but please be aware that mdcat itself is distributed under the MPL license. And finally please run scripts/check to see if your change passes all required tests and checks.

cljoly commented

Poster

Did you have a chance to look at this crate you mentioned?

I’m looking into it, I have not finished writing the code yet. Not sure I’ll finish today, might have to wait till next weekend.
If that works better for you, I can @ you and request a review when it’s done and rustfmt'd?

Also the copyright headers don't fit the license. You're free to submit your contribution under the Apache license of ocurse but please be aware that mdcat itself is distributed under the MPL license.

Sorry I messed up the headers. I’ll fix this and use the same as what is used elsewhere in mdcat, much easier for everyone.

And finally please run scripts/check to see if your change passes all required tests and checks.

Got it.

> Did you have a chance to look at this crate you mentioned? I’m looking into it, I have not finished writing the code yet. Not sure I’ll finish today, might have to wait till next weekend. If that works better for you, I can @ you and request a review when it’s done and rustfmt'd? > Also the copyright headers don't fit the license. You're free to submit your contribution under the Apache license of ocurse but please be aware that mdcat itself is distributed under the MPL license. Sorry I messed up the headers. I’ll fix this and use the same as what is used elsewhere in mdcat, much easier for everyone. > And finally please run scripts/check to see if your change passes all required tests and checks. Got it.

swsnr commented

Owner

Yes, just ping me again when this is ready, that'd be nice 🙏

cljoly commented

Poster

TODO

Fix sixel rendering of the image
Fix error handling from sixel lib
Fix the files headers
Update Readme to add instructions to install the sixel library (or lazy load it?)
General self review

## TODO - [x] Fix sixel rendering of the image - [x] Fix error handling from sixel lib - [x] Fix the files headers - [x] Update Readme to add instructions to install the sixel library (or lazy load it?) - [ ] General self review

cljoly force-pushed foot-support from c05aefe52d to bfc4d2cec8

cljoly commented

Poster

(not in a fully working state yet, see the TODO comment)

(not in a fully working state yet, see the [TODO comment](https://codeberg.org/flausch/mdcat/pulls/204#issuecomment-583647))

cljoly commented

Poster

So it works ok on some images. For instance, the README:

but with other ones (like the sample.md file), not that well:

So it works ok on some images. For instance, the README: ![image](/attachments/5ceb0998-07f9-4beb-927e-7b12671dd8a0) but with other ones (like the sample.md file), not that well: ![image](/attachments/3a617a33-742c-4af4-a91e-2cea7a5d363e)

image.png

215 KiB

image.png

120 KiB

cljoly commented

Poster

I think libsixel does not work well with transparent PNG. Perhaps that’s something we can live with, as I have not found another easily accessible sixel lib in rust…

cljoly commented

Poster

Anyway, this is not ready for review yet.

cljoly force-pushed foot-support from bfc4d2cec8 to e417ecc56a

cljoly force-pushed foot-support from e417ecc56a to 57377df534

cljoly added 2 commits

7308410f53 fixup! Add basic foot support

af7d61186d fixup! Add inline images for foot terminal

cljoly added 1 commit

827bb74bb1 doc: mention the libsixel dependency in the README

This means that packages in distributions will have to be updated.

cljoly force-pushed foot-support from aa385d1bfe to 827bb74bb1

cljoly added 2 commits

ed323aca9e fixup! Add inline images for foot terminal

b95e5763ae chore: cargo fmt

cljoly commented

Poster

@flausch I have

Fixed the header to use the MPL license
Updated the README to mention that mdcat know depends on libsixel
Run rust fmt and scripts/check.

Unfortunately, the libsixel crate uses some very old crates with security issues.

@flausch I have 1. Fixed the header to use the MPL license 2. Updated the README to mention that mdcat know depends on libsixel 3. Run *rust fmt* and `scripts/check`. Unfortunately, the `libsixel` crate uses some very old crates with security issues.

cljoly commented

Poster

I’ll look further into updating the dependencies of the libsixel crate, but you are still happy with adding the libsixel (C library) system dependancy, right?

I’ll look further into updating the dependencies of the `libsixel` crate, but you are still happy with adding the `libsixel` (C library) system dependancy, right?

swsnr commented

Owner

you are still happy with adding the libsixel (C library) system dependancy, right?

I am not sure; I wasn't aware that you were adding a C dependency. You never mentioned that and I understood this

I'll look into using the sixel crate to limit external dependencies then.

as to say that sixel was a pure Rust implementation. Though you never mentioned that either, so I've really misunderstood something.

I'm sorry to say that I'm not really in favour of adding a C dependency to support an inferior image format for a niche terminal. mdcat already supports the two major image formats (namely iterm and kitty), and ideally foot would just support one of these.

If there's no other way I'm okay w/ sixel, but it needs to have up-to-date transitive dependency, and the code must be feature-guarded in mdcat, and the feature off by default, to ensure that mdcat builds clean even if the library is not present or no C compiler installed.

> you are still happy with adding the libsixel (C library) system dependancy, right? I am not sure; I wasn't aware that you were adding a C dependency. You never mentioned that and I understood this > I'll look into using the sixel crate to limit external dependencies then. as to say that sixel was a pure Rust implementation. Though you never mentioned that either, so I've really misunderstood something. I'm sorry to say that I'm not really in favour of adding a C dependency to support an inferior image format for a niche terminal. `mdcat` already supports the two major image formats (namely iterm and kitty), and ideally foot would just support one of these. If there's no other way I'm okay w/ sixel, but it needs to have up-to-date transitive dependency, and the code must be feature-guarded in mdcat, and the feature off by default, to ensure that mdcat builds clean even if the library is not present or no C compiler installed.

cljoly commented

Poster

I’m sorry I did not make it clear there would be this new C dependancy. I probably figured it out along the way, initially I thought it would be a nice pure Rust dependancy.

Reading this PR again, you also made it clear that you would prefer no external C dependancies early on. I might have lost sight of that when I worked on the sixel crate integration. So really, I should not have asked “but you are still happy with adding the libsixel (C library) system dependancy, right?”.

If there's no other way I'm okay w/ sixel, but it needs to have up-to-date transitive dependency, and the code must be feature-guarded in mdcat, and the feature off by default, to ensure that mdcat builds clean even if the library is not present or no C compiler installed.

That makes total sense!

Now, on the one hand, if we are feature-guarding the sixel support in mdcat, a user of a terminal that would support sixel would need a custom build of mdcat. That would also likely not be supported by the package manager of their GNU/Linux distribution.

On the other hand, with a clean integration of an external command like chafa (cleaner than my initial prototype), we could dynamically enable sixel support for foot users. If the user does not have chafa installed, we fallback to the default support (just a link to the image). If the user does have chafa in the $PATH, then mdcat detects it and uses chafa to print the image as sixel. This also means that the user can use an out of the box distribution package and just install the external chafa dependancy for sixel support.

This used of an external command would be similar to what is done with rsvg-convert, for the image support of iterm2.

Based on your experience maintaining the rsvg-convert code, do you think introducing something similar with chafa would be an option, instead of pursuing the path of a feature-guarded libsixel integration?

I’m sorry I did not make it clear there would be this new C dependancy. I probably figured it out along the way, initially I thought it would be a nice pure Rust dependancy. Reading this PR again, you also made it clear that you would prefer no external C dependancies early on. I might have lost sight of that when I worked on the sixel crate integration. So really, I should not have asked “but you are still happy with adding the libsixel (C library) system dependancy, right?”. > If there's no other way I'm okay w/ sixel, but it needs to have up-to-date transitive dependency, and the code must be feature-guarded in mdcat, and the feature off by default, to ensure that mdcat builds clean even if the library is not present or no C compiler installed. That makes total sense! ******* Now, on the one hand, if we are feature-guarding the sixel support in mdcat, a user of a terminal that would support sixel would need a custom build of mdcat. That would also likely not be supported by the package manager of their GNU/Linux distribution. On the other hand, with a clean integration of an external command like `chafa` (cleaner than my initial prototype), we could dynamically enable sixel support for foot users. If the user does not have chafa installed, we fallback to the default support (just a link to the image). If the user does have chafa in the $PATH, then mdcat detects it and uses chafa to print the image as sixel. This also means that the user can use an out of the box distribution package and just install the external `chafa` dependancy for sixel support. This used of an external command would be similar to what is done [with `rsvg-convert`](https://codeberg.org/flausch/mdcat/src/commit/3dadfc4976b0a1cb90a6aa4300a2ceb6ae1cfb15/src/svg.rs#L20), for the image support of iterm2. Based on your experience maintaining the `rsvg-convert code`, do you think introducing something similar with `chafa` would be an option, instead of pursuing the path of a feature-guarded libsixel integration?

swsnr commented

Owner

I'm not sure. SVG support is no role model here.

SVG is a terribly complex format, and requires a whole 2D rendering pipeline to turn into pixel graphics, and back then this meant rsvg, Cairo, SVGKit and a hell lot of other native C dependencies. These days I'd just go with resvg and call it a day, even if SVG coverage is perhaps incomplete compared to rsvg.

Getting rid of rsvg-convert is definitely a goal.

So, if there's really no other way to get images on foot, then I'll grudgingly accept an external command, but I definitely don't like it, and I'd really prefer if we had some other means. After all, Sixel isn't SVG, it doesn't need a rendering pipeline, it just turns pixels into a bunch of ASCII characters. How hard can that be?

I'm not sure. SVG support is no role model here. SVG is a terribly complex format, and requires a whole 2D rendering pipeline to turn into pixel graphics, and back then this meant rsvg, Cairo, SVGKit and a hell lot of other native C dependencies. These days I'd just go with resvg and call it a day, even if SVG coverage is perhaps incomplete compared to rsvg. Getting rid of rsvg-convert is definitely a goal. So, if there's really no other way to get images on foot, then I'll grudgingly accept an external command, but I definitely don't like it, and I'd really prefer if we had some other means. After all, Sixel isn't SVG, it doesn't need a rendering pipeline, it just turns pixels into a bunch of ASCII characters. How hard can that be?

swsnr commented

Owner

Coming back I realized that my last comment was very negative, which wasn't really my intention.

Please do update this merge request so that it uses an external command for sixel conversion, in absence of any good Rust implementation. I'll review it again and merge it 🙂

I'm just a tad disappointed that there's no pure Rust implementation, and that foot doesn't support a more powerful image format 🙈

Coming back I realized that my last comment was very negative, which wasn't really my intention. Please do update this merge request so that it uses an external command for sixel conversion, in absence of any good Rust implementation. I'll review it again and merge it 🙂 I'm just a tad disappointed that there's no pure Rust implementation, and that foot doesn't support a more powerful image format 🙈

cljoly added 1 commit

27c90e6834 Move to sixel-rs

It is an fork of the sixel crate, in particular with up-to-date
dependencies.

The changes introduced in the fork look safe: https://github.com/rabite0/sixel-rs/compare/rabite0:73a2ba0...orhun:5d5f1f9

cljoly commented

Poster

Coming back I realized that my last comment was very negative, which wasn't really my intention.

It’s ok, the situation is quite frustrating: no native Rust library, bindings only to libsixel which requires an intermediary file and to separately install a library.

it just turns pixels into a bunch of ASCII characters. How hard can that be?

That prompted me to look more into the sixel format and to start a pure Rust library (it's still in very early stage though).

Please do update this merge request so that it uses an external command for sixel conversion, in absence of any good Rust implementation. I'll review it again and merge it 🙂

The last commit I pushed was just using a crate with up to date dependencies for libsixel. I’ve just seen your message, so probably next week I'll integrate chafa.

> Coming back I realized that my last comment was very negative, which wasn't really my intention. It’s ok, the situation is quite frustrating: no native Rust library, bindings only to libsixel which requires an intermediary file and to separately install a library. > it just turns pixels into a bunch of ASCII characters. How hard can that be? That prompted me to look more into the sixel format and to start a pure Rust library (it's still in very early stage though). > Please do update this merge request so that it uses an external command for sixel conversion, in absence of any good Rust implementation. I'll review it again and merge it 🙂 The last commit I pushed was just using a crate with up to date dependencies for libsixel. I’ve just seen your message, so probably next week I'll integrate `chafa`.

This repo is archived. You cannot comment on pull requests.

No reviewers