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

allow quality argument to be passed to save #29

Merged
merged 2 commits into from
Mar 31, 2024
Merged

Conversation

jamarju
Copy link
Contributor

@jamarju jamarju commented Mar 29, 2024

No description provided.

@Isotr0py Isotr0py self-requested a review March 30, 2024 05:12
@Isotr0py
Copy link
Owner

Isotr0py commented Mar 30, 2024

Thanks for your contribution!

Note that the quality used in jpegxl-rs is the FrameDistance in libjxl in fact, which ranges from 0 .. 25 and lower = higher quality.

I'm afraid that if we just expose quality simply, this may cause a confusing situation. Because a JPEG-style quality factor usually ranges up to 100 and higher = higher quality.

For example, if user mistakes the quality here is the JPEG-style one, and then:

with Image.open("1.png") as im:
    im.save("1.jxl", quality=98)

This will cause a confusing panic in Rust binding:

PanicException: called `Result::unwrap()` on an `Err` value: ApiUsage

pillow_jxl/JpegXLImagePlugin.py Outdated Show resolved Hide resolved
@Isotr0py Isotr0py self-assigned this Mar 30, 2024
@jamarju
Copy link
Contributor Author

jamarju commented Mar 30, 2024

I added a mapping according to aforementioned function, take a look and let me know what you think. Might be even cleaner to call the native function from the rust side of the code?

@Isotr0py
Copy link
Owner

LGTM.

I will update the forked jpegxl-rs with JxlEncoderDistanceFromQuality and submit a PR to jpegxl-rs main repo for further refinement in the near future.

@Isotr0py Isotr0py merged commit 401be45 into Isotr0py:main Mar 31, 2024
4 of 5 checks passed
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