feat(cell): add voluntary skipping capability for sixel #215

benjajaja · 2023-06-02T22:53:56Z

feat(cell): add voluntary skipping capability for sixel

Sixel is a bitmap graphics format supported by terminals.
"Sixel mode" is entered by sending the sequence ESC+Pq.
The "String Terminator" sequence ESC+\ exits the mode.

The graphics are then rendered with the top left positioned at the
cursor position.

It is actually possible to render sixels in ratatui with just
buf.get_mut(x, y).set_symbol("^[Pq ... ^[\"). But any buffer covering
the "image area" will overwrite the graphics. This is most likely the same
buffer, even though it consists of empty characters ' ', except for
the top-left character that starts the sequence.

Thus, either the buffer or cells must be specialized to avoid drawing
over the graphics. This patch specializes the Cell with a
set_skip(bool) method, based on James' patch:
https://github.com/TurtleTheSeaHobo/tui-rs/tree/sixel-support
I unsuccessfully tried specializing the Buffer, but as far as I can tell
buffers get merged all the way "up" and thus skipping must be set on the
Cells. Otherwise some kind of "skipping area" state would be required,
which I think is too complicated.

Having access to the buffer now it is possible to skipp all cells but the
first one which can then set_symbol(sixel). It is up to the user to
deal with the graphics size and buffer area size. It is possible to get
the terminal's font size in pixels with a syscall.

A new experimental but fully fledged image widget that uses the skip flag is published at https://github.com/benjajaja/ratatu-image. It does have nice docs but I can't publish as crate until this PR is merge. I am also using said image widget (the "fixed" one) in a branch of iamb to render attachment inline.

codecov · 2023-06-02T22:58:29Z

Codecov Report

Merging #215 (9a13da0) into main (b9290b3) will increase coverage by 2.08%.
Report is 56 commits behind head on main.
The diff coverage is 78.41%.

@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
+ Coverage   85.08%   87.16%   +2.08%     
==========================================
  Files          40       40              
  Lines        8608     9886    +1278     
==========================================
+ Hits         7324     8617    +1293     
+ Misses       1284     1269      -15

Files Changed	Coverage Δ
src/backend/crossterm.rs	`0.00% <0.00%> (ø)`
src/backend/termion.rs	`27.46% <0.00%> (-0.59%)`	⬇️
src/text/masked.rs	`98.64% <0.00%> (ø)`
src/title.rs	`100.00% <ø> (ø)`
src/widgets/calendar.rs	`81.06% <0.00%> (-1.25%)`	⬇️
src/widgets/canvas/circle.rs	`97.14% <0.00%> (ø)`
src/widgets/canvas/line.rs	`59.67% <0.00%> (ø)`
src/widgets/canvas/mod.rs	`91.61% <0.00%> (-0.27%)`	⬇️
src/widgets/canvas/points.rs	`85.71% <0.00%> (+39.56%)`	⬆️
src/widgets/canvas/rectangle.rs	`0.00% <0.00%> (ø)`
... and 25 more

... and 2 files with indirect coverage changes

joshka · 2023-06-02T23:32:54Z

Can you explain this a bit more. What would you write in the commit body of this feature to help someone reading the change log that hasn’t ever heard of a sixel?

Are there docs or tests that you can add to help this?

I wonder if the method to set skipped cells should take a Rect rather than just setting skipped cells individually?

benjajaja · 2023-06-03T07:55:11Z

I added full detail in the commit now.

I wonder if the method to set skipped cells should take a Rect rather than just setting skipped cells individually?

Do you think it's worth adding to Buffer? There's no performance improvement, it would just be convenient.

pub fn skip(&mut self, area: Rect) {
    for y in area.top()..area.bottom() { => u16
        for x in area.left()..area.right() { => u16
            let i = self.index_of(x, y); => usize
            self.content[i].skip = true;
        }
    }
}

joshka · 2023-06-03T11:36:01Z

I spent some time understanding the underlying terminal code. To summarize this a little and to help future developers that might also be unfamiliar with the innards of this code. This particular area of the terminal code is not super well documented, but it's key to understanding this change.

The terminal code uses a double buffer technique, where each frame writes to a buffer in memory, and then the terminal swaps that out for the currently displayed buffer. To avoid having to write a character to the screen for every character in the buffer on every frame the terminal computes the differences between the current buffer and previous buffer, and only provides the backend with the difference to draw.

For displaying a sixel, we set the symbol of a cell to contain an escape sequence followed by image data. BTW, this also works with the iterm image protocol - I tested that by replacing the sixel with some output from imgcat).

