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

keepIccProfile doesn't retain profile on cmyk image #3906

Closed
3 tasks done
martinj opened this issue Dec 19, 2023 · 11 comments
Closed
3 tasks done

keepIccProfile doesn't retain profile on cmyk image #3906

martinj opened this issue Dec 19, 2023 · 11 comments

Comments

@martinj
Copy link

martinj commented Dec 19, 2023

Possible bug

Is this a possible bug in a feature of sharp, unrelated to installation?

  • Running npm install sharp completes without error.
  • Running node -e "require('sharp')" completes without error.

If you cannot confirm both of these, please open an installation issue instead.

Are you using the latest version of sharp?

  • I am using the latest version of sharp as reported by npm view sharp dist-tags.latest.

If you cannot confirm this, please upgrade to the latest version and try again before opening an issue.

If you are using another package which depends on a version of sharp that is not the latest, please open an issue against that package instead.

What is the output of running npx envinfo --binaries --system --npmPackages=sharp --npmGlobalPackages=sharp?

  System:
    OS: macOS 13.4.1
    CPU: (12) arm64 Apple M2 Max
    Memory: 16.30 GB / 96.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node
    npm: 10.2.3 - ~/.nvm/versions/node/v20.10.0/bin/npm
  npmPackages:
    sharp: ^0.33.1 => 0.33.1 

What are the steps to reproduce?

Load an cmyk image with embedded icc profile. Try to do anything with it using keepIccProfile and no profile is in the output.
What I'm actually want to achieve is to resize an image with cmyk where output is cmyk and icc profile is retained.

What is the expected behaviour?

That it retains the icc profile and output color space is cmyk.

Please provide a minimal, standalone code sample, without other dependencies, that demonstrates this problem

const sharp = require('sharp');
sharp('Channel_digital_image_CMYK_color.jpg')
	.keepIccProfile()
        .toColorspace('cmyk')
	.toBuffer()
	.then((data) => sharp(data).metadata())
	.then((metadata) => console.log('ICC', metadata.icc))
	.catch((err) => console.error(err));

Please provide sample image(s) that help explain this problem

https://upload.wikimedia.org/wikipedia/en/2/25/Channel_digital_image_CMYK_color.jpg

@martinj martinj added the triage label Dec 19, 2023
@lovell lovell added question and removed triage labels Dec 19, 2023
@lovell
Copy link
Owner

lovell commented Dec 19, 2023

Have you tried setting the pipeline colourspace to ensure the image remains in CMYK throughout?

 .pipelineColourspace('cmyk')

https://sharp.pixelplumbing.com/api-colour#pipelinecolourspace

@martinj
Copy link
Author

martinj commented Dec 19, 2023

Yes, tried several combinations both with pipelineColorspace and keepMetadata as well.. but it always seems to comeout as srgb and without the icc profile.

@rambo-panda
Copy link

When using the keepMetadata method, similar situations are also encountered

// test code
const sharp = require("sharp");
(async () => {
  console.log('origin metadata', await sharp("./cmyk.jpg").metadata());
  console.log('new metadata use toColorspace', await sharp(await sharp("./cmyk.jpg").toColorspace("cmyk").keepMetadata().toBuffer()).metadata());
  console.log('new metadata use pipelineColorspace', await sharp(await sharp("./cmyk.jpg").pipelineColorspace("cmyk").keepMetadata().toBuffer()).metadata());
})();

``

```js
origin metadata {
  format: 'jpeg',
  width: 500,
  height: 333,
  space: 'cmyk',
  channels: 4,
  depth: 'uchar',
  density: 180,
  chromaSubsampling: '4:4:4:4',
  isProgressive: false,
  resolutionUnit: 'inch',
  hasProfile: true,
  hasAlpha: false,
  orientation: 1,
  exif: <Buffer 45 78 69 66 00 00 49 49 2a 00 08 00 00 00 0a 00 0f 01 02 00 06 00 00 00 86 00 00 00 10 01 02 00 17 00 00 00 8c 00 00 00 12 01 03 00 01 00 00 00 01 00 ... 9492 more bytes>,
  icc: <Buffer 00 08 80 70 41 44 42 45 02 10 00 00 70 72 74 72 43 4d 59 4b 4c 61 62 20 07 d0 00 07 00 1a 00 05 00 29 00 35 61 63 73 70 41 50 50 4c 00 00 00 00 41 44 ... 557118 more bytes>,
  iptc: <Buffer 50 68 6f 74 6f 73 68 6f 70 20 33 2e 30 00 38 42 49 4d 04 25 00 00 00 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 38 42 49 4d 03 ed 00 00 ... 9948 more bytes>,
  xmp: <Buffer 3c 3f 78 70 61 63 6b 65 74 20 62 65 67 69 6e 3d 22 ef bb bf 22 20 69 64 3d 22 57 35 4d 30 4d 70 43 65 68 69 48 7a 72 65 53 7a 4e 54 63 7a 6b 63 39 64 ... 16899 more bytes>
}
new metadata use toColorspace {
  format: 'jpeg',
  size: 93328,
  width: 500,
  height: 333,
  space: 'srgb',
  channels: 3,
  depth: 'uchar',
  density: 180,
  chromaSubsampling: '4:2:0',
  isProgressive: false,
  resolutionUnit: 'inch',
  hasProfile: false,
  hasAlpha: false,
  orientation: 1,
  exif: <Buffer 45 78 69 66 00 00 49 49 2a 00 08 00 00 00 0a 00 0f 01 02 00 06 00 00 00 86 00 00 00 10 01 02 00 17 00 00 00 8c 00 00 00 12 01 03 00 01 00 00 00 01 00 ... 9490 more bytes>,
  iptc: <Buffer 50 68 6f 74 6f 73 68 6f 70 20 33 2e 30 00 38 42 49 4d 04 25 00 00 00 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 38 42 49 4d 03 ed 00 00 ... 9948 more bytes>,
  xmp: <Buffer 3c 3f 78 70 61 63 6b 65 74 20 62 65 67 69 6e 3d 22 ef bb bf 22 20 69 64 3d 22 57 35 4d 30 4d 70 43 65 68 69 48 7a 72 65 53 7a 4e 54 63 7a 6b 63 39 64 ... 16899 more bytes>
}
new metadata use pipelineColorspace {
  format: 'jpeg',
  size: 92010,
  width: 500,
  height: 333,
  space: 'srgb',
  channels: 3,
  depth: 'uchar',
  density: 180,
  chromaSubsampling: '4:2:0',
  isProgressive: false,
  resolutionUnit: 'inch',
  hasProfile: false,
  hasAlpha: false,
  orientation: 1,
  exif: <Buffer 45 78 69 66 00 00 49 49 2a 00 08 00 00 00 0a 00 0f 01 02 00 06 00 00 00 86 00 00 00 10 01 02 00 17 00 00 00 8c 00 00 00 12 01 03 00 01 00 00 00 01 00 ... 9490 more bytes>,
  iptc: <Buffer 50 68 6f 74 6f 73 68 6f 70 20 33 2e 30 00 38 42 49 4d 04 25 00 00 00 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 38 42 49 4d 03 ed 00 00 ... 9948 more bytes>,
  xmp: <Buffer 3c 3f 78 70 61 63 6b 65 74 20 62 65 67 69 6e 3d 22 ef bb bf 22 20 69 64 3d 22 57 35 4d 30 4d 70 43 65 68 69 48 7a 72 65 53 7a 4e 54 63 7a 6b 63 39 64 ... 16899 more bytes>
}

the space is not cmyk

@aamirsdev
Copy link

Do we have any update on this. I am also facing kind of similiar issue. Where we have an image which is cmyk and when I am trying to process it using sharp it is changing the color space. And my image is not having the icc profile in it.

@adriaanmeuris
Copy link
Contributor

The test keep existing ICC profile seems to test for a Generic RGB Profile ICC profile in the output file.

Confirmed that the current implementation of keepIccProfile won't work for CMYK profiles with following test:

it('keep existing CMYK ICC profile', async () => {
  const data = await sharp(fixtures.inputJpgWithCmykProfile)
    .keepIccProfile()
    .toBuffer();

  const metadata = await sharp(data).metadata();
  const { description } = icc.parse(metadata.icc);
  assert.strictEqual(description, 'U.S. Web Coated (SWOP) v2');
});

Taking a look at the pipeline, it seems this fails because it converts to sRGB/P3 using the embedded profile:

sharp/src/pipeline.cc

Lines 330 to 352 in 26d0b71

if (
sharp::HasProfile(image) &&
image.interpretation() != VIPS_INTERPRETATION_LABS &&
image.interpretation() != VIPS_INTERPRETATION_GREY16 &&
!baton->input->ignoreIcc
) {
// Convert to sRGB/P3 using embedded profile
try {
image = image.icc_transform(processingProfile, VImage::option()
->set("embedded", TRUE)
->set("depth", sharp::Is16Bit(image.interpretation()) ? 16 : 8)
->set("intent", VIPS_INTENT_PERCEPTUAL));
} catch(...) {
sharp::VipsWarningCallback(nullptr, G_LOG_LEVEL_WARNING, "Invalid embedded profile", nullptr);
}
} else if (
image.interpretation() == VIPS_INTERPRETATION_CMYK &&
baton->colourspaceInput != VIPS_INTERPRETATION_CMYK
) {
image = image.icc_transform(processingProfile, VImage::option()
->set("input_profile", "cmyk")
->set("intent", VIPS_INTENT_PERCEPTUAL));
}

Updating that code to the following fixes the issue:

// Convert to sRGB/P3 using embedded profile
try {
  if ((baton->keepMetadata & VIPS_FOREIGN_KEEP_ICC) && baton->withIccProfile.empty()) {
    // New implementation
    if (image.interpretation() == VIPS_INTERPRETATION_CMYK) {
      baton->colourspace = VIPS_INTERPRETATION_CMYK;
    }

    image = image.icc_transform(inputProfile.first, VImage::option()
      ->set("input_profile", inputProfile.first)
      ->set("embedded", TRUE)
      ->set("depth", sharp::Is16Bit(image.interpretation()) ? 16 : 8)
      ->set("intent", VIPS_INTENT_PERCEPTUAL));

  } else {
    // Current implementation
    image = image.icc_transform(processingProfile, VImage::option()
      ->set("embedded", TRUE)
      ->set("depth", sharp::Is16Bit(image.interpretation()) ? 16 : 8)
      ->set("intent", VIPS_INTENT_PERCEPTUAL));
  }
} catch(...) {
  sharp::VipsWarningCallback(nullptr, G_LOG_LEVEL_WARNING, "Invalid embedded profile", nullptr);
}

@lovell All tests, including the new one for preserving a CMYK profile, pass successfully. I am uncertain if this adjustment is located in the most suitable part of the codebase though, as I have a limited understanding of C and the specific workings of ICC tranformations in the pipeline.

I'm willing to work on a preliminary PR if that helps facilitate a more detailed review and discussion on how best to integrate these changes.

@lovell
Copy link
Owner

lovell commented Feb 11, 2024

The change in commit fb70fbb prevents the erroneous sRGB transformation when using CMYK everywhere, plus adds a slightly-modified version of @adriaanmeuris' example test case to help prevent regression. Thanks all for reporting and helping analyse this issue.

@adriaanmeuris
Copy link
Contributor

@lovell thank you so much for the recent update; I noticed that the profile is correcly attached now.

However, it seems this icc transform to sRGB is always happening:

sharp/src/pipeline.cc

Lines 330 to 344 in 60f4048

if (
sharp::HasProfile(image) &&
image.interpretation() != VIPS_INTERPRETATION_LABS &&
image.interpretation() != VIPS_INTERPRETATION_GREY16 &&
!baton->input->ignoreIcc
) {
// Convert to sRGB/P3 using embedded profile
try {
image = image.icc_transform(processingProfile, VImage::option()
->set("embedded", TRUE)
->set("depth", sharp::Is16Bit(image.interpretation()) ? 16 : 8)
->set("intent", VIPS_INTENT_PERCEPTUAL));
} catch(...) {
sharp::VipsWarningCallback(nullptr, G_LOG_LEVEL_WARNING, "Invalid embedded profile", nullptr);
}

Which results in a color shift. Commenting out the icc_transform solves this. Here's a comparison visual to illustrate the difference:

comparison

Adding this condition to the if statement instead of commenting out the icc_transform also fixed this, but it does result in the test tints cmyk image red to fail.

image.interpretation() != VIPS_INTERPRETATION_CMYK &&

Additionally, I found that to maintain the color profile, it's essential to use .pipelineColourspace('cmyk') and .toColourspace('cmyk'). I noticed that using await sharp(inFile).keepIccProfile().toFile(outFile); still results in a conversion to sRGB, despite the keepIccProfile option being set. Is this the expected behavior?

I'm happy to help out or provide additional information if needed.

@lovell
Copy link
Owner

lovell commented Mar 6, 2024

@adriaanmeuris Are you able to test to see if the following patch fixes the problem you're seeing?

--- a/src/pipeline.cc
+++ b/src/pipeline.cc
@@ -331,6 +331,7 @@ class PipelineWorker : public Napi::AsyncWorker {
         sharp::HasProfile(image) &&
         image.interpretation() != VIPS_INTERPRETATION_LABS &&
         image.interpretation() != VIPS_INTERPRETATION_GREY16 &&
+        baton->colourspaceInput != VIPS_INTERPRETATION_CMYK &&
         !baton->input->ignoreIcc
       ) {
         // Convert to sRGB/P3 using embedded profile

(I need to refactor some of the variable naming in here as colourspaceInput should really be called something like colourspacePipeline instead.)

@adriaanmeuris
Copy link
Contributor

@lovell Yes, the patch works as expected. The profile is attached, and the output colors now align with the input colors. Looks great!

@lovell
Copy link
Owner

lovell commented Mar 6, 2024

@adriaanmeuris Great, thanks for confirming, added via commit 3eeaee7

@lovell
Copy link
Owner

lovell commented Mar 23, 2024

v0.33.3 now available, thanks everyone for your help fixing/testing this.

@lovell lovell closed this as completed Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants