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

Add transform_colorspace_to_srgb operation and use it to fix inaccurate colors when saving specific image files #142

Merged

Conversation

Stormheg
Copy link
Member

@Stormheg Stormheg commented Jan 14, 2024

Please see #139 for a detailed description of what this is trying to accomplish. Took me longer than expected to prepare a PR, writing tests turned out to be quite time consuming!

I've expanded the automatic conversion discussed in #139 to include png files too as they are also affected by the color issue described in #139. Technically, GIF files are also affected but I've not bothered to fix them because there isn't much to gain when the colors are butchered to fit in a palette of 256 colors.

The automatic conversion during save of avif, heic, webp and png images only happens when we encounter an image that is affected by the issue (CMYK mode, with embedded ICC profile). For all other images, the new code path is not taken and there should be no impact.

I'd appreciate reviews from anyone! (not necessarily Willow maintainers only).

I've done my best to explain how and why in the docs and comments but I'm no expert on the subject of color spaces and gamuts and such. Please let me know if there are any inaccuracies or ambiguities.

tests/test_pillow.py Outdated Show resolved Hide resolved
@Stormheg
Copy link
Member Author

I've added a new image for the test suite: dog_and_lake_cmyk_with_icc_profile.jpg. I've modified this image specifically to be affected. The image has been heavily downsized to make its size as small as possible but the embedded ICC profile still takes up a lot of space. It is a little over 1MB in size.

I took the image a couple years ago during a hike with my dog. Willow has my permission to use the downsized image file for its test suite.

Here is a full-sized image for your viewing pleasure. Try opening and saving it as webp or avif with an unpatched version of Willow and this patch to see the difference.
zaar-lake_iso_coatedv2.jpg.zip

image

screenshot comparing the original JPEG on the left with an unpatched WEBP version.

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Nice one @Stormheg
Left a couple of notes

willow/plugins/pillow.py Outdated Show resolved Hide resolved
tests/test_pillow.py Outdated Show resolved Hide resolved
@andre-fuchs
Copy link
Contributor

@Stormheg Very cool! I suggest to add a system wide setting to convert all images to sRGB. Two setting variables would desirable, I think. A boolean type to turn this conversion on or off. And another one for the rendering intend (could be both an integer or char type). This is mainly an update of Wagtail, but I guess that Willow also would need some preparation for that. Does this make sense?

@Stormheg
Copy link
Member Author

@andre-fuchs lets discuss automatically converting to sRGB in a separate issue 👍

Since most images and devices consuming the web are only capable of displaying sRGB this should be mostly fine; the space saving would be worth the cost in most cases I think. I could imagine that a photography site might not want to perform such conversions however. Saturated sRGB colors look a bit bland compared to what is possible with P3 and Adobe RGB.

Maybe there could be a middle ground where we provide functionality to detect what color gamut the original image is in? And if it is in a color other than sRGB, Wagtail could output something akin to the snippet below. This will only load the wide gamut with original ICC profile for devices that support displaying that color gamut.

<picture>
  <source media="(color-gamut: p3)" srcset="photo-wide.jpg">
  <img src="photo-srgb.jpg">
</picture>

Somewhat niche functionality maybe, but with the number of devices that are capable of recording and displaying wide-gamut images increasing (in particular recent iPhones are capable of this) it would be good future proofing for Wagtail to be aware of wide gamut images.

Anyway, let's discuss separately from this PR 😄

CMYK is not supported by PNG, WEBP, AVIF and HEIC Pillow encoders.

When a CMYK image is encoded, it is converted to RGB but this conversion
results in inaccurate colors because Pillow ignores the ICC profile when
performing the conversion.

As a workaround, we manually force an accurate conversion to RGB _before_
encoding the image. This results in a much more accurate representation
of the original CMYK image.
@Stormheg Stormheg force-pushed the fix/inaccurate-color-fix-for-cymk-images branch 2 times, most recently from d6c24a7 to 0d3dd06 Compare January 15, 2024 22:20
The loose check appears to not be necesary. There do not appear to be
differences between systems.
@Stormheg Stormheg force-pushed the fix/inaccurate-color-fix-for-cymk-images branch from 0d3dd06 to 4cf47e1 Compare January 15, 2024 22:22
@zerolab zerolab merged commit 9565d94 into wagtail:main Jan 16, 2024
7 checks passed
@zerolab
Copy link
Collaborator

zerolab commented Jan 16, 2024

Nice one @Stormheg. Let's discuss the suggestion from @andre-fuchs separately

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.

3 participants