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 ICC profile option to withMetadata #2271

Closed
wants to merge 11 commits into from

Conversation

roborourke
Copy link
Contributor

This addresses issue #1323 by adding support for applying a custom ICC profile on output.

While this implements the proposed API in the issue I might be missing some of the cases described still. I'd love to get some feedback on whether this is actually the correct approach (bearing in mind I have basically no C++ experience before this!).

Augments the `withMetadata()` function to accept a `profile` option to provide the path to a custom output colour profile eg. .icc or .icm file.

Related to lovell#1323
@coveralls
Copy link

coveralls commented Jun 26, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 1604d3b on roborourke:withmetadata-profile into c91373f on lovell:master.

@lovell
Copy link
Owner

lovell commented Jun 27, 2020

Hi Robert, thank you for the PR, I'll review it as soon as I can.

Updates the parameter check to `throw` an invalid parameter error in keeping with the orientation check above.
@NhatAnh
Copy link

NhatAnh commented Jul 3, 2020

Hi! Is there an option to use the ICC profile embedded in the image instead of stripping it or set custom ICC profile?

@roborourke
Copy link
Contributor Author

@NhatAnh not with this implementation. It's getting a bit outside of the original scope / intent of this library though I think. Correct me if I'm wrong @lovell

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 again for this PR, I've left a few questions/comments inline.

lib/output.js Outdated Show resolved Hide resolved
lib/output.js Outdated Show resolved Hide resolved
test/unit/metadata.js Outdated Show resolved Hide resolved
test/unit/metadata.js Outdated Show resolved Hide resolved
lib/output.js Outdated Show resolved Hide resolved
- Added CMYK output test
- Updated custom ICC output test to use max colour distance function
@roborourke
Copy link
Contributor Author

Hi @lovell - I made the updates you suggested but still only added one extra test for the cmyk colour space. I'm not sure of other common ones I could add tests for but thought I'd get your feedback on the implementation of the test itself first.

@roborourke roborourke requested a review from lovell July 29, 2020 14:50
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, I've left a couple of comments inline but otherwise this is good to merge.

- `options.orientation` **[number][9]?** value between 1 and 8, used to update the EXIF `Orientation` tag.
- `options.icc` **[string][2]?** path to an ICC profile file
Copy link
Owner

Choose a reason for hiding this comment

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

This content differs from the JSDoc. Was npm run docs-build used to update these?

@@ -131,6 +132,7 @@ function toBuffer (options, callback) {
*
* @param {Object} [options]
* @param {number} [options.orientation] value between 1 and 8, used to update the EXIF `Orientation` tag.
* @param {string} [options.icc] path to output ICC profile.
Copy link
Owner

Choose a reason for hiding this comment

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

Might be worth prefixing with "filesystem" here i.e. filesystem path to...

@lovell
Copy link
Owner

lovell commented Aug 19, 2020

Landed as commit eaecb73, thank you very much for persisting with this Robert. A tea/coffee/beer/relish of your choice next time I'm in Sheffield or you're in London.

@lovell lovell closed this Aug 19, 2020
@lovell lovell added this to the v0.26.0 milestone Aug 19, 2020
@roborourke
Copy link
Contributor Author

@lovell ace! Thanks very much - sorry I’ve been off this week so didn’t update the PR yet. Thanks for getting it over the line. Really appreciate the code review and advice on this feature. Will definitely take you up on that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants