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

preview-tui: mpv kitty and sixel support for img, gif & vid preview #1749

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

abhinav3398
Copy link
Contributor

adds sixel image, video and gif preview support when tmux is buit with ./configure --enable-sixel
requirements for preview apart from sixel supported tmux:

  • mpv
  • img2preview
    2023-10-15T00:31:13,007901883-06:00
    2023-10-15T00:31:44,573211029-06:00

@luukvbaal
Copy link
Collaborator

luukvbaal commented Oct 15, 2023

Please check the shellcheck errors and style/documentation format.

Also, which terminals does this target? I guess sixel terminals that do not themselves support multiplexing? Or I guess one might prefer tmux as multiplexer even in those that do (like wezterm)?

@abhinav3398
Copy link
Contributor Author

abhinav3398 commented Oct 15, 2023

yes you are right. This is targeted towards foot, putty, wezterm, alacritty, and other terminals wherein which the user wants to use the same terminal with tmux as multiplexer(rather than using terminal's built-in multiplexing capabilities) for nnn media previewer.

@abhinav3398 abhinav3398 changed the title + tmux sixel support for img, gif & vid preview preview-tui: tmux sixel support for img, gif & vid preview Oct 17, 2023
@jarun
Copy link
Owner

jarun commented Oct 18, 2023

@luukvbaal or @N-R-K please merge when this is ready.

@luukvbaal
Copy link
Collaborator

Sure, I haven't had the chance to test this yet but it looks better now.

plugins/preview-tui Outdated Show resolved Hide resolved
plugins/preview-tui Outdated Show resolved Hide resolved
@jarun
Copy link
Owner

jarun commented Oct 31, 2023

@N-R-K please merge when ready.

Copy link
Collaborator

@N-R-K N-R-K left a comment

Choose a reason for hiding this comment

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

The code looks okay, but I don't have my mpv built with sixel support so I didn't test.

plugins/preview-tui Outdated Show resolved Hide resolved
plugins/preview-tui Outdated Show resolved Hide resolved
@N-R-K N-R-K linked an issue Oct 31, 2023 that may be closed by this pull request
@N-R-K N-R-K requested a review from luukvbaal November 11, 2023 10:33
@luukvbaal
Copy link
Collaborator

luukvbaal commented Nov 11, 2023

Sorry for the delay, finally got around to looking into this. Looks alright but I think some more work is needed to implement this properly.

We need to make sure we don't try to use the sixel backend on terminals that do not support it. I actually haven't gotten the mpv sixel backend to work yet on my machine. It prints the raw sixel data when running tmux with sixel support, in a terminal that doesn't support sixel.
In terminals that supposedly do support sixel, I'm getting [vo] Video output sixel not found! when using the mpv sixel backend, even though img2sixel works fine.

#1590 also mentions the kitty backend, I tried adding support for that and that seems to work fine. I'll try to add a commit and merge this PR this weekend if I can get sixel to work...

@luukvbaal
Copy link
Collaborator

OK mpv in the arch repo is not built with the sixel backend, installing mpv-full did the trick. What terminal are you using this on @abhinav3398?

I'm currently testing on WezTerm which supports both the sixel and kitty backends. Kitty backend works all right but the sixel backend is horribly slow. Is performance alright for you?

@luukvbaal luukvbaal force-pushed the master branch 3 times, most recently from bca9f9e to 5384a6f Compare November 11, 2023 17:28
@luukvbaal
Copy link
Collaborator

luukvbaal commented Nov 11, 2023

Added a commit to also support the mpv kitty backend. Also try to detect sixel support and better honor the NNN_PREVIEWIMGPROG and newly added NNN_PREVIEWVIDEO variables.

I'm fine to merge this now, but I'll test a bit more and wait for reviews one more time.

@abhinav3398
Copy link
Contributor Author

installing mpv-full did the trick.

That's exactly what I used.

What terminal are you using this on @abhinav3398

I'm currently testing on WezTerm which supports both the sixel and kitty backends. Kitty backend works all right but the sixel backend is horribly slow. Is performance alright for you?

I'm using foot. works perfectly fine for me. Mpv used to display videos fine about 2 weeks ago. It just recently started lagging for me. But the lag is fine for prewing purposes. Haven't tried on other terminals yet but I'll try it out on other terminals and try to add a check for sixel support for the terminal after the weekend.

Thanks for the help @luukvbaal as I'm busy during the weekend.

@luukvbaal
Copy link
Collaborator

luukvbaal commented Nov 11, 2023

It just recently started lagging for me. But the lag is fine for prewing purposes.

Yeah I suppose so. I doubt there is much we can do about performance anyways besides hoping it gets improved upstream ^^ Thanks for kicking off the mpv support, it's cool :)

plugins/preview-tui Outdated Show resolved Hide resolved
@N-R-K
Copy link
Collaborator

N-R-K commented Nov 12, 2023

I doubt there is much we can do about performance anyways besides hoping it gets improved upstream

Doesn't seem like there's any performance issues on sixel output being reported upstream (that's probably because no one uses it). Pinging @mia-0.

@mia-0
Copy link

mia-0 commented Nov 12, 2023

Performance is mostly bottlenecked by libsixel (the quantization step in particular; dithering options may affect it) and the terminal emulator. So there’s not much we can do about it on mpv’s side without replacing libsixel I’m afraid.

@N-R-K
Copy link
Collaborator

N-R-K commented Nov 15, 2023

I'm fine to merge this now, but I'll test a bit more and wait for reviews one more time.

Assuming this is good to go then?

@luukvbaal luukvbaal changed the title preview-tui: tmux sixel support for img, gif & vid preview preview-tui: mpv kitty and sixel support for img, gif & vid preview Nov 16, 2023
@luukvbaal luukvbaal merged commit 4009e21 into jarun:master Nov 16, 2023
6 checks passed
@jarun
Copy link
Owner

jarun commented Nov 17, 2023

Thanks guys!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mpv sixel/kitty support for preview
5 participants