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 image preview is not cleared by itself #102

Closed
Goosegit11 opened this issue Sep 10, 2023 · 32 comments · Fixed by #117
Closed

SIXEL image preview is not cleared by itself #102

Goosegit11 opened this issue Sep 10, 2023 · 32 comments · Fixed by #117

Comments

@Goosegit11
Copy link

Goosegit11 commented Sep 10, 2023

I used gokcehan lf with ctpv as previewer (ueberzug as backend)

Recently I decided to move to sixel previews. Tinkered a lot, and here I am - with lf-sixel and lfimg-sixel as previewer.

The problem I have is that it doesn't clean previews.

See the video below:
lf sixel issue

Related: #30

@bakkeby
Copy link
Owner

bakkeby commented Sep 10, 2023

I suppose that depends on what that tool does. Sixel images are not cleared in st unless the program sends through the escape codes to clear everything. Maybe it is designed to work specifically with mlterm or the version of xterm with sixel support?

@Goosegit11
Copy link
Author

i'm not sure. with WezTerm everything is okay

@bakkeby
Copy link
Owner

bakkeby commented Sep 11, 2023

I was able to replicate the issue after figuring out how to set up lfimg-sixel. Out of the escape codes that were sent through the case l for Reset Mode (RM) seemed the most likely candidate.

I don't know if that is the best place to add it, but it does clear the preview images in this case. If it causes issues in other contexts then I'll have to revisit.

@schrmh
Copy link

schrmh commented Sep 12, 2023

For me it does seem to work as well after that change. Only thing I noticed after writing reset: I still was able to scroll up (images were cleared however as expected) — but that issue might have existed prior to updating (I don't use the reset command that often).

@bakkeby
Copy link
Owner

bakkeby commented Sep 12, 2023

Not sure what the consensus is when it comes to reset; if it should clear the history or not. st will clear the history with the clear command, but not with clear -x.

xterm and alacritty both clears the history with reset so st should probably do it as well.

@schrmh
Copy link

schrmh commented Sep 12, 2023

To throw three other common names in:
xfce4-terminal (and thus likely vte)
konsole
mlterm

I think I have never noticed a terminal that kept history after reset.

@bakkeby
Copy link
Owner

bakkeby commented Sep 12, 2023

I can't say that I have seen that either. Generally one would only ever use reset if the terminal is graphically scrambled, but I added a proposed change to clear history under RIS -- Reset to initial state.

I am not so sure about the sixel deletion under RM -- Reset Mode though. It looks like this escape code comes through every time I press enter in the terminal. This means that if I use img2sixel or lsix in the terminal then those images are going to disappear as soon as I do anything else.

@bakkeby
Copy link
Owner

bakkeby commented Sep 12, 2023

Changed it so that it only clears sixel images on RM -- Reset Mode escape codes if we are in alt screen.

@veltza
Copy link
Contributor

veltza commented Sep 12, 2023

You can use the following minimal scripts to preview images in lf:

lfrc:

set preview
set previewer ~/.config/lf/previewer
set cleaner ~/.config/lf/cleaner
set sixel

previewer:

#!/bin/sh
case "$(readlink -f "$1")" in
    *.bmp|*.gif|*.jpg|*.jpeg|*.png|*.webp|*.six|*.svg|*.xpm)
        chafa -s "${2}x${3}" -f sixels --dither ordered --dither-intensity 1.0 "$1"
        exit 1 ;;
    *)
        bat -pp -f "$1" ;;
esac

cleaner:

#!/bin/sh
printf "\033[6J" >/dev/tty

ref. #99

Edit: after the commit 1c03f10, you don't need the cleaner script anymore. So do not use exit 1 in the previewer because it disables the sixel caching in lf and triggers the cleaner script. The above scripts still work, but it's a less optimal solution.

@bakkeby
Copy link
Owner

bakkeby commented Sep 12, 2023

Very good @veltza.

Do you think the change ref. 1c03f10 that adds sixel clearing for the Reset Mode (RM) should be removed?

@Goosegit11
Copy link
Author

Goosegit11 commented Sep 13, 2023

@bakkeby

I was able to replicate the issue after figuring out how to set up lfimg-sixel. Out of the escape codes that were sent through the case l for Reset Mode (RM) seemed the most likely candidate.

Sorry for the long reply. Yes, everything works now, thanks!
I recorded a video comparing WezTerm and st in terms of sixel performance.
You can see that st is faster, but dirtier. I couldn't get it on the video, but there also small black bars appear under the images. And WezTerm is clean, but feels slow.
https://github.com/bakkeby/st-flexipatch/assets/89806596/1c3717e7-0f32-42f4-af34-78cd2bad23d3

upd: for some reason the video is not showing up on GitHub, only as link. maybe it's too big (6 MB), idk.

@bakkeby
Copy link
Owner

bakkeby commented Sep 13, 2023

I have seen that as well, characters drawn on this form:

⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀

That is repeating the same character (⠀) over and over again and this is the Braille Pattern Blank character.
https://www.compart.com/en/unicode/U+2800

Looks like it is something that lf is printing with the aim of cleaning the preview area.

@Goosegit11
Copy link
Author

I mean, is there a way to make it cleaner, like in WezTerm?

@bakkeby
Copy link
Owner

bakkeby commented Sep 13, 2023

https://github.com/gokcehan/lf/blob/ffc756c5ff2e6c1552019a3b2df909783a26c12f/sixel.go#L12

I tried changing that filler character to \u0020 (space character) and it seems to work just fine.

@Goosegit11
Copy link
Author

Do I need to compile lf from source for this "patch"?

@bakkeby
Copy link
Owner

bakkeby commented Sep 13, 2023

Of course.

@bakkeby
Copy link
Owner

bakkeby commented Sep 14, 2023

So I looked into this more and I found that interestingly wezterm does not manage to render sixel images with that change.

I downloaded the wezterm source code and I did not find that it specifically references this character in relation to sixel rendering.

But I did find this character in the braille-wezterm-logo.txt test file.

How it looks in st.
image

How it looks in Alacritty.
image

and finally how it looks in wezterm.
image

Clearly the latter has some special handling for braille characters.

In the changelog there is this line:

Improved: implement braille characters as custom glyphs, to have perfect rendering when custom_block_glyphs is enabled.

That custom_block_glyphs defaults to true and it has this description.

When set to true (the default), WezTerm will compute its own idea of what the glyphs in the following unicode ranges should be, instead of using glyphs resolved from a font.

So it is very much the same as the boxdraw patch for st, just for braille characters.

If I disable that config the font renders like in other terminals.

image

and the same characters appear / ficker behind the image when using sixel previews in lf (like in st).

With that config disabled wezterm still does not render sixel images if using a space character for the filler.

As such I am inclined to think that the use of the braille character for clearing the sixel image may be an internal thing specifically for lf (or tcell which is a dependency) and that it looks clean in wezterm could just be a happy coincidence.

I checked the kitty terminal and that also has transparent braille characters in the same manner as wezterm has, but kitty does not support sixels (but instead has its own image format).

@veltza
Copy link
Contributor

veltza commented Sep 14, 2023

@bakkeby I'm not familiar with how that RM sequence works, but I'm afraid it can create nasty side effects if it deletes sixels every time it is called. But let it be for now because your solution doesn't disable the sixel caching in lf unlike mine. So, it is a more optimal solution.

I also have another idea about how to automatically clear or delete sixels. When a new sixel is created, the underlying cells get the ATTR_SIXEL attribute. So, before drawing a sixel, we check if underlying cells still have that attribute and delete the sixel if any underlying cell is overwritten (we have to delete the whole sixel, because we can't partially clear sixels). That should work fine in many cases, but I bet there are some edge cases too. This needs to be investigated more.

@Goosegit11 st-flexipatch indeed has a faster sixel implementation than WezTerm and you can improve image quality with dithering:

chafa -s "${2}x${3}" -f sixels --dither ordered --dither-intensity 1.0 "$1"

You can also fix that braille issue by just using different fonts because the braille pattern blank character you see is coming from one of your fonts. However, that doesn't change the fact that U+2800 shouldn't be used as a blank character. I have already pushed a proposed fix to the lf's upstream, which uses U+2000 as the filler character, because it works well in my tests. So, let's see if it will be merged before the next release.

Edit: added info about the sixel caching.

@Goosegit11
Copy link
Author

github-lf-issue5.mp4

Changed filler char to u+2000.
Black bars are still present, but it's not a big issue for me.

See what happens when I resize the window - the only way is to restart lf, which I don't like

@bakkeby
Copy link
Owner

bakkeby commented Sep 14, 2023

The black bars I suppose comes from when the image can't be resized to cover exactly the amount of pixels that a block takes up.

May have something to do with this, not sure.

st-flexipatch/sixel.c

Lines 151 to 156 in 1c03f10

if (height > image->height) { /* if height is extended */
/* fill extended area with background color */
memset(alt_buffer + width * image->height,
0,
(size_t)(width * (height - image->height)) * sizeof(sixel_color_no_t));
}

That the image does not resize has something to do with the previewer. It looks like the image only resizes automatically if the height of the window changes - if the window only changes size horizontally then the image does not resize.

If you try veltza's minimal script then you will see that this does not happen there. I suspect that it may have something to do with the cache that is used in the lfimg-sixel preview file.

@veltza
Copy link
Contributor

veltza commented Sep 14, 2023

@Goosegit11 lf caches sixels in memory, but it doesn't throw away cached images when the window is resized as it should. The same problem also occurs on WezTerm. This resizing issue should be reported to the lf maintainers because we can't fix it here.

@bakkeby You are right. There are actually two routines that draw those bars at 137 and 152. I never noticed them because I use the black background color. I'm just wondering if we really need to fit the images into the cells and add padding. Or could we just draw those images as they are?

@Tanish2002
Copy link

Hello. I just checked this out myself and lf previews seem to work fine on the latest commit!

However,
I use a zsh plugin called fzf-tab. It supports file previews as well.
I tried it with sixel and it prints fine but the image isn't cleared.
I tried the same thing with xterm with -ti 340 flag for sixel support it seems to work fine there.

Here the preview script:

#!/bin/sh
# Only works with Terminals supporting sixel graphics

case "$(file -L --mime-type "$1")" in
*text*)
	bat --color always --plain --theme gruvbox "$1"
	;;
*image* | *pdf)
	if command -v chafa; then
		chafa -s "50x50" -f sixels --dither ordered --dither-intensity 1.0 --animate off --polite on "$1"
	else
		echo "Install chafa and use a terminal with sixel graphics"
	fi
	;;
*directory* | *symbolic*)
	ls -1 --color=always "$1"
	;;
*)
	echo "unknown file format"
	;;
esac

and here is how I use it in zshrc.

zstyle ':fzf-tab:complete:*:*' fzf-preview '/path/to/preview.sh $realpath'

Here is video comparing xterm and st-flexipatch with sixel patch.
https://github.com/bakkeby/st-flexipatch/assets/55488165/37f2d347-d223-4686-b405-c60f939f090c

@veltza
Copy link
Contributor

veltza commented Feb 22, 2024

st-flexipatch is using the Reset Mode (RM) control sequence to clear sixel images, which only works with lf file manager but not with fzf or any other application, unfortunately.

In my previous post, some time ago, I presented another idea to clear sixels automatically:

I also have another idea about how to automatically clear or delete sixels. When a new sixel is created, the underlying cells get the ATTR_SIXEL attribute. So, before drawing a sixel, we check if underlying cells still have that attribute and delete the sixel if any underlying cell is overwritten (we have to delete the whole sixel, because we can't partially clear sixels). That should work fine in many cases, but I bet there are some edge cases too. This needs to be investigated more.

And after that, I built my own fork based on that idea, and so far it has worked fine with every application I've used. So st-flexipatch would need a similar implementation for clearing sixels.

@Tanish2002
Copy link

@veltza just tried out your fork. It works perfectly! The images are created and cleared perfectly.
Would be really apreciated if you could create a patch with your sixel implementation.

@bakkeby I think it would be nice if st-flexipatch's implementation was based upon @veltza implementation if it's possible

@bakkeby
Copy link
Owner

bakkeby commented Feb 23, 2024

I don't think veltza needs to prepare a patch. I'll have a look and compare when time allows (I have a few other things going on at the moment).

There never were a patch for sixel; we are just building on someone else's work trying to integrate sixel into st (and veltza has been very helpful addressing various bugs). Creating a patch is also not that straightforward due to how it needs to be integrated with a variety of other patches (e.g. scrollback).

@veltza
Copy link
Contributor

veltza commented Feb 23, 2024

@bakkeby Although my fork is built on st-flexipatch, I've run it through flexipatch-finalizer, so if you need help just ask.

@bakkeby
Copy link
Owner

bakkeby commented Feb 27, 2024

@veltza I have to hand it to you - you have a very impressive build of st. Seems you also have reflow working without being mangled if you resize the window to the minimum width and back again (although it screws up the history / scrollback).

The separate files (sixel.c/g, sixel_hls.c/h) are fairly straightforward; the main complexity will be working out what needs to change in st.c. I'll start a branch at some point for this work, just have a few things going on at the moment.

@Tanish2002
Copy link

I don't think veltza needs to prepare a patch. I'll have a look and compare when time allows (I have a few other things going on at the moment).

yeah sure haha, I never forced veltza to create a patch, Also I never intended for a patch to be made specifically for st-flexipatch, really all I wanted was a patch for vanilla st.

Anyways, It's always appreciated if you're working on it! Great work by both of you 😄

@veltza
Copy link
Contributor

veltza commented Feb 28, 2024

@Tanish2002 Why do you need the sixel patch for the vanilla st? Can't you just use st-flexipatch? Because you get pretty much the same result when patching manually or using st-flexipatch. You can even remove unused patches with flexipatch-finalizer.

@step-
Copy link
Contributor

step- commented Feb 29, 2024

subscribe

@bakkeby
Copy link
Owner

bakkeby commented Feb 29, 2024

I started a branch https://github.com/bakkeby/st-flexipatch/tree/sixel_sx trying to integrate veltza's changes. Will need testing and I am sure that I may have missed a few things.

@step-
Copy link
Contributor

step- commented Mar 1, 2024

Hats off to @veltza's st-sx and to you for this integration! The first test inside tmux 3.4 (and outside) worked well. The only snag is that the picture covers the bottom part of the previous prompt line. I'll attach a screenshot later but it's obvious when you see it. This doesn't happen in st-sx.

For testing I'm using using shell's "PS1=> ", the same patches.h that I attached to #111, and image.sixel downloaded from tmux/tmux Tools directory.

I've also added a termcap entry for sixel on tmux. I'll push the PR for your consideration #112.

st-sixel-102-20240301

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 a pull request may close this issue.

6 participants