Introducing the maximum size preview #14879

oparoz · 2015-03-13T15:00:20Z

Replaces #13674

Fixes

Always store the preview returned by Imagemagick before resizing it. Always store the preview returned by Imagemagick before resizing it #12465

The problem

You've just uploaded a 100MB TIFF file that you need to share with a restricted audience. Here is what currently happens once the file has been uploaded.

A 36x36 thumbnail is generated in 1 minute
Go to the Gallery, an album thumbnail might be generated in 1 minute
Go inside the album, another thumbnail will be generated in 1 minute
Click on the picture to get a full screen preview. 1 minute
Go back to the Files app and share the file
Client 1 visits the link. 1 minute to get the preview
Client 2 with a different screen resolution visits the link. 1 minute again

You're starting to get it. Every time a slightly different preview size is asked, a full conversion of that 100MB TIFF to PNG is performed. This is highly inefficient.

Proposed solution

Enforce maximum preview dimensions in the config. They're currently set to null.
The first time we're asked to generate a preview we'll generate one of the maximum dimension indicated in the configuration and all future resizing requests will be done on that preview in order to not waste time converting the same large file over and over

Default size is 2048x2048
The maximum size preview is keeping the aspect ratio of the original and is not upscaled.
It's is immediately stored in the cache with a special max tag and size identifiers based on its real size.

Every time a preview of a different size is asked, we store it in the cache. This also speeds up operation at the cost of disk space.

This make a huge difference in libraries holding several large files, for people using apps such as the Gallery.

Recreated after the loss of #13582

karlitschek · 2015-03-15T16:05:30Z

@georgehrke What is your opinion?

georgehrke · 2015-03-15T16:07:30Z

lib/private/preview.php

-		$this->configMaxX = \OC_Config::getValue('preview_max_x', null);
-		$this->configMaxY = \OC_Config::getValue('preview_max_y', null);
-		$this->maxScaleFactor = \OC_Config::getValue('preview_max_scale_factor', 2);
+		$this->configMaxX = \OC::$server->getConfig()->getSystemValue('preview_max_x', 2048);


Can we maybe modify the constructor to expect some OCP\IConfig and if null is given fall back to \OC::$server->getConfig() (edit: and the same for logging)?

Seems doable, but is it the right patch to modify the signature? Wouldn't it be better to introduce that in a PR which can be given as a reference to the change so that people can start modifying their code? cc @MorrisJobke @LukasReschke

People should never create a \OC\Preview instance directly and use getPreviewManager anyway.
(but removing all direct creations of \OC\Preview in core should definitely be done in a separate pr. (will take care))

Agreed, except that, sometimes, it's not possible to use it as PreviewManager is not yet a true replacement of \OC\Preview
See #12772 for what's missing

But if it's confirmed that the signature change should happen here, I'll do it.

That's the right approach, but will conflict greatly with #14857

Maybe you can wait, until that is done, since it is a major rework on the same file aswell ;)

Sounds reasonable. I'll let you adjust the signature then and will update this PR when you're done.

georgehrke · 2015-03-15T16:21:42Z

Code looks good to me otherwise

Add unit tests for newly introduced methods

nickvergessen · 2015-03-16T11:25:43Z

lib/private/preview.php

@@ -921,7 +1056,7 @@ public static function post_delete($args, $prefix='') {
 	 * @return bool
 	 */
 	public static function isAvailable(\OC\Files\FileInfo $file) {
-		if (!\OC_Config::getValue('enable_previews', true)) {
+		if (!\OC::$server->getConfig()->getSystemValue('enable_previews', true)) {


This will conflict with #14857

oparoz · 2015-03-19T12:31:39Z

Since there were no newly introduced public methods, I just did an extensive preview size test.

I'm also forcing a resize since the image provider is not doing it's resizing job properly and since that's what people are going to use the most.

I've found that testing the file individually (using the bootstrap) will WIPE your admin's data.

Note: It's possible to tweak the preview size and get the test to fail, because of this bug: #14712

oparoz · 2015-04-07T12:40:52Z

@georgehrke @MorrisJobke @PVince81 @DeepDiver1975
Could me merge this before the PR goes, once again, out of sync?

oparoz · 2015-04-07T12:43:29Z

Maybe @icewind1991 and @jancborchardt want to test it as well since this benefits the Gallery apps.

nickvergessen · 2015-04-07T13:28:52Z

lib/private/preview.php

+						$file, $this->configMaxX, $this->configMaxY, $scalingUp = false, $this->fileView
+					);
+
+					if (!($preview instanceof \OC_Image)) {


Use the interface please

Yep, thanks.

The first time we're asked to generate a preview we'll generate one of the maximum dimension indicated in the configuration and all future resizing requests will be done on that preview in order to not waste time converting the same file over and over. One of the fixes required for #12465

scrutinizer-notifier · 2015-04-07T15:12:23Z

The inspection completed: 2 new issues, 6 updated code elements

ghost · 2015-04-07T15:19:14Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11279/
🚀 Test PASSed.🚀

MorrisJobke · 2015-04-07T16:12:54Z

Tested and works 👍

georgehrke · 2015-04-07T16:15:10Z

mentioned issues fixed, works 👍

Introducing the maximum size preview

oparoz mentioned this pull request Mar 13, 2015

Introducing the maximum size preview #13674

Closed

oparoz added 3 - To Review performance feature:previews labels Mar 13, 2015

MorrisJobke added this to the 8.1-current milestone Mar 13, 2015

georgehrke reviewed Mar 15, 2015
View reviewed changes

nickvergessen reviewed Mar 16, 2015
View reviewed changes

nickvergessen added 1 - To develop and removed 3 - To Review labels Mar 24, 2015

oparoz added 3 - To Review and removed 1 - To develop labels Mar 31, 2015

nickvergessen reviewed Apr 7, 2015
View reviewed changes

MorrisJobke added a commit that referenced this pull request Apr 7, 2015

Merge pull request #14879 from oparoz/fix-preview-caching

6c327f8

Introducing the maximum size preview

MorrisJobke merged commit 6c327f8 into owncloud:master Apr 7, 2015

oparoz deleted the fix-preview-caching branch April 23, 2015 22:17

lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019

Introducing the maximum size preview #14879

Introducing the maximum size preview #14879

Uh oh!

Conversation

oparoz commented Mar 13, 2015

Replaces #13674

Fixes

The problem

Proposed solution

Uh oh!

karlitschek commented Mar 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgehrke commented Mar 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oparoz commented Mar 19, 2015

Uh oh!

oparoz commented Apr 7, 2015

Uh oh!

oparoz commented Apr 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scrutinizer-notifier commented Apr 7, 2015

Uh oh!

ghost commented Apr 7, 2015

Uh oh!

MorrisJobke commented Apr 7, 2015

Uh oh!

georgehrke commented Apr 7, 2015

Uh oh!

Uh oh!