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

Take advantage of libvips new "keep" feature to increase control over output metadata #3856

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

lovell
Copy link
Owner

@lovell lovell commented Nov 16, 2023

libvips v8.15.0 deprecated the "strip" option for removing all metadata, instead replacing it with a set of more fine-grained "keep" flags.

This PR adds new functions to take advantage of this:

  • keepExif
  • withExif
  • withExifMerge
  • keepIccProfile
  • withIccProfile
  • keepMetadata

EXIF can now be either replaced or merged, plus there are new TypeScript definitions for EXIF metadata structure.

The input ICC profile can now be attached to the output image.

The exif and icc properties of withMetadata are now deprecated. The ability to provide a boolean to withMetadata has been removed.

Relates to #3824 and #2910

@adriaanmeuris
Copy link
Contributor

I'm thrilled to see the progress on this! After reviewing it, I wanted to share some thoughts.

Using withIccProfile is quite powerful, handling both the transformation of image data and embedding that profile into the output file. Reflecting on our previous conversation in #3824 (comment), I understand that the default behaviour, when no functions are used, aligns with libvips' keep=none approach.

Here's where I see a potential enhancement. With libvips we can convert the color space and strip all metadata, including ICC profiles. For instance:

vips icc_transform input_cmyk_fogra39_with_icc.jpg "output_cmyk_swop_stripped.jpg[keep=none]" "U.S. Web Coated (SWOP) v2.icc"

In the current implementation of this PR, achieving this seems not possible because using any of the new functions inadvertently adds an ICC profile to the output file, so its behaviour does not entirely match what keep=none does in the example above.

Recognizing that the current approach might be tailored to more common use cases, I wonder if it's possible to introduce an optional boolean flag to keepMetadata? This addition could offer a way to replicate libvips behaviour.

Alternatively, integrating the same metadata flags as in libvips as discussed here might provide an even finer level of control. Of course, this could mean a bit more work:

 * @VIPS_FOREIGN_KEEP_NONE: don't attach metadata
 * @VIPS_FOREIGN_KEEP_EXIF: keep Exif metadata
 * @VIPS_FOREIGN_KEEP_XMP: keep XMP metadata
 * @VIPS_FOREIGN_KEEP_IPTC: keep IPTC metadata
 * @VIPS_FOREIGN_KEEP_ICC: keep ICC metadata
 * @VIPS_FOREIGN_KEEP_OTHER: keep other metadata (e.g. PNG comments and some TIFF tags)
 * @VIPS_FOREIGN_KEEP_ALL: keep all metadata

Would love to hear your thoughts on this.

@lovell
Copy link
Owner Author

lovell commented Nov 16, 2023

@adriaanmeuris I'd like to avoid an API that allows people to create invalid or incorrectly-rendered images. An image that uses a profile should attach that profile (the exception perhaps being device independent profiles such as sRGB). I'm aware your use case is for PDFs, but sharp isn't really designed for PDF manipulation.

@adriaanmeuris
Copy link
Contributor

@lovell I completely share your concern about preventing the creation of incorrectly rendered images with sharp, and it's not my intention to compromise this aspect of the library.

I do want to point out that my use of sharp is not for PDF manipulation, but rather for preprocessing images for a target that is not a browser. Unlike browsers, where stripping out the sRGB profile by default is standard due to browsers assuming untagged images are in sRGB, other targets can offer more precise control over color consistency.

For ICC-based images in PDFs, the format refers to a separate ICC buffer (the one available in sharp's metadata function) rather than embedding the profile in the image. I completely agree it's not the most common scenario, but it does mirror the rationale behind stripping out the sRGB profile by default, while recognizing that not all use cases target a browser environment.

Considering this, would you be open to a separate function such as removeIccProfile? Being a distinct function ensures intentional use and allows for clear documentation of its purpose. This should address the concern about accidentally creating incorrectly-rendered images.

I'd certainly be open to creating a PR to make this possible.

@lovell lovell force-pushed the withexif-and-friends branch 2 times, most recently from 243e2ca to 1ec4ad6 Compare November 21, 2023 16:11
@lovell
Copy link
Owner Author

lovell commented Nov 21, 2023

@adriaanmeuris The new withIccProfile() function now accepts an optional attach parameter, defaulting to true, so you could use:

.withIccProfile('profile.icc', { attach: false })

Add new withX and keepX functions that take advantage of
libvips 8.15.0 new 'keep' metadata feature.
@adriaanmeuris
Copy link
Contributor

Thanks, @lovell! I've tested the new withIccProfile() with attach parameter and it works exactly as expected. Really appreciate you considering and implementing this.

@lovell lovell merged commit e78200c into gauge Nov 22, 2023
34 checks passed
@lovell lovell deleted the withexif-and-friends branch November 22, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants