Skip to content

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

Open
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

olokelo
Copy link

@olokelo olokelo commented Mar 2, 2024

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 and JxlImagePlugin.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.

@olokelo olokelo mentioned this pull request Mar 2, 2024
@brunoais
Copy link

brunoais commented Mar 2, 2024

May you please commit the images as LFS?

@olokelo
Copy link
Author

olokelo commented Mar 2, 2024

May you please commit the images as LFS?

Do you mean those .jxl files under Tests/images I've committed? I'm not really sure how to do that.

@brunoais
Copy link

brunoais commented Mar 3, 2024

The explanation is outlined here: https://docs.github.com/en/repositories/working-with-files/managing-large-files/configuring-git-large-file-storage
Nevermind. I just noticed they are not putting the test images as lfs:
image

They already setup lfs and they only use for other larger stuff. Let it stay as you did.

@radarhere radarhere added the JPEG label Mar 11, 2024
@olokelo
Copy link
Author

olokelo commented Mar 11, 2024

Mac OS builds were failing because clang complained about goto labels being declared before variables in scope.
I also merged @radarhere's commits that remove the jxl feature which was causing troubles before.
Now it should be fine. Almost all checks pass except codecov.

@@ -47,6 +47,7 @@
"IptcImagePlugin",
"JpegImagePlugin",
"Jpeg2KImagePlugin",
"JxlImagePlugin",
Copy link
Member

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?

Suggested change
"JxlImagePlugin",
"JpegXlImagePlugin",

Copy link
Author

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?

Copy link
Member

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.

Comment on lines 14 to 16
# TODO: lower the limit, I'm not sure what is correct limit
# since I have libjxl debug system-wide
mem_limit = 16 * 1024 # kb
Copy link
Member

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?

Copy link
Author

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.

# 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
Copy link
Member

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?


def seek(self, frame):
if not self._seek_check(frame):
return
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hugovk
Copy link
Member

hugovk commented Mar 19, 2024

I've merged in main to include #7827 to fix a lot of the warnings I'm seeing on macOS 14.4 Sonoma with CommandLineTools installed.

And this is failing to build for me with:

      src/_jxl.c:174:12: error: incompatible integer to pointer conversion returning 'int' from a function with result type 'void *' [-Wint-conversion]
          return true;
                 ^~~~
      /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/include/stdbool.h:21:14: note: expanded from macro 'true'
      #define true 1
                   ^

Full log: log2.txt

@olokelo
Copy link
Author

olokelo commented Mar 19, 2024

I've merged in main to include #7827 to fix a lot of the warnings I'm seeing on macOS 14.4 Sonoma with CommandLineTools installed.

And this is failing to build for me with:

      src/_jxl.c:174:12: error: incompatible integer to pointer conversion returning 'int' from a function with result type 'void *' [-Wint-conversion]
          return true;
                 ^~~~
      /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/include/stdbool.h:21:14: note: expanded from macro 'true'
      #define true 1
                   ^

Full log: log2.txt

Thanks for reporting. The _jxl_decoder_count_frames function was declared to return void * instead of bool. That was an obvious mistake and I'm even not sure why it worked under gcc.

Comment on lines 30 to 31
if is_jxl and not SUPPORTED:
return "image file could not be identified because JXL support not installed"
Copy link
Member

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?

Copy link
Author

@olokelo olokelo Mar 19, 2024

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.


// count all JXL_DEC_NEED_IMAGE_OUT_BUFFER events
while (decp->status != JXL_DEC_SUCCESS) {
decp->status = JxlDecoderProcessInput(decp->decoder);
Copy link
Member

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.

#7311 (comment)

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

@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

self.n_frames = None
if n_frames > 0:
self.n_frames = n_frames
self._tps_dur_secs = tps_num / tps_denom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that n_frames is zero when there is an animated file but one of the frames can't be loaded? As in, a well-formed image shouldn't cause this, but it is a file where we will never know the number of frames even when seeking forward, because it will fail.

@j99ca
Copy link

j99ca commented Jun 25, 2025

@radarhere couple of questions:

  1. Will this decoding allow us to decode float32 JXL images? This is important for us as JXL files are great for scientific data (depth, thermal, etc)
  2. Is there any chance that PIL will be able to extract the metadata that might be included in the JXL image, like XMP?

@radarhere
Copy link
Member

  1. The first commit here left a comment in the C file that 'PIL doesn't support high bit depth images'. I don't know what the thinking was there - in general, Pillow does support single channel 32-bit float images. Perhaps that specific depth just wasn't added.

  2. EXIF and XMP data can be extracted with this PR, yes.

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

Successfully merging this pull request may close these issues.