Fix image preview for file versions

[JammingBen]

Description

Changed the way how preview thumbnails are being rendered so they will be rendered properly for file versions.

Related Issue

• Fixes Preview of versions doesn't show #38766

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[jvillafanez]

I think we should put a limit there to avoid loading a 500MB+ file into memory. I don't know how the native functions have been loading the image, but it wasn't our responsibility. If we're going to load the whole file into memory, I think we must check the filesize before to ensure it's within the limits.

[AlexAndBear]

@jvillafanez what is an appropriate file size? Any default image/placeholder to render ?

[JammingBen]

What do you suggest for a limit? Or make it configurable even?

[AlexAndBear]

https://github.com/owncloud/core/blob/9a6df29dc746ba8f4c768a4571c058dc3c890e4f/lib/private/Preview/Image.php L39 already there..

[jvillafanez]

https://github.com/owncloud/core/blob/9a6df29dc746ba8f4c768a4571c058dc3c890e4f/lib/private/Preview/Image.php L39 already there..

Is it just for the thumbnail? I mean, It seems to me you'll still load a 1GB file into memory and then reject the thumbnail generation because the file is too big.

Reusing that config key makes sense though.

[JammingBen]

Is it just for the thumbnail? I mean, It seems to me you'll still load a 1GB file into memory and then reject the thumbnail generation because the file is too big.

I'm pretty sure this is just for the thumbnail, yes. I get your point, but this is not related to this issue, is it? I mean, we would need to check other places where the image gets loaded into memory initially (if so). But the preview thumbnails seem to be safe because of the if-condition here.

[jvillafanez]

https://github.com/owncloud/core/pull/38778/files#diff-ff6994338643431270cf761760b151580f971dd03f7df4ae572ca41f50ef8fb5R48 is one of those places, unless I'm wrong.
I'm fine assuming that's the only place we need to check

[JammingBen]

Yes, but the if-clause which returns if the limit exceeds it literally 5 lines above that statement 😄 So there is not way we exceed the limit here because of this change.

[jvillafanez]

Sometimes I just need to scroll further... I thought the changes were elsewhere 🐧
Ok, nothing more from me. 👍

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information