-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add JPEG XL Open/Read support via libjxl #7848
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
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
May you please commit the images as LFS? |
Do you mean those |
They already setup lfs and they only use for other larger stuff. Let it stay as you did. |
Removed jxl feature
for more information, see https://pre-commit.ci
Mac OS builds were failing because clang complained about goto labels being declared before variables in scope. |
src/PIL/__init__.py
Outdated
@@ -47,6 +47,7 @@ | |||
"IptcImagePlugin", | |||
"JpegImagePlugin", | |||
"Jpeg2KImagePlugin", | |||
"JxlImagePlugin", |
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.
Naming things: like other plugins, shall we use the name of the format rather than the extension?
"JxlImagePlugin", | |
"JpegXlImagePlugin", |
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.
That's a good point. Not sure why I decided to go with "jxl" from the beginning.
Would you want me to change all the references to jxl (including tests, function/class names, feature.jxl) or only plugin and .c
extension filename?
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 think let's just use jxl for the filename extension (so the Tests/images/jxl/
) dir is fine too) and of course the MIME type, and change the rest.
Tests/test_jxl_leaks.py
Outdated
# TODO: lower the limit, I'm not sure what is correct limit | ||
# since I have libjxl debug system-wide | ||
mem_limit = 16 * 1024 # kb |
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.
How can we find out the correct limit?
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 tested it on release version of libjxl and I've set it to 6 megabytes which seem to work well, that's also similar to memory usage of djxl
for decoding hopper.jxl
.
I think libjxl memory usage could depend also on number of decoding threads used. By default we use all available cores.
src/PIL/JxlImagePlugin.py
Outdated
# jpeg xl does some weird shenanigans when storing exif | ||
# it omits first 6 bytes of tiff header but adds 4 byte offset instead | ||
if len(exif) <= 4: | ||
return None |
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.
Can we add a test case for this and _getexit
?
src/PIL/JxlImagePlugin.py
Outdated
|
||
def seek(self, frame): | ||
if not self._seek_check(frame): | ||
return |
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.
And a test for this?
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.
There's already test_file_jxl_animated.py
test which also does seeking.
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.
The return
isn't covered: https://app.codecov.io/gh/python-pillow/Pillow/pull/7848/blob/src/PIL/JxlImagePlugin.py
I've merged in And this is failing to build for me with:
Full log: log2.txt |
Thanks for reporting. The |
src/PIL/JxlImagePlugin.py
Outdated
if is_jxl and not SUPPORTED: | ||
return "image file could not be identified because JXL support not installed" |
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.
Exception instead of return string?
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.
Should we raise SyntaxError
like here in Jpeg2K plugin?
I based my work on WebP plugin which just returns string.
Fedora is changing to JXL files as default backgrounds files (Implemented in Fedora 42 Beta). Problem is that utilities to change wallpapers based in python relies on python-pillow to read images. I hope this change can move forward because the Spin I help maintain relies on Azote, that in turn depends on Pillow. Please, let us know how we can help here? |
@olokelo @radarhere both Fedora 42 and Ubuntu 25.04 support JPEG XL. Is there any remaining work/help that is needed to push this along? |
|
||
// count all JXL_DEC_NEED_IMAGE_OUT_BUFFER events | ||
while (decp->status != JXL_DEC_SUCCESS) { | ||
decp->status = JxlDecoderProcessInput(decp->decoder); |
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 actually think it would probably be preferable to not call JxlDecoderProcessInput
on open at all, to improve performance, and only set n_frames
when the user asks for it.
For me this is one of the key concepts in Pillow: you can safely open the image file without decoding the whole image (which is much more expensive operation) and get any info from the file.
That could be as simple as
Pillow/src/PIL/GifImagePlugin.py
Lines 125 to 135 in 3c71559
@property | |
def n_frames(self) -> int: | |
if self._n_frames is None: | |
current = self.tell() | |
try: | |
while True: | |
self._seek(self.tell() + 1, False) | |
except EOFError: | |
self._n_frames = self.tell() + 1 | |
self.seek(current) | |
return self._n_frames |
with Image.open(BytesIO(im_data)) as im: | ||
im.load() | ||
|
||
self._test_leak(core) |
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.
Was this purely added to copy test_webp_leaks.py? It's not a scenario that is tested for most formats, so I'm not sure it's necessary.
Helps #4247
This PR enables opening and reading JPEG XL images and animations.
Supported image modes are: RGB, RGBA, RGBa, L, LA, La.
A relatively recent libjxl version is needed to compile Pillow with libjxl support.
The main changes are the addition of
_jxl.c
andJxlImagePlugin.py
.I'm also the author of jxlpy so this PR was influenced by the work of contributors there. This PR is also largely based on
WebPImagePlugin.py
which had similar implementation.Why?
JPEG XL has recently seen increased adoption especially in Apple ecosystem. A lot of users are requesting Pillow support for JPEG XL as their products use Pillow and need to be able to handle
jxl
files.I'm open to suggestions and comments. I understand such change would need a lot of testing and probably changes. After all Pillow would need to become somewhat dependent on libjxl. Creating documentation will not be a big problem however I decided to wait for feedback from Pillow core developers.