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

Internal LibRaw #6887

Open
wants to merge 22 commits into
base: dev
Choose a base branch
from
Open

Internal LibRaw #6887

wants to merge 22 commits into from

Conversation

Lawrence37
Copy link
Collaborator

As an experiment for #4462, I've added LibRaw directly into RawTherapee and borrowed heavily from ART for the implementation. It is functional and can potentially be adopted as the solution for replacing dcraw. I chose to go this route to familiarize myself with the raw image decoding part of the code. It didn't take much time or effort and at least I gained relevant knowledge if we decide to go with another route.

The LibRaw source code is included via git-subtree at rtengine/libraw. Like git submodules, git-subtree facilitates git subprojects, but tracks the commits in the parent project. The first two commits here add the LibRaw code to RawTherapee. Having our own copy of LibRaw allows us to make tweaks to it just as we have done with dcraw. There are no modifications to the LibRaw code yet in this pull request.

LibRaw has its own configuration script to generate the Makefile. At some point, it may be worth it to directly configure and build LibRaw through CMake. For now, I simply have CMake run the configuration script and added a task to call make. It works seamlessly except for the clean target. There's no way to modify the clean target, so cleaning the LibRaw build requires a second command: make clean-libraw. That means a full clean requires make clean and make clean-libraw.

The integration is fairly simple with most of the changes contained within the RawImage class. LibRaw is the first decoder. If it fails or does not support decoding the image, it falls back to dcraw. Since the code is adapted from ART, it accumulated some edge-case handling during the time it lived in ART.

git-subtree-dir: rtengine/libraw
git-subtree-split: cccb97647fcee56801fa68231fa8a38aa8b52ef7
@Lawrence37 Lawrence37 added type: enhancement Something could be better than it currently is scope: file format Camera or image file formats labels Nov 19, 2023
@Lawrence37 Lawrence37 added this to the v6.0 milestone Nov 19, 2023
@Lawrence37 Lawrence37 self-assigned this Nov 19, 2023
@Lawrence37 Lawrence37 force-pushed the libraw-copylib branch 2 times, most recently from 0a5fd2f to 2c57680 Compare November 20, 2023 01:59
@mattiaverga
Copy link
Contributor

From a Fedora Linux packager POV, going from bundling (almost) a single file to bundling an entire project like libraw is an horrible idea... and it will cause a real pain for me to continue providing RT as a Fedora official package.
Can you consider linking to libraw as an external library and propose patches upstream, if that become necessary?

@Lawrence37
Copy link
Collaborator Author

Lawrence37 commented Nov 21, 2023

I'm not familiar with the packaging process, so could you explain why adding many files is a burden? Is there anything that would help make it easier? It could be an issue if we decide to go with rawspeed instead. rawspeed is intended to be built into the program. A quick search in repositories for several distributions shows no results for rawspeed.

@kmilos
Copy link
Contributor

kmilos commented Nov 21, 2023

LibRaw has its own configuration script to generate the Makefile. At some point, it may be worth it to directly configure and build LibRaw through CMake.

You have https://github.com/LibRaw/LibRaw-cmake

Re bundling - we had the same issue w/ packaging darktable for distros, so there is a CMake option to either use the system LibRaw library, or the in-tree copy.

@mattiaverga
Copy link
Contributor

I'm not familiar with the packaging process, so could you explain why adding many files is a burden? Is there anything that would help make it easier? It could be an issue if we decide to go with rawspeed instead. rawspeed is intended to be built into the program. A quick search in repositories for several distributions shows no results for rawspeed.

It is a pain because Fedora "highly discourage" bundling libraries, so I will probably have to ask an exception for that.
More generally, bundling a library means that if a security issue is found and addressed upstream, it is not true that will be fixed in the bundled version, at least in a timely manner. I have already to deal with several CVEs filed for dcraw and I need every time to check if the bundled version is affected or not (see for example #4061, #4084, #5722).

Re bundling - we had the same issue w/ packaging darktable for distros, so there is a CMake option to either use the system LibRaw library, or the in-tree copy.

Sure, having the option to use system libraries or the bundled one would be acceptable. But if the bundled one is going to be modified to adapt to RT code, that would not be a feasible option.

@Lawrence37
Copy link
Collaborator Author

Ok, that makes more sense. So it's not about the structure of the code, but having to resolve problems across multiple copies. Libraw is fortunately a proper library and I was planning on adding a compile-time option to use the RT version or the system library. The same is not true of rawspeed and dcraw (mostly).

But if the bundled one is going to be modified to adapt to RT code, that would not be a feasible option.

Do you mean if there is a custom LibRaw in the codebase, it's not feasible to implement the ability to choose which LibRaw to use, or not feasible to maintain a package of RawTherapee for Fedora? In either case I don't see the issue.

@mattiaverga
Copy link
Contributor

Do you mean if there is a custom LibRaw in the codebase, it's not feasible to implement the ability to choose which LibRaw to use, or not feasible to maintain a package of RawTherapee for Fedora? In either case I don't see the issue.

Referring to what you wrote above: "Having our own copy of LibRaw allows us to make tweaks to it just as we have done with dcraw". If the custom library here and the vanilla upstream are going to differ, trying to use the system provided library might become broken.

@Lawrence37
Copy link
Collaborator Author

Lawrence37 commented Nov 22, 2023

That won't be an issue. We don't need to make any fundamental modifications that would affect the API or ABI.

Edit: Actually, I think only the API matters since the program is linked to one or the other LibRaw, not both. That means everything will work if we don't touch libraw.h, which we have no need to do.

Decode raw files with LibRaw and fall back to dcraw if LibRaw is unable
to read the file.
Set the frame number when reading image metadata.
Prevent accidental use of these functions that will not supply the
correct data when using LibRaw.
The include directory for LibRaw is properly set by CMake so
    `#include <libraw/libraw.h>`
will work for both the internal and system versions.
1ef70158d 0.21.2 release
62f042366 tag type => tag size mapping fixed
ee087e3fe cubic_spline: better handling of non-integer data
af755b991 extra metadata check in arq_load_raw
0fadd8819 Better incorrect data handling in cubic_spline
d7fb66053 skip invalid pattern in xtrans_interpolate
d059ed280 Check HL recovery coeffs before processing
104730519 limit wavelet denoise minimum size
cae09838e raw-identify: use fallback if PATH_MAX not available
d6c677608 additional check against corrupted ljpeg layout
1001a6ac1 Disable color conversion for Canon 16-bit thumbnails
a5130b01b docs/changelog: explained the case when no thumbnail is found in specific file
600c0c63d rename swapXX to libraw_swapXX to avoid name conflict
299c8a11b Check against corrupted LJPEG header in Canon sRAW decoder
ec8671ad9 Limit embedded color profile allocation/read size
5229d5942 Wrong alloc result check for 16-bit bitmap thumbnail
b278b775f check pana_data/buffer offset before use
7f4b8d3af Check P1 quadrant linearization coeff[15] against zero
e942a7db6 avoid integer overflow in buffer space check
f6a57cfb8 prevent buffer overrun in buffer_datastream::scanf_one
3e62ed304 ensure correct T.tlength for 16b bitmap thumbnails(2)
8e52d81cd ensure correct T.tlength for 16b bitmap thumbnails
8e1af15e2 Do not run sraw decoder on (crafted) bayer files
0ace959c2 better striped thumbnails handling
477e0719f do not set shrink flag for 3/4 component images
c8efae6c5 allow more decoders for fuji-rotated RAWs

git-subtree-dir: rtengine/libraw
git-subtree-split: 1ef70158d7fde1ced6aaddb0b9443c32a7121d3d
Update white level for Fujifilm HS20EXR/HS22EXR and add constants for
Panasonic DMC-TZ82.
Works if the FixBadPixelsConstant is zero, as implemented for dcraw.
Other constant values may be implemented in the future.
The model, rather than the normalized model, gives the name that matches
those given by dcraw for Hasselblad cameras. This is important for
matching the model name in camconst.json.
@Lawrence37
Copy link
Collaborator Author

Some updates since my last comment:

  1. Fixed DNG gain map detection and reading when using LibRaw.¹
  2. Added option Preferences > Image Processing > Raw Decoder > Use LibRaw. If enabled (default), LibRaw will be used to decode raw files except if the file is known not to work or work sub-optimally.
  3. Fixed crash when loading some multi-frame files.
  4. Used proper #include statement in case the system LibRaw has an incompatible API.
  5. Updated internal LibRaw to v0.21.2 (from v0.21.1).
  6. Fixed Sony Pixel Shift loading with a LibRaw implementation.
  7. Added a few missing camera constants.²
  8. Fixed DNG bad pixels detection and reading when using LibRaw.¹
  9. Fixed application of camera constants for Hasselblad cameras.
  10. Implement levels and flat-field correction for Hasselblad cameras when using LibRaw.²

I went through the RawTherapee edits to dcraw and checked if LibRaw addresses them. For those that LibRaw does not address, I made the necessary changes to bring the decoding in line with our custom dcraw decoder. I hope I didn't miss anything. This pull request is now ready for consideration.


¹ Originally implemented in our custom dcraw. The dcraw implementation has been removed as it now works with any raw decoder.
² Changes to the internal LibRaw. Not available if compiled for the system LibRaw.

@Lawrence37 Lawrence37 marked this pull request as ready for review January 14, 2024 02:20
@kmilos
Copy link
Contributor

kmilos commented Jan 14, 2024

Changes to the internal LibRaw.

Please consider upstreaming any relevant changes. (I guess you can skip the new camera constants as those are presumably already lined up for the next public snapshot.)

@epadepa
Copy link

epadepa commented Feb 5, 2024

I compiled and gave this a run, am I the only one seeing a considerable performance penalty? I can barely compile so the problem may well lie behind the keyboard. Any suggestions how I can get performance metrics?

I downloaded https://raw.pixls.us/getfile.php/2474/nice/Pentax%20-%20K-1%20-%2014bit%20(3:2).DNG and opening + any operation is extremely slow.

Version: 5.9-477-g67da8a963
Branch: libraw
Commit: 67da8a963
Commit date: 2024-01-11
Compiler: cc 13.2.0
Processor: x86_64
System: Linux
Bit depth: 64 bits
Gtkmm: V3.24.8
Lensfun: V0.3.4.0
Build type: Debug
Build flags:  -std=c++11 -ffp-contract=off  -Werror=unused-label -Werror=delete-incomplete -fno-math-errno -Wno-attributes -Wall -Wuninitialized -Wcast-qual -Wno-deprecated-declarations -Wno-unused-result -Wunused-macros -fopenmp -Werror=unknown-pragmas -g -ftree-vectorize
Link flags:   
OpenMP support: ON
MMAP support: ON
Build OS: Linux 6.3.0-1-amd64 x86_64
Build date: Sun, 04 Feb 2024 22:15:51 +0000 UTC
Build epoch: 1707084951
Build UUID: 

edit: did som simple time runs with two of my local builds
First libraw build

time ~/programs/rt-libraw/rawtherapee-cli -o /tmp/rt-libraw.jpg -p ~/.cache/RawTherapee5-libraw/profiles/Pentax\ -\ K-1\ -\ 14bit\ \(3\ 2\).DNG.bca3bd1b22bdab560fb70ce264c3358f.pp3   -c ~/dwn/Pentax\ -\ K-1\ -\ 14bit\ \(3\ 2\).DNG
RawTherapee, version 5.9-477-g67da8a963, command line.
Output is 8-bit integer.
Processing: /home/epadepa/dwn/Pentax - K-1 - 14bit (3 2).DNG
 Merging procparams #0

real    1m42.062s
user    9m37.321s
sys     0m2.618s

Then a lacam16n branch build (sorry for not being a release)

time ~/programs/rt-lacam/rawtherapee-cli -o /tmp/rt-lacam.jpg -p ~/.cache/RawTherapee5-libraw/profiles/Pentax\ -\ K-1\ -\ 14bit\ \(3\ 2\).DNG.bca3bd1b22bdab560fb70ce264c3358f.pp3   -c ~/dwn/Pentax\ -\ K-1\ -\ 14bit\ \(3\ 2\).DNG
RawTherapee, version 5.9-718-gd1750e827, command line.
Output is 8-bit integer.
Processing: /home/epadepa/dwn/Pentax - K-1 - 14bit (3 2).DNG
  Merging procparams #0

real    0m18.669s
user    1m48.271s
sys     0m6.243s

@Lawrence37
Copy link
Collaborator Author

Your build type is debug. Try changing it to release. The RawPedia page for compiling on Linux should explain how, in case you need it.

@Desmis
Copy link
Collaborator

Desmis commented Feb 22, 2024

@Lawrence37

Hello

I would like to inform you of my feedback to install this PR under Windows (some will say that it is enough to be under Linux...), the fact remains that I am not there, and probably not the alone...and in any case it is necessary to be able to do it in the 3 systems (Windows, Mac, Linux).
Plus as you know, I'm bad at computers, I work like a monkey copying what people tell me.

As soon as I wanted to compile, while my version of Msys2 was up to date (done on February 17, 2024). I got this compiler errors.

"autoreconf" is unknown in Msys.

After searching on google I found this command to enter on Msys2.
$ pacman -S autotools

Then in "Mingw64" I run "cmake --build . --target install"

I was a little scared by all the messages from the compiler... it's quite impressive, but in the end everything works.

Just a detail. When I change a file in my editor, and re-run cmake, I get this message "Building LibRaw" which lasts a few seconds (about 4 to 10 sec)

jdesm@PCSPECIALIST01 MINGW64 /e/code/repo-rt3/build
$ cmake --build . --target install
[1/7] Creating AboutThisBuild.txt and other version-dependent files
-- git command found: C:/msys64/usr/bin/git.exe
-- Git checkout information:
-- Commit description: 5.9-477-g67da8a963
-- Branch: libraw
-- Commit: 67da8a9
-- Commit date: 2024-01-11
-- Commits since tag: 477
-- Commits since branch: 477
-- Version (unreliable): 5.9.477
-- Build information:
-- Build OS: MINGW64_NT-10.0-22635 3.4.10.x86_64 x86_64
-- Build date: Thu, 22 Feb 2024 14:41:23 +0000 UTC
-- Epoch: 1708612883
-- UUID: b4959f48-d5c6-46ea-bfbe-5b44b4c173a4
-- CACHE_NAME_SUFFIX is "5-dev"
[2/7] Building LibRaw
make: Nothing to be done for 'all'.
[6/7] Install the project...-- Install configuration: "release"
-- Up-to-date: E:/code/repo-rt3/build/release/./AUTHORS.txt
-- Up-to-date: E:/code/repo-rt3/build/release/./LICENSE
-- Installing: E:/code/repo-rt3/build/release/./AboutThisBuild.txt
-- Up-to-date: E:/code/repo-rt3/build/release/./RELEASE_NOTES.txt
-- Up-to-date: E:/code/repo-rt3/build/release/./share/man/man1/rawtherapee.1
-- Up-to-date: E:/code/repo-rt3/build/release/./licenses
-- Up-to-date: E:/code/repo-rt3/build/release/./licenses/DroidSansMonoDotted.txt
etc.

Thank you for this work...because Dcraw is almost dead :)

Jacques

@Lawrence37
Copy link
Collaborator Author

Right, the documentation for compiling in macOS and Windows will need to be updated. In the workflow .yml files, you can see that macOS needs automake and Windows (msys2) needs autotools. By default, the build process for LibRaw does emit many messages. As long as there are no warnings or errors, everything should be fine. When rebuilding, LibRaw will take some time to check if any source files have been changed. It's the same for rtengine, rtgui, etc.

@Desmis
Copy link
Collaborator

Desmis commented Feb 23, 2024

@Lawrence37
OK thank you
Jacques

…r xtrans files where the code expects to find it
@kmilos
Copy link
Contributor

kmilos commented Feb 23, 2024

After searching on google I found this command to enter on Msys2.
$ pacman -S autotools

Actually, you want pacman -S --needed mingw-w64-x86_64-autotools (or mingw-w64-ucrt-x86_64-autotools if you're building in the UCRT64 environment) to ensure other dependencies are there as well.

Btw, libraw is available in msys2 so there is no real need to build it...

@Desmis
Copy link
Collaborator

Desmis commented Feb 23, 2024

@kmilos

OK, thank you, I update Msys2 with pacman -S --needed mingw-w64-x86_64-autotools, and libraw

Jacques

Use same type for variable comparison in loop condition. Fixes CodeQL
alert 96.
@Desmis
Copy link
Collaborator

Desmis commented Mar 3, 2024

@Lawrence37
Apart from a lot of warnings during compilation (decoders_dcraw.cpp, libraw,...), this took place without problems and upon execution everything seems (from what I can verify) to work correctly.

Thank you

Jacques

@Lawrence37
Copy link
Collaborator Author

Now that a new LibRaw snapshot is out, I wonder if it is a good idea to use snapshot or stick to releases. Snapshots have a few limitations stated on the website. There will also be additional effort required to merge minor releases into the snapshots.

@Lawrence37
Copy link
Collaborator Author

Given LibRaw's long release cycle, I will go with using the public snapshots.

12b0e5d60 Snapshot 202403
a4c9b1981 loop parameters in remove_trailing_spaces
e231b01a4 CR3-Qstep table: avoid wrong 64-bit code generation
21368133a 0.21.2 release
a6f212a4a tag type => tag size mapping fixed
41506ef73 cubic_spline: better handling of non-integer data
17294b5fd extra metadata check in arq_load_raw
47d9dd8e2 Better incorrect data handling in cubic_spline
3baa51068 skip invalid pattern in xtrans_interpolate
eaf63bf5f Check HL recovery coeffs before processing
a14574080 limit wavelet denoise minimum size
4e597e4e9 Merge pull request Beep6581#594 from pinotree/path_max
086dcb9a6 raw-identify: use fallback if PATH_MAX not available
0e105f9f8 additional check against corrupted ljpeg layout
0c8b68e9f Disable color conversion for Canon 16-bit thumbnails
cd1abd695 docs/changelog: explained the case when no thumbnail is found in specific file
6fffd414b rename swapXX to libraw_swapXX to avoid name conflict
af78ae48a Check against corrupted LJPEG header in Canon sRAW decoder
ba780e600 Limit embedded color profile allocation/read size
122bf7c5b Wrong alloc result check for 16-bit bitmap thumbnail
e6dd2709e check pana_data/buffer offset before use
ae2dc5884 Check P1 quadrant linearization coeff[15] against zero
f2998bacc avoid integer overflow in buffer space check
443b7fb51 prevent buffer overrun in buffer_datastream::scanf_one
35a6d3615 ensure correct T.tlength for 16b bitmap thumbnails(2)
b69ea8be5 ensure correct T.tlength for 16b bitmap thumbnails
2b6eca897 Do not run sraw decoder on (crafted) bayer files
68808b57f better striped thumbnails handling
9ab70f6dc do not set shrink flag for 3/4 component images
32425dd96 allow more decoders for fuji-rotated RAWs
REVERT: 1ef70158d 0.21.2 release
REVERT: 62f042366 tag type => tag size mapping fixed
REVERT: ee087e3fe cubic_spline: better handling of non-integer data
REVERT: af755b991 extra metadata check in arq_load_raw
REVERT: 0fadd8819 Better incorrect data handling in cubic_spline
REVERT: d7fb66053 skip invalid pattern in xtrans_interpolate
REVERT: d059ed280 Check HL recovery coeffs before processing
REVERT: 104730519 limit wavelet denoise minimum size
REVERT: cae09838e raw-identify: use fallback if PATH_MAX not available
REVERT: d6c677608 additional check against corrupted ljpeg layout
REVERT: 1001a6ac1 Disable color conversion for Canon 16-bit thumbnails
REVERT: a5130b01b docs/changelog: explained the case when no thumbnail is found in specific file
REVERT: 600c0c63d rename swapXX to libraw_swapXX to avoid name conflict
REVERT: 299c8a11b Check against corrupted LJPEG header in Canon sRAW decoder
REVERT: ec8671ad9 Limit embedded color profile allocation/read size
REVERT: 5229d5942 Wrong alloc result check for 16-bit bitmap thumbnail
REVERT: b278b775f check pana_data/buffer offset before use
REVERT: 7f4b8d3af Check P1 quadrant linearization coeff[15] against zero
REVERT: e942a7db6 avoid integer overflow in buffer space check
REVERT: f6a57cfb8 prevent buffer overrun in buffer_datastream::scanf_one
REVERT: 3e62ed304 ensure correct T.tlength for 16b bitmap thumbnails(2)
REVERT: 8e52d81cd ensure correct T.tlength for 16b bitmap thumbnails
REVERT: 8e1af15e2 Do not run sraw decoder on (crafted) bayer files
REVERT: 0ace959c2 better striped thumbnails handling
REVERT: 477e0719f do not set shrink flag for 3/4 component images
REVERT: c8efae6c5 allow more decoders for fuji-rotated RAWs

git-subtree-dir: rtengine/libraw
git-subtree-split: 12b0e5d60c57bb795382fda8494fc45f683550b8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: file format Camera or image file formats type: enhancement Something could be better than it currently is
Projects
None yet
6 participants