Skip to content

Conversation

@invario
Copy link
Contributor

@invario invario commented Jun 21, 2025

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

  • Need more testing.
  • Open to suggestions, critiques, criticisms.

Checklist

@invario invario requested a review from a team as a code owner June 21, 2025 05:34
@invario invario requested review from leftybournes, nfebe and skjnldsv and removed request for a team June 21, 2025 05:34
@skjnldsv skjnldsv requested review from come-nc and juliusknorr and removed request for skjnldsv June 21, 2025 07:56
@invario invario force-pushed the preview-direct-download branch from 2adc819 to e9cdee4 Compare June 22, 2025 14:25
@invario
Copy link
Contributor Author

invario commented Jun 22, 2025

Updated with a test to see if encryption is enabled on the file, and if so, do not attempt to connect direct.

@invario invario force-pushed the preview-direct-download branch from e9cdee4 to 14b91cd Compare June 22, 2025 14:30
Copy link
Contributor

@come-nc come-nc left a 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?

@invario
Copy link
Contributor Author

invario commented Jun 23, 2025

Looks good codewise, but needs tests. Not sure why there is the str_starts_with test for http?

The test is needed because getDirectDownload varies by the external mount provider. SMB, for example, doesn't currently return anything but in the future it might return something like smb://user@example.domain/foo.bar

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.

@invario invario force-pushed the preview-direct-download branch from 14b91cd to ebefeb7 Compare June 24, 2025 01:38
@invario
Copy link
Contributor Author

invario commented Jun 24, 2025

Fixed my commit message and force pushed.

@invario invario changed the title enhancement(previews): allow ffmpeg to connect direct for AWS S3 buckets feat(previews): allow ffmpeg to connect direct for AWS S3 buckets Jun 24, 2025
@come-nc
Copy link
Contributor

come-nc commented Jun 24, 2025

Looks good codewise, but needs tests. Not sure why there is the str_starts_with test for http?

The test is needed because getDirectDownload varies by the external mount provider. SMB, for example, doesn't currently return anything but in the future it might return something like smb://user@example.domain/foo.bar

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.

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.

@invario
Copy link
Contributor Author

invario commented Jun 24, 2025

Looks good codewise, but needs tests. Not sure why there is the str_starts_with test for http?

The test is needed because getDirectDownload varies by the external mount provider. SMB, for example, doesn't currently return anything but in the future it might return something like smb://user@example.domain/foo.bar
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.

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 url one. Perhaps a presigned one and check for its value?

edit: Added.

@invario invario force-pushed the preview-direct-download branch from ebefeb7 to a581ed6 Compare June 24, 2025 14:23
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2025

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

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.)

@invario
Copy link
Contributor Author

invario commented Jul 25, 2025

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 ffmpeg to handle operations itself directly.

@skjnldsv skjnldsv added the 2. developing Work in progress label Aug 19, 2025
@invario
Copy link
Contributor Author

invario commented Aug 21, 2025

Squashed commits and cs fixed.

@invario invario force-pushed the preview-direct-download branch from 5658824 to 7fede77 Compare August 25, 2025 16:28
@invario
Copy link
Contributor Author

invario commented Aug 25, 2025

Pushed again, cs should be good now.

@invario invario force-pushed the preview-direct-download branch 2 times, most recently from 8701212 to 1381ee4 Compare September 5, 2025 15:07
@invario
Copy link
Contributor Author

invario commented Sep 5, 2025

Rebased to 33 and reworked the code into the new Movie.php

@invario invario force-pushed the preview-direct-download branch 7 times, most recently from f58eefe to 73a42d6 Compare September 8, 2025 13:06
@invario invario force-pushed the preview-direct-download branch from 73a42d6 to a3ec763 Compare October 18, 2025 16:09
Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
Signed-off-by: invario <67800603+invario@users.noreply.github.com>
@invario invario force-pushed the preview-direct-download branch from a3ec763 to ca45b70 Compare October 18, 2025 16:28
@invario
Copy link
Contributor Author

invario commented Oct 18, 2025

Not sure if this will ever get merged but rebased and pushed just in case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants