-
Notifications
You must be signed in to change notification settings - Fork 10
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
add jxl support #43
base: main
Are you sure you want to change the base?
add jxl support #43
Conversation
@@ -19,6 +19,7 @@ av-metrics-decoders = { version = "0.3.1", features = [ | |||
clap = { version = "4.0.18", features = ["derive"] } | |||
crossterm = "0.27.0" | |||
indicatif = "0.17.1" | |||
jxl-oxide = { version = "0.10.1", features = ["image"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This increases binary size from 3.4M to 6.0M on my machine.. Should probably be a feature.
let source = if let Ok(image) = image::open(source) { | ||
image | ||
} else if let Ok(decoder) = jxl_oxide::integration::JxlDecoder::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way of doing this without opening the file twice? Maybe check if image
supports the file format (via extension) first, and then either open one of the built-in decoders or the external JXL one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing file extension isn't the most reliable as some users like to mess with file extensions, some apps might use their own file extensions etc.
what about doing bufread instead? There is a better way of handling bufread instead of using fill_buf, limiting the amount of bytes needed for the buffer for image probing will be faster then this, but the current hit is not bad, and I can't think of the best way to do that ATM.
let srcfile = std::fs::File::open(source)
.ok().expect("could not open source file");
let mut srcbuf = BufReader::new(srcfile);
let source: DynamicImage = match image::guess_format(&mut srcbuf.fill_buf().unwrap()) {
Ok(image) => {
image::load(srcbuf, image).unwrap()
}
Err(decode_error) => {
println!("{}",decode_error);
println!("trying jxl-oxide");
match jxl_oxide::integration::JxlDecoder::new(srcbuf) {
Ok(decoder) => match image::DynamicImage::from_decoder(decoder) {
Ok(img) => img,
Err(e) => panic!("failed to decode source jxl: {}", e),
},
Err(e) => panic!("Failed to open the source file: {}", e),
}
}
};
the difference is
AFTER:
time ssimulacra2_rs image Pahtra3-Edited.png Pahtra3-Edited.png
Score: 100.00000000
ssimulacra2_rs image Pahtra3-Edited.png Pahtra3-Edited.png 2.07s user 0.48s system 99% cpu 2.573 total
avg shared (code): 0 KB
avg unshared (data/stack): 0 KB
total (sum): 0 KB
max memory: 624 MB
BEFORE:
ssimulacra2_rs image Pahtra3-Edited.png Pahtra3-Edited.png
Score: 100.00000000
~/code/ssimu2/ssimulacra2_bin/target/release/ssimulacra2_rs image 2.05s user 0.49s system 99% cpu 2.562 total
avg shared (code): 0 KB
avg unshared (data/stack): 0 KB
total (sum): 0 KB
max memory: 623 MB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I'll keep the match statement since adding more codecs in the future like this won't be great. I still need to think on the best way to keep this sleek while keeping jxl-oxide optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to go by file content, I don't think there's a simple way to avoid re-opening the file anyways, so that is probably fine. Keeping it sleek isn't that important as long as it works well.
BTW: Is there a reason for doing File::open(path).ok().expect("...")
? The .ok()
is unnecessary, right?
jxl-oxide now has image interop, we can support it like so