-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(previews): allow ffmpeg to connect direct for AWS S3 buckets #53634
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
base: master
Are you sure you want to change the base?
Conversation
2adc819 to
e9cdee4
Compare
|
Updated with a test to see if encryption is enabled on the file, and if so, do not attempt to connect direct. |
e9cdee4 to
14b91cd
Compare
come-nc
left a comment
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.
Looks good codewise, but needs tests.
Not sure why there is the str_starts_with test for http?
The test is needed because ffmpeg doesn't support smb out of the box and I'm not even sure it would work without passing credentials directly on the command line. Same thing for ftp and potentially others. |
14b91cd to
ebefeb7
Compare
|
Fixed my commit message and force pushed. |
Hmm, yeah but a provider may also give an http link that does not work without credentials then? It feels a bit arbitrary to me that http* is fine and all else is not. |
Very true. I can add a parameter in addition to the edit: Added. |
ebefeb7 to
a581ed6
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
1f8ffd3 to
42ca705
Compare
|
Updated to ensure the stream is seekable as supposedly not all AWS S3 streams are, So even though I have PR #53952 in the works using a sparse file generation method, I think having this one put in place also would be worthwhile, as it allows |
7a18ea3 to
5658824
Compare
|
Squashed commits and cs fixed. |
5658824 to
7fede77
Compare
|
Pushed again, cs should be good now. |
8701212 to
1381ee4
Compare
|
Rebased to 33 and reworked the code into the new |
f58eefe to
73a42d6
Compare
73a42d6 to
a3ec763
Compare
Co-authored-by: Côme Chilliet <[email protected]> Signed-off-by: invario <[email protected]>
a3ec763 to
ca45b70
Compare
|
Not sure if this will ever get merged but rebased and pushed just in case. |
Sorry got sidetracked, but it's in my personal todo list to get that and the change in #54436 in 33. |
Summary
Allow preview generation on AWS S3 buckets by creating a presigned URL for the file and then passing it directly to ffmpeg which is able to handle http/https already. It can locate the moov atom even if it's at the end of the video file and it does not need to retrieve the whole file to do it.
The presigned URL expires after 10 minute which should be more than enough for this purpose.
TODO
Checklist