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

JXL-Oxide support #2112

Closed
2 of 3 tasks
Quackdoc opened this issue Dec 28, 2024 · 5 comments
Closed
2 of 3 tasks

JXL-Oxide support #2112

Quackdoc opened this issue Dec 28, 2024 · 5 comments
Labels
feature New feature request

Comments

@Quackdoc
Copy link

yazi --debug output

yazi --debug

Yazi
    Version: 0.3.3 (termux 2024-09-05)
    Debug  : false
    OS     : android-aarch64 (unix)

Ya
    Version: 0.3.3 (termux 2024-09-05)

Emulator
    Emulator.via_env: ("xterm-256color", "")
    Emulator.via_csi: Ok(Unknown([]))
    Emulator.detect : Unknown([])

Adapter
    Adapter.matches: X11

Desktop
    XDG_SESSION_TYPE           : None
    WAYLAND_DISPLAY            : None
    DISPLAY                    : Some("localhost:10.0")
    SWAYSOCK                   : None
    HYPRLAND_INSTANCE_SIGNATURE: None
    WAYFIRE_SOCKET             : None

SSH
    shared.in_ssh_connection: true

WSL
    WSL: false

Variables
    SHELL              : Some("/data/data/com.termux/files/usr/bin/zsh")
    EDITOR             : None
    VISUAL             : None
    YAZI_FILE_ONE      : None
    YAZI_CONFIG_HOME   : None

Text Opener
    default: Some(Opener { run: "${EDITOR:-vi} \"$@\"", block: true, orphan: false, desc: "$EDITOR", for_: None, spread: true })
    block  : Some(Opener { run: "${EDITOR:-vi} \"$@\"", block: true, orphan: false, desc: "$EDITOR", for_: None, spread: true })

Multiplexers
    TMUX               : false
    tmux version       : No such file or directory (os error 2)
    ZELLIJ_SESSION_NAME: None
    Zellij version     : No such file or directory (os error 2)

Dependencies
    file             : 5.45
    ueberzugpp       : No such file or directory (os error 2)
    ffmpegthumbnailer: No such file or directory (os error 2)
    magick           : 7.1.1-39
    fzf              : No such file or directory (os error 2)
    fd               : No such file or directory (os error 2)
    rg               : No such file or directory (os error 2)
    chafa            : 1.14.5
    zoxide           : No such file or directory (os error 2)
    7z               : No such file or directory (os error 2)
    7zz              : No such file or directory (os error 2)
    jq               : No such file or directory (os error 2)


--------------------------------------------------
When reporting a bug, please also upload the `yazi.log` log file - only upload the most recent content by time.
You can find it in the "/data/data/com.termux/files/home/.local/state/yazi" directory.

Please describe the problem you're trying to solve

Imagemagick requires a lot of resources to decode jxl images. while jxl-oxide itself is less efficient then libjxl is, magick actually winds up adding a decent amount of overhead. This can cause low end systems to crash when trying to render jxl images.

JXL-oxide may also increase the speed due to not needing to go through magick

Would you be willing to contribute this feature?

  • Yes, I'll give it a shot

Describe the solution you'd like

jxl-oxide support. jxl-oxide has image-rs integration making it fairly easy to use if you are using dynamicimage

Additional context

While I can't implement it myself now due to pains in my wrists when typing, implementing jxl-oxide can be fairly easy. for prior work you can see what I did here https://github.com/forbjok/blazebooru/pull/5/files blazebooru is apache 2.0 or MIT licenced so just copying the code as is if compatible would be fine.

I'm not sure how well this would translate to yazi as I haven't really looked at the code, and I know as is, something like this isn't ideal as it doesn't leave tons of room for expansion in the future. But it would be really nice to have jxl-oxide support over

Checklist

  • I have searched the existing issues/discussions
  • The latest nightly build doesn't already have this feature
@Quackdoc Quackdoc added the feature New feature request label Dec 28, 2024
@sxyazi
Copy link
Owner

sxyazi commented Dec 29, 2024

Fallback to jxl-oxide when image decoding fails is hacky because not all failed decodes are necessarily in the JXL format. This could introduce unnecessary performance overhead for failures.

Yazi uses image to handle all images in a unified way, so it's better to wait for upstream support for JXL directly image-rs/image#1765

Also, it would be helpful if some benchmark data could be provided. For example, does JXL-Oxide really outperform ImageMagick, and by how much? What about memory and CPU usage? This data will help assess whether it's worth enabling this as a feature once upstream supports it, to switch from ImageMagick to JXL-Oxide.

When doing the benchmarks, it's best to run both ImageMagick and JXL-Oxide on a single thread, because once Yazi integrates JXL-Oxide, it will disable its multi-threaded decoding. Yazi already handles multi-threaded concurrency at the outer task scheduling level.

@Quackdoc
Copy link
Author

Quackdoc commented Dec 30, 2024

jxl-oxide is unlikely to be supported in upstream due to image-rs decision on how to handle formats with paywalled specifications. Though as per image-rs/image#2363 and image-rs/image#2367 it's possible that ImageReader may gain support for detecting jxl-oxide in the future which would lead to a trivial cleanup. Or some other future change, but nothing has been merged as of yet that will help clean this up that I know of.

I don't think you will have much overhead on dealing with failed images since by the looks of it you are already trying magick for specific formats first, and then image-rs. This would mean that the chain would be magick > image-rs > jxl-oxide. jxl-oxide will fail at parsing the image pretty fast so there shouldn't be much overhead.

I don't have any hard benchmarks but this hack is quite literally the difference between decoding JXL and outright crashing termux due to OOM. I'm not really sure how to benchmark this properly as using valgrind didn't really work. However the results at least on android, my 4G LG G7 thinq running LineageOS is massive. when using jxl-oxide yazi is usable, when using magisk, termux crashes due to oom.

I made a quick and dirty proof on concept just using some dirty code to showcase using jxl-oxide. The following patch will fail on image-rs' image detection as it throws an unsupported format error. It's not pretty but it is usable for testing. Larger images will still pose some issues, and the large you go, likely the better magick will be, but for most images I have, this is a large improvement.

If jxl-oxide does get added to yazi, Leaving it as opt-in via toml should be good for both worlds.

diff --git a/yazi-adapter/Cargo.toml b/yazi-adapter/Cargo.toml
index 6c9106b6..09db412b 100644
--- a/yazi-adapter/Cargo.toml
+++ b/yazi-adapter/Cargo.toml
@@ -25,6 +25,7 @@ ratatui     = { workspace = true }
 scopeguard  = { workspace = true }
 tokio       = { workspace = true }
 tracing     = { workspace = true }
+jxl-oxide = { version = "0.11.0", features = ["image"] }
 
 [target.'cfg(target_os = "macos")'.dependencies]
 crossterm = { workspace = true, features = [ "use-dev-tty", "libc" ] }
diff --git a/yazi-adapter/src/image.rs b/yazi-adapter/src/image.rs
index a2ba06e4..663a284a 100644
--- a/yazi-adapter/src/image.rs
+++ b/yazi-adapter/src/image.rs
@@ -3,6 +3,7 @@ use std::path::{Path, PathBuf};
 use anyhow::Result;
 use image::{DynamicImage, ExtendedColorType, ImageDecoder, ImageEncoder, ImageError, ImageReader, ImageResult, Limits, codecs::{jpeg::JpegEncoder, png::PngEncoder}, imageops::FilterType, metadata::Orientation};
 use ratatui::layout::Rect;
+use jxl_oxide::integration::JxlDecoder;
 use yazi_config::{PREVIEW, TASKS};
 
 use crate::Dimension;
