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

oparoz · 2015-01-23T17:12:42Z

This is a fix for #13607
and is part of the global programme to fix preview caching: #13609

Original TIFF: 140MB. Waiting an insane amount of time every time you need a preview because a conversion is asked every time
With Introducing the maximum size preview #13674: Preview only resizes a 48MB file. Waiting about a minute for each preview
Combined with this: Preview only resizes a 6.7MB file. Waiting about 3 SECONDS.

oparoz · 2015-03-06T15:05:23Z

The idea behind a "max" preview is to have the same image, including aspect ratio, that fits into max dimensions, so I've removed the "fill" action.
A "fill" option could be implemented at a later stage when and if the interface changes and if it is deemed useful to received images which are of a predefined dimension such as 200x200

MorrisJobke · 2015-03-11T12:40:35Z

lib/private/preview/bitmap.php

@@ -8,30 +10,93 @@

 namespace OC\Preview;

+use Imagick;


@DeepDiver1975 @LukasReschke Can we rely on this? is this a hard dependency already?

Check is in preview:
https://github.com/owncloud/core/blob/master/lib/private/preview.php#L767

MorrisJobke · 2015-03-11T13:06:31Z

I will test this with following image: https://commons.wikimedia.org/wiki/File:Aerial_view_washington_monument_with_scaffolding.tif

MorrisJobke · 2015-03-11T13:15:58Z

Works 👍 20s down to 2s with this image: https://commons.wikimedia.org/wiki/File:Allen_Ludden_Stumpers_1976.tiff

MorrisJobke · 2015-03-11T13:16:05Z

code looks good

MorrisJobke · 2015-03-11T13:16:25Z

@nickvergessen @georgehrke Interested in reviewing?

nickvergessen · 2015-03-11T13:27:45Z

lib/private/preview/bitmap.php

 		} catch (\Exception $e) {
-			\OC_Log::write('core', $e->getmessage(), \OC_Log::ERROR);
+			\OC_Log::write('core', 'ImageMagick says : ' . $e->getmessage(), \OC_Log::ERROR);


No space before :, we are not french ;)

MorrisJobke · 2015-03-11T17:24:35Z

@owncloud-bot retest this please

MorrisJobke · 2015-03-11T18:23:48Z

00:15:07.970 1) OC\Core\Avatar\AvatarControllerTest::testPostAvatarFile
00:15:07.973 file_put_contents(/dev/shm/data-autotest5/eiyadirgdeao/cache/avatar_upload): failed to open stream: No such file or directory
00:15:07.973 
00:15:07.973 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/storage/local.php:165
00:15:07.973 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:956
00:15:07.973 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:517
00:15:07.973 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/tests/core/avatar/avatarcontrollertest.php:248
00:15:07.973 
00:15:07.973 2) OC\Core\Avatar\AvatarControllerTest::testPostAvatarFileGif
00:15:07.973 file_put_contents(/dev/shm/data-autotest5/fkdxltjwmwhj/cache/avatar_upload): failed to open stream: No such file or directory
00:15:07.973 
00:15:07.973 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/storage/local.php:165
00:15:07.973 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:956
00:15:07.973 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:517
00:15:07.973 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/tests/core/avatar/avatarcontrollertest.php:288

oparoz · 2015-03-11T18:45:31Z

@MorrisJobke - This is unrelated to this PR afaik

MorrisJobke · 2015-03-11T19:24:48Z

@oparoz But it fails just on this branch ;)

nickvergessen · 2015-03-12T09:03:30Z

Nop #14813

MorrisJobke · 2015-03-12T09:05:46Z

I know.

icewind1991 · 2015-03-24T15:03:36Z

lib/private/preview/bitmap.php

 	/**
 	 * {@inheritDoc}
 	 */
 	public function getThumbnail($path, $maxX, $maxY, $scalingup, $fileview) {
+		$this->maxDims = [$maxX, $maxY];


pass this to getResizedPreview instead of saving it in the class.

Is there a particular reason? I originally had 3 private methods, all needing that information and I didn't want to have to make the variables traverse each method.
Now with 2, it's less of a hassle, but I'm wondering what the issue is with leaving it as is.

EDIT: If it's to improve readability (currently the resize method has no size arguments), then I fully understand.

The max dimensions is not part of the preview provider but determined by thr getThumbnail call, thus they should not be part of the state of the provider

This is a fix for #13607 and is part of the global programme: #13609 The image aspect ratio is kept intact.

scrutinizer-notifier · 2015-03-31T23:28:39Z

A new inspection was created.

oparoz · 2015-03-31T23:29:12Z

Had forgotten to rebase...

oparoz · 2015-04-01T11:22:52Z

Weird... the "Passed" test result went away...
The same PR here #15349 was OK.
Should I just forget this one and restore the other one?

oparoz · 2015-04-01T13:19:58Z

Moved to #15349

This was referenced Jan 23, 2015

Fixing preview caching #13609

Closed

Preview providers should take maxX and maxY into consideration #13607

Closed

oparoz added feature:previews performance 3 - To Review labels Jan 23, 2015

MorrisJobke modified the milestones: 8.0-current, 8.1-next Jan 24, 2015

oparoz self-assigned this Jan 26, 2015

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

LukasReschke changed the title ~~The bitmap preview class now takes maxX and maxY into consideration~~ [WIP] The bitmap preview class now takes maxX and maxY into consideration Feb 24, 2015

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

oparoz changed the title ~~[WIP] The bitmap preview class now takes maxX and maxY into consideration~~ The bitmap preview class now takes maxX and maxY into consideration Feb 24, 2015

MorrisJobke reviewed Mar 11, 2015
View reviewed changes

nickvergessen reviewed Mar 11, 2015
View reviewed changes

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

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

icewind1991 reviewed Mar 24, 2015
View reviewed changes

PVince81 added 2 - Developing and removed 3 - To Review labels Mar 26, 2015

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

The bitmap preview class now takes maxX and maxY into consideration

2e703f8

This is a fix for #13607 and is part of the global programme: #13609 The image aspect ratio is kept intact.

oparoz mentioned this pull request Apr 1, 2015

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

Merged

oparoz closed this Apr 1, 2015

MorrisJobke removed this from the 8.1-current milestone Apr 1, 2015

oparoz deleted the limit-bitmap-previews branch April 1, 2015 13:20

oparoz removed 3 - To Review feature:previews performance labels Apr 1, 2015

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oparoz commented Jan 23, 2015

oparoz commented Mar 6, 2015

MorrisJobke Mar 11, 2015

oparoz Mar 11, 2015

MorrisJobke commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

nickvergessen Mar 11, 2015

MorrisJobke commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

oparoz commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

nickvergessen commented Mar 12, 2015

MorrisJobke commented Mar 12, 2015

icewind1991 Mar 24, 2015

oparoz Mar 24, 2015

icewind1991 Mar 24, 2015

scrutinizer-notifier commented Mar 31, 2015

oparoz commented Mar 31, 2015

oparoz commented Apr 1, 2015

oparoz commented Apr 1, 2015

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

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

Conversation

oparoz commented Jan 23, 2015

oparoz commented Mar 6, 2015

MorrisJobke Mar 11, 2015

Choose a reason for hiding this comment

oparoz Mar 11, 2015

Choose a reason for hiding this comment

MorrisJobke commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

nickvergessen Mar 11, 2015

Choose a reason for hiding this comment

MorrisJobke commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

oparoz commented Mar 11, 2015

MorrisJobke commented Mar 11, 2015

nickvergessen commented Mar 12, 2015

MorrisJobke commented Mar 12, 2015

icewind1991 Mar 24, 2015

Choose a reason for hiding this comment

oparoz Mar 24, 2015

Choose a reason for hiding this comment

icewind1991 Mar 24, 2015

Choose a reason for hiding this comment

scrutinizer-notifier commented Mar 31, 2015

oparoz commented Mar 31, 2015

oparoz commented Apr 1, 2015

oparoz commented Apr 1, 2015