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 option to change number of threads for imagemagick convert #6210

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Add option to change number of threads for imagemagick convert #6210

merged 3 commits into from
Feb 1, 2024

Conversation

caplod
Copy link
Contributor

@caplod caplod commented Jan 26, 2024

This PR …

If you are converting a lot of big images with imagemagick it can get really slow.

This PR adds an option to configure the threads imagemagick can use. (default is 1)
So you can benefit from capable hardware.

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@distantnative distantnative added this to the 4.2.0 milestone Jan 26, 2024
Copy link
Member

@bastianallgeier bastianallgeier left a comment

Choose a reason for hiding this comment

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

I know that the option in imagemagick is called thread. But I feel like the option should still be called threads. @distantnative @lukasbestle could this be something that would actually also help for @Iskris' issue?

@caplod
Copy link
Contributor Author

caplod commented Jan 30, 2024

Yeah, I thought that too. I changed the name of the option into 'threads'

Copy link
Member

@bastianallgeier bastianallgeier left a comment

Choose a reason for hiding this comment

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

I'm happy with it. But it would be good to get a second opinion (@distantnative @afbora @lukasbestle)

@distantnative distantnative requested a review from a team January 30, 2024 19:06
Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

Makes a lot of sense to me. I think the original limit was added for shared hosting setups, but if the hardware is available, using multithreading can indeed improve the performance a lot.

@bastianallgeier bastianallgeier merged commit 589e5bf into getkirby:develop-minor Feb 1, 2024
7 checks passed
@bastianallgeier
Copy link
Member

Thanks for your help @caplod We will make sure to mention you in the release notes.

@caplod caplod deleted the feature/imagemagick-thread branch February 5, 2024 07:33
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.

None yet

4 participants