Ensured large image is unloaded from memory when generating previews

[elijahnyp]

PR to resolve#21995
likely also resolvesnextcloud/previewgenerator#108
incorporates suggestion by @kesselb to use the destroy method of IImage instead of destroying the resource directly.

This change should be very low risk as it's destroying an image object that is only sometimes created. It's created if there isn't a cached version of the preview and it is independent of the newly generated preview image.

It has been tested on 19.0.1

[kesselb]

Thanks 👍

1. Mind to rebase your branch with the latest changes from master? That should fix a lot of CI warnings.
2. GeneratorTest needs an update. The test mocks there are create from the IImage interface but the interface does not have a destory method hence the mocks do not contain the method and the tests are failing.

Index: tests/lib/Preview/GeneratorTest.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- tests/lib/Preview/GeneratorTest.php	(revision 701a44bcf93d26b2c27457ee4890992b6d861a14)
+++ tests/lib/Preview/GeneratorTest.php	(date 1595853526830)
@@ -183,7 +183,7 @@
$this->fail('Unexpected provider requested');
});
- $image = $this->createMock(IImage::class);
+ $image = $this->createMock(\OC_Image::class);
$image->method('width')->willReturn(2048);
$image->method('height')->willReturn(2048);
$image->method('valid')->willReturn(true);
@@ -318,7 +318,7 @@
}
private function getMockImage($width, $height, $data = null) {
- $image = $this->createMock(IImage::class);
+ $image = $this->createMock(\OC_Image::class);
$image->method('height')->willReturn($width);
$image->method('width')->willReturn($height);
$image->method('valid')->willReturn(true);

[elijahnyp]

Phew! I think that did it. The rebase pulled in a bunch of other commits though - I hope that's okay. I should probably pull them....... back in a minute.....

[elijahnyp]

There we go, nice and clean, should be good for reviews. My apologies for the sloppy process to get to this point.

[icewind1991]

Strange that gc doesn't trigger cleaning this up already

[elijahnyp]

Strange that gc doesn't trigger cleaning this up already

From what I could find, it's just the way php has handled images for years. There's something in the image libraries that keeps the resource referenced so GC doesn't see it as garbage unless the image is explicitly destroyed.

[lifesbest23]

Thanks for the solution, works well for me.
Shouldn't the $maxPreviewImage instanceof IImage part be changed to $maxPreviewImage instanceof OCP\Image as IImage does not implement the destroy function?

I find this to be the simplest solution for this problem 👍
Another way more complex way to fix this would be to remove usage of gd functions in OCImage for another image library,

[kesselb]

Shouldn't the $maxPreviewImage instanceof IImage part be changed to $maxPreviewImage instanceof OCP\Image as IImage does not implement the destroy function?

Yes.

[MorrisJobke]

Looks good and works 👍 I pushed a commit to fix the instanceof check.

[MorrisJobke]

/backport to stable19

[MorrisJobke]

/backport to stable18

[MorrisJobke]

/backport to stable17

[kesselb]

imagedestroy() function is still available in PHP 8.0, but it does nothing. Memory and other resources will be freed when the \GdImage object is removed either with an explicit unset() call or when it falls out of scope.

https://php.watch/versions/8.0#imagedestroy good to know.

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[MorrisJobke]

/backport to stable18

[MorrisJobke]

/backport to stable17