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

Change thumbnails API and implement gif thumbnails #3272

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Mar 4, 2022

By popular demand, I have implement animated thumbnails for gifs.
This PR is not ready yet. I want to refactor the thumbnail code a little bit more and also the download of the thumbnails from the thumbnails service shouldn't be done via grpc. I have run into the problem that the gif thumbnails were bigger than the allowed grpc message size. So to prevent this I would implement a similar solution like the downloads in reva were there is a http endpoint for the actual download.

API changes are implemented.

Here is a preview of the animated thumbnails:

2022-03-04.19-10-06.mp4

In addition the thumbnails API was changed a bit because GRPC only allows certain message sizes and while testing some gif thumbnails I already ran into the maximum. These changes allow bigger thumbnails. (file size not resolution, but it most often correlates so...)

@update-docs
Copy link

update-docs bot commented Mar 4, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@dschmidt
Copy link
Member

dschmidt commented Mar 7, 2022

Probably not neccessary for the first iteration, but would it make sense to add an option to retrieve a still image nevertheless?

I assume when opening a folder full of animated gifs, you might not want to see every animation at once but only see animations in the mediaviewer (still scaled down) or when hovering over the icon (or something like that).

@C0rby
Copy link
Contributor Author

C0rby commented Mar 7, 2022

Yeah, I thought about that too. Would make sense IMO.

@kulmann
Copy link
Member

kulmann commented Mar 7, 2022

I assume when opening a folder full of animated gifs, you might not want to see every animation at once but only see animations in the mediaviewer (still scaled down) or when hovering over the icon (or something like that).

+9000 for animated gif thumbnail on hover in the web ui 😍

 Improved the thumbnails API so that the binary files won't be
transported via GRPC. GRPC has a limited message size and isn't very
effiefficient with large binary data.
@sonarcloud
Copy link

sonarcloud bot commented Mar 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.9% 0.9% Coverage
12.9% 12.9% Duplication

@C0rby C0rby changed the title generate animated thumbnails for animated gifs Change thumbnails API and implement gif thumbnails Mar 8, 2022
@C0rby C0rby marked this pull request as ready for review March 8, 2022 22:38
Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me! 👍

@butonic butonic merged commit 139e437 into master Mar 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the animated-gif-thumbnails branch March 9, 2022 11:06
ownclouders pushed a commit that referenced this pull request Mar 9, 2022
Merge: 0dcf952 95ae3b8
Author: Jörn Friedrich Dreyer <jfd@owncloud.com>
Date:   Wed Mar 9 12:06:16 2022 +0100

    Merge pull request #3272 from owncloud/animated-gif-thumbnails

    Change thumbnails API and implement gif thumbnails
@micbar micbar mentioned this pull request Mar 29, 2022
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants