sxyazi / yazi

💥 Blazing fast terminal file manager written in Rust, based on async I/O.
https://yazi-rs.github.io
MIT License
16.49k stars 384 forks source link

Preview is distorted on some images #577

Closed SolitudeSF closed 9 months ago

SolitudeSF commented 9 months ago

What system are you running Yazi on?

Linux X11

What terminal are you running Yazi in?

kitty v0.32.0

Yazi version

yazi 0.2.2 (ba0c860 2024-01-25)

Some images seem to be scaled improperly, because they have jarring horizontal lines that look like resizing artifacts. image

To Reproduce

Source image F7Rb51YbkAALpOB

reproduces consistently on my machine, but it should be dependant on monitor resolution, terminal size and cell size, i guess.

Configuration

quality setting is irrelevant and it doesnt look like a compression artifact.

sxyazi commented 9 months ago

I think it might be related to the filter used during downscale.

https://github.com/sxyazi/yazi/blob/ba0c860b2e4098318fb1a07f67ac81d7c08f230d/yazi-adaptor/src/image.rs#L29

Yazi currently uses the Triangle filter: Triangle

This is switching to the Lanczos3 filter: Lanczos3

It seems like Lanczos3 better preserves the texture of the background, but I'm not sure, my eyes have stopped working. 😅 Perhaps we can make the filter a configurable option?

cc @bvr-yr, do you think it's related to the issue you mentioned before - is it possible that the difference in image quality is due to different filters rather than Sixel itself?

bvr-yr commented 9 months ago

@sxyazi i'll try it now. btw, just for curiosity and to be sure, i removed validation and tried with

image_quality   = 100
sixel_fraction  = 1

wich gave almost no quality increase but alot CPU load :laughing:

SolitudeSF commented 9 months ago

I have the same artifacts with Lanczos3. And they arent visible on the resulting image itself, only when previewed in yazi. So its about the generated image then getting squished into the preview box.

sxyazi commented 9 months ago

Have you changed both filters at

https://github.com/sxyazi/yazi/blob/ba0c860b2e4098318fb1a07f67ac81d7c08f230d/yazi-adaptor/src/image.rs#L29

and

https://github.com/sxyazi/yazi/blob/ba0c860b2e4098318fb1a07f67ac81d7c08f230d/yazi-adaptor/src/image.rs#L72

and do a yazi --clear-cache?

SolitudeSF commented 9 months ago

shoud've grepped, no, only one.

bvr-yr commented 9 months ago

well i tried them all, changed both lines:

so it seems like not the sixel problem itself, rather it is as whole conversion chain

Nearest gives best result, but it might look too pixelated (keep in mind github converts imges itself)

I think best option is to give freedom (as always :wink:) and just introduce another config option to choose filter, as it might be different results with sixel/kitty/iterm/etc

SolitudeSF commented 9 months ago

Nope, doesnt help. Same artifacts are visible. So my problem doesnt look like filter related and more of how yazi draws/positions it. Just for the reference, in ranger, no matter how i resize the terminal, image stays clean without artifacts.

sxyazi commented 9 months ago

Nope, doesnt help. Same artifacts are visible. So my problem doesnt look like filter related and more of how yazi draws/positions it. Just for the reference, in ranger, no matter how i resize the terminal, image stays clean without artifacts.

Does increasing the pre-cache image size help?

https://github.com/sxyazi/yazi/blob/ba0c860b2e4098318fb1a07f67ac81d7c08f230d/yazi-config/preset/yazi.toml#L17-L18

bvr-yr commented 9 months ago

as offtopic about sixel: ranger does primary caching slow due to calling imagemagick, but it does another caching (as for sixel atleast), for current running ranger instance so that sixel doesn't need to be generated every time image is previewed and as a result it is rendered instantly, whereas yazi regenerates sixel data every time

is this thing hard to implement? if not so, i can create feature request if needed

SolitudeSF commented 9 months ago

Does increasing the pre-cache image size help?

https://github.com/sxyazi/yazi/blob/ba0c860b2e4098318fb1a07f67ac81d7c08f230d/yazi-config/preset/yazi.toml#L17-L18

increased to 2000x2000. no change.

sxyazi commented 9 months ago

as offtopic about sixel: ranger does primary caching slow due to calling imagemagick, but it does another caching (as for sixel atleast), for current running ranger instance so that sixel doesn't need to be generated every time image is previewed and as a result it is rendered instantly, whereas yazi regenerates sixel data every time

is this thing hard to implement? if not so, i can create feature request if needed

How ranger creates new sixel data on existing sixel data when the user terminal size changes? We can create sixel data on images that have already been downscaled, but we cannot create new sixel data on already encoded sixel data. This requires some form of reverse parsing of the sixel format

sxyazi commented 9 months ago

Does increasing the pre-cache image size help? https://github.com/sxyazi/yazi/blob/ba0c860b2e4098318fb1a07f67ac81d7c08f230d/yazi-config/preset/yazi.toml#L17-L18

increased to 2000x2000. no change.

Hmm no more idea. All image processing is done in the image.rs file, and then it's just base64 encoded according to the Kitty documentation. I don't think the encoding would be any different for ranger.

sxyazi commented 9 months ago

Ah forgot to mention, have you changed the image_quality and sixel_fraction?

https://github.com/sxyazi/yazi/blob/ba0c860b2e4098318fb1a07f67ac81d7c08f230d/yazi-config/preset/yazi.toml#L20-L21

Docs here https://yazi-rs.github.io/docs/configuration/yazi#preview

SolitudeSF commented 9 months ago

yes, it didnt matter, because its not related.

sxyazi commented 9 months ago

Does this problem exist in other terminals like Konsole, WezTerm? Please try using the kitty old protocol to see if the problem still by changing:

https://github.com/sxyazi/yazi/blob/ba0c860b2e4098318fb1a07f67ac81d7c08f230d/yazi-adaptor/src/adaptor.rs#L86

to

            Emulator::Kitty => vec![Self::KittyOld],
SolitudeSF commented 9 months ago

With WezTerm or with KittyOld preview is correct.

sxyazi commented 9 months ago

Oh I see now. Ranger and Yazi use different kitty graphics protocols.

Ranger uses the classic protocol, which does not support tmux and lacks support for partial erasure (resulting in overlapping popup components and images).

And Yazi uses the new Unicode placeholders. The old protocol is currently only in use by a few old terminals and is expected to be deprecated in the future. So I guess this might be a bug related to kitty Unicode placeholders, and I'm not sure what Yazi can do about it.

SolitudeSF commented 9 months ago

Can this be isolated so it can be reported upstream? And can a config option be provided to override whatever graphics protocol is yazi selecting, so i dont have to run a patched version?

sxyazi commented 9 months ago

Can this be isolated so it can be reported upstream?

I think you can reproduce it using the built-in icat command in Kitty:

kitten icat --unicode-placeholder image.jpg

And can a config option be provided to override whatever graphics protocol is yazi selecting, so i dont have to run a patched version?

You can trick Yazi into using the OldKitty protocol through TERM_PROGRAM to start:

TERM_PROGRAM=ghostty yazi
SolitudeSF commented 9 months ago

thanks. reported upstream.

SolitudeSF commented 9 months ago

https://github.com/kovidgoyal/kitty/issues/7070#issuecomment-1913120358

i think that yazi shouldnt target unicode placeholders by default with kitty. or at least give proper config option for that, if the user knows that he doesnt need it, since it should be less efficient than regular image display.

sxyazi commented 9 months ago

No, that's not feasible.

Yazi needs to partially erase the overlapping parts of the pop-up component with the image, as I mentioned before, regular image display does not have this capability. See:

Unicode placeholders: Correctly

placeholders

Classic protocol: The pop-up component is being covered by the image

regular

Classic protocol with z=-1: The image penetrates the pop-up component, becoming the background, incorrectly

regular-with-z

github-actions[bot] commented 8 months ago

I'm going to lock this issue because it has been closed for 30 days. ⏳ This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.