Make sixel output flicker free #1943
Conversation
There was a problem hiding this comment.
Thanks for working on this - I have tested this myself on wezterm and found that the overall experience was smoother.
I should also mention that sixel support was originally implemented in #1211, and it predates the LockRegion/TTY support added in tcell 2.7.0, so thank you for taking the time to update the code accordingly.
I do have a few comments that I would like to see addressed before merging this, and even then I think I will leave it here for a while in case other users wish to test the changes themselves.
| sxs.altFill = !sxs.altFill | ||
| func (sxs *sixelScreen) clearSixel(win *win, screen tcell.Screen, filePath string) { | ||
| if filePath != sxs.lastFile || win.w != sxs.lastWinW || win.h != sxs.lastWinH { | ||
| screen.LockRegion(win.x, win.y, win.w, win.h, false) |
There was a problem hiding this comment.
Does Screen.LockRegion (with lock = false) actually clear an existing sixel image? I was under the impression that it merely unlocks the region so that changes can be made from tcell.
The reason I mention this is that when I was testing I found that images of different sizes would just be overlaid on top of each other, instead of clearing the existing image first and then drawing a new one. This only occurs when the sixel data is cached like below (note that there is no exit 1):
#!/bin/sh
case "$(file -Lb --mime-type -- "$1")" in
image/*)
chafa -f sixel -s "$2x$3" --animate off --polite on "$1"
;;
text/*)
cat "$1"
;;
esacI'm guessing what happens is that when the sixel data is not cached, then there is no issue:
- The region is unlocked in
clearSixel - A new blank
regobject is created as a temporary placeholder while the sixel data is being reloaded - When the sixel data has finished loading, the screen is cleared at the start of the
ui.drawfunction, thereby clearing the existing sixel image - The new sixel image is drawn
And if the sixel data is cached, then steps 2 and 3 are skipped. But this is just a guess on my part and I could be wrong.
There was a problem hiding this comment.
This is a very insightful comment. My config included an exit 1 and I was quite frankly oblivious to the whole caching mechanism while trying to implement this. I'll keep this in mind. I got a bit overexcited about this implementation so I'm keeping it a draft until I make it solid.
|
A prominent issue that I've encountered is how when I open up some file that spawns another window and the |
| // Get the terminfo for our current terminal | ||
| ti, err := tcell.LookupTerminfo(os.Getenv("TERM")) | ||
| if err != nil { | ||
| log.Printf("terminal lookup failed during sixel render %s", err) |
There was a problem hiding this comment.
I think you are missing a return here.
| if sxs.lastFile != filePath { | ||
| sxs.altFill = !sxs.altFill | ||
| func (sxs *sixelScreen) clearSixel(win *win, screen tcell.Screen, filePath string) { | ||
| if filePath != sxs.lastFile || win.w != sxs.lastWinW || win.h != sxs.lastWinH { |
There was a problem hiding this comment.
So I tried lf using the default settings without the sixel option enabled, and found that this was still being called. I'm not sure how expensive calling Screen.LockRegion is. Maybe it is better to only call this when a sixel image is currently being shown, i.e. add the condition sxs.lastFile != "".
Majorly overhaul sixel preview. Basically fixes #1738
Largely inspired by how it is done in tcell's sixel demo
I upload the comparison between current master, this pr and the last known version when there was no flicker. Terminal emulator: foot.
flicker-small.mp4
It might miss some edge cases, so please suggest any changes.