Introducing the maximum size preview #13674

oparoz · 2015-01-26T14:45:08Z

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

oparoz · 2015-01-26T14:47:08Z

I lost PR #13582, so this is a copy.

cc @georgehrke

ghost · 2015-02-24T15:40:24Z

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

PVince81 · 2015-03-02T16:59:58Z

This looks like a nice improvement. I can't seem to get my head around all this logic though.

georgehrke · 2015-03-02T17:18:54Z

Code changes itself seem fine (although I agree that it's a lot changes for one commit 😄)
Will test asap

oparoz · 2015-03-02T18:48:51Z

Maybe I should have split it in 3 commits. I did move one test and 2 other methods around.

As for the logic, you have to start with

`getPreview()`

We first check if we have a cached preview

`isCached()`

In order for everyone to benefit from the feature without having to re-generate all their thumbnails, we don't consider a preview to be cached until we find one with the -max tag

If we do, we retrieve it

`getCachedPreview`

retrieves the "max" preview
resizes the preview to fulfil the request

and if not we generate one.

`generatePreview()`

finds a provider
generates one preview using the maximum size defined in the configuration
stores it using the special -max tag.
resizes the preview to fulfil the request

MorrisJobke · 2015-03-11T13:26:36Z

lib/private/preview.php

@@ -100,8 +100,8 @@ public function __construct($user = '', $root = '/', $file = '', $maxX = 1, $max
 		$this->userView = new \OC\Files\View('/' . $user);

 		//set config
-		$this->configMaxX = \OC_Config::getValue('preview_max_x', null);
-		$this->configMaxY = \OC_Config::getValue('preview_max_y', null);
+		$this->configMaxX = \OC_Config::getValue('preview_max_x', 2048);


Can I ask you to use \OCP\IConfig?

Yep, I guess, it's a good time to upgrade all those calls...

\OC::$server->getConfig()->getSystemValue()

or just add ifconfig to the constructor

That means every call to new Preview() will need to be updated to include new parameters or am I missing something?

I would go with the \OC::$server->getConfig() right inside this class in the short term.

MorrisJobke · 2015-03-11T13:27:47Z

@georgehrke Please have a look too

MorrisJobke · 2015-03-11T13:28:01Z

@georgehrke Once you're fine with this I will review.

georgehrke · 2015-03-11T13:39:08Z

@oparoz Isn't 2048x2048 a bit much? That's something you might need for public sharing previews on a high-resolution screen. Wouldn't 512x512 be enough for most use-cases?

oparoz · 2015-03-11T14:04:16Z

@georgehrke - Phones today have 2k screens, a 512x512 picture on a small, high DPI screen will look tiny. That's the main reason for me to keep it as is. And let's not forget that this is used only for full screen previews. Thumbnails will be much smaller.
It's user adjustable anyway and apps (mobile per example) could introduce settings which put another limit on previews they ask for if bandwidth is an issue.
That's my view :)

georgehrke · 2015-03-11T14:05:17Z

Okay, I can see your point there :)

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

ghost · 2015-03-11T19:56:48Z

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

Build result: FAILURE

[...truncated 14 lines...] > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config --add remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git8497915014349887160.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse refs/remotes/origin/pr/13674/merge^{commit} # timeout=10 > git rev-parse refs/remotes/origin/origin/pr/13674/merge^{commit} # timeout=10Checking out Revision f045ee672a187a5f53ef1e94558180f4be069c8c (refs/remotes/origin/pr/13674/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f f045ee672a187a5f53ef1e94558180f4be069c8c > git rev-list ee6566d64e5f795732cf9e0a8d9ca2972db6bf14 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILURESetting status of 07b5402 to FAILURE with url https://ci.owncloud.org/job/pull-request-analyser-ng-simple/10378/ and message: Merged build finished.
Test FAILed.

scrutinizer-notifier · 2015-03-11T20:08:34Z

The inspection completed: 3 new issues, 5 updated code elements

MorrisJobke · 2015-03-11T20:09:58Z

Failing unit tests are unrelated

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

Replaced by #14879

oparoz added 3 - To Review performance feature:previews and removed performance labels Jan 26, 2015

oparoz self-assigned this Jan 26, 2015

oparoz added this to the 8.1-next milestone Jan 26, 2015

LukasReschke added 2 - Developing and removed 3 - To Review labels Feb 24, 2015

LukasReschke changed the title ~~Introducing the maximum size preview~~ [WIP] Introducing the maximum size preview Feb 24, 2015

oparoz added 3 - To Review and removed 2 - Developing labels Feb 24, 2015

oparoz changed the title ~~[WIP] Introducing the maximum size preview~~ Introducing the maximum size preview Feb 24, 2015

MorrisJobke reviewed Mar 11, 2015
View reviewed changes

MorrisJobke added 2 - Developing and removed 3 - To Review labels Mar 11, 2015

oparoz added 2 commits March 11, 2015 18:09

Removed deprecated methods

07b5402

oparoz added 3 - To Review and removed 2 - Developing labels Mar 13, 2015

oparoz closed this Mar 13, 2015

oparoz mentioned this pull request Mar 13, 2015

Introducing the maximum size preview #14879

Merged

oparoz mentioned this pull request Apr 1, 2015

The bitmap preview class now takes maxX and maxY into consideration #15349

Merged

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

Introducing the maximum size preview #13674

Introducing the maximum size preview #13674

Uh oh!

Conversation

oparoz commented Jan 26, 2015

Fixes

The problem

Proposed solution

Uh oh!

oparoz commented Jan 26, 2015

Uh oh!

ghost commented Feb 24, 2015

Uh oh!

PVince81 commented Mar 2, 2015

Uh oh!

georgehrke commented Mar 2, 2015

Uh oh!

oparoz commented Mar 2, 2015

getPreview()

isCached()

getCachedPreview

generatePreview()

Uh oh!

MorrisJobke Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

oparoz Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

oparoz Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

georgehrke Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

oparoz Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

MorrisJobke Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Mar 11, 2015

Uh oh!

MorrisJobke commented Mar 11, 2015

Uh oh!

georgehrke commented Mar 11, 2015

Uh oh!

oparoz commented Mar 11, 2015

Uh oh!

georgehrke commented Mar 11, 2015

Uh oh!

ghost commented Mar 11, 2015

Build result: FAILURE

Uh oh!

scrutinizer-notifier commented Mar 11, 2015

Uh oh!

MorrisJobke commented Mar 11, 2015

Uh oh!

oparoz commented Mar 13, 2015

Uh oh!

Uh oh!

`getPreview()`

`isCached()`

`getCachedPreview`

`generatePreview()`