Skip to content

Add the basic support for loading RAW format files with libRAW #4562

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
Merged
merged 1 commit into from
Jun 21, 2025
Merged

Add the basic support for loading RAW format files with libRAW #4562

merged 1 commit into from
Jun 21, 2025
+365 −0

Conversation

lxsameer
Copy link
Contributor

@lxsameer lxsameer commented Jun 9, 2025

This PR adds support for basic libraw supports. I tried to do the bare minimum so I can get early feedback and build upon that.

Looking forward to your feedback.

jcupitt
jcupitt reviewed Jun 9, 2025
View reviewed changes
libvips/foreign/librawload.c Outdated
object_class->description = _("load raw camera files");
object_class->build = vips_foreign_load_libraw_build;

foreign_class->priority = 100;
Copy link
Member

Choose a reason for hiding this comment

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

That's quite high priority, are you are it needs to be this high? Slower sniffers need to be near the back of the queue or they'll delay things like JPEG detection.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect this is due to the issue mentioned in #3380 (comment). As an alternative, we could lower the priority of tiffload, guarded by HAVE_LIBRAW.

Copy link
Member

Choose a reason for hiding this comment

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

... thinking about this further, we also could prevent TIFF-based RAW images from being loaded via libtiff. I previously mentioned this approach in the linked comment above and just cleaned up the commit (now guarded behind HAVE_LIBRAW) in kleisauke@955c467.

While this solution isn't very future-proof (and is essentially a wall of shame), it might help stop vendors from using standard TIFF signatures for their own custom RAW formats. 😅

jcupitt
jcupitt reviewed Jun 9, 2025
View reviewed changes
@jcupitt
Copy link
Member

jcupitt commented Jun 9, 2025

This is great @lxsameer, thank you for doing this work!

It seems to work OK for me, here's nip4 loading a cr2 sample:

image

I made some (mostly) general comments.

I think the big one is probably to only define _load() and not _header(). Unfortunately, libraw seems not to have a thing to detect supported RAW files quickly, nor something to just get the metadata. This means this will have to be a low priority loader, like magickload (which has similar problems).

@mertalev
Copy link

mertalev commented Jun 9, 2025

Would you be able to set output_bps=16 and use_camera_wb=1 by default like ImageMagick?

@lxsameer
Copy link
Contributor Author

lxsameer commented Jun 9, 2025

Hey folks, thanks for the review, I'll improve the code and ping you again for a review

@lxsameer
Copy link
Contributor Author

lxsameer commented Jun 9, 2025

@jcupitt just a quick question, do you have a certain priority in mind?

@jcupitt
Copy link
Member

jcupitt commented Jun 9, 2025 •
edited
Loading

Now I think again, I think you can just not set priority.

You define a set of suffixes and no _is_a() method, so your loader will be picked based JUST on the filename suffix, there will be no file type testing and the performance won't matter.

@jcupitt
Copy link
Member

jcupitt commented Jun 10, 2025

I had another thought last night -- the librawload name is a bit awkward, we don't use the lib prefix anywhere else. How about dcrawload, since libraw is a wrapper for dcraw?

@lxsameer
Copy link
Contributor Author

@jcupitt absolutely, I'll change the name

@lxsameer
Copy link
Contributor Author

Updated the commit. But unfortunately, in my test program, when I call vips_image_write_to_file on the VipsImage that I get from calling vips_dcrawload, I get the following error:

VipsRegion: images do not match in pixel size

I don't know what might be the issue, as far as I can see, I set the same size for both load->out and load->real. Any idea?

@lxsameer lxsameer requested review from jcupitt and kleisauke June 12, 2025 11:47
@kleisauke
Copy link
Member

It looks like everything needs to be moved into _header(), and ->load should be set to NULL, similar to how it's done in vipsload. I did a quick refactor with commit kleisauke@624accf, which seems to work on my end. Feel free to cherry-pick.

BTW, seeing #include <vips/intl.h> made me wonder, is this code already used somewhere? Or just based on an old loader by accident?

mertalev
mertalev reviewed Jun 14, 2025
View reviewed changes
mertalev
mertalev reviewed Jun 14, 2025
View reviewed changes
@lxsameer
Copy link
Contributor Author

@kleisauke Thank you, used your commit in my and added you as the co-author

@lxsameer lxsameer requested a review from mertalev June 16, 2025 09:12
mertalev
mertalev reviewed Jun 16, 2025
View reviewed changes
Copy link

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

I can't comment much on the overall implementation, but the extensions and libraw params look good!

jcupitt
jcupitt reviewed Jun 17, 2025
View reviewed changes
Copy link
Member

@jcupitt jcupitt left a comment

Choose a reason for hiding this comment

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

It looks great! I think I found a small bug in threading. and I have a question about the sort of interface we should support.

@lxsameer @kleisauke
Add the basic support for loading RAW format files with libRAW
4bd4892 
Co-authored-by: Kleis Auke Wolthuizen <github@kleisauke.nl>
@lxsameer lxsameer requested a review from jcupitt June 18, 2025 16:53
@jcupitt
Copy link
Member

jcupitt commented Jun 21, 2025

I think this looks great. Thank you again for doing all this work @lxsameer.

I suppose we should have some tests, but they can come in a followup PR. This is an API change, so we need a line in the changelog too (and we should credit you!), but again that can be a followup.

I've made a stable 8.17 branch for the current release, so I think we are ready to merge to master.

@lxsameer
Copy link
Contributor Author

Awesome, thank you.
Looking forward to this PR being merged and starting new PRs

@jcupitt jcupitt merged commit b6938ec into libvips:master Jun 21, 2025
6 checks passed
@jcupitt
Copy link
Member

jcupitt commented Jun 21, 2025

OK, let's merge and fix anything else later. Nice job!

@jcupitt
Copy link
Member

jcupitt commented Jun 21, 2025

I bumped the version in master to 8.18 ready for the next dev cycle, and credited you in the changelog @lxsameer, I hope that's OK.

8c5c65f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mertalev mertalev mertalev left review comments

@jcupitt jcupitt jcupitt approved these changes

@kleisauke kleisauke Awaiting requested review from kleisauke

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lxsameer @jcupitt @mertalev @kleisauke