Skip to content

Add Magic Kernel support #4237

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

Merged
merged 5 commits into from
Nov 3, 2024
Merged

Add Magic Kernel support #4237

merged 5 commits into from
Nov 3, 2024

Conversation

akimon658
Copy link
Contributor

Closes #4089

Added support for Magic Kernel Sharp (both 2013 and 2021 version).
Additions in reduceh.cpp are based on this comment:

Mask size is always calculated with this pattern: 2 * rint( mask_range * shrink ) + 1, where the range is 1 for linear, 2 for cubic, 3 for lanczos3.

@kleisauke
Copy link
Member

Thanks for working on this! According to ResampleScope, the 2013 variant seems incorrectly(?) implemented, see:
https://github.com/kleisauke/libvips-pr-4237#mks2013

Any thoughts on why? If not, I'd be glad to take a closer look.

Copy link
Member

@kleisauke kleisauke left a comment

Choose a reason for hiding this comment

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

It looks like the support window of the VIPS_KERNEL_MKS2013 was wrong. For reference, here are the updated ResampleScope graphs:
https://github.com/kleisauke/libvips-pr-4237/tree/after-review#mks2013

@lovell
Copy link
Member

lovell commented Nov 1, 2024

Thank you @akimon658 for the PR. Do you think it's worth including both kernels, or would we be better to focus on the more recent 2021 "edition" only? (In addition, I note this kernel is rather useful for edge detection, so a future possible improvement there also.)

@akimon658
Copy link
Contributor Author

Do you think it's worth including both kernels, or would we be better to focus on the more recent 2021 "edition" only?

Currently, my team is using the 2013 version. I think adding the 2013 edition would be beneficial as it allows us to migrate to libvips without altering application's behavior.
Additionally, it would not significantly increase the maintenance burden on the libvips team.

@jcupitt
Copy link
Member

jcupitt commented Nov 2, 2024

Thanks for doing this work, @akimon658 !

Could you also:

@akimon658
Copy link
Contributor Author

In the ChangeLog file, which version should I put my change under?

@jcupitt
Copy link
Member

jcupitt commented Nov 2, 2024

This is an API addition, so it'll be in the next stable version (8.17). Thanks!

@jcupitt jcupitt merged commit ec0e361 into libvips:master Nov 3, 2024
6 checks passed
@jcupitt
Copy link
Member

jcupitt commented Nov 3, 2024

👍

@j-p-c
Copy link

j-p-c commented Dec 10, 2024

Ah, sorry I missed this—my son Jack just flagged it to me. I would have been happy to provide advice, but it looks like you have it sorted.

@jcupitt
Copy link
Member

jcupitt commented Dec 10, 2024

Hopefully! Thanks for checking @j-p-c

@j-p-c
Copy link

j-p-c commented Dec 10, 2024

Hopefully! Thanks for checking @j-p-c

For extreme clarity: I have not checked the correctness of your implementation; I assume that that has been covered off.

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

Successfully merging this pull request may close these issues.

add magic scaler
5 participants