feat: add JPEG XL (JXL) encoding/decoding support#8
Conversation
Clean-room libjxl (BSD-3-Clause) binding approach for JXL encoding support, independent of GPL-licensed jpegxl-sys/jpegxl-rs.
9 tasks covering libjxl submodule setup, sys crate build, safe wrapper, Codec trait integration, and CI updates.
- Rename crate from libjxl-enc-sys to libjxl-sys (now provides both encoder and decoder bindings) - Add libjxl decoder bindings (JxlDecoder*) to build.rs bindgen - Create decoder.rs safe wrapper using libjxl event-based decode API - Rewrite JxlCodec to use libjxl for both encode and decode (replacing image crate's limited JXL decode support) - Add roundtrip tests (lossy + lossless) that verify encode→decode - Update pipeline test to expect JXL encoding success - Update format.rs can_encode to return true for all formats
- Add cmake to dist-workspace.toml system dependencies (homebrew, apt, chocolatey) for cargo-dist release builds - Add submodules: recursive to kotlin/python bindings workflows - Add cmake to system dependencies install steps in both workflows - Add crates/libjxl-sys/** to path triggers for bindings workflows
JXL encoding is now supported, so add it to FormatArg enum to allow --format jxl in convert, resize, crop, extend commands.
- Rename crate from `libjxl-sys` to `slimg-libjxl-sys` (existing name taken on crates.io) - Remove `publish = false`, add repository/homepage metadata - Use `package = "slimg-libjxl-sys"` alias in slimg-core to keep `libjxl_sys` import name - Add slimg-libjxl-sys publish step to publish.yml (before slimg-core) Note: crates.io 10MB limit prevents vendoring libjxl source directly. Prebuilt binary download approach (build.rs) is planned as next step.
Solve the crates.io 10MB package limit by downloading prebuilt static libraries from GitHub Releases instead of including the ~95MB libjxl submodule source. Build paths (in priority order): 1. LIBJXL_SYS_DIR env var — user-provided prebuilt directory 2. vendored feature — cmake source build (if submodule present) 3. prebuilt download — curl from GitHub Releases (crates.io default) Also add build-libjxl-prebuilt.yml CI workflow for building prebuilt archives across 5 platforms, and update publish.yml to use --no-default-features for the slimg-libjxl-sys crate. Remove separate skcms_transform compilation as the transform code is now integrated into skcms.cc (included in jxl_cms).
Summary of ChangesHello @clroot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the image processing capabilities by introducing full support for the JPEG XL format. It involves creating a new FFI crate to interface with the Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces JPEG XL (JXL) encoding and decoding capabilities, a significant feature addition. The implementation utilizes a new slimg-libjxl-sys crate for handling vendored source builds and pre-built binary downloads, with robust safe Rust wrappers for the libjxl FFI. However, several security concerns have been identified: the build script lacks checksum verification for downloaded binaries, posing a supply chain risk; the decoder is vulnerable to memory exhaustion DoS attacks from malicious image headers; and the encoder relies on debug-only assertions for critical buffer size validation. Additionally, there are suggestions to enhance the build script's robustness and a note regarding a potentially temporary documentation file. Addressing these points will further improve the security and overall quality of this excellent contribution.
| let status = Command::new("curl") | ||
| .args([ | ||
| "--proto", | ||
| "=https", | ||
| "--tlsv1.2", | ||
| "-L", | ||
| "--fail", | ||
| "--silent", | ||
| "--show-error", | ||
| "-o", | ||
| ]) | ||
| .arg(&archive_path) | ||
| .arg(&url) | ||
| .status() | ||
| .unwrap_or_else(|e| { | ||
| panic!( | ||
| "slimg-libjxl-sys: failed to run curl: {e}\n\ | ||
| Hint: install curl, or set LIBJXL_SYS_DIR, or enable the vendored feature" | ||
| ) | ||
| }); | ||
|
|
||
| assert!( | ||
| status.success(), | ||
| "slimg-libjxl-sys: failed to download prebuilt (HTTP error).\n\ | ||
| URL: {url}\n\ | ||
| Hint: check network connectivity, or set LIBJXL_SYS_DIR, or enable the vendored feature" | ||
| ); | ||
|
|
There was a problem hiding this comment.
The build script downloads prebuilt binaries from GitHub Releases but does not verify their integrity using a checksum (e.g., SHA-256). This exposes the build process to supply chain attacks if the download source or the network connection is compromised. Malicious binaries could be injected and executed during the build or at runtime.
| return Err(Error::Decode("failed to get output buffer size".into())); | ||
| } | ||
|
|
||
| pixels.resize(buf_size, 0); |
There was a problem hiding this comment.
The decoder allocates memory for the output buffer based on the dimensions reported in the JXL header (info.xsize and info.ysize) without any upper bound validation. A malicious JXL file claiming extremely large dimensions could trigger a massive allocation via pixels.resize(buf_size, 0), leading to a Denial of Service (DoS) through memory exhaustion (OOM).
| }; | ||
|
|
||
| let expected = (width as usize) * (height as usize) * 4; | ||
| debug_assert_eq!(pixels.len(), expected); |
There was a problem hiding this comment.
The encoder uses debug_assert_eq! to validate that the input pixel buffer size matches the expected dimensions. Since debug_assert! is removed in release builds, this check is effectively bypassed in production. If the provided buffer is smaller than the dimensions specified to the encoder, it could lead to a buffer over-read during the FFI call to libjxl. This should be replaced with a robust check that returns an error.
let expected = (width as usize) * (height as usize) * 4;
if pixels.len() != expected {
return Err(Error::Encode(format!("invalid pixel buffer size: expected {}, got {}", expected, pixels.len())));
}| .header(src_include.join("jxl/encode.h").to_str().unwrap()) | ||
| .header(src_include.join("jxl/decode.h").to_str().unwrap()) | ||
| .header(src_include.join("jxl/types.h").to_str().unwrap()) | ||
| .header( | ||
| src_include | ||
| .join("jxl/codestream_header.h") | ||
| .to_str() | ||
| .unwrap(), | ||
| ) | ||
| .header(src_include.join("jxl/color_encoding.h").to_str().unwrap()) |
There was a problem hiding this comment.
Using .unwrap() on the result of .to_str() can lead to a panic with a generic message if a path contains non-UTF-8 characters. While unlikely for these project-internal paths, it's more robust to use .expect() with a specific message to aid debugging in case of cross-platform build issues.
| .status() | ||
| .expect("slimg-libjxl-sys: failed to run tar"); |
There was a problem hiding this comment.
The error handling for the tar command is less descriptive than for curl. If tar is not found, the build will panic with a generic message. It would be better to provide a more helpful hint to the user, consistent with how a missing curl is handled.
| .status() | |
| .expect("slimg-libjxl-sys: failed to run tar"); | |
| .status() | |
| .unwrap_or_else(|e| { | |
| panic!( | |
| "slimg-libjxl-sys: failed to run tar: {e}\n\ | |
| Hint: install tar, or set LIBJXL_SYS_DIR, or enable the vendored feature" | |
| ) | |
| }) |
| @@ -0,0 +1,792 @@ | |||
| # JPEG XL Encoding Implementation Plan | |||
There was a problem hiding this comment.
This file appears to be an AI-generated implementation plan. It's written in Korean, which is inconsistent with the English used elsewhere in the project, and contains AI-specific directives (e.g., "For Claude: ..."). Please consider if this file is intended as permanent project documentation. If it was a temporary planning aid, it might be better to remove it before merging to keep the repository clean and consistent.
1 participant
Summary
slimg-libjxl-syscrate with prebuilt binary download support for crates.io publishingslimg-coreKey architecture decisions
default = ["vendored"]builds from source when the submodule is present (local dev), falls back to prebuilt download when source is absent (crates.io consumers)build-libjxl-prebuilt.ymlbuilds static libraries for 5 platforms (linux-x86_64, linux-aarch64, macos-x86_64, macos-aarch64, windows-x86_64)Test plan
cargo build -p slimg-libjxl-sys— vendored build passescargo build -p slimg-core— downstream build passescargo test -p slimg-libjxl-sys -p slimg-core— all tests passcargo package -p slimg-libjxl-sys --no-default-features --no-verify— 29KB packagebuild-libjxl-prebuilt.ymlworkflow to generate prebuilt archives🤖 Generated with Claude Code