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 for preview panel #840

Closed
wants to merge 44 commits into from

Conversation

horriblename
Copy link
Contributor

@horriblename horriblename commented May 20, 2022

see issue

This PR aims to add support for sixel graphics

It does this by printing directly to stdout, without going through tcell

- [ ] check for sixel support
- [ ] manually reset DECSDM ESC[?80l on app init (not important)

  • new variable to save current screen size in pixels, perhaps in ui
    • function to read current screen size (unix)
    • function to read current screen size (windows - are there even any sixel terminals there?)
    • recalculate on resize
  • add struct for holding sixel data, then add field ui.sixels
  • print sixel sequence to stdout
    • process single-line sixel sequence
    • process multi-lined sixel sequence
    • print to stdout
  • cleanup image
    • SetContent cells in the image to braille space \u2800
    • measure size of sixel (in px)
    • utility functions for dealing with sizes in pixel/cell
    • maybe manually clean preview panel in between different images
  • redraw event actions
    • clear sixel cache (if needed?)
  • update wiki

Problems

  • Images that are too large in height (on screen) will cause the terminal to scroll down, breaking the entire ui.
    • large images should be trimmed in height, large widths will be allowed as they do not break anything (The preview script already has the dimensions anyway)
  • sixel sequences can get reaaally long, if it's on a single line it might error out with bufio.Scanner: token too long
    • preview script should separate long sixel sequence into multiple lines (i.e. pass into sed 's/#/\n#/g').
    • maybe print an error string in preview panel so users are not confused?
  • Commands (: etc.) lags a lot and cursor shows at the wrong location

@cain-dev
Copy link

Whoa, this is advancing really fast! Glad to see that! Anxious to have sixels working here ç.ç

Please keep up the good work, won't be unnoticed o/

@volfyd
Copy link

volfyd commented May 21, 2022

Showing the previews works for me with foot, with this preview script

#!/bin/sh
if [ "${1: -4}" == ".jpg" ]; then
    /usr/bin/chafa -f sixel "$1" -s "$4"x | sed 's/#/\n#/g'
    exit 1
fi

And my cleaner script is

#!/bin/sh
killall -s SIGWINCH lf # lol

Excited for this patch!

@cain-dev
Copy link

chafa -f sixel ftw, the universal solution. ANSI for non sixel support and chafa -f sixel for foot users xD

@cain-dev
Copy link

Hey @horriblename, is it possible to make the converted image cache permanent and read it from there or this is 100% dependent on the preview script? I have a very (very, very..) slow computer and it takes some (noticeable) time to convert the image to sixels. I bet i am not the only one xD

Thanks again for your work mate

@horriblename
Copy link
Contributor Author

horriblename commented May 25, 2022

is it possible to make the converted image cache permanent and read it from there or this is 100% dependent on the preview script?

I noticed that loading from cache takes too long as well, I will work on that later. Clearing the image (efficiently) takes priority now, if I can't do that there's no point going on lol.

Bringing up command prompts (via :, $ etc) lags a lot, I was trying to make the rendering loop as close to the existing "write to buffer then Show" model as possible but the writing part and Show run very often and printing the sixels are way too slow for this. I have to reassess when to draw sixels and when to erase them

@horriblename
Copy link
Contributor Author

Konsole behaves wierdly with sixels, writing text over images just writes on top of the image, whereas wlterm and foot overwrites the cells

@horriblename
Copy link
Contributor Author

horriblename commented May 25, 2022

I've settled on this:

  • use Braille blank \u2800 to fill the buffer area where the image is supposed to be, using SetContent
  • after screen.Show, print sixel image to stdout
  • SetContent the same area with braille blank for as long as we want the image exist, so that tcell doesn't touch this part
  • the image will be overwritten by whatever text/blank space we want to preview next time

This doesn't work on Konsole because their implementation is bugged and the only way to delete the image there is to redraw the entire screen

@cain-dev
Copy link

cain-dev commented May 25, 2022

" Clearing the image (efficiently) takes priority now, if I can't do that there's no point going on lol."
Fair! I agree with you.
Don't need to rush, you are doing a great job already , just keep up the good work o/

@horriblename
Copy link
Contributor Author

horriblename commented May 26, 2022

Don't need to rush, you are doing a great job already , just keep up the good work o/

I have a bunch of free time lately so no worries haha

That said, it's working mostly flawlessly now, still a few bugs here and there. If anyone wants to test it keep these in mind:

  • do not exit 1 in your preview script. this tells lf that the script itself will handle caching and cleanup. Sixels are handled internally, a cleaner script is not needed
  • sample preview script:
#!/bin/sh
case "$(printf "%s\n" "$1" | awk '{print tolower($0)}')" in
   *.bmp|*.jpg|*.jpeg|*.png|*.xpm|*.webp|*.gif)
      chafa "$1" -f sixel -s "$(($2-2))x$(($3-2))" | sed 's/#/\n#/g'
   ;;
   *) cat "$1";;
esac
exit 0
  • note that due to my mistake, the image might have a 1 row & 1 column empty space before it
  • In the preview script, pipe the sixel into sed 's/#/\n#/g to split the string into shorter lines, or else it will error out and show nothing (see sample above)
  • be careful not to set the image size too large, it will break the entire ui and refreshing won't help
  • caching should be working fine now

That's all, and leave a comment if you encounter a bug

Bugs

  • unneeded 1 row & col of padding before image
  • cursor out of place during commands :,$ etc.

@cain-dev
Copy link

Hey @horriblename , sorry if this is a dumb question but why don't just move the preview script to something integrated with LF itself and just use a config file? Wouldn't this make it more stable and have less bugs? Preview scripts at this point are just more of the same: text for texts, image for images, videos and pdfs.

I know this isn't ranger but scope.sh is a very convenient way to make chages to preview. Why the preview script like like lfimg or sptv are really necessary at this point?

One thing at a time, i know, but what is the limitation?

@horriblename
Copy link
Contributor Author

Not really a question that I can answer for others, but I'm guessing that the preview script makes it more customizable and flexible. I use pdftotext to preview pdf files but others convert it into an image thumbnail, so having the preview script gives you full control to do whatever you want

Wouldn't this make it more stable and have less bugs?

I'm not sure what bugs you're talking about, but my experience with lf was fairly smooth, I used the lfimg scripts for a long time and I didn't even know how it works behind the scene, up till weeks ago when I switched to wayland and ueberzug didn't work.

If you're talking about the bugs from this PR, (don't exit 1 and such), they're not really bugs, passing a sixel image to lf is just completely different from using external apps to preview, cache and so on; so you need to write your previewer a little differently

to be frank, if this PR is accepted, it would simplify my preview script a LOT, but that's still not enough reason to eliminate scripts, as they work just fine, plus other people might still find the extra bit of flexibility useful.

tl;dr preview scripts are more flexible and they work just fine, so there's no reason to eliminate them

@horriblename
Copy link
Contributor Author

do not exit 1 in your preview script. this tells lf that the script itself will handle caching and cleanup. Sixels are handled internally, a cleaner script is not needed

To reiterate, you treat a sixel sequence as you would any other form of text, give it to lf and it'll handle the rest.

No external cleaner scripts, no exiting with signal 1, nothing. It's a normal text and nothing more.

@horriblename horriblename marked this pull request as ready for review July 13, 2022 13:29
@horriblename
Copy link
Contributor Author

horriblename commented Jul 13, 2022

Things still missing:

  • windows (complete getTermPixels and things should work)
  • not sure if it works correctly under other unix systems that is not linux
  • wiki
  • clear cache after window resize: right now cache only clears if height is different after resize

If anyone wants to help with any of these, please feel free.

@gokcehan Hope you can review this, thanks!

@tinywrkb
Copy link

I'm looking to switch from ranger to lf due to the portable binaries, so it is great seeing here the sixel support being worked on.

One thing I noticed with sixel in ranger is that resizing the images make a huge difference in performance, and the UI is much more responsive. BTW, my terminal emulator is foot.
I hope this would be taken into consideration. I'm sure I'm not the only one who wouldn't mind having images scaled down for the benefit of a more responsive UI.
Maybe a setting for a max resolution for the output sixel image should be added?

@horriblename
Copy link
Contributor Author

If you want to limit the image size you can do it in the preview script:

w=$2
# set w to 40 if greater than 40
["$2" -gt 40 ] && w=40
h=$3
["$3" -gt 30 ] && h=30

# if you're using chafa:
chafa "$1" -f sixel -s "${w}x${h}" | sed 's/#/\n#/g'

I'm not very good at bash so there might be a better way

@horriblename
Copy link
Contributor Author

@tinywrkb there are also some optimization options in chafa that might help you:

  • -c limits the number of colors. I think 240 and 256 give somewhat acceptable quality. From my testing 256 halves the final image size compared to full; 240 halves the size compared to 256
  • -O is some kind of optimization parameter, I don't really know what it does, you'll have to test them yourself

@horriblename
Copy link
Contributor Author

  • -c limits the number of colors. I think 240 and 256 give somewhat acceptable quality. From my testing 256 halves the final image size compared to full; 240 halves the size compared to 256

By size I meant size as in memory, the resolution would stay the same, and I don't think this is going to help much tbh, just something you can consider

@gokcehan
Copy link
Owner

@horriblename Thanks for working on this and sorry for not getting back to you earlier. As you might be aware, I have mostly been ignoring anything but the simplest patches in lf as I have burned out and also I was busy irl. But I felt like dropping a comment for this patch as it seems to be getting popular. Personally I don't think it is a good idea to directly handle terminal with non-standard escape codes in general as I feel like such things belong to the terminal handling library to be portable (e.g. tcell). We have had issues with such implementations in the past so I don't want to take responsibility for it. I should say that I think it is perfectly feasible to maintain this patch in your own fork as changes in this fork are rare these days. I'm aware there are many people interested in sixels and I'm sure they would be happy to see this implementation. In the past, I suggested some other contributors to create a wiki page for actively maintained forks with patches not included in this fork. Instead, we could also add a forks section in the readme for better visibility.

@Guiltybyte
Copy link

@horriblename How do I build your fork? I am interested in testing it out, but not sure how the build instructions differ from the main project.

@horriblename
Copy link
Contributor Author

@Guiltybyte I just updated the instructions on my repo

On Unix (Go version < 1.17):

env CGO_ENABLED=0 GO111MODULE=on go get -u -ldflags="-s -w" github.com/horriblename/lf

On Unix (Go version >= 1.17):

env CGO_ENABLED=0 go install -ldflags="-s -w" github.com/horriblename/lf@latest

@horriblename
Copy link
Contributor Author

@gokcehan yeah I understand that it's really hacky, thanks for taking a look though.

I'll maintain a fork moving forward

Instead, we could also add a forks section in the readme for better visibility.

Alright, I'll submit a separate pr when I get around to doing that :)

I'm closing this PR, any further developments will be done on the fork

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.

7 participants