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

Use heic-convert npm package to process images #3

Merged
merged 5 commits into from Jun 18, 2020

Conversation

glmaljkovich
Copy link

@glmaljkovich glmaljkovich commented Jun 12, 2020

Spawn one child process per request and convert images sequentially, avoiding blocking the main thread.

Note that there are many possible ways to use child processes (worker.js instances) here:

  • One worker per image
  • A singleton worker shared across requests
  • A worker pool.

In the end I ended up using one worker per request because I think it's flexible enough to scale with the server load but without unnecessarily straining the cpu like one worker per image would.

For a comparison of how those different approaches work (except for the worker pool) you can try the benchmark_test.js file on this branch


closes #2

Gabriel L. Maljkovich added 2 commits June 11, 2020 18:27
- Spawn one child process per request and convert images sequentially
@glmaljkovich
Copy link
Author

@boutell I could use some help testing this with apostrophe. I believe I have to:

  • npm link the middleware to a copy of apostrophe that uses the middleware
  • npm link that copy of apostrophe to a project made with the apostrophe cli

Correct me if I'm wrong.

Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

This looks great to me! As for integration testing with apostrophe, the easiest way would be to npm link both this module and apostrophe in your project, and patch lib/modules/apostrophe-attachments/index.js to add this middleware as self.expressMiddleware in that module. I don't mind doing that test if you prefer.

The only "change" I'm requesting is that you update the documentation to address the change (no more need for tifig).

index.js Show resolved Hide resolved
Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

Nice!

Will this build on Windows? Any idea? Will it build if a compiler is not available on Linux?

If we're not sure about those things, or the answers aren't ideal, I can wrap it as an optional dependency in apostrophe, and we'll bump the major version of the middleware because the requirements will have changed. In practice most people will have a compiler available when installing npm packages.

@glmaljkovich
Copy link
Author

Good questions. I was looking at the dependencies from heic-convert and the only use of code that isn't javascript only is this pre-compiled webassembly library: https://github.com/catdad-experiments/libheif-js/blob/master/scripts/install.js#L16

That being said I think it's reasonable to wrap it as an optional dependency as I haven't tested it on windows.

@boutell
Copy link
Member

boutell commented Jun 18, 2020

If it's prebuilt webassembly code windows should be fine. No need for optional dependency.

@boutell boutell merged commit e42af37 into apostrophecms:master Jun 18, 2020
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.

Use heic-convert to do the format conversion
2 participants