Imaginary WebP support by JanisPlayer · Pull Request #38032 · nextcloud/server

JanisPlayer · 2023-05-03T01:49:50Z

Resolves: # Imaginary url-signature-key missing and webp and avif support #37993)

Summary

Hello, is my first pull, it was suggested to me in a comment and I must admit I don't really know how to do something like this.
Anyway, this code adds Imaginary WebP support and allows to add an API key.

In theory you could also extend this with e.g. Imaginary_query in the config, where you could just add something like that in the config itself.

However, until now I only wanted to add these two options.

  'preview_format' => 'webp',
  occ config:app:set preview webp_quality --value="30"

If preview_format or webp_quality doesn't exist the default will be used again.

So far WebP works exclusively for Imaginary, if it should work for the other generations I might need some help to find my way around the code.

As a small warning, I am not a skilled application developer only a hobby developer, because it has bothered that WebP does not work.

But I wanted to contribute something to the project and make it work faster.

Hope I did this right.

TODO

for image/png application/illustrator you could use lossless or close to lossless WebP, because i don't know now how to do that lossless with imaginary. image/heic you could actually add in WebP support.

Checklist

Code is properly formatted
Tested
On web server, android

lib/private/Preview/Imaginary.php

artonge · 2023-05-03T13:02:21Z

This looks good IMO.
Some documentation will be needed, can you take care of that @JanisPlayer ?

artonge · 2023-05-04T08:39:55Z

@JanisPlayer there are some lint errors. Can you run composer run cs:fix and push the changes? It should be enough.

lib/private/Preview/Imaginary.php

szaimen · 2023-05-04T08:46:20Z

lib/private/Preview/Imaginary.php

+						$mimeType = 'jpeg';
+					    break;
+					case 'webp':
+						$mimeType = 'webp';


what I wonder about is if webp previews would need to be explicitely enabled in the client apps. cc @tobiasKaminsky on this

Clients ask via thumbnail api for any image type, as long as mimetype is "image/" or "video/".

so the filetype is set by the server? sounds nice!

In general yes. I do not know nothing about this imaginary.
But in e.g. NC26 I can upload any image format and ask for a thumbnail.
Either server can handle it, then I receive a thumbnail or it cannot, then clients show placeholder.

API is: /index.php/apps/files/api/v1/thumbnail/128/128/test/image.tiff

This also works for office files, etc.

What I was wondering about was if it is a problem if at this endpoint a webp is returner?

Not sure if it is somewhere stated, or just done all the time, but thumbnails are all pngs.
This should be kept this way.

So for thumbnails, above endpoint, all must convert to png.

config/config.sample.php

Since it is a getAppValue and not a getSystemValue, the setting cannot be changed like this. But since it is not yet clear for the rest of the settings which setting in the config is changeable and which is not, there may be another change. nextcloud/server#38032 (comment) Signed-off-by: JanisPlayer <[email protected]>

lib/private/Preview/Imaginary.php

nextcloud#38032 (comment) Signed-off-by: JanisPlayer <[email protected]>

szaimen · 2023-05-19T08:08:15Z

Hi @JanisPlayer can you please create a separate PR for the imaginary_key feature and remove it from this PR? This allows for an easier review and mergeability. Thanks a lot in advance! :)

miaulalala · 2023-08-31T13:58:42Z

@JanisPlayer if you want some help rebasing your PR, join us here https://cloud.nextcloud.com/call/xs25tz5y and we can chat / have a call

Signed-off-by: John Molakvoæ <[email protected]>

skjnldsv · 2023-09-02T07:36:55Z

Rebased for you here is how I did it

git reset --hard 92477bc018785eea473cf0e471437add09e3bc3e, to reset to the last commit before the last three merge you had
git rebase -i --autosquash HEAD~14 and then edit all but the first to add f instead of pick and squash all commits into one
git fetch origin master:master
git rebase master
Then force push

Since it is a getAppValue and not a getSystemValue, the setting cannot be changed like this. But since it is not yet clear for the rest of the settings which setting in the config is changeable and which is not, there may be another change. nextcloud/server#38032 (comment) Signed-off-by: JanisPlayer <[email protected]>

invario · 2024-03-14T17:33:09Z

Wondering if anyone has experienced what I detailed in #43878 ?

solracsf changed the title ~~This code adds Imaginary WebP support and allows to add an API key in the config.~~ Imaginary WebP support and allows to add an API key in the config May 3, 2023

solracsf reviewed May 3, 2023

View reviewed changes