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

add jxl support #43

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Quackdoc
Copy link

jxl-oxide now has image interop, we can support it like so

@@ -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"] }
Copy link
Contributor

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.

Comment on lines +156 to +158
let source = if let Ok(image) = image::open(source) {
image
} else if let Ok(decoder) = jxl_oxide::integration::JxlDecoder::new(
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Author

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.

Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants