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

update(Thumbnail): Thumb without external libraries #499

Closed
wants to merge 14 commits into from

Conversation

lgmarchi
Copy link
Contributor

@lgmarchi lgmarchi commented Mar 22, 2023

What this PR does 📖

1. Updated the code, to just use crates and not necessary more external libraries to upload thumbnails for pdf files

a. External libraries can let app with bigger size and can affect performance
b. Video thumbs will keep ffmpeg lib for now

Which issue(s) this PR fixes 🔨

Special notes for reviewers 🗒️

Additional comments 🎤

@lgmarchi lgmarchi self-assigned this Mar 22, 2023
@github-actions github-actions bot added Don't merge yet DO NOT MERGE Missing dev review Still needs to be reviewed by a dev labels Mar 22, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 22, 2023

UI Automated Test Results on MacOS

    2 files  14 suites   31m 2s ⏱️
117 tests 97 ✔️ 19 💤 0  1 🔥
117 runs  96 ✔️ 19 💤 1  1 🔥

For more details on these errors, see this check.

Results for commit 5882d0f.

♻️ This comment has been updated with latest results.

@dariusc93
Copy link
Contributor

Is there any reason not to use image crate for thumbnails for media files?

@sdwoodbury
Copy link
Contributor

The image crate looks a lot more mature. 3700 stars on Github vs 5 for thumbnailer.
https://github.com/image-rs/image
https://github.com/Trivernis/thumbnailer

Also, please make the Cargo workflow pass. cargo clippy --fix would probably fix most of it for you.

image

@lgmarchi
Copy link
Contributor Author

Is there any reason not to use image crate for thumbnails for media files?

When you say media, is images and video files?

@lgmarchi lgmarchi added the Draft PR is still a draft and needs more work label Mar 23, 2023
@dariusc93
Copy link
Contributor

Is there any reason not to use image crate for thumbnails for media files?

When you say media, is images and video files?

That is correct. Looking at the dependency used, it looks like they also use image for producing thumbnails too (would have to double check though)

@lgmarchi
Copy link
Contributor Author

Is there any reason not to use image crate for thumbnails for media files?

When you say media, is images and video files?

That is correct. Looking at the dependency used, it looks like they also use image for producing thumbnails too (would have to double check though)

For video files, I didn't find any other solution without using ffmpeg, so I will back ffmpeg lib for it

thumbnailer, inside crate, is using ffmpeg lib as well to make thumb from videos

@lgmarchi lgmarchi removed the Draft PR is still a draft and needs more work label Mar 24, 2023
@phillsatellite
Copy link
Contributor

phillsatellite commented Mar 24, 2023

Hello my friend, so far on MacOS preview opened to a black screen

MacOS

Screen_Recording_2023-03-24_at_3.29.08_PM.mov

Windows-

Recording.2023-03-24.153137.mp4

@phillsatellite phillsatellite added the Changes requested When other dev or QA request a change label Mar 24, 2023
@phillsatellite phillsatellite removed the Changes requested When other dev or QA request a change label Mar 27, 2023
@phillsatellite
Copy link
Contributor

Sent Lucas a DM I had tried building but got this error thrown at me, he said he already knows the fix 💥

Screenshot 2023-03-28 at 1 01 06 PM

@phillsatellite phillsatellite added the Changes requested When other dev or QA request a change label Mar 28, 2023
@phillsatellite
Copy link
Contributor

So I pulled latest changes and error was gone, but I did have @InfamousVague and @WanderingHogan both test with me and neither of us could get the thumbnail to appear with PDF or MOV (verified we have ffmeg installed

@phillsatellite phillsatellite added Ready for testing Ready for QA to test and removed Changes requested When other dev or QA request a change Ready for testing Ready for QA to test labels Apr 3, 2023
@WanderingHogan
Copy link
Contributor

With the release coming up we are going to close this, and come back to it after we make the public builds.

@stavares843 stavares843 deleted the thumb-without-command branch May 2, 2023 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Don't merge yet DO NOT MERGE Missing dev review Still needs to be reviewed by a dev
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

chore(files): Change code from video and pdf thumbnail
5 participants