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 the option to provide a specific background colour to trim #3332

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

mart-jansink
Copy link
Contributor

I've got a use case to trim just a specific background colour of an image. Mostly to detect letter and pillar boxes in images by trimming off any black areas. This pull request adds the option to do so, by providing this colour as a string or object, which will be parsed by the color module.

For me, it'd be more typical to provide a specific colour and use the default threshold, so I made the former the first optional argument. However, the Javascript side will handle the case where a threshold is provided as the first argument to retain backwards compatibility. I'm not sure if I made it fully clear in the docs that both arguments are optional, maybe you can improve my phrasing if you agree?

The output of the color module is always 8-bit, so it's scaled accordingly if the image is 16-bit. I've noticed that the threshold option is used as an absolute value in find_trim instead of being scaled relatively to the interpretation, therefore that's now multiplied as well.

Instead of .flatten()ing the background color if the image HasAlpha(), the alpha value is now simply discarded. I believe that this should give the same result because of how find_trim uses the same background color when it flattens the image. At least all existing unit tests still pass.

Finally, I've based this pull request on the eagle branch because e0d3c6e and cbf741c affect the same part of the code. And I also encountered some failing tests when running the main branche on my M1 MacBook Pro.

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.

Thank you very much for the PR, this is a welcome addition, I've left a couple of questions/comments inline.

src/pipeline.cc Outdated Show resolved Hide resolved
test/unit/trim.js Show resolved Hide resolved
lib/resize.js Outdated Show resolved Hide resolved
lib/resize.js Show resolved Hide resolved
@mart-jansink
Copy link
Contributor Author

mart-jansink commented Aug 23, 2022

Could you please have another look? I've modified the pull request according to your suggestions.

Also, it now allows the threshold to be zero to trim just the pixels with the exact same color. I couldn't think of any reason why this shouldn't work. Of course I'll revert it if there was one, but https://github.com/libvips/libvips/blob/f4a3c7eb5478ee77bd206353308eab3b02db923a/libvips/arithmetic/find_trim.c#L193 seems to be fine with anything between 0 and INFINITY inclusive.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 526b7e2 on mart-jansink:trim-specific-colour into 3a44748 on lovell:eagle.

@lovell lovell merged commit c3a852e into lovell:eagle Aug 23, 2022
@lovell
Copy link
Owner

lovell commented Aug 23, 2022

Dankjewel Mart!

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.

3 participants