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

Add check config.php for ffmpeg_path in PreviewManager.php. fix for snap no video preview #36225

Merged
merged 6 commits into from
Jan 18, 2023

Conversation

Jachhj-sc
Copy link
Contributor

add a check in config.php for a configured movieBinary path. so now it first checks in config.php if preview_ffmpeg_path is configured. this makes it possible to install the ffmpeg binary and use it with the previewprovider and get video previews in the snap package.

So to get previews in videos with this fix.

  1. adding ffmpeg binary to /var/snap/nextcloud/current/[create dir 'bin']/
  2. adding the following line to config.php
'preview_ffmpeg_path' => '/var/snap/nextcloud/current/bin/ffmpeg',
  1. sudo snap restart nextcloud

Signed-off-by: William william.hak57@gmail.com

Summary

TODO

  • [ ]

Checklist

add a check in config.php for a configured movieBinary path.
so now it first checks in config.php if  preview_ffmpeg_path is configured.

Signed-off-by: William <william.hak57@gmail.com>
@Jachhj-sc Jachhj-sc changed the title Update PreviewManager.php Add check config.php for ffmpeg_path in PreviewManager.php. fix for snap no video preview Jan 18, 2023
@szaimen szaimen added enhancement 3. to review Waiting for reviews labels Jan 18, 2023
@szaimen szaimen modified the milestones: Nextcloud 27, Nextcloud 26 Jan 18, 2023
@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

Hi, thanks a lot for your PR! Please add this value also to https://github.com/nextcloud/server/blob/master/config/config.sample.php. in order to document it. Thank you! :)

@szaimen szaimen requested review from nickvergessen, a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team January 18, 2023 16:32
@tcitworld
Copy link
Member

I'm not sure why the snap package can't just set /var/snap/nextcloud/current/bin (or whatever it looks like from inside) into the $PATH so that Nextcloud can use binaries put there straight away?

@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

I'm not sure why the snap package can't just set /var/snap/nextcloud/current/bin (or whatever it looks like from inside) into the $PATH so that Nextcloud can use binaries put there straight away?

cc @kyrofa ?

@kyrofa
Copy link
Member

kyrofa commented Jan 18, 2023

Because there's nothing useful there: the snap doesn't include ffmpeg, and it's read-only so users can't put things in its existing bin directories either. However, users CAN download static ffmpeg binaries and put them somewhere the snap can access, they just need to be able to tell Nextcloud where they are.

Could we have a writable, empty directory that we add to the PATH, just waiting for user customization? Sure, but that turns into both an attack vector and a support nightmare, and is thus not something we're interested in. Note that other apps have configs like this, seems pretty reasonable to add one more, if for no other reason than consistency.

@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

Yeah, fine to add this config value imho.

@nextcloud/server-backend WDYT?

@nickvergessen
Copy link
Member

Config is fine, but should be documented in the sample file

@tcitworld
Copy link
Member

tcitworld commented Jan 18, 2023

In that case, instead of adding a specific config key for the ffmpeg binary (for instance, the memories app also needs ffprobe), let's add a generic extra_path config key that can be used into the OC/BinaryFinder class. Then the calls to findBinaryPath will take this new path into account.

@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

Config is fine, but should be documented in the sample file

Thanks nickvergessen! Then @Jachhj-sc please do as advised :)

@Jachhj-sc
Copy link
Contributor Author

alright ill document it in the sample file.
shall i create a separate pull request, or link to this one for that? or what is the proper way?

@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

alright ill document it in the sample file. shall i create a separate pull request, or link to this one for that? or what is the proper way?

Please do it in this one. Thanks! :)

add preview_ffmpeg_path documentation.
document custom path for ffmpeg so it can be used by the previewprovider to create video previews with the snap package of nextcloud.

Signed-off-by: William <william.hak57@gmail.com>
add documentation preview_ffmpeg_path
@Jachhj-sc
Copy link
Contributor Author

Done added the documentation

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

LGTM but didnt test

add better description

Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: William <william.hak57@gmail.com>
fix lint issue

Co-authored-by: Simon L. <szaimen@e.mail.de>
Signed-off-by: William <william.hak57@gmail.com>
@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 18, 2023
@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

In that case, instead of adding a specific config key for the ffmpeg binary (for instance, the memories app also needs ffprobe), let's add a generic extra_path config key that can be used into the OC/BinaryFinder class. Then the calls to findBinaryPath will take this new path into account.

Oh sorry, I overread this. Not sure about this apporach. Sounds a bit over-engineered to me...

@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

Wdyt @nickvergessen?

@szaimen szaimen added 3. to review Waiting for reviews and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jan 18, 2023
@nickvergessen
Copy link
Member

yeah, also not sure about that.

@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

Okay, lets merge then as is since the memories app already has config values anyway :)

@szaimen
Copy link
Contributor

szaimen commented Jan 18, 2023

CI failure unrelated

@szaimen szaimen merged commit 752f3a3 into nextcloud:master Jan 18, 2023
@welcome
Copy link

welcome bot commented Jan 18, 2023

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@ZOKOB

This comment was marked as off-topic.

@szaimen

This comment was marked as off-topic.

@ZOKOB

This comment was marked as off-topic.

@szaimen

This comment was marked as off-topic.

@ZOKOB

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to install ffmpeg?
6 participants