Skip to content

Make sixel output flicker free#1943

Merged
joelim-work merged 11 commits into
gokcehan:masterfrom
Kuchteq:master
Apr 28, 2025
Merged

Make sixel output flicker free#1943
joelim-work merged 11 commits into
gokcehan:masterfrom
Kuchteq:master

Conversation

@Kuchteq

@Kuchteq Kuchteq commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

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-fix.mp4

Notice how the screen stops flickering once the images get loaded into cache. This is thanks to overwriting the existing image instead of first writing blank cells and then reprinting the image. This requires the sixel output to fit exactly into the terminal's cell size.
In practice, you can make chafa do it by adding the -t 1.0 flag (read more on this in chafa's man). You can also adjust the background that it then prints with the --bg flag.

@Kuchteq Kuchteq marked this pull request as draft April 2, 2025 14:10

@joelim-work joelim-work left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread sixel.go Outdated
Comment thread sixel.go Outdated
Comment thread sixel.go Outdated
Comment thread sixel.go Outdated
@Kuchteq

Kuchteq commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

A prominent issue that I've encountered is how when I open up some file that spawns another window and the open function is a regular shell (marked with $), it will cause a rerender after the command is executed and then if the window gets resized in the meantime (say due to running a tiling wm) there will be another rerender caused by the dimension change. This frequently corrupts the screen but it's not a guarantee, so there might be some race condition as to which event gets launched first. I'll have a go at it to try to address that.

Comment thread sixel.go Outdated
Comment thread sixel.go Outdated
This fixes issues related to clearing the previous sixel data
@Kuchteq

Kuchteq commented Apr 6, 2025

Copy link
Copy Markdown
Contributor Author

New commit addresses the issue where if the new image was wider/taller then the previous image, there would be some residue from the previous image. The idea is that we look up the width and height of the upcoming sixel image data and lock only the region required to fit those dimensions. Next time the image gets cleared, we unlock that same region. The two calls, clearing and printing sixel can be done one after another given that the new one will simply overwrite the previous one. This effectively also takes care of thinking about clearing the image.

Comment thread eval.go
Comment thread sixel.go Outdated
Comment thread sixel.go Outdated
Comment thread sixel.go Outdated
Comment thread sixel.go Outdated
@joelim-work

Copy link
Copy Markdown
Collaborator

@Kuchteq Thanks for looking into the overlapping image residue problem. I think it makes sense to restrict locking to only the size of the sixel image itself. Based on my testing, locking the entire preview window seems to prevent the residue from the previous image from being cleaned up.

I have a few more comments, after that I'm mostly happy with the PR assuming there aren't any other bugs.

@Kuchteq

Kuchteq commented Apr 10, 2025

Copy link
Copy Markdown
Contributor Author
corruption.mp4

I'm unfortunately facing an issue where the preview gets corrupted if files are flipped too often. I'm not sure why this happens exactly but it probably has to do something with the caches as adding exit 1 to the end of my preview script dismisses the issue.

@joelim-work

joelim-work commented Apr 11, 2025

Copy link
Copy Markdown
Collaborator

I haven't been able to reproduce that issue of the preview becoming corrupt so far, it might be because the terminal is receives the sixel data interleaved with something else. But I'm not sure what it has to do with the sixel data being cached, unless that itself is corrupt. Have you tried testing on other terminals? So far I have only really used wezterm.


Another thing is that I tested the behavior of lf when changing various settings, and I found some issues:

  • set drawbox!/set ratios 1:1:1 - Both of these change the size of the preview window, and the residue left from the previous sixel image isn't cleaned up properly. I ended up fixing this by using the area of the previous preview window for unlocking during clearSixel, as opposed to the area of the current preview window.
  • redraw (alternatively press <c-l>) - This made the sixel image disappear but not be redrawn again. I ended up having to clear the regCache to ensure the image gets reloaded and drawn again.
  • set sixel! - This doesn't clean up the existing sixel image. I ended up adding a flag to forcibly clear the sixel image in this case.
  • set preview! - Similar to set sixel!, but I also had to move the clearSixel function call to outside the if gOpts.preview condition, since the sixel image should be cleared even after disabling preview.

Also just note that there were some recent changes committed to the master branch to fix various other issues. There are a few merge conflicts which you'll have to resolve.

BTW I ended up writing my own branch for this - it's based on your work but closer to how I would implement it, and I wanted to resolve the merge conflicts on my end so I could test on top of all the recent changes. Feel free to take anything from there if it's helpful.

@Kuchteq

Kuchteq commented Apr 11, 2025

Copy link
Copy Markdown
Contributor Author

Thank you for the work you put in and checking out the various options, I haven't really done so myself as I was trying to make sense of the corruption issue. I checked out your branch and it unfortunately faces the same issue, both in foot as well as wezterm (I attach a screen recording) but in wezterm the corruption looks a bit different.

wezterm-corruption.mp4

What I do notice is that its occurance is much rarer when the terminal spans the entire width of the screen (or is just big). Btw I might not have mentioned it but I test it by first going to a marked directory and then alternating between the jump list (I guess, default mapped to double apostrophes, mine's at ).

@joelim-work

Copy link
Copy Markdown
Collaborator

I haven't been able to trigger this corruption issue at all. So you don't have anything in your config file that could trigger it? You probably are doing this already, but for testing it's easier to just have a minimal config file (just need to set previewer and sixel) to ensure that nothing else can interfere.

@Kuchteq

Kuchteq commented Apr 11, 2025

Copy link
Copy Markdown
Contributor Author

Though so obvious, I've been oblivious to it. Minimal config works flawlessly, the reason it failed on mine is because of the on-cd command:

cmd on-cd &{{
    printf "\033]0; $PWD\007" > /dev/tty
}}

Which just sets the title name of the terminal for title bar purposes and that must corrupt the output because of an escape there. Perhaps a lock could help resolving that or making sure that on-cd runs before preview as this seems to be the source of the issue? This complicates things so maybe there's some way to solve it on the configuration side, though I doubt it. Furthermore, this is part of lf docs so this configuration might be commonplace.

--- Edit
This is out of scope of this mr but maybe to mitigate this there would be some new flag that would sync the terminal title (and hopefully cwd OSC 7). Of course it would need to be more complicated than that but testing a solution similar to this pseudocode avoids the buffer corruption:

func onChdir(app *app) {
	tty, ok := app.ui.screen.Tty()
	if ok && syncTerminalFlag {
               tty.Write([]byte(fmt.Sprintf("\x1b]7;file://%s\x1b\x1b]0;%s\x1b", app.nav.currDir().path, app.nav.currDir().path)))
	}
       ...
}

@joelim-work

Copy link
Copy Markdown
Collaborator

Yes, writing to /dev/tty like that would definitely be a problem because without some kind of locking mechanism the writes won't be synchronized. AFAIK there are only a few places where data is actually written to the tty:

  • Displaying the UI via Screen.Show:

    lf/ui.go

    Line 1099 in 0df0c0f

    ui.screen.Show()
  • Redrawing the display from scratch based on the internal state via Screen.Sync:

    lf/eval.go

    Line 1416 in 0df0c0f

    app.ui.screen.Sync()
  • Displaying sixel images, which happens somewhere during the ui.draw function.

All of these happen in the event loop of the main thread so there is no problem.

For the purpose of implementing features like changing the window title, rather than adding a flag option to control whether lf should additionally write specific escape sequences when changing directories, I would prefer to implement this as a command like tty-write that users can call as it is more flexible. Something like below:

case "tty-write":
	if len(e.args) != 1 {
		app.ui.echoerr("tty-write: requires an argument")
		return
	}

	tty, ok := app.ui.screen.Tty()
	if !ok {
		log.Printf("tty-write: failed to get tty")
		return
	}

	tty.Write([]byte(e.args[0]))

lfrc:

cmd on-cd &{{
    lf -remote "send $id tty-write \"\033]0;$PWD\007\""
}}

cmd on-init :{{
    on-cd
}}

@Kuchteq

Kuchteq commented Apr 12, 2025

Copy link
Copy Markdown
Contributor Author

Great idea, this way adds flexibility! Would we implement it as a separate merge request?

@joelim-work

Copy link
Copy Markdown
Collaborator

I've already submitted it as a separate PR: #1961

Can you please test it to see if it fixes the issue for you? If so then I will merge it.

@Kuchteq

Kuchteq commented Apr 12, 2025

Copy link
Copy Markdown
Contributor Author

Yup, this works, no corruption occurs from now on, I've left one comment there regarding documentation.

@joelim-work joelim-work left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this stage I'm mostly happy with the changes - I did find one issue however during testing though. Fixed, see #1943 (comment)

Also I was originally thinking about leaving this PR open for at least a month to get feedback, however I think there generally isn't enough interest from users that they will bother testing PR branches. So the only way to get proper feedback is to actually merge it, but then there is a chance I will have to revert it if it causes too many issues. What is your opinion on this?

Comment thread sixel.go Outdated
@Kuchteq

Kuchteq commented Apr 13, 2025

Copy link
Copy Markdown
Contributor Author

Another issue to solve is when a user updates the currently previewed image to something else it currently doesn't update in the preview, somewhere there needs to be a call to forcefully update the preview with the new contents. I couldn't have figured out what's the best place to put it. Maybe somewhere in some update channel code?

Comment thread sixel.go
Comment thread sixel.go Outdated
return
}
cw, ch := ws.CellDimensions()
if cw < 0 || ch < 0 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant to write:

if cw <= 0 || ch <= 0

Or this should work too:

if !(cw > 0 && ch > 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't have a fallback plan for this condition, just be aware that this means images will no longer work on Windows, or remote connections over telnet. And there might still be some Linux terminals that will be affected as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, minor mistake on my front, thanks for catching that.

Good point on the windows support, I haven't seen any references in tcell to fetching that info there. So that complicates things. We could fall back to locking the entire preview window and clearing it by writing blanks... Otherwise there's no way to avoid flicker without knowing the cell size in pixels.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are escape sequences you can use to query the cell size, but I suspect that's something that would have to be done by tcell. Otherwise I think it's fine to fall back to clearing the entire preview window the way it worked before - that's certainly better than having no images at all.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Kuchteq, I noticed you marked the PR from a draft to being ready for review. What is your plan for the case where TIOCGWINSZ does not report the size in pixels?

@joelim-work

Copy link
Copy Markdown
Collaborator

Another issue to solve is when a user updates the currently previewed image to something else it currently doesn't update in the preview, somewhere there needs to be a call to forcefully update the preview with the new contents. I couldn't have figured out what's the best place to put it. Maybe somewhere in some update channel code?

@Kuchteq lf should be able to detect file changes via the set watch true/set period 1 options, and invoke the previewer script accordingly. For sixel images, what actually happens is that the new preview is sent to app.nav.regChan (this is normal), but when the UI is drawn afterwards the sixelScreen object doesn't know that the image has changed as it only compares the path and window size.

I think in this case it's fine to just set the forceClear flag like below:

diff --git a/app.go b/app.go
index e03c07e..3ca7611 100644
--- a/app.go
+++ b/app.go
@@ -431,14 +431,17 @@ func (app *app) loop() {
 		case r := <-app.nav.regChan:
 			app.nav.regCache[r.path] = r
 
 			curr, err := app.nav.currFile()
 			if err == nil {
 				if r.path == curr.path {
 					app.ui.regPrev = r
+					if gOpts.sixel {
+						app.ui.sxScreen.forceClear = true
+					}
 				}
 			}
 
 			app.ui.draw(app.nav)
 		case f := <-app.nav.fileChan:
 			for _, dir := range app.nav.dirCache {
 				if dir.path != filepath.Dir(f.path) {

@Kuchteq Kuchteq marked this pull request as ready for review April 15, 2025 10:50

@joelim-work joelim-work left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I think this is fine to merge now. Since r35 was just released, there won't be another release for a while so there's plenty of time to test these changes.

Thanks once again for the patch.

@joelim-work joelim-work merged commit 7a89808 into gokcehan:master Apr 28, 2025
@joelim-work joelim-work added the fix Pull requests that fix existing behavior label Apr 28, 2025
@joelim-work joelim-work added this to the r36 milestone Apr 28, 2025
@joelim-work joelim-work mentioned this pull request Apr 28, 2025
heather7283 added a commit to heather7283/lf that referenced this pull request May 20, 2025
While new sixel impl looks better and works faster, it breaks my custom
previewer script that prints additional information about images/videos
below sixel preview
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix existing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lf image preview flickering

3 participants