search

apostrophecms / apostrophe

A full-featured, open-source content management framework built with Node.js that empowers organizations by combining in-context editing and headless architecture in a full-stack JS environment.
https://apostrophecms.com
Other
4.36k stars 590 forks source link

HEIF support so iPhone/iPad owners can naively upload to Apostrophe #1349

Closed boutell closed 4 years ago

boutell commented 6 years ago

A heads up: iphones now snap by default in HEIF format. People uploading naively from iphones/ipads to Apostrophe will have trouble as a result until we figure out how to handle HEIF in uploadfs. Since most browsers are going to go “whuh?” I would recommend converting it. One wonders if there is even an open source toolchain with a suitable license for converting it.

Since we don't have a mobile UI yet this is mostly an issue after they sync that photo to some other device for now, or on sites that have frontend code that accepts submissions and utilizes apostrophe-attachments (which is pretty common).

boutell commented 5 years ago

There's middleware for this - by me as it happens:

https://github.com/boutell/heic-to-jpeg-middleware

It would certainly work in ApostropheCMS, not sure if the demand is there.

boutell commented 5 years ago

Actually pretty sure this is a good idea. Tagging this as a good first issue.

Here's the hint: the file lib/modules/apostrophe-attachments/lib/routes.js contains the upload route. This route already takes one middleware function. Just add another. That middleware function can get registered in the apos.middleware object in lib/modules/apostrophe-express/index.js. And the middleware function you want is the one in my npm module heic-to-jpeg-middleware.

glmaljkovich commented 4 years ago

Hello 👋, is this still an issue? If so, I would like to help.

boutell commented 4 years ago

[Has to check]

... Yep! Sure is. Feel free to dive in.

glmaljkovich commented 4 years ago

Hey, I'm finally getting into this one.

I see https://github.com/boutell/heic-to-jpeg-middleware requires https://github.com/monostream/tifig which has a tedious setup process that I assume we would have to add as part of the steps a user needs to do when installing apostrophe.

Would it make sense to first update https://github.com/boutell/heic-to-jpeg-middleware to use ImageMagick (I can open a PR there too)?

It seems like a trivial change and we would only have to change the minimum supported version of image magick in the docs.

Here's an example of using imagemagick to do the conversion https://zwbetz.com/convert-heic-images-to-jpg/

boutell commented 4 years ago

Indeed, that would make more sense. It should gracefully do nothing if the version of mogrify is too old or mogrify is absent. Perhaps log a warning if the file is indeed HEIC and mogrify is absent or too old. For performance checking the mogrify version could be done just once and remembered for the lifetime of the process.

On Thu, May 28, 2020 at 3:09 PM Gabriel L. Maljkovich < notifications@github.com> wrote:

Hey, I'm finally getting into this one.

I see https://github.com/boutell/heic-to-jpeg-middleware requires https://github.com/monostream/tifig which has a tedious setup process that I assume we would have to add as part of the steps a user needs to do when installing apostrophe.

Would it make sense to first update https://github.com/boutell/heic-to-jpeg-middleware to use ImageMagick (I can open a PR there too)?

It seems like a trivial change and we would only have to change the minimum supported version of image magick in the docs.

Here's an example of using imagemagick to do the conversion https://zwbetz.com/convert-heic-images-to-jpg/

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/1349#issuecomment-635540426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27JLQTCEHRWHKV3IZLTRT2ZH7ANCNFSM4E56H2CA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 4 years ago

(We should probably document whether the standard version of mogrify installed by the imagemagick packages of CentOS 7 or current Ubuntu is good enough, or the developer has to install newer things)

On Thu, May 28, 2020 at 3:45 PM Tom Boutell tom@apostrophecms.com wrote:

Indeed, that would make more sense. It should gracefully do nothing if the version of mogrify is too old or mogrify is absent. Perhaps log a warning if the file is indeed HEIC and mogrify is absent or too old. For performance checking the mogrify version could be done just once and remembered for the lifetime of the process.

On Thu, May 28, 2020 at 3:09 PM Gabriel L. Maljkovich < notifications@github.com> wrote:

Hey, I'm finally getting into this one.

I see https://github.com/boutell/heic-to-jpeg-middleware requires https://github.com/monostream/tifig which has a tedious setup process that I assume we would have to add as part of the steps a user needs to do when installing apostrophe.

Would it make sense to first update https://github.com/boutell/heic-to-jpeg-middleware to use ImageMagick (I can open a PR there too)?

It seems like a trivial change and we would only have to change the minimum supported version of image magick in the docs.

Here's an example of using imagemagick to do the conversion https://zwbetz.com/convert-heic-images-to-jpg/

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/1349#issuecomment-635540426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27JLQTCEHRWHKV3IZLTRT2ZH7ANCNFSM4E56H2CA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

glmaljkovich commented 4 years ago

I opened https://github.com/boutell/heic-to-jpeg-middleware/issues/2 to keep track of this

glmaljkovich commented 4 years ago

It seems both CentOS 7 and Ubuntu 20.04 have outdated ImageMagick versions:

boutell commented 4 years ago

Well, that's a bummer (: I think this middleware should be behind an option flag in apostrophe for now.

On Fri, May 29, 2020 at 5:16 PM Gabriel L. Maljkovich < notifications@github.com> wrote:

It seems both CentOS 7 and Ubuntu 20.04 have outdated ImageMagick versions:

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/1349#issuecomment-636195606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27PLADUS3BIY27OEJ5LRUAQZRANCNFSM4E56H2CA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

glmaljkovich commented 4 years ago

Sure, just let me know how you handle feature flags on apostrophe (env variables, config files, etc) and I'll add it.

As an alternative to using ImageMagick there are:

But if you feel more comfortable with ImageMagick let me know and I'll get started with it.

boutell commented 4 years ago

I'm open to a better way! Browser side is intriguing but would be a lot to cram in at this point in Apostrophe 2.x's lifecycle. I'd say the server side which can be used to upgrade the middleware. How does heic-convert look to you?

On Thu, Jun 4, 2020 at 2:05 PM Gabriel L. Maljkovich < notifications@github.com> wrote:

Sure, just let me know how you handle feature flags on apostrophe (env variables, config files, etc) and I'll add it.

As an alternative to using ImageMagick there are:

But if you feel more comfortable with ImageMagick let me know and I'll get started with it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/1349#issuecomment-639015964, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27IT57D6KDT6X34F2ODRU7O7XANCNFSM4E56H2CA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

glmaljkovich commented 4 years ago

It looks pretty good and works out of the box. My only concern is the warning at the end of their readme:

Note that while the converter returns a Promise and is overall asynchronous, a lot of work is still done synchronously, so you should consider using a worker thread in order to not block the main thread in highly concurrent production environments.

So I did a few tests:

So it's true what they say that most of the work is thread blocking.

It all boils down to how often images get uploaded on a typical apostrophe instance. If a lot of images get uploaded concurrently, you're probably going to block requests for a few seconds. An alternative is to spawn a child process but that can be tricky.

glmaljkovich commented 4 years ago

Sorry, I meant worker thread not child process https://nodesource.com/blog/worker-threads-nodejs/

boutell commented 4 years ago

Spawning a child process is actually not so bad.

child_process.spawn could be reasonable to use in this case. You can pass the work to be done on the command line.

This improves the chances the work is offloaded to a separate core entirely and generally avoids the portability challenges of threads.

On Thu, Jun 4, 2020 at 3:43 PM Gabriel L. Maljkovich < notifications@github.com> wrote:

Sorry, I meant worker thread not child process https://nodesource.com/blog/worker-threads-nodejs/

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/1349#issuecomment-639076022, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27MIHO4Q7OT5GWKE33LRU72OHANCNFSM4E56H2CA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 4 years ago

Note that in this case it's similar to invoking the CLI tools we talked about before but instead you're invoking a node module. You'd need to start that command line with the node executable though, hmm. child_process.fork might be best because it finds that for you.

On Fri, Jun 5, 2020 at 8:58 AM Tom Boutell tom@apostrophecms.com wrote:

Spawning a child process is actually not so bad.

child_process.spawn could be reasonable to use in this case. You can pass the work to be done on the command line.

This improves the chances the work is offloaded to a separate core entirely and generally avoids the portability challenges of threads.

On Thu, Jun 4, 2020 at 3:43 PM Gabriel L. Maljkovich < notifications@github.com> wrote:

Sorry, I meant worker thread not child process https://nodesource.com/blog/worker-threads-nodejs/

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/1349#issuecomment-639076022, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27MIHO4Q7OT5GWKE33LRU72OHANCNFSM4E56H2CA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his