Fix max preview, some resizing and caching issues and force preview providers to resize their previews properly #16382

oparoz · 2015-05-16T19:52:08Z

TL;DR

This PR:

introduces a method in OC_Image which doesn't stretch images when trying to make them fit in a box
adds the method to all key providers so that they can do their job, as expected by the Preview class
improves the caching mechanism of Preview in order to reduce I/O and to avoid filling the available disk space
fixes some long standing issues
contains mostly tests

Description

#13607 describes what needed to be done at the provider level in order to support the max preview feature, but unfortunately, only the bitmap provider was fixed until now and this led to some max previews being cropped (#16355).

Since all the other providers end up with an \OC_Image object, they can simply use one of the available methods to make sure the preview they create fits in the asked dimension. I decided to call that method from within each provider since they're responsible to provide a preview which matches the requirements.

Unfortunately, since none of the available method does the job, a new one had to be introduced to make sure a smaller preview wouldn't be stretched to fit in the max dimensions.

One test per group of provider is provided.

Also, since when working on this I encountered many bugs, both introduced by the max preview and through the years, I've decided to write tests (41 x 4 samples) for the Preview class to try and detect most of the problems and to fix them.

This branch will now also fix

Never cache previews larger than the max preview Never cache previews larger than the max preview #16545
Don't store the exact same preview every time we check for its existence Don't store the exact same preview every time we check for its existence #16544
Preview can't resize wide images properly Preview can't resize wide images properly #14712

Enjoy and hopefully this will be working well for all of you!

I'm also open to discussing testing strategies as this is not easy to do properly and requires large data samples.

Test data

I suggest you upload at least these 4 pictures from the tests/data folder to your test folder:

testimage.eps | standard eps file, converted by ImageMagick
testimage.jpg | normal jpg with some text in it
testimage-wide.png | panoramic picture
testimageblah.gif | small animated gif

They were chosen because they trigger different scenarios in the resizing mechanism. You're welcome to test with your libraries as well, just make sure you've deleted the thumbnail folder before proceeding.

Your configuration should have the max preview dimensions set at 756x756 if you want to be able to really test the various restrictions the resizing can encouter.
If you want to test upscaling, then you should have a max_factor of above 1.0 in your configuration

Tests

URLs

No aspect ratio
/index.php/core/preview.png?file=testimage.jpg&x=756&y=756&forceIcon=1
With ratio
/index.php/core/preview.png?file=testimage.jpg&x=756&y=756&forceIcon=1&a

You're encouraged to play with various sizes, to see the effect it has on the rendering

It's also possible to disable upscaling by adding this to the URL: &scalingup=0, but you will need to delete your thumbnails folder in order to be able to get real results, since there is no mechanism in place which detects if a preview has been upscaled or not. We currently only track with or without aspect ratio.

Gallery

I also invite you to install Gallery 8.1 to test this since it will generate thumbnails and large previews and would be a good quick test.
Make sure you increase the the max preview dimensions and delete the thumbnails folder prior to testing or your large previews will look tiny.

Mobile clients

You should be getting the same previews you were getting before, except maybe a few "fixed" ones.

Test results

Pictures should never be bigger than 756x756 (using the config described above)
Whenever a picture is smaller than what is asked, it will be sent as is if the aspect ratio is required and with a transparent background if no aspect ratio is required.

I've left lots of debugging messages (to be cleanup later), so by looking at the logs, you should be getting an idea of what the class is trying to do with the request it's been given.

oparoz · 2015-05-16T20:29:06Z

Seems like the CI environment doesn't support movie, office and SVG conversion and my detection routine has failed, so I will update it.

oparoz · 2015-05-16T21:25:49Z

@libasys - Since you've reported the problem, please test this.
Other interested parties: @georgehrke @DeepDiver1975 @MorrisJobke

oparoz · 2015-05-16T21:29:38Z

Also, I've introduced some test documents, feel free to give me replacements if you have better ones. I tried to use oC material, but couldn't find a short MP3 and MP4.

libasys · 2015-05-18T05:58:41Z

looks good!

nickvergessen · 2015-05-19T16:34:14Z

lib/private/image.php

+	 * @param integer $maxHeight
+	 * @return bool
+	 */
+	public function fitInIfTooBig($maxWidth, $maxHeight) {


missing from public interface?
Also I don't think the name is very suitable.

Yes, I missed it.

OK about the name.
Let's first validate the concept, then I'll accept any name. I find the original fitIn ambiguous because it actually stretches the image to fill the space, so we need a descriptive method name.

scaleToFit() ?
scaleDownToFit() ?

scaleToFit() sounds good. If a small image is stretched to fill the space, this info should be added to the PHPDoc as well.

PVince81 · 2015-05-26T10:36:49Z

lib/private/image.php

+		$widthOrig = imageSX($this->resource);
+		$heightOrig = imageSY($this->resource);
+
+		if ($widthOrig > $maxWidth || $heightOrig >$maxHeight) {


Should this be >= (just asking, I'm not sure myself)

I don't think so. If it fits in the box already, we don't need to process it further.

oparoz · 2015-05-28T12:53:44Z

I'm going to push updated tests and the fixed Preview class which goes with it. It fixes a lot of reported issues with previews...

PHPUnit 4.5.0 by Sebastian Bergmann and contributors.

................................................................. 65 / 84 ( 77%)
...................

Time: 40.56 seconds, Memory: 34.50Mb

OK (84 tests, 567 assertions)

oparoz · 2015-05-29T01:47:31Z

Nice, so if I've done my job right, this shows that the current Preview class fails 51 out of 164 tests.
I'll try to upload my fixed Preview class tomorrow.

https://ci.owncloud.org/job/pull-request-analyser-ng-simple/label=SLAVE/12578/console

ghost · 2015-05-30T00:02:37Z

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

oparoz · 2015-05-30T00:17:54Z

OK, so 164 tests were passed and 1127 assertions were verified for the Preview class alone

I've also performed extensive real world testing and now it's time for you to do the same
@georgehrke @jancborchardt @jospoortvliet @libasys @davivel @setnes @PVince81 @DeepDiver1975 @MorrisJobke @RealRancor @rullzer

EDIT: Testing methodologies moved to OP

oparoz · 2015-05-30T00:23:21Z

lib/private/preview.php


 		//save parameters
 		$this->setFile($file);
-		$this->setMaxX($maxX);
-		$this->setMaxY($maxY);
+		$this->setMaxX((int)$maxX);


The idea is to make sure we get valid dimensions

LukasReschke · 2015-06-05T18:27:59Z

lib/private/preview.php

 	 * @throws \Exception
 	 * @return mixed (bool / string)
 	 *                    false if thumbnail does not exist
 	 *                    path to thumbnail if thumbnail exists
 	 */
-	public function __construct($user = '', $root = '/', $file = '', $maxX = 1, $maxY = 1, $scalingUp = true) {
+	public function __construct(
+		$user = '', $root = '/', $file = '', $maxX = 1, $maxY = 1, $scalingUp = true


While squashing please move each to it's own line - THX.

…roviders to resize their previews properly * introduces a method in OC_Image which doesn't stretch images when trying to make them fit in a box * adds the method to all key providers so that they can do their job, as expected by the Preview class * improves the caching mechanism of Preview in order to reduce I/O and to avoid filling the available disk space * fixes some long standing issues * **contains mostly tests**

oparoz · 2015-06-06T14:30:22Z

Thanks for the review @LukasReschke 🍻
I've squashed + rebased and will add another commit once I understand your preference for $this in PHPDocs and what you need in terms of type hinting for arrays which are already typed.

ghost · 2015-06-06T15:18:24Z

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

MorrisJobke · 2015-06-08T06:32:07Z

@LukasReschke Is this now okay from your side?

DeepDiver1975 · 2015-06-08T11:48:17Z

lib/private/image.php

+	 * @return bool
+	 */
+	public function scaleDownToFit($maxWidth, $maxHeight) {
+		$widthOrig = imageSX($this->resource);


test if $this->resource is valid is missing here - use

if (!$this->valid()) { $this->logger->error(__METHOD__ . '(): No image loaded', array('app' => 'core')); return false; }

I removed this on purpose since we either return false or call fitIn which will make sure there is an image to work on before trying to process it, but I can see now why it's still a good idea to have it since the method is first determining the size of the resource in order to make a decision.

I think in a future version of oC, we could even merge the two methods by adding an extra argument. I didn't want to touch the original this late in the game.

Or should I modify the original method in this PR to avoid duplication?
public function fitIn($maxWidth, $maxHeight, $scalingUp = false)

The method is tested and only doing something very specific, so it should be a low risk. Also, the method has not been public until now.

I think in a future version of oC, we could even merge the two methods by adding an extra argument. I didn't want to touch the original this late in the game.

I'd walk this path for now - THX

scrutinizer-notifier · 2015-06-08T13:10:53Z

A new inspection was created.

ghost · 2015-06-08T13:58:39Z

🚀 Test PASSed.🚀

DeepDiver1975 · 2015-06-08T14:14:14Z

👍 from my side - THX a lot!

…he-size-of-their-preview Fix max preview, some resizing and caching issues and force preview providers to resize their previews properly

jospoortvliet · 2015-06-09T11:49:15Z

Yes, super awesome @oparoz thanks a bunch!

oparoz self-assigned this May 16, 2015

oparoz added the 2 - Developing label May 16, 2015

oparoz added this to the 8.1-current milestone May 16, 2015

oparoz added the feature:previews label May 16, 2015

oparoz added 3 - To Review and removed 2 - Developing labels May 17, 2015

oparoz mentioned this pull request May 18, 2015

Preview providers should take maxX and maxY into consideration #13607

Closed

6 tasks

nickvergessen reviewed May 19, 2015
View reviewed changes

oparoz mentioned this pull request May 23, 2015

Thumbnail generation: does not obey aspect ratio / max values for x and y #16250

Closed

PVince81 reviewed May 26, 2015
View reviewed changes

oparoz added 2 - Developing regression and removed 3 - To Review labels May 29, 2015

oparoz changed the title ~~Make sure preview providers resize their previews properly~~ Fix max preview, some resizing and caching issues and force preview providers to resize their previews properly May 29, 2015

This was referenced May 29, 2015

Preview can't resize wide images properly #14712

Closed

Don't store the exact same preview every time we check for its existence #16544

Closed

Never cache previews larger than the max preview #16545

Closed

oparoz reviewed May 30, 2015
View reviewed changes

LukasReschke reviewed Jun 5, 2015
View reviewed changes

DeepDiver1975 reviewed Jun 8, 2015
View reviewed changes

Make sure we have a resource before measuring its size

3d0a523

DeepDiver1975 merged commit 64c9c27 into owncloud:master Jun 8, 2015

oparoz removed the 3 - To Review label Jun 18, 2015

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix max preview, some resizing and caching issues and force preview providers to resize their previews properly #16382

Fix max preview, some resizing and caching issues and force preview providers to resize their previews properly #16382

oparoz commented May 16, 2015

oparoz commented May 16, 2015

oparoz commented May 16, 2015

oparoz commented May 16, 2015

libasys commented May 18, 2015

nickvergessen May 19, 2015

oparoz May 19, 2015

oparoz May 23, 2015

PVince81 May 26, 2015

PVince81 May 26, 2015

oparoz May 26, 2015

oparoz commented May 28, 2015

oparoz commented May 29, 2015

ghost commented May 30, 2015

oparoz commented May 30, 2015

oparoz May 30, 2015

LukasReschke Jun 5, 2015

LukasReschke Jun 5, 2015

oparoz Jun 6, 2015

oparoz commented Jun 6, 2015

ghost commented Jun 6, 2015

MorrisJobke commented Jun 8, 2015

DeepDiver1975 Jun 8, 2015

oparoz Jun 8, 2015

oparoz Jun 8, 2015

DeepDiver1975 Jun 8, 2015

scrutinizer-notifier commented Jun 8, 2015

ghost commented Jun 8, 2015

DeepDiver1975 commented Jun 8, 2015

jospoortvliet commented Jun 9, 2015

Fix max preview, some resizing and caching issues and force preview providers to resize their previews properly #16382

Fix max preview, some resizing and caching issues and force preview providers to resize their previews properly #16382

Conversation

oparoz commented May 16, 2015

TL;DR

Description

Test data

Tests

URLs

Gallery

Mobile clients

Test results

oparoz commented May 16, 2015

oparoz commented May 16, 2015

oparoz commented May 16, 2015

libasys commented May 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oparoz commented May 28, 2015

oparoz commented May 29, 2015

ghost commented May 30, 2015

oparoz commented May 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oparoz commented Jun 6, 2015

ghost commented Jun 6, 2015

MorrisJobke commented Jun 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrutinizer-notifier commented Jun 8, 2015

ghost commented Jun 8, 2015

DeepDiver1975 commented Jun 8, 2015

jospoortvliet commented Jun 9, 2015