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
Imaginary WebP support #38032
Imaginary WebP support #38032
Conversation
This looks good IMO. |
This comment was marked as outdated.
This comment was marked as outdated.
@JanisPlayer there are some lint errors. Can you run |
lib/private/Preview/Imaginary.php
Outdated
$mimeType = 'jpeg'; | ||
break; | ||
case 'webp': | ||
$mimeType = 'webp'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I wonder about is if webp previews would need to be explicitely enabled in the client apps. cc @tobiasKaminsky on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clients ask via thumbnail api for any image type, as long as mimetype is "image/" or "video/".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the filetype is set by the server? sounds nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was wondering about was if it is a problem if at this endpoint a webp is returner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 <54918417+JanisPlayer@users.noreply.github.com>
nextcloud#38032 (comment) Signed-off-by: JanisPlayer <54918417+JanisPlayer@users.noreply.github.com>
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! :) |
@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 |
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Rebased for you here is how I did it
|
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 <54918417+JanisPlayer@users.noreply.github.com>
Wondering if anyone has experienced what I detailed in #43878 ? |
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.
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
Checklist
On web server, android