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

Implement brightness, saturation and hue modulation #609 #1601

Merged
merged 16 commits into from Mar 25, 2019

Conversation

Projects
None yet
3 participants
@Goues
Copy link
Contributor

commented Mar 5, 2019

Hello, I've read about feature request #609 because this is what I would like to be able to do too. I have no experience in C/C++ but I followed all advice I could find to stitch those changes.

Can you please look at it and point me to all the missing pieces I should do as well?

Thank you very much!
Goues

@Goues Goues force-pushed the Goues:feature-modulate branch from 246abdd to f750f1b Mar 5, 2019

@Goues

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

p.s. Unit tests are not passing, because it claims that output is not similar enough to expected output, should I just set a higher threshold?

@coveralls

This comment has been minimized.

Copy link

commented Mar 6, 2019

Coverage Status

Coverage increased (+0.03%) to 98.522% when pulling a90d8b6 on Goues:feature-modulate into 18afcf5 on lovell:master.

@lovell

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2019

Thank you very much for this PR Jakub. It looks good from a quick scan and I expect to have some time to take a proper look at it next week.

It's always pleasure to see tests - perhaps try assertMaxColourDistance instead of assertSimilar as this feature is all about colours, plus that might avoid some of those threshold problems you mentioned.

There should probably also be some verification in the JavaScript to throw Errors for things like invalid property values (there are a few examples of this in other operations).

@lovell lovell added the enhancement label Mar 6, 2019

@Goues

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

I've updated to include input validation and error throwing. Also, I made it work with PNG with alpha channel, which wasn't the case in the first implementation.

Looks like I need some help with actually making test assertions right.

@lovell
Copy link
Owner

left a comment

Thanks again for the PR Jakub and apologies for the delay getting around to reviewing this properly.

I've left a couple of comments inline around input validation.

I think the remaining test failures can be resolved by switching to use the lossless PNG format. (There are slight variations in the lossy JPEG decoder on different platforms.)

Show resolved Hide resolved lib/operation.js
if (is.number(options.saturation) && options.saturation >= 0) {
this.options.saturation = options.saturation;
} else {
throw new Error('Invalid saturation ' + options.brightness);

This comment has been minimized.

Copy link
@lovell

lovell Mar 17, 2019

Owner

I think this error message should refer to options.saturation (ditto for the hue message below). There's an is.invalidParameterError helper function that might be useful here too.

@@ -309,6 +309,20 @@ namespace sharp {
0.0, 0.0, 0.0, 1.0));
}

VImage Modulate(VImage image, double const brightness, double const saturation, double const hue) {
image = image.colourspace(VIPS_INTERPRETATION_LCH);

This comment has been minimized.

Copy link
@lovell

lovell Mar 17, 2019

Owner

It might be slightly more efficient to perform the colourspace conversion after the removal of alpha channel.

Show resolved Hide resolved src/operations.cc Outdated

@Goues Goues force-pushed the Goues:feature-modulate branch from 77d156e to b89d4b9 Mar 19, 2019

@Goues

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Thank you for the review, I've updated the PR and rebased over master.

Goues added some commits Mar 19, 2019

@lovell

This comment has been minimized.

Copy link
Owner

commented Mar 19, 2019

Thanks for all the updates, this is looking great. Were you able to experiment with using PNG input (and output) for the tests to avoid having to set a threshold?

@lovell lovell added this to the v0.22.1 milestone Mar 19, 2019

@Goues

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Yes, I added this file for testing hue rotation https://github.com/lovell/sharp/blob/bed50294bd59fb5f1a22c1edbd98414c85f733c6/test/fixtures/test-pattern.png with hue from 30 to 360 by steps of 30degs. It came exactly as css filter: hue-rotate(30deg) does, because I was testing all those three transforms against css filters.

Also I had this branch in my app for about a week already and QA didn't find anything wrong with uploaded images.

@lovell
Copy link
Owner

left a comment

One small change to make hue integral rather than floating-point and this will be good to merge, thank you.

if (is.number(options.hue)) {
this.options.hue = options.hue % 360;
} else {
throw is.invalidParameterError('saturation', 'number', options.saturation);

This comment has been minimized.

Copy link
@lovell

lovell Mar 24, 2019

Owner

The validation error for hue incorrectly refers to saturation.

}
}
if ('hue' in options) {
if (is.number(options.hue)) {

This comment has been minimized.

Copy link
@lovell

lovell Mar 24, 2019

Owner

I think hue needs to be an integer value (can use is.integer check for this) as it is additive.

@@ -87,6 +87,9 @@ struct PipelineBaton {
std::vector<double> flattenBackground;
bool negate;
double blurSigma;
double brightness;
double saturation;
double hue;

This comment has been minimized.

Copy link
@lovell

lovell Mar 24, 2019

Owner

I think hue can be a int (signed, as negative values are acceptable).

This comment has been minimized.

Copy link
@Goues

Goues Mar 24, 2019

Author Contributor

I actally started out with hue as an integer, but I don't know how to make it work in C type system. linear expects vectors with doubles, so it fails with: error: non-constant-expression cannot be narrowed from type 'int' to 'double' in initializer list [-Wc++11-narrowing]

Should I somehow convert int back to double in operations.cc?

This comment has been minimized.

Copy link
@lovell

lovell Mar 24, 2019

Owner

Try something like {0.0, 0.0, static_cast<double>(hue)} in the call to linear(), which the compiler should be able to use to initialise a std::vector<double>.

Goues added some commits Mar 24, 2019

@lovell lovell merged commit b494b2e into lovell:master Mar 25, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 98.522%
Details
@lovell

This comment has been minimized.

Copy link
Owner

commented Mar 25, 2019

Fantastic, thanks again for this Jakub, it will be in v0.22.1.

lovell added a commit that referenced this pull request Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.