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 tests for validating output colors when doing cmyk to cmyk conversion with embedded profiles #4096

Merged
merged 2 commits into from
May 13, 2024

Conversation

adriaanmeuris
Copy link
Contributor

As discussed in #4045, this PR:

  1. Adds a test to validate output colors when doing CMYK to CMYK colourspace transformations with embedded profiles using perceptual intent (the current default). This test might be updated in the future if the default intent changes to relative instead of perceptual, or if an optional parameter is provided to select the preferred intent for CMYK transformations.

  2. Moves the negate operation to the bottom of the pipeline, preventing colourspace transformations to be applied to already negated image data resulting in output colors being incorrect.

  3. Adds a test that validates (2) by asserting similarity with expected color values when negating in combination with a CMYK colourspace transformation.

Currently, all tests pass except for negate (png, trans). This seems related to premultiplication of the alpha channel as commenting out the resize operation of the test results in the test passing.

@lovell I'm happy to implement any changes to ensure the test passes. Can you give me some pointers on how to tackle negating in combination with premultiplication of the alpha channel if present?

…ion with embedded profiles + move negate to bottom of the pipeline and add test to validate output colors when doing cmyk to cmyk conversion in combination with `negate`
@lovell
Copy link
Owner

lovell commented May 9, 2024

Thanks for working on this Adriaan, always good to have more testing around CMYK logic.

I took a look at the failing test and I think the current expectation is wrong, and that your change produces the correct output, so please go ahead and update the negate-trans.png file as part of this PR.

Do you know the source of the U.S. Web Coated (SWOP) v2.icc file? I ask as it might be subject to copyright. If the ownership/licensing is unclear, perhaps one of the profiles in the Debian icc-profiles package could be used?

adriaanmeuris added a commit to adriaanmeuris/sharp that referenced this pull request May 13, 2024
adriaanmeuris added a commit to adriaanmeuris/sharp that referenced this pull request May 13, 2024
adriaanmeuris added a commit to adriaanmeuris/sharp that referenced this pull request May 13, 2024
…ion with embedded profiles + move negate to bottom of the pipeline and add test to validate output colors when doing cmyk to cmyk conversion in combination with `negate`

update expected file to match correct output, following lovell#4096 (comment)

use license free cmyk profile, following lovell#4096 (comment)

Add test for validating output colors when doing cmyk to cmyk conversion with embedded profiles + move negate to bottom of the pipeline and add test to validate output colors when doing cmyk to cmyk conversion in combination with `negate`#
@adriaanmeuris
Copy link
Contributor Author

Great, I've updated the expected file as requested and used the XCMYK 2017.icc profile from the Debian package. Let me know if there are any further changes needed.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thanks for the updates.

@lovell lovell merged commit 29336f4 into lovell:main May 13, 2024
15 checks passed
@lovell lovell added this to the v0.33.4 milestone May 13, 2024
lovell added a commit that referenced this pull request May 14, 2024
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