When the buffer contains spaces (which is the default character that every cell is reset to), the backend to write the first cell (which displays the image), and then overwrites the rest of the image with the spaces.

This change addresses that problem by introducing an optional skip flag on each cell to indicate that the cell should not be included in the diff calculation (i.e. letting whatever was previously rendered in that space on the terminal stay there).

TL;DR

user code calls frame.render_widget()
widget calls buffer.get_mut(x,y).set_symbol()
the terminal flush method computes the diff (which will contain 1 cell with image data and then many blank cells)
the backend uses the diff to draw each cell in order, which overwrites every cell after the first cell

--

Now, I'm not 100% certain that skip is the right approach here - and hence I'm wary about adding that to the cell's public API without thinking a bit more about this. It definitely works though which is a good thing. One concern is that this may cause items that were previously rendered on the screen not to be replaced (though perhaps this could be a neat effect).

It sounds like what we're really looking at here is a symbol that effectively takes up horizontal and vertical space in the buffer rather than just horizontal space. I wonder if we could make that more explicit by making the cell better specify the amount of the space which it covers? This is kind of related to some of the ongoing work around displaying unicode grapheme clusters and handling how they wrap.

In summary, this is a neat idea. I want it because it would make it super easy to display images in the command line mastodon client I have been building (toot-rs), but I wonder if there's an ergonomic set_symbol_with_size() method that could better fit here.

TLDR; I'm going to think about this a bit more over the next couple of days.

Regarding implementation - I think I'd prefer to see an actual test image (jpg/png/gif) in the lib than a bunch of sixel data (and then do the conversion on start). Perhaps also create a specific example for this feature rather than using the custom_widget (I can imagine that once people see this, they might actually want this as a built-in widget).

benjajaja · 2023-06-03T20:17:19Z

Okay, I am moving it to a standalone "image" example, and creating a Sixel widget in src/widgets/image so that later we can add e.g. iTerm widget or even pick an implementation at runtime based on terminal capabilities / feature / API. I wouldn't add a sixel library dependency to this crate, though. Creating the data can be left to the user, so that we don't force a specific library / version from ratatui.

I understand the concern about adding set_skip(bool) as public API to Cell. This new "skip flag" hacks directly onto the existing grapheme skipping logic - so yes, there is a big obvious overlap. I also think it's worth exploring a common concept of "this cell will render over multiple other cells so don't render some specific other cells". For graphemes it's only horizontal and the amount is derived by the grapheme symbol data, for images it should also be vertical but the amount is not easily derivable from the symbol data (or not something to be done on render).

I would prefer not to do too much specific stuff here. Your set_symbol_with_size() sounds good. Buffer::set_stringn, which deals with finding multi-width graphemes, could then perhaps set_symbol_with_size() instead of set_symbol() and reset()s. This could be enough unification for now.

BTW, I am working on showing images in iamb, a matrix client. It only works on Alacritty with a patch at the moment, because Alacritty ignores draw-overs for some reason.

sayanarijit · 2023-06-04T05:55:34Z

Really excited about this PR. Many users of xplr are looking forward to native image preview support. While this is easily hackable, I want to use a proper API to do so.

benjajaja · 2023-06-04T07:05:52Z

Now that I think about it, we could add a sixel library dependency behind a feature only, and let the sixel widget work with or without it. E.g. having a data(string) method, and some convert(PathBuf, size) if the feature is enabled.

joshka · 2023-06-04T07:59:22Z

What about implementing From<SixelImage> / From<ItermImage> / From<KittyImage> (or whatever the crates that support those pull in.

aschey · 2023-06-04T15:08:20Z

There's also viuer: https://github.com/atanunq/viuer which abstracts over the different image protocols (Kitty, iTerm, and Sixel). Might be useful here.

benjajaja · 2023-06-05T07:56:01Z

I used viuer as reference for using sixel-rs. Unfortunately I couldn't find a way to get the wrapped libsixel to just return the data instead of writing to a file.

orhun · 2023-06-07T23:14:39Z

FWIW, I'm the maintainer of sixel-rs crate: https://github.com/orhun/sixel-rs (active fork of libsixel)

Let me know if I can help with anything on that side.

Just tested the feature and it works well. As @joshka said it would be nice to have an actual image data. Also, having a Sixel widget sounds super exciting!

joshka · 2023-06-07T23:17:41Z

FWIW, I'm the maintainer of sixel-rs crate: https://github.com/orhun/sixel-rs (active fork of libsixel)

That @orhun guy sure gets around :D

benjajaja · 2023-06-08T07:55:43Z

I have started making a sixel widget and going over everything, and for now I would not create an image/sixel widget - at least in this PR.

Given that we can't query the pixels with our default backend, we can't know what area an image will cover, and thus cannot provide a useful widget.

Minimal requirements for an image widget:

That the image does not overflow outside of the widget's buffer area.
It is not enough to simply "overwrite" with Cell data, as that produces flickering artifacts or in some cases is completely ignored. Therefore,
It is necessary to know the font/terminal size in pixels.
termion and termwiz can query the pixel size, but our default backend crossterm can NOT as far as I can tell. Might be possible to query it in raw mode, but I suspect that may not work consistently, e.g. in Windows.
Then, inferring the pixel size of a given area, the image must be cropped.
Either by (re-)encoding the image (expensive) or traversing the pure sixel data (complex).
This must be done each time the widget/area size changes.
Further "fancy" expectations from an image widget: dynamic resize-to-fit, encoding options, animations, support iTerm and Kitty.

Other than that, the user might not be able to use a widget at all where they want to display an image! E.g. they are already implementing Widget. This is my case on iamb: I must draw the image on the buffer in render() of some widget that pushes a conceptual grid of spans into Text. In such cases having an image widget but not being able to control skipping of cells would not work at all.

@joshka @sayanarijit how would you include sixel images - as widget or extending your own widgets? I can imagine a file manager using a widget, but maybe the mastodon use case is similar to my chat-lines use case.

I will push my changes addressing the "separate example" shortly.

benjajaja · 2023-06-08T07:59:18Z

@orhun I feel like I must be mistaken here, I can only see a method to output data to a file in libsixel (and thus sixel-rs). Is there some way to get the bytes directly? Is there some file-descriptor trick that I'm not aware of?

joshka · 2023-06-08T23:52:03Z

Perhaps something like:

    let image = image::open("examples/assets/rust.png")?;
    let image = image.crop_imm(0, 0, 200, 100);
    // can't use image.write_to() unless the buffer is Seekable https://github.com/image-rs/image/issues/1922
    let image_buffer = image.as_bytes();
    let mut sixel = Vec::new(); // not sure what this should be
    let encoder = SixelEncoder::new(sixel); // SixelEncoder doesn't exist - it should though
    encoder.write_image(image_buffer, 200, 100, ColorType::Rgb8);

then later

let image_widget = ImageWidget::new(sixel, 200, 100).pixels_per_cell(16, 10);
f.render_widget(image_widget, area);

Perhaps solve the inability to calculate the area by making that manual for now - i.e. if you can work it out, then use that, if you can't then ask the user to provide it in a config (or at runtime).

Perf issue for cropping: do you have a real use case where perf is an issue? Is the performance slow enough that it matters? Does it happen often enough that it matters? Can you cache the calculation to avoid the perf hit?

E.g. they are already implementing Widget...

Why can't your widget just call render on the image widget?

impl Widget for Grid {
    fn render(self, area: Rect, buf: &mut Buffer) {
        // ... the other stuff
        Image::new(sixel, width, height).render(area, buf);
    }
}

I think it's probably worth considering alacrity ignoring draw overs as a bug rather than a feature. Useful in your very specific case, but annoying otherwise ;)

Thinking more about this over the last few days and simplifying it, I think we either need:

Each cell can be marked as skipped (we currently skip cells implicitly due to wide Unicode chars); or
Each cell has a size that more explicitly captures which cells to skip.

I think we can implement option 2 on top of option 1 later, which makes that choice fairly easy (and basically confirms your approach is the right way to me).

Are there any other concerns we might have around this?

Regarding implementation of the image widget. I'd prefer making a small crate for the image widget and stabilize the API there before bringing it into ratatui core.

benjajaja · 2023-06-09T09:02:45Z

Perhaps solve the inability to calculate the area by making that manual for now - i.e. if you can work it out, then use that, if you can't then ask the user to provide it in a config (or at runtime).

The pixel-area is really dynamic. Some terminals change the font-size depending on the DPI of the screen. Alacritty does this but only when spawned, but maybe some other terminals even change at runtime when moved from one screen to another (which Alacritty should do but doesn't).
We don't have to handle all this stuff right now, my point is just that letting the actual end-user provide this will also not work correctly. You see where this is going, to absolutely nail it we would have to check the terminal's pixel sixe before each render. And again, currently we can only get the size with termion and termwiz.

Perf issue for cropping: do you have a real use case where perf is an issue? Is the performance slow enough that it matters? Does it happen often enough that it matters? Can you cache the calculation to avoid the perf hit?

First, I was wrong and you are correct that we don't need cropping, as only Alacritty is an offender here, and its sixel support is not even official.
I don't have any perf data here, I just assumed that stuff like image resizing and encoding or querying terminal caps would be a no-go for a render method / UI thread. But maybe it's not so important for TUIs. We could probably cache stuff by the Rect area.

So we could provide a dumb image widget that does neither crop nor scale the image to fit its area, it just either gets cut off or does not fill its area. But...

One concern is that this may cause items that were previously rendered on the screen not to be replaced (though perhaps this could be a neat effect).

This does really happen:

Previous render	Current render

...and again we can't really prevent it unless we know the pixel-size of a character. We have to clear only the matching area, and the image must be resized to a multiple of character-pixel-size, so that we don't leave a previously rendered character partly around the right/bottom edges of the image.

Why can't your widget just call render on the image widget?

Oh yes, I didn't think about that, thanks!

Re: Options for the next steps, yes thank you, let's go with option 1 and later make the grapheme stuff and images use a "skip area" instead of one flag per cell.

I also like the idea of moving all this image stuff to its own crate. Then it can get away be somewhat unstable and incomplete, and we can develop faster while seeing the actual use cases.

So these would be the next steps:

Clean up this PR with just the set_skip(bool) added to Cell
- Merge grapheme-skipping with per-cell-skipping
Create a new crate ratatui-image or similar with primitive image rendering
- From here, we can explore pixel-area querying to polish the widget, adding iTerm and Kitty protocols, etc.

benjajaja · 2023-06-14T15:55:50Z

Converting to draft. I am working on the image-widget crate, and I already found that ~~we also need the opposite of skipping: forcing draw even if the diff of the sixel cell did not change, but surrounding cells did~~.
Edit: not correct.

benjajaja · 2023-07-22T11:27:34Z

Just an update on this, I've got a crate that uses this PR's "skipping" to draw image widgets in the works here: https://github.com/benjajaja/ratatu-image

benjajaja · 2023-07-29T19:32:26Z

@joshka I think the image-widget-crate is good enough so far, you may check it out at https://github.com/benjajaja/ratatu-image if you wish. With that, I think we could merge to get the skipping flag, unless there is something else still in the way.

joshka

General approval - if you have some time, can you please add a doc comment to this?

We're aiming for a 0.23 release early next week.

src/buffer.rs

> Sixel is a bitmap graphics format supported by terminals. > "Sixel mode" is entered by sending the sequence ESC+Pq. > The "String Terminator" sequence ESC+\ exits the mode. The graphics are then rendered with the top left positioned at the cursor position. It is actually possible to render sixels in ratatui with just `buf.get_mut(x, y).set_symbol("^[Pq ... ^[\")`. But any buffer covering the "image area" will overwrite the graphics. This is most likely the same buffer, even though it consists of empty characters `' '`, except for the top-left character that starts the sequence. Thus, either the buffer or cells must be specialized to avoid drawing over the graphics. This patch specializes the `Cell` with a `set_skip(bool)` method, based on James' patch: https://github.com/TurtleTheSeaHobo/tui-rs/tree/sixel-support I unsuccessfully tried specializing the `Buffer`, but as far as I can tell buffers get merged all the way "up" and thus skipping must be set on the Cells. Otherwise some kind of "skipping area" state would be required, which I think is too complicated. Having access to the buffer now it is possible to skipp all cells but the first one which can then `set_symbol(sixel)`. It is up to the user to deal with the graphics size and buffer area size. It is possible to get the terminal's font size in pixels with a syscall. An image widget for ratatui that uses this `skip` flag is available at https://github.com/benjajaja/ratatu-image.

kdheepak · 2023-08-24T13:38:34Z

I haven't explored this problem space enough to have an opinion on whether this is the right approach or not; but this is a cool feature and the demo and ratatui-image looks great!

If we are going to merge this, I do like the idea of this kind of convenience function, quoted from one of the comments above:

pub fn skip(&mut self, area: Rect) {
    for y in area.top()..area.bottom() { => u16
        for x in area.left()..area.right() { => u16
            let i = self.index_of(x, y); => usize
            self.content[i].skip = true;
        }
    }
}

benjajaja · 2023-08-24T18:02:04Z

I haven't explored this problem space enough to have an opinion on whether this is the right approach or not; but this is a cool feature and the demo and ratatui-image looks great!

If we are going to merge this, I do like the idea of this kind of convenience function, quoted from one of the comments above:
pub fn skip(&mut self, area: Rect) {
    for y in area.top()..area.bottom() { => u16
        for x in area.left()..area.right() { => u16
            let i = self.index_of(x, y); => usize
            self.content[i].skip = true;
        }
    }
}

This starts to look like a higher level API that allows to set a "cell size" (or specifically "cell render size") but without good support or certainty for merging / diffing buffers. I would wait for an attempt to converge this individual-cell-skipping with multi-width-graphemes. I am currently experimenting with this.

joshka · 2023-08-24T18:28:16Z

I think we can add the convenience after in a .1 release if we want and it makes sense to simplify thinks.

joshka · 2023-08-25T09:21:11Z

Merging this - thanks again for the PR and sticking with it over all this time.

benjajaja changed the title ~~Added per-cell voluntary skipping capability; sixel~~ feat(cell): add voluntary skipping capability for sixel Jun 2, 2023

benjajaja force-pushed the sixel-support branch from c52d240 to 9b5a350 Compare June 2, 2023 22:57

benjajaja force-pushed the sixel-support branch from 9b5a350 to bbfd826 Compare June 2, 2023 23:03

benjajaja force-pushed the sixel-support branch from bbfd826 to ad91508 Compare June 3, 2023 07:40

benjajaja force-pushed the sixel-support branch 2 times, most recently from 0ad8ba8 to bc69767 Compare June 3, 2023 08:07

mindoodoo added the Type: Enhancement New feature or request label Jun 3, 2023

benjajaja force-pushed the sixel-support branch 5 times, most recently from ce9dfa9 to fcbb9b7 Compare June 13, 2023 06:57

benjajaja requested a review from joshka as a code owner June 13, 2023 06:57

benjajaja marked this pull request as draft June 14, 2023 15:54

benjajaja mentioned this pull request Jun 25, 2023

feat(backend): add window size api #276

Merged

benjajaja force-pushed the sixel-support branch 3 times, most recently from 26fec19 to b115591 Compare July 28, 2023 07:02

benjajaja marked this pull request as ready for review July 30, 2023 08:39

benjajaja requested review from orhun, mindoodoo, sayanarijit, sophacles and kdheepak as code owners July 30, 2023 08:39

joshka added this to the v0.23.0 milestone Aug 21, 2023

joshka approved these changes Aug 23, 2023

View reviewed changes

src/buffer.rs Show resolved Hide resolved

benjajaja force-pushed the sixel-support branch from b115591 to 9a13da0 Compare August 24, 2023 08:51

joshka approved these changes Aug 24, 2023

View reviewed changes

kdheepak approved these changes Aug 25, 2023

View reviewed changes

joshka added this pull request to the merge queue Aug 25, 2023

Merged via the queue into ratatui:main with commit e4bcf78 Aug 25, 2023

EdJoPaTo mentioned this pull request May 27, 2024

perf(buffer)!: filled moves the cell to be filled #1148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cell): add voluntary skipping capability for sixel #215

feat(cell): add voluntary skipping capability for sixel #215

benjajaja commented Jun 2, 2023 •

edited

Loading

codecov bot commented Jun 2, 2023 •

edited

Loading

joshka commented Jun 2, 2023

benjajaja commented Jun 3, 2023

joshka commented Jun 3, 2023

benjajaja commented Jun 3, 2023

sayanarijit commented Jun 4, 2023 •

edited

Loading

benjajaja commented Jun 4, 2023

joshka commented Jun 4, 2023

aschey commented Jun 4, 2023

benjajaja commented Jun 5, 2023

orhun commented Jun 7, 2023 •

edited

Loading

joshka commented Jun 7, 2023

benjajaja commented Jun 8, 2023

benjajaja commented Jun 8, 2023

joshka commented Jun 8, 2023

benjajaja commented Jun 9, 2023 •

edited

Loading

benjajaja commented Jun 14, 2023 •

edited

Loading

benjajaja commented Jul 22, 2023

benjajaja commented Jul 29, 2023

joshka left a comment

kdheepak commented Aug 24, 2023 •

edited

Loading

benjajaja commented Aug 24, 2023

joshka commented Aug 24, 2023

joshka commented Aug 25, 2023

feat(cell): add voluntary skipping capability for sixel #215

feat(cell): add voluntary skipping capability for sixel #215

Conversation

benjajaja commented Jun 2, 2023 • edited Loading

codecov bot commented Jun 2, 2023 • edited Loading

Codecov Report

joshka commented Jun 2, 2023

benjajaja commented Jun 3, 2023

joshka commented Jun 3, 2023

benjajaja commented Jun 3, 2023

sayanarijit commented Jun 4, 2023 • edited Loading

benjajaja commented Jun 4, 2023

joshka commented Jun 4, 2023

aschey commented Jun 4, 2023

benjajaja commented Jun 5, 2023

orhun commented Jun 7, 2023 • edited Loading

joshka commented Jun 7, 2023

benjajaja commented Jun 8, 2023

benjajaja commented Jun 8, 2023

joshka commented Jun 8, 2023

benjajaja commented Jun 9, 2023 • edited Loading

benjajaja commented Jun 14, 2023 • edited Loading

benjajaja commented Jul 22, 2023

benjajaja commented Jul 29, 2023

joshka left a comment

Choose a reason for hiding this comment

kdheepak commented Aug 24, 2023 • edited Loading

benjajaja commented Aug 24, 2023

joshka commented Aug 24, 2023

joshka commented Aug 25, 2023

benjajaja commented Jun 2, 2023 •

edited

Loading

codecov bot commented Jun 2, 2023 •

edited

Loading

sayanarijit commented Jun 4, 2023 •

edited

Loading

orhun commented Jun 7, 2023 •

edited

Loading

benjajaja commented Jun 9, 2023 •

edited

Loading

benjajaja commented Jun 14, 2023 •

edited

Loading

kdheepak commented Aug 24, 2023 •

edited

Loading