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 blur precision #4168

Merged
merged 1 commit into from
Jul 20, 2024
Merged

Add blur precision #4168

merged 1 commit into from
Jul 20, 2024

Conversation

marcosc90
Copy link
Contributor

This PR adds precision option to .blur().

Currently, using blur on large images is quite slow. In my case, setting precision to approximate significantly improves performance, reducing the processing time from over a minute to just a few seconds.

@kleisauke
Copy link
Contributor

FWIW, I'm uncertain about exposing VIPS_PRECISION_APPROXIMATE because it's not production-quality and remains incomplete, see e.g. issue libvips/libvips#1162.

@marcosc90
Copy link
Contributor Author

FWIW, I'm uncertain about exposing VIPS_PRECISION_APPROXIMATE because it's not production-quality and remains incomplete, see e.g. issue libvips/libvips#1162.

That's unfortunate, maybe I could remove approximate from the docs and expose it only for advanced users?

@lovell
Copy link
Owner

lovell commented Jul 18, 2024

Thank you very much for the PR Marcos.

It looks like libvips/libvips#1162 relates to (u)short images processed by vips_conv /vips_conva, but Gaussian blur uses a (faster) separable matrix via vips_convsep / vips_convasep, so might be unaffected.

I guess as a compromise we could limit the use of the approximate value to uchar images, or force-cast to uchar?

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.

Once we're happy with the API for this, please can you update the TypeScript definitions at https://github.com/lovell/sharp/blob/main/lib/index.d.ts

lib/operation.js Outdated Show resolved Hide resolved
lib/operation.js Outdated Show resolved Hide resolved
@marcosc90
Copy link
Contributor Author

Thank you very much for the PR Marcos.

It looks like libvips/libvips#1162 relates to (u)short images processed by vips_conv /vips_conva, but Gaussian blur uses a (faster) separable matrix via vips_convsep / vips_convasep, so might be unaffected.

I guess as a compromise we could limit the use of the approximate value to uchar images, or force-cast to uchar?

You're welcome! Would you prefer to fall back to integer in case approximate is passed for ushort images, or throw an error?

lib/operation.js Outdated Show resolved Hide resolved
@kleisauke
Copy link
Contributor

It looks like libvips/libvips#1162 relates to (u)short images processed by vips_conv /vips_conva, but Gaussian blur uses a (faster) separable matrix via vips_convsep / vips_convasep, so might be unaffected.

I guess as a compromise we could limit the use of the approximate value to uchar images, or force-cast to uchar?

Good catch, vips_convasep() is indeed not affected.

I opened PR libvips/libvips#4055 to address the crash in vips_conva() that occurs with {u,}{short,int} images, so ignore my previous comment.

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 otherwise this is good to go.

test/unit/blur.js Outdated Show resolved Hide resolved
test/unit/blur.js Outdated Show resolved Hide resolved
test/unit/blur.js Outdated Show resolved Hide resolved
test/unit/blur.js Show resolved Hide resolved
@lovell lovell merged commit 67a4592 into lovell:main Jul 20, 2024
15 checks passed
@lovell lovell added this to the v0.33.5 milestone Jul 20, 2024
@lovell
Copy link
Owner

lovell commented Jul 20, 2024

Thank you very much for working on this Marcos.

lovell added a commit that referenced this pull request Jul 20, 2024
@marcosc90 marcosc90 deleted the blur-precision branch July 20, 2024 14:40
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