Add ability to import JXL images #6367

xiota · 2021-09-22T08:03:23Z

This allows RawTherapee to import JXL images. Addresses [the import portion] of #6273.

Thanatomanic · 2021-09-22T08:09:45Z

@xiota Thanks for this!

Addresses half of #6273.

Which half? And can you elaborate a bit about what is still missing?

Floessie · 2021-09-22T11:23:55Z

@xiota Thanks for your contribution! 👍 There seem to be some (error) conditions under which ImageIO::loadJxl() leaks allocated memory in buffer. Could you double-check?

xiota · 2021-09-22T15:12:47Z

@Floessie Thanks for letting me know. I added a couple lines to free the buffer.

Thanatomanic · 2021-09-22T16:28:44Z

@xiota Just out of curiosity, do you have a particular reason to force-push the last few commits instead of committing regularly with descriptions? We can squash-merge things in the end so the master commit log won't be polluted.

xiota · 2021-09-22T17:10:22Z

@Thanatomanic Was just to keep the commit log clean. Didn't know you would squash merge.

xiota · 2021-09-22T21:10:34Z

This was working yesterday, but it isn't working after I recompiled today without making any changes.

xiota · 2021-09-23T21:06:24Z

It's working again. I was trying to make compiling with libjxl optional, but something was being cached, so it looked like it was working when it wasn't. Someone else will have to make the changes needed to make cmake detect and load libjxl as needed. (Right now it's required.)

rtgui/thumbnail.cc

rtdata/options/options.lin

heckflosse · 2021-10-24T12:50:44Z

One of the disabled extensions is png. Why is that?

There was a time, where some png files led to crashes of RT when starting RT on a folder which such a file.
I think we can enable png per default now. Iirc the crashes should be fixed...

Lawrence37 · 2021-10-24T18:09:00Z

I remember now. It was issue #3794 right? There was also a question about it in #4915.

xiota · 2021-10-24T19:20:51Z

Should I reenable PNG in this PR? Also, any advice/help for conditional compilation?

Thanatomanic · 2021-12-16T06:46:32Z

I think we can enable png per default now.

Yes, but not in this PR imo (keep it cleanly focused on JXL).

Someone else will have to make the changes needed to make cmake detect and load libjxl as needed. (Right now it's required.)

@xiota Well, this is an issue indeed. I found out that on Windows MSYS2 does not provide the libjxl package yet. So anyone on Windows should first manually compile libjxl or we should start working with submodules or something. Not very convenient.
My suggestion is to create a build option WITH_JXL here

RawTherapee/CMakeLists.txt

Lines 197 to 225 in 0e122a4

    
           option(USE_EXPERIMENTAL_LANG_VERSIONS "Build with -std=c++0x" OFF) 
        
           option(BUILD_SHARED "Build with shared libraries" OFF) 
        
           option(WITH_BENCHMARK "Build with benchmark code" OFF) 
        
           option(WITH_MYFILE_MMAP "Build using memory mapped file" ON) 
        
           option(WITH_LTO "Build with link-time optimizations" OFF) 
        
           option(WITH_SAN "Build with run-time sanitizer" OFF) 
        
           option(WITH_PROF "Build with profiling instrumentation" OFF) 
        
           option(WITH_SYSTEM_KLT "Build using system KLT library." OFF) 
        
           option(OPTION_OMP "Build with OpenMP support" ON) 
        
           option( 
        
               STRICT_MUTEX 
        
               "True (recommended): MyMutex will behave like POSIX Mutex; False: MyMutex will behave like POSIX RecMutex; Note: forced to ON for Debug builds" 
        
               ON) 
        
           option( 
        
               TRACE_MYRWMUTEX 
        
               "Trace custom R/W Mutex (Debug builds only); redirecting std::out to a file is strongly recommended!" 
        
               OFF) 
        
           option( 
        
               AUTO_GDK_FLUSH 
        
               "Use gdk_flush on all gdk_thread_leave other than the GUI thread; set it ON if you experience X Server warning/errors" 
        
               OFF) 
        
           # option(TARGET32BIT "Build for 32-bit architecture when ON, otherwise 64-bit. 
        
           # Default is OFF" OFF) 
        
           option(ENABLE_TCMALLOC "Use the tcmalloc library if available" OFF) 
        
           set(TCMALLOC_LIB_DIR 
        
               "" 
        
               CACHE PATH "Custom path for the tcmalloc library")

and set it to OFF by default.
Then you should be able to conditionally check for the package, like here

RawTherapee/CMakeLists.txt

Lines 496 to 498 in 0e122a4

    
           if(WITH_SYSTEM_KLT) 
        
               find_package(KLT REQUIRED) 
        
           endif()

But I'm not at all familiar with the intricacies of CMake, so perhaps this doesn't work at all...

EwoutH · 2022-11-30T20:44:54Z

Thanks for this effort! I would love to have JPEG XL import support in RawTherapee, what's needed to move this PR forward?

Beep6581 · 2022-12-18T01:44:14Z

Builds fine in Manjaro against libjxl-0.7.0 and libjxl_threads-0.7.0, and opens a sample .jxl file fine, though without any metadata.

Thanatomanic · 2022-12-18T08:53:53Z

though without any metadata

Is it to be expected that once we support evix2 this automagically works for jxl too?

kmilos · 2023-01-26T14:11:38Z

Is it to be expected that once we support evix2 this automagically works for jxl too?

If exiv2 is built w/ BMFF support, then yes.

Lawrence37 · 2023-03-11T03:44:35Z

Converting this to a draft because the GitHub Actions workflows still need to be updated to use the JXL library. Making JXL optional may also be necessary for the moment.

Beep6581 · 2023-09-05T22:37:18Z

Moving to 6.0 due to lack of activity.

EwoutH · 2023-09-26T12:37:40Z

@xiota Would you be able to further work on this? If so, is there anything you need to move this forward?

xiota · 2023-10-01T08:39:16Z

Issues I think holding this back:

Conditional compilation to build libjxl support only when the library is present.
Windows library availability?
I'd need a refresher on how rawtherapee works
I'd need a refresher on how libjxl works
update and rebase to current rawtherapee
update for current libjxl
I don't understand how libjxl color management works

kmilos · 2023-10-02T10:34:07Z

Windows library availability?

No problem w/ this one: https://packages.msys2.org/base/mingw-w64-libjxl

I don't understand how libjxl color management works

You are not alone on that one 😉

xiota · 2024-04-16T02:46:15Z

You can use the official forum ... to generate publicity.

RawTherapee JPEG XL Import

Floessie

Quick review of the diff with some minor change requests, most prominently the change from raw pointer buffer to something more sane. 😉

rtengine/imageio.cc

rtengine/imageio.h

rtengine/utils.cc

rtgui/thumbnail.cc

xiota · 2024-04-16T17:47:11Z

@Floessie Thank you for reviewing. Let me know if I missed something or there's more.

rtengine/imageio.cc

Floessie · 2024-04-17T09:23:17Z

@xiota It's quite okay from my point of view now. Let @Lawrence37 comment on the Glib::ustring problem and functionality.

Thanks for your contribution! 👍

* Remove cast in scanline * Improve Glib::ustring conversion

xiota · 2024-05-12T01:50:26Z

Checking in, since it's been nearly a month... anything else needed on my end for this?

Lawrence37 · 2024-05-12T02:10:58Z

No, you are just waiting for functionality testing. It's unfortunate that no one has commented about their experience on the Pixls topic. I will test if no one else does, but I do have a backlog of other pull requests to review.

xiota · 2024-05-12T04:02:18Z

@Lawrence37 Thanks for letting me know. No hurry... Just wanted to know the status.

Lawrence37 · 2024-05-25T05:56:56Z

I got this error compiling with libjxl 0.8.2.

/RawTherapee/rtengine/imageio.cc: In member function ‘int rtengine::ImageIO::loadJXL(const Glib::ustring&)’:
/RawTherapee/rtengine/imageio.cc:831:19: error: cannot convert ‘JxlColorProfileTarget’ to ‘const JxlPixelFormat*’
  831 | #define _PROFILE_ JXL_COLOR_PROFILE_TARGET_ORIGINAL
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                   |
      |                   JxlColorProfileTarget
/RawTherapee/rtengine/imageio.cc:904:60: note: in expansion of macro ‘_PROFILE_’
  904 |                     JxlDecoderGetICCProfileSize(dec.get(), _PROFILE_, &icc_size)
      |                                                            ^~~~~~~~~
In file included from /usr/include/jxl/decode_cxx.h:20,
                 from /RawTherapee/rtengine/imageio.cc:29:
/usr/include/jxl/decode.h:779:50: note:   initializing argument 2 of ‘JxlDecoderStatus JxlDecoderGetICCProfileSize(const JxlDecoder*, const JxlPixelFormat*, JxlColorProfileTarget, size_t*)’
  779 |     const JxlDecoder* dec, const JxlPixelFormat* unused_format,
      |                            ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
/RawTherapee/rtengine/imageio.cc:831:19: error: cannot convert ‘JxlColorProfileTarget’ to ‘const JxlPixelFormat*’
  831 | #define _PROFILE_ JXL_COLOR_PROFILE_TARGET_ORIGINAL
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                   |
      |                   JxlColorProfileTarget
/RawTherapee/rtengine/imageio.cc:920:40: note: in expansion of macro ‘_PROFILE_’
  920 |                             dec.get(), _PROFILE_,
      |                                        ^~~~~~~~~
/usr/include/jxl/decode.h:799:50: note:   initializing argument 2 of ‘JxlDecoderStatus JxlDecoderGetColorAsICCProfile(const JxlDecoder*, const JxlPixelFormat*, JxlColorProfileTarget, uint8_t*, size_t)’
  799 |     const JxlDecoder* dec, const JxlPixelFormat* unused_format,
      |                            ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~

kmilos · 2024-05-25T06:31:51Z

There was a change in API at some point, 0.9 I think...

xiota · 2024-05-25T06:36:14Z

I test built only with 0.7 and 0.10 because those are the versions readily available in repos. Looks like the cutoff version I ~~got from examples~~ [chose] isn't accurate.

#if JPEGXL_NUMERIC_VERSION < JPEGXL_COMPUTE_NUMERIC_VERSION(0, 8, 0)

I'll look for the commit that made the change

kmilos · 2024-05-25T06:40:35Z

You can also compare to the darktable #ifdefs, I think we got it right there: https://github.com/darktable-org/darktable/blob/master/src/imageio/imageio_jpegxl.c

xiota · 2024-05-25T06:46:06Z

Maybe I misinterpreted how to choose the cutoff before. The changes were made in v0.9-snapshot-409-gb08a7049. darktable uses 0,9,0. Will change it.

Lawrence37 · 2024-05-25T16:30:58Z

They make a note of API changes in their change log: https://github.com/libjxl/libjxl/blob/main/CHANGELOG.md
This can be helpful for determining if there are other places we can improve compatibility. It can also be used for setting a minimum required version if necessary.

Lawrence37

The functionality seem fine. It properly loads the embedded color profiles. EXIF data reads without issue (images without a container load without bugs). HDR images retain data. Animated and transparent images load (with necessary limitations).

2 things that could be improved but do not block this pull request:

Transparent parts of images are not completely black. There is some weird smearing of colors from opaque areas. See the Dice image from https://jpegxl.info/test-page/.
JXL status/library version could be added to AboutThisBuild.txt.

rtengine/imageio.cc

xiota · 2024-05-26T13:09:18Z

Switched from g_printerr to std::cerr. Added libjxl version to AboutThisBuild.txt.

I suspect the color smearing may be how the file is actually encoded. To get rid of it, might have to decode and process the alpha channel. darktable shows the same effect. Does RT have any existing code that deals with transparency that I could look at?

It properly loads the embedded color profiles.

I think this is done by code elsewhere.

Lawrence37 · 2024-05-27T01:05:49Z

I don't know if the code is in RawTherapee itself, but transparency is handled for pngs and tiffs.

Lawrence37 · 2024-05-27T06:10:50Z

Do you know what it shows in AboutThisBuild.txt if jxl support is disabled or if libjxl is not installed?

One more possible improvement. Users who are upgrading will not see jxl images because their settings carry over. It would be good to automatically add the jxl extension to the list of file types to load if the version in the options file shows 5.10 or older.

xiota · 2024-05-27T06:45:44Z

Do you know what it shows in AboutThisBuild.txt if jxl support is disabled or if libjxl is not installed?

When not installed, it shows something like: libjxl: V (the version number is missing). Should be the same for the other libraries.

When disabled, it shows the version that would have been installed.

I'll change it to something nicer like n/a for both cases.

Users who are upgrading will not see jxl images because their settings carry over.

How is this handled when new raw formats are added?

Lawrence37 · 2024-05-27T18:09:22Z

How is this handled when new raw formats are added?

I don't think this was ever handled. The last addition was .ori, which is disabled by default anyway. The other extension in the git history is .cr3. Nothing special was done for that even though I think something should have been.

xiota · 2024-05-27T21:57:52Z

Maybe enabling new file extensions could go in a separate PR that takes care of all of them together?

Handling the transparency case could also go in a separate PR someday, like after jxl export is added.

xiota force-pushed the jxl-import branch from 01c829b to fbf3afd Compare September 22, 2021 15:10

xiota force-pushed the jxl-import branch 2 times, most recently from 1d4ec65 to dde047a Compare September 22, 2021 15:38

Lawrence37 reviewed Oct 23, 2021

View reviewed changes

rtgui/thumbnail.cc Outdated Show resolved Hide resolved

Lawrence37 reviewed Oct 23, 2021

View reviewed changes

rtdata/options/options.lin Outdated Show resolved Hide resolved

Beep6581 added this to the v5.10 milestone Dec 1, 2022

Lawrence37 marked this pull request as draft March 11, 2023 03:45

Beep6581 modified the milestones: v5.10, v6.0 Sep 5, 2023

Lawrence37 mentioned this pull request Oct 6, 2023

Radiance RGBE (*.hdr) file format support #6606

Open

Lawrence37 added the scope: file format Camera or image file formats label Apr 15, 2024

Lawrence37 modified the milestones: v6.0, v5.11 Apr 15, 2024

Floessie requested changes Apr 16, 2024

View reviewed changes

Make requested changes

4d715cf

Floessie reviewed Apr 17, 2024

View reviewed changes

rtengine/imageio.cc Outdated Show resolved Hide resolved

Make requested changes

8c5fc60

* Remove cast in scanline * Improve Glib::ustring conversion

Fix version for libjxl API change

106ade3

Lawrence37 reviewed May 26, 2024

View reviewed changes

rtengine/imageio.cc Outdated Show resolved Hide resolved

xiota added 2 commits May 26, 2024 11:11

Switch g_printerr to std::cerr

a5954ad

Add libjxl version to AboutThisBuild.txt

40f9eef

AboutThisBuild: Distinguish JXL unavailable and disabled

e817246

Add ability to import JXL images #6367

Are you sure you want to change the base?

Add ability to import JXL images #6367

Conversation

xiota commented Sep 22, 2021 • edited

Thanatomanic commented Sep 22, 2021

Floessie commented Sep 22, 2021

xiota commented Sep 22, 2021

Thanatomanic commented Sep 22, 2021

xiota commented Sep 22, 2021

xiota commented Sep 22, 2021

xiota commented Sep 23, 2021 • edited

heckflosse commented Oct 24, 2021 • edited

Lawrence37 commented Oct 24, 2021

xiota commented Oct 24, 2021

Thanatomanic commented Dec 16, 2021

EwoutH commented Nov 30, 2022

Beep6581 commented Dec 18, 2022

Thanatomanic commented Dec 18, 2022

kmilos commented Jan 26, 2023

Lawrence37 commented Mar 11, 2023

Beep6581 commented Sep 5, 2023

EwoutH commented Sep 26, 2023

xiota commented Oct 1, 2023 • edited

kmilos commented Oct 2, 2023 • edited

xiota commented Apr 16, 2024

Floessie left a comment

Choose a reason for hiding this comment

xiota commented Apr 16, 2024

Floessie commented Apr 17, 2024

xiota commented May 12, 2024

Lawrence37 commented May 12, 2024

xiota commented May 12, 2024

Lawrence37 commented May 25, 2024

kmilos commented May 25, 2024

xiota commented May 25, 2024 • edited

kmilos commented May 25, 2024

xiota commented May 25, 2024 • edited

Lawrence37 commented May 25, 2024

Lawrence37 left a comment

Choose a reason for hiding this comment

xiota commented May 26, 2024 • edited

Lawrence37 commented May 27, 2024

Lawrence37 commented May 27, 2024

xiota commented May 27, 2024 • edited

Lawrence37 commented May 27, 2024

xiota commented May 27, 2024

xiota commented Sep 22, 2021 •

edited

xiota commented Sep 23, 2021 •

edited

heckflosse commented Oct 24, 2021 •

edited

xiota commented Oct 1, 2023 •

edited

kmilos commented Oct 2, 2023 •

edited

xiota commented May 25, 2024 •

edited

xiota commented May 25, 2024 •

edited

xiota commented May 26, 2024 •

edited

xiota commented May 27, 2024 •

edited