@@ -110,10 +111,17 @@ impl Image {
 
 		let path = path.to_owned();
 		tokio::task::spawn_blocking(move || {
-			let mut reader = ImageReader::open(path)?;
+			let mut reader = ImageReader::open(&path).unwrap();
 			reader.limits(limits);
 
-			let mut decoder = reader.with_guessed_format()?.into_decoder()?;
+			let mut decoder: Box<dyn ImageDecoder> = match reader.with_guessed_format()?.into_decoder() {
+				Ok(decoder) => Box::new(decoder),
+				Err(_) => {
+					let decoder = JxlDecoder::new(std::fs::File::open(path)?)?;
+					Box::new(decoder)
+               		//image::DynamicImage::from_decoder(decoder)?
+				}
+			};
 			let orientation = decoder.orientation().unwrap_or(Orientation::NoTransforms);
 			let icc = decoder.icc_profile().unwrap_or_default();

EDIT: the reader change was to simplfy showing me what it reported as since I forgot.

@sxyazi
Copy link
Owner

sxyazi commented Dec 30, 2024

I don't think you will have much overhead on dealing with failed images since by the looks of it you are already trying magick for specific formats first, and then image-rs

Right now, Yazi uses the image/* rule for built-in image decoding:

# Image
{ mime = "image/{avif,hei?,jxl,svg+xml}", run = "magick" },
{ mime = "image/*", run = "image" },

This means that, aside from a few formats like avif, hei?, jxl, svg+xml which are handled by Magick, all other files recognized by file(1) as image/ will be attempted to be decoded by image-rs. Among these, there are many formats that image-rs doesn't support, which leads to decoding failures.

The reason for using image/* instead of specific formats is because file(1) doesn't always correctly identify image formats, and there are differences in the mimetype returned by different versions of file(1), while image-rs is better at recognizing them.

If we try to decode with image-rs and fail, then try decoding with jxl-oxide, it would add extra overhead from opening files and attempting decoding, which I'd rather avoid. I really don't think "error chains" are the solution to anything.

I don't have any hard benchmarks but this hack is quite literally the difference between decoding JXL and outright crashing termux due to OOM

There are many reasons why ImageMagick could cause OOM. Before simply replacing it, did you try to figure out the specific cause?

  • Do you experience OOM when using ImageMagick alone, or only when it's being used in Yazi? Yazi runs multiple instances of ImageMagick concurrently, which could be causing OOM. Did you try running just one at a time?
  • Does reducing Yazi's concurrency help? I recommend lowering it on low-spec devices.
  • Does limiting ImageMagick to a single thread help? You can do that with export MAGICK_THREAD_LIMIT=1. Yazi doesn't currently limit the number of threads for ImageMagick.
  • Does this happen with specific JXL files, or with all of them? Do other formats besides JXL also cause OOM?

@Quackdoc
Copy link
Author

Setting both Yazi's concurrency to one, as well as imagemagick's threads to one, does help prevent OOM, however on JXL images, it becomes extremely brutally slow.

I do agree that fail-chaining is not great. It would be best to do a match depending on the imagereaders result. Of course, the ideal solution is to wait for when upstream image supports some kind of method for querying formats that are not supported. But like I said earlier, it is planned. Just don't know how long it's going to take.

A short-term solution would be to match for unsupported file format and then parse the file itself. This isn't great for all image formats, however JXL only has two different headers an image can be, and sniffing via this is very reliable. As well as flexible enough to support other formats in the future.

The reason I didn't do this is simply because typing hurts. I use voice to text for most things like what I'm writing right now. and this I only needed for testing.

This would also be relatively easy to swap out when image does support parsing formats even when It does not currently support the format itself.

It happens with all sorts of JXL images. I do have a number of fairly decently sized ones that are rather difficult to decode. I have only tested JXL, PNG, and JPEG, since that's all the devices I actually have on my phone.

@sxyazi
Copy link
Owner

sxyazi commented Dec 31, 2024

I still haven't seen any specific numbers. You said that after limiting ImageMagick to a single thread, it's "extremely brutally slow", but how slow exactly? Are you comparing it to JXL-Oxide? Is it several times slower than JXL-Oxide? Does JXL-Oxide also use single-threaded decoding?

In this case, I'll close this issue for now, and wait for image-rs to support JXL-Oxide directly (I've subscribed to the upstream issue). Once it's supported, I'll test the performance difference between it and ImageMagick myself.

In the meantime, you can use your patched Yazi if it's already working well for you. Or, you can replace ImageMagick with any other decoder you think has better performance by customizing the previewer. Thanks!

@sxyazi sxyazi closed this as not planned Won't fix, can't repro, duplicate, stale Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

No branches or pull requests

2 participants