Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Max sized preview gets generated, regardless of specifications #518

Open
5 of 8 tasks
marco-calautti opened this issue Oct 13, 2024 · 5 comments
Open
5 of 8 tasks
Labels

Comments

@marco-calautti
Copy link

marco-calautti commented Oct 13, 2024

⚠️ This issue respects the following points: ⚠️

Bug description

It seems that when a preview generation is triggered, e.g., using the preview generator app, regardless of the given specification, the generatePreviews method always generates the maxPreview with resolution using the max possible, according to the preview_max_x and preview_max_y parameters.

This is definitely undesirable, as it defeats the whole point of having small pre-generated previews, while having large on-demand previews, when needed. In fact, in the current state, the max preview is going to take the same space as the original picture in most cases, and thus, it does not make sense to pre-generate it.

The affected line of code is here.
In particular, the function getMaxPreview will generate the max preview, if it is not found, but this should not happen, if the specification parameter does not require this preview to be generated.

Steps to reproduce

  1. Trigger preview generation with the preview generator app specifying only, e.g., 64 256 as sizes
  2. Look for previews in the data/app_xxx/preview/ folder and see they are generated at max resolution, regardless of the 64 256 specification

Expected behavior

Only previews of the specified size are generated, while other previews of larger size (up to the max specified in the config) are only generated on-demand.

And before anyone tries to say this, no, reducing the value of the preview_max_x/y parameters is not a solution as this will also force the on-demand generation of previews to be capped at that value, which is also undesirable, as this will make fullscreen pictures in the Photo app to be rendered at a low resolution, while in this case, it is desirable to get a preview at the maximum resolution possible: either the one specified in the preview_max_x/y parameter, or if they are not set, the closest to the screen resolution.

Nextcloud Server version

30

Operating system

Other

PHP engine version

PHP 8.3

Web server

None

Database engine version

None

Is this bug present after an update or on a fresh install?

None

Are you using the Nextcloud Server Encryption module?

None

What user-backends are you using?

  • Default user-backend (database)
  • LDAP/ Active Directory
  • SSO - SAML
  • Other

Configuration report

No response

List of activated Apps

No response

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

@joshtrichards
Copy link
Member

This is definitely undesirable, as it defeats the whole point of having small pre-generated previews, while having large on-demand previews, when needed.

The previewgenerator app is intended to be used to pre-generate previews instead of generating them on-demand: https://github.com/nextcloud/previewgenerator?tab=readme-ov-file#preview-generator

In fact, in the current state, the max preview is going to take the same space as the original picture in most cases, and thus, it does not make sense to pre-generate it.

The goal isn't to save disk space. It's to eliminate on-demand preview generation in environments where that is deemed desirable by the admin. And this is the documented behavior. See https://github.com/nextcloud/previewgenerator?tab=readme-ov-file#i-dont-want-to-generate-all-the-preview-sizes

This app is primarily meant for small Nextcloud servers running on cheap hardware where on-demand generation of previews is not quick enough. The app effectively trades higher disk usage for quicker previews and this trade-off should be considered carefully.

@joshtrichards joshtrichards transferred this issue from nextcloud/server Oct 13, 2024
@marco-calautti
Copy link
Author

This is definitely undesirable, as it defeats the whole point of having small pre-generated previews, while having large on-demand previews, when needed.

The previewgenerator app is intended to be used to pre-generate previews instead of generating them on-demand: https://github.com/nextcloud/previewgenerator?tab=readme-ov-file#preview-generator

In fact, in the current state, the max preview is going to take the same space as the original picture in most cases, and thus, it does not make sense to pre-generate it.

The goal isn't to save disk space. It's to eliminate on-demand preview generation in environments where that is deemed desirable by the admin. And this is the documented behavior. See https://github.com/nextcloud/previewgenerator?tab=readme-ov-file#i-dont-want-to-generate-all-the-preview-sizes

This app is primarily meant for small Nextcloud servers running on cheap hardware where on-demand generation of previews is not quick enough. The app effectively trades higher disk usage for quicker previews and this trade-off should be considered carefully.

I would say that, regardless of the use of preview generator, the function has a bug. It generates a preview it has never been requested to generate. The $specification parameter is ignored, and the max resolution preview is generated in all circumstances.

@marco-calautti
Copy link
Author

Isn't the affected code in the server repo? Why the issue has been moved to preview generator?

@marco-calautti
Copy link
Author

@joshtrichards
Copy link
Member

Related: nextcloud/server#34904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants