discord / lilliput

Resize images and animated GIFs in Go
https://discord.com/blog/how-discord-resizes-150-million-images-every-day-with-go-and-c
Other
1.95k stars 122 forks source link

Resized images lose color profile #40

Closed fredgido closed 6 years ago

fredgido commented 6 years ago

29

Steps to reproduce is using a image with a color profile. s2fzq

AllYourBase commented 6 years ago

@fredgido I don't see any reference to the lcms2 library in the dependency build scripts for OS X or Linux. I think that might be necessary to enable correct handling of color profiles. I know it's necessary to enable color management when using ImageMagick and GraphicsMagick.

brian-armstrong-discord commented 6 years ago

@fredgido Can you provide steps to detect whether the profile is there? Is there a command-line tool for this? And what parameters did you use to resize?

AllYourBase commented 6 years ago

@brian-armstrong-discord for just detecting the profile, you can use the very useful ExifTool by Phil Harvey. Typical command line invocation is something like this:

exiftool -G -D -a -ICC_Profile:* path/to/image_file

See sample output at this gist.

For profile manipulation (add, remove, convert, export) look at ImageMagick.

fredgido commented 6 years ago

Sorry I am not that expert on image processing libraries so I just used a image viewer/editor that shows all kind of details on a image. XnViewMP xnviewmp_2018-08-08_21-31-44 The image I sent before.

I think it will not keep the icc profile regardless of the input parameters. The icc profile could just be copied to the new image? and not really perform extra work on color when resizing. Most browsers nowadays render images with icc profiles correctly, so would do the discord app. Not sure about android and iPhone app rendering. So resizing and applying the color profile might be usefull?

@AllYourBase looks like is pointing to something that could be used to do it. 🙂

brian-armstrong-discord commented 6 years ago

Can you show me how you invoked lilliput to get this? Code snippet or example program invoke would be perfect.

fredgido commented 6 years ago

I dont get the point but ok. Here is the example

wget https://i.imgur.com/a2dPQwk.jpg ./examples -input a2dPQwk.jpg -output resizetest.jpg -height 100 -stretch

the result is https://i.imgur.com/eNeILCk.png

brian-armstrong-discord commented 6 years ago

lilliput isn't really intended to deal with jpeg exif metadata, and it would be fairly complex to add metadata support for all container types as well as whitelist specific kinds of exif. I'd recommend using another tool to add the exif metadata once lilliput resizes it.

AllYourBase commented 6 years ago

Color management is 20+ year old tech, and is critical to maintaining color fidelity in digital workflows. Lilliput should handle it correctly to be taken seriously as an image processing app.

brian-armstrong-discord commented 6 years ago

It's not an app, it's a library, and it makes sense to limit scope creep. If you want to add the EXIF color profile back in, it should be easy enough to use a library for that specifically -- EXIF tags are actually fairly hairy to work with and it wouldn't be trivial to manage them here.

If someone wants to open a PR that adds EXIF data management, I'm happy to review and consider it. Or if you have a EXIF library you like, let me know and I can add something to the readme.

AllYourBase commented 6 years ago

ICC profiles are not EXIF data. Some of the color profile metadata may be stored and displayed in EXIF fields, but they are different kinds of metadata.

In my opinion, as a 20 year professional in high-end high-volume digital image processing, a resizing tool that drops ICC profiles is only peripherally useful.

I’d understand if you don’t want to get into profile conversion. But without color profile retention, I wouldn’t consider using this library. By the time I’ve done the extra steps of detecting, saving, and re-embedding the color profile, any speed advantage I had with your library is gone, any my workflow is much more complex and difficult to maintain.

If it’s meant to be a pixels-only library that doesn’t care about any metadata, that’s fine. Probably best to say that up front so people aren’t surprised. Image metadata is important to many people/groups for a multitude of reasons, though. It’s worth understanding what you’re throwing away.

brian-armstrong-discord commented 6 years ago

Image metadata is a vector for leaking personal information. Most people don't know it exists and if they knew it embedded precise coordinates of where they live or their real name, they certainly wouldn't want it. JPEG EXIF data is especially tricky in this regard because it's so poorly standardized. You will find most image resizing libraries discard all EXIF data for this reason.

Some image containers do better at ICC than others. With JPEG, I don't see how you can maintain ICC information without getting into EXIF. Like I said before, if someone wants to add this functionality, I'm happy to look at it. Probably the most direct way would be to add a new wrapper to libexif that's exposed at the Go layer. Even if it existed though, I still don't think it's correct for the library to apply any EXIF by default -- it very much makes sense to have the developer opt in to metadata preservation, and probably on a tag by tag basis.

fredgido commented 6 years ago

If having metadata is the problem you could just apply the color profile permanently on the image before resizing and final image will look good without any meta data.

But yeah it still needs someone who can code it to check if there is ICC and apply the color calibration if there is.

AllYourBase commented 6 years ago

@brian-armstrong-discord as I already mentioned: ICC profiles are not EXIF data. They have nothing to do with GPS data or other types of metadata in images. All an ICC profile in an image does is describe the color capabilities of the device that generated the image, to ensure the most consistent color reproduction on screens and in print.

One can easily remove EXIF, XMP, IPTC/IIM, and other metadata from an image while retaining the ICC profiles, using ImageMagick:

mogrify +profile '!icc,*' test_image.jpg

You can verify with exiftool -G -D -a -ALL test_image.jpg before and after.

I recommend anyone involved in image manipulation learn about and understand color management and ICC profiles. The International Color Consortium website is a good resource. There's also a great book, "Real World Color Management", by Bruce Fraser, Chris Murphy, and Fred Bunting, which explains the science and theory and also goes into detail about practical digital workflow applications.

If you want to include color management with lilliput, you should look at Little CMS. Open source, written in C, actively developed and supported.

brian-armstrong-discord commented 6 years ago

It sounds like I misspoke a little, conflating the APP markers with EXIF. But all the same, the library currently has no way to navigate (read or write) APP markers, and the distinction feels like splitting hairs. The content in this metadata section does indeed leak personally identifying information more often than not.

Ultimately this isn't really an issue of color management as this library isn't doing any work in color space. Lilliput removes all metadata, but there is an argument to be made for keeping some of it. For now, the library does what it is intended to do, and it's possible to use another library to apply whatever APP data you want afterwards. Since the vast majority of JPEGs don't contain this information, I don't see this as being very high priority.