Jpeg use imagick

[nachoparker]

See nachoparker#1

My main intention is to try to improve performance by using a native method and explore multithreaded native approaches.

I am not necessarily advocating for Imagick, since I know there are security concerns around it, but I wanted to start the conversation since the performance is really bad and throws people off.

This is a PoC, but we could do something like this with some native library that is not GD to start working towards that direction. All being said I was impressed to find out that GD was not that much slower in equal conditions (single threaded and so on), which probably doesn't say much about Imagick.

It would be interesting to try out other libraries too.

[kesselb]

👍 There is a script to update the autoloaders: https://github.com/nextcloud/server/blob/master/build/autoloaderchecker.sh

[kesselb]

Thank you for your research 👍 🚀

[jospoortvliet]

@rullzer your input needed for the security side of it, I suppose...

[phpbg]

Hi,
this is very interesting work.

1. Please don't forget that when uploading files from Web page, client require previews in parallel, so many php-fpm process are spawned, that generally use 100%CPU across all cores.
   In that use case, enabling multi-threading will probably have no effect as you CPU is already on its knees: see Practically useless for performance reasons gallery#458 (comment)
   The real problem here is that generating previews for one image takes around 10s.
   If we can achieve very fast preview generation, we may later think of a limit on the previews cache, to prevent it growing too much

2. During the file upload, two previews are generated (at least on my configuration, which is nextcloud's default): a 256 one, and a max size one.
   The max size is not required here, so it shouldn't be rendered.

3. On the jpeg quality side, do you think we could have a per-size quality setting ?
   I'm not a photo expert, but maybe we want better quality for bigger images...
   E.g.

   • 60 for 32 and 256 sizes
   • 80 for max size

In the end, I guess we need:

• a faster image processing library
• when doing web file upload, generate only one preview, not the max sized one
• a more flexible configuration, with its documentation

[rullzer]

I'm not a big fan of enabling imagick so widely with user input.

I have been playing (but time... you know) of making the preview backend more replacable and plugin capable. So you could for example use some 3rdparty service for preview generation.

Or have more modular in general to also allow generation of more modern types (for browsers that support it etc).

So I'm not against chaing the preview system. But then we should have it more pluggable so this doesn't have to all be fixed in the server.

[nachoparker]

We basically are saying similar things. I don't care what we use but this needs to be improved. User experience is very bad and one of the most popular things people want to use NC for is for their photo collection, then they get instantly turned off. It works awfully.

I wrote down a bunch of ideas while I was looking around this issue. Off the top of my head things that could be done:

• Different qualities for different sizes (same as you suggest)
• Right now all the previews are generated by operating on the max size preview. We could explore using the next bigger size each time to see if the quality is decent enough. Probably this would speed things up. For instance, instead of generating all previews from the 2048x2048 max preview, generate the 512 from the 2048, then the 256 from the 512 and so on

About using a different library... I am fine with that. I do not particularly advocate for imagick but wanted to test out multithreading to see if it would help. I think that a library that can do multithread and a config option would be handy.

I am not inside the project that much to say what needs to be done, but I wanted to start this conversation since the current situation makes NC look pretty bad to users.

Then there is the problem of the NC architecture, which is highly inefficient for the gallery since it has to spawn a thread and load everything including apps for every request, but that would be a separate issue. See #14953

[skjnldsv]

@rullzer we do encourage imagick though, as this is needed for proper avatar generation too :)

[iskradelta]

The security concerns with imagick can be solved, by placing it in a seccomp mode, with only a whitelist of system calls allowed, perhaps even seccomp mode 2 would be enough.

[rullzer]

So. I have been playing with this as well a bit.

So again. I do not like imagick. My approach has been playing with https://github.com/h2non/imaginary which is insanely fast. And nicely sandboxed in docker. (yay!).

The issue right now is that we can use that for the initial preview but for the scaling as pointed out here GD is still used.
@nachoparker so I like the idea you had of splitting the images in different providers so to say.

What I would suggest we do

1. The OCP\IImage class has to much clutter. We have to move this to something better with less functions that make no sense.
2. We have a basic fallback implentation to handle images in server based on GD
3. We have a plugin system where apps can hook in so that we can outsource image manipulation (resize, scale, cut whatever). Then we could have different providers depending on the settings of the user and what they want/need/require.

• For home users that already have that setup most likely just runs out of the box
• h2non/imaginary docker app that uses that so you can fully outsource it
• some other fancy method

Does that sound somewhat sane?

[kesselb]

Does that sound somewhat sane?

Yes 👍 I guess the structural changes will take some time. Adding a way to use imagick for preview generation sounds good to me in the meantime. With some code cleanup I would accept this pr. As long as there is a way to opt out.

[kesselb]

Suggested change

	$imagick_mode = (bool)$this->config->getSystemValue('preview_use_imagick', false);
	$imagick_mode = $this->config->getSystemValueBool('preview_use_imagick', false) && extension_loaded('imagick');

[mailinglists35]

@nachoparker is the PR applying cleanly on 19.0.3?

Waiting for current previewgenerator to only use a small percent of 1 of 8 cpus is not a pleasant experience.
While I do appreciate dev efforts to find a better solution than image magick, I need something that works right now.

I tried to apply your PR like this:

went to Commit tab of this PR, right click on each one commit, copy link. went to shell, wget copied_url and append ".patch", like this

wget https://github.com/nextcloud/server/pull/16158/commits/20ac35e01351cb79b2c00545c444c8d7c94c6ab6.patch

then cd /var/www/nextcloud; patch --dry-run -p1 < /root/20ac35e01351cb79b2c00545c444c8d7c94c6ab6.patch

but it fails like this:

root@next:/var/www/nextcloud# patch --dry-run -p1 < /root/20ac35e01351cb79b2c00545c444c8d7c94c6ab6.patch
checking file lib/private/Preview/Generator.php
Hunk #1 FAILED at 96.
1 out of 1 hunk FAILED
checking file lib/private/Preview/GeneratorHelper.php
Hunk #1 succeeded at 35 (offset 3 lines).
Hunk #2 succeeded at 72 (offset 4 lines).
checking file lib/private/Preview/Image.php
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored
checking file lib/private/Preview/JPEG.php
Hunk #1 FAILED at 1.
Hunk #2 succeeded at 23 (offset 1 line).
Hunk #3 succeeded at 33 (offset 1 line).
1 out of 3 hunks FAILED
can't find file to patch at input line 355
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/lib/private/legacy/image.php b/lib/private/legacy/image.php
|index d9af45f2226..cb4c4566d84 100644
|--- a/lib/private/legacy/image.php
|+++ b/lib/private/legacy/image.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.
1 out of 1 hunk ignored

[maanloper]

Any update on this?

[h4knet]

Hello,

I'm discovering this thread while I was looking for a solution about the very slowly loading thumbnails on my server/NC clients (I'm already using APCu).

Is the work done by @nachoparker has been integrated into NC ? I'm running NC 20 and previewgenerator is still using 1 CPU but I wonder if NC does use more CPU's when it generates preview on the fly.

[J0WI]

Quoting myself from #13099 (comment):

Anyone familiar with libvips? The benchmarks and reviews look great.

There is a vips module for PHP that outperforms imagick in trivial tasks like thumbnail generation.

[J0WI]

I'm discovering this thread while I was looking for a solution about the very slowly loading thumbnails on my server/NC clients (I'm already using APCu).

This might be related to #15075

[CarlSchwan]

It seems like a popular approach nowadays is to run the preview generator inside a separate docker with software based on libvips e.g. https://github.com/h2non/imaginary or https://github.com/cshum/imagor If we move the preview generation to a separate process this might also help a bit with the performances...

libvips is part of OSS Fuzz and is contently fuzzed for potential new vulnerabilities so this should hopefully reduce the risks a bit with additionally running it in a docker image.

[J0WI]

If we move the preview generation to a separate process this might also help a bit with the performances...

This would break the concept of user key encryption.

[PVince81]

we have work in progress to generate previews faster using imaginary: #24166

[skjnldsv]

As there is no feedback since a while I will close this ticket.
We now support imaginary, which tremendously increase performances for wider instances.

@nachoparker thanks for the work put into Nextcloud (this PR might not be in, but many of others from you are 🤗)