Add quality setting for JP(E)G preview images

[AlexAndBear]

Description

A new config setting previewJPEGImageDisplayQuality has been introduced with which the quality of generated JP(E)G previews can be determined.

Related Issue

• Relates to https://github.com/owncloud/enterprise/issues/4702

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: Run config-to-docs for previewJPEGImageDisplayQuality docs#4142
• Changelog item

[mmattel]

As usual, add a docs issue when close to merge 😄

[phil-davis]

Just a small typo.

[JammingBen]

Updated the PR with a changelog and improved the docs text.

I don't think we can test this in a meaningful way. imagejpeg() will always get called with the new parameter, therefore it's already being unit tested (assuming the original implementation is being tested...).

[AlexAndBear]

@JammingBen what do you think about adding the \imagejpeg PHP docs to the config.sample.php description?

[phil-davis]

If this goes ahead, I wonder what will be a suitable test? Do we save a couple of JPG previews at different quality, and the test sets the quality, let's the system generate a preview, and does a binary comparison with the saved JPG test fixture?

And existing previews that have already been generated will be at the default (or previous) quality. Is there any requirement to be able to optionally "regenerate all existing previews" (occ command or...) after the quality setting has been changed?

[AlexAndBear]

@phil-davis we were already been thinking about a command like this, which simply deletes every preview.
This would also be helpful for the txt preview improvement we have done.

But @pmaier1 needs to decide if this is a good idea

[JammingBen]

Guys, we got it wrong. Previews will always be stored as png file on the local disk. Therefore it will always have "full quality", regardless out the newly introduced setting. The setting only affects the resource in the response which is being delivered to the client (= the WebUI in our case). This is good for us, it means there is no need to regenerate the previews.

Same request, quality 100 (see the difference in size):

Quality 1:

On my local disk, the file is always exactly the same.

[AlexAndBear]

@JammingBen this is lovely, even if I don't understand why we use PNG in favour of JPGEG (we should not change it right now).

[JammingBen]

That also means we could test the behavior simply by comparing the size of 2 responses (one quality 1, the other quality 100). @phil-davis Can that be done via acceptance tests?

[phil-davis]

That also means we could test the behavior simply by comparing the size of 2 responses (one quality 1, the other quality 100). @phil-davis Can that be done via acceptance tests?

Sure can. If we just want to confirm that there are size differences, then we can just mention in the tests the expected length of the response body (or be more fancy and look inside the data for some JPG information about the "file" size and/or resolution)

[JammingBen]

At first sight I couldn't find a nice way to actually analyse the quality of an JPEG image via PHP. The quality string is included somewhere in the response body, but it seems a bit "random" to me. For now I added a test to check for the file size.

[mmattel]

...Previews will always be stored as png file on the local disk. Therefore it will always have "full quality", regardless out the newly introduced setting...

Then the config parameter should be renamed to avoid misleading and confusement.

[mmattel]

Note that I have adopted my text suggestion regarding config.sample above with regards that it not for preview images!

[mmattel]

Even a small detail,
you may want to rename the PR reflecting that the setting is not for preview images...

[JammingBen]

Note that I have adopted my text suggestion regarding config.sample above with regards that it not for preview images!

What do you mean? The setting is for preview images. Not how they are being stored, but how they are being delivered/displayed. IMO the value name is as clear as it gets now. We cannot use an endless long value name just to ensure every little detail is included :/

[mmattel]

What do you mean? The setting is for preview images. Not how they are being stored, but how they are being delivered/displayed. IMO the value name is as clear as it gets now. We cannot use an endless long value name just to ensure every little detail is included :/

The term 'preview' is confusing as people will think these are the thumbnails. It would be better to use the term 'display'. This would perfectly point to what it is and what it is not. In case we stick to preview (also ok) we have to highlight in the config.sample text that this has nothing to do with thumbnails. Just let me know the direction an I will adopt the text suggestion.

[JammingBen]

In case we stick to preview (also ok) we have to highlight in the config.sample text that this has nothing to do with thumbnails.

But we are talking about thumbnails! The only misconception here is the fact that the files_mediaviewer app uses our thumbnail API to display images. But that's an issue of the app itself. We shouldn't generalize the name because, once again, it only affects our thumbnail generation. Whoever is using it at the end.

[mmattel]

For better understanding:
We define for which types thumbnails are created in config.php via the 'enabledPreviewProviders' key.

1. Based on this setting we see in the files view thumbnails based on the content.
2. When clicking an image, it gets enlarged by the app, but possibly not in the original quality.

Is this new config setting about 1. or 2. ?

[JammingBen]

Is this new config setting about 1. or 2. ?

It's always about 1. In detail that means: the little thumbnails in the files table as well as the quite larger thumbnails on top of the sidebar.

Additionally it can be about 2., too, if you use the files_mediaviewer app to open images. This is because the app uses the preview API. If you disable previews via 'enable_previews' => false, the app will stop working, even though it's enabled. That's just a conceptional problem in general as I see it.

[mmattel]

This is a valuable info, I will adopt the config.sample suggestion above accordingly.

As this change is also for true thumbnails, how does this effect existing thumbs? is there a need for recreation?

[mmattel]

@JammingBen I have adopted the config.sample text above based on the discussion. Please have a look and commit in case you agree.

[JammingBen]

is there a need for recreation?

No, that change will be applied immediately.

@JammingBen I have adopted the config.sample text above based on the discussion. Please have a look and commit in case you agree.

Hm I cannot find it, could you link the comment here please? I only see your first review on the outdated text.

[mmattel]

Hm I cannot find it, could you link the comment here please? I only see your first review on the outdated text.

It is just below of #39349 (comment)
(note, just identified, that the suggestion itself cant be referenced...)

[JammingBen]

It's not showing for me :/

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication