Skip to content

possible regression in JXL save from ICC profile? #2872

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.

Sign up for GitHub

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

Jump to bottom
Closed
richardmonette opened this issue Jun 17, 2022 · 6 comments
Closed

possible regression in JXL save from ICC profile? #2872

richardmonette opened this issue Jun 17, 2022 · 6 comments

Comments

@richardmonette
Copy link

@jcupitt @f1ac is it possible #2815 caused a regression in terms of the compressed filesize?

> vips jxlsave photo-1595841696677-6489ff3f8cd1.jpeg photo-1595841696677-6489ff3f8cd1_direct.jxl
> vips -v
vips-8.12.2-Tue Jan 25 09:34:32 UTC 2022
> ll photo-1595841696677-6489ff3f8cd1_direct.jxl
-rw-r--r--  1 richardmonette  staff  337549 17 Jun 12:49 photo-1595841696677-6489ff3f8cd1_direct.jxl

when I run against 8.12.2 it appears saving a JXL is about 338kb whereas when I build and run against 6260a37 (from https://github.com/libvips/libvips/archive/6260a37136973f3d231d0e2c28af6190126c664e.tar.gz) the filesize jumps to about 931kb.
@richardmonette
Copy link
Author

Here is an example with the original jpeg, and then the 😄 JXL from 8.12 and the 😢 JXL from the SHA above: jxl_icc_regression_example.zip

@f1ac
Copy link

f1ac commented Jun 17, 2022 •
edited
Loading

Sounds like uses_original_profile = true causes this. This setting forces libjxl to not use XYB colourspace, which is necessary for lossless encoding, but is less effective. IMHO this should be set conditionally, out->uses_original_profile = in->lossless.

@f1ac
Copy link

f1ac commented Jun 17, 2022

BTW uses_original_profile does not seem to have anything to do with ICC profiles, just a confusing name for lossy XYB colourspace toggle.

jcupitt added a commit that referenced this issue Jun 18, 2022
@jcupitt
try to improve jxl lossless, again
156e6a9 
now seems to handle scrgb and srgb, lossless and lossy, with and without
an ICC profile

see #2872
jcupitt added a commit that referenced this issue Jun 18, 2022
@jcupitt
add jxlsave tests
c61b458 
and revise colour encoding set

see #2872
@jcupitt
Copy link
Member

jcupitt commented Jun 18, 2022

.... I added some tests too. It checks lossy and lossless mode, with and without an ICC profile, plus 8 bit, 16 bit, and float. It also checks lossy is much smaller than lossless.

Any more testing very welcome!

@jcupitt
Copy link
Member

jcupitt commented Jun 18, 2022

@richardmonette I tried your test image after these changes:

$ vips copy photo-1595841696677-6489ff3f8cd1.jpeg x.jxl
$ ls -l photo-1595841696677-6489ff3f8cd1.jpeg x.jxl
-rw-r--r-- 1 john john 1765625 Jun 16 14:46 photo-1595841696677-6489ff3f8cd1.jpeg
-rw-rw-r-- 1 john john  337774 Jun 18 16:42 x.jxl

Thanks for taking the time to try this and make the report.

@jcupitt
Copy link
Member

jcupitt commented Jun 19, 2022

... this fix is in 8.13-rc1, please open a new issue if it's still broken.

https://github.com/libvips/libvips/releases/tag/v8.13.0-rc1

@jcupitt jcupitt closed this as completed Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jcupitt @richardmonette @f1ac