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
vapoursynth: add withPlugins interface #110365
Conversation
This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch). Result of 1 package failed to build and are new build failure:
2 packages built:
|
This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch). Result of 3 packages built:
|
ec2e231
to
c90519e
Compare
Force pushed to rebase and to fix dependencies ( |
c90519e
to
6465b89
Compare
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.
Thank you for this! I have just tested it with mpv and it's working great.
I'm just not entirely convinced by the buildEnv
workaround: I'll try asking around if there's a better way.
This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch). Result of 1 package failed to build and are new build failure:
1 package built:
|
Ok, I managed to simplify it a bit. I think commit cd7026359549cf0ee6efa7de256c52e31a80e3de
Author: Simon Bruder <simon@sbruder.de>
Date: Thu Jan 21 15:53:58 2021 +0100
vapoursynth: add withPlugins interface
Since nixpkgs does not currently have an attribute set with plugins, the
usage differes from python-style withPackages as it accepts a list
instead of a function as a parameter.
Using plugins in other programs that require vapoursynth (like `vsedit`)
can be achieved by adding it to the plugin list (e.g.
`vapoursynth.withPlugins [ vapoursynth-editor … ]`) or by passing the
`VAPOURSYNTH_PLUGIN_PATH` variable.
diff --git a/pkgs/development/libraries/vapoursynth/default.nix b/pkgs/development/libraries/vapoursynth/default.nix
index 93f8d3c5ae5..6723868746d 100644
--- a/pkgs/development/libraries/vapoursynth/default.nix
+++ b/pkgs/development/libraries/vapoursynth/default.nix
@@ -1,4 +1,6 @@
-{ lib, stdenv, fetchFromGitHub, pkg-config, autoreconfHook, makeWrapper
+{ lib, stdenv, fetchFromGitHub, pkg-config, autoreconfHook
+, symlinkJoin, makeWrapper
+, vapoursynth
, zimg, libass, python3, libiconv
, ApplicationServices
, ocrSupport ? false, tesseract ? null
@@ -21,6 +23,10 @@ stdenv.mkDerivation rec {
sha256 = "1krfdzc2x2vxv4nq9kiv1c09hgj525qn120ah91fw2ikq8ldvmx4";
};
+ patches = [
+ ./plugin-path-environment-variable.patch
+ ];
+
nativeBuildInputs = [ pkg-config autoreconfHook makeWrapper ];
buildInputs = [
zimg libass
@@ -42,6 +48,30 @@ stdenv.mkDerivation rec {
# wrapper, what python3 version was used to build vapoursynth so
# the right python3.sitePackages will be used there.
inherit python3;
+
+ withPlugins = plugins:
+ let
+ pythonEnvironment = python3.buildEnv.override { extraLibs = plugins; };
+ in
+ symlinkJoin {
+ name = "${vapoursynth.name}-with-plugins";
+ paths = [ vapoursynth ] ++ plugins;
+ buildInputs = [ makeWrapper ];
+ postBuild = ''
+ for symlink in $out/bin/*; do
+ # remove the original symlink
+ original="$(realpath "$symlink")"
+ rm "$symlink"
+
+ makeWrapper "$original" "$symlink" \
+ --set VAPOURSYNTH_PLUGIN_PATH $out/lib/vapoursynth \
+ --prefix PYTHONPATH : ${pythonEnvironment}/${python3.sitePackages}
+ done
+ '';
+
+ # passthrus must be specified here, again
+ passthru = { inherit python3; };
+ };
};
postInstall = ''
@@ -54,7 +84,6 @@ stdenv.mkDerivation rec {
homepage = "http://www.vapoursynth.com/";
license = licenses.lgpl21;
platforms = platforms.x86_64;
- maintainers = with maintainers; [ rnhmjoj tadeokondrak ];
+ maintainers = with maintainers; [ rnhmjoj sbruder tadeokondrak ];
};
-
}
diff --git a/pkgs/development/libraries/vapoursynth/plugin-path-environment-variable.patch b/pkgs/development/libraries/vapoursynth/plugin-path-environment-variable.patch
new file mode 100644
index 00000000000..80f98633d7f
--- /dev/null
+++ b/pkgs/development/libraries/vapoursynth/plugin-path-environment-variable.patch
@@ -0,0 +1,16 @@
+diff --git a/src/core/vscore.cpp b/src/core/vscore.cpp
+index 2d29844d..fd4cbea5 100644
+--- a/src/core/vscore.cpp
++++ b/src/core/vscore.cpp
+@@ -1351,6 +1351,11 @@ VSCore::VSCore(int threads) :
+ } // If neither exists, an empty string will do.
+ #endif
+
++ const char *pluginPath = getenv("VAPOURSYNTH_PLUGIN_PATH");
++ if (pluginPath) {
++ loadAllPluginsInPath((std::string)pluginPath, filter);
++ }
++
+ VSMap *settings = readSettings(configFile);
+ const char *error = vs_internal_vsapi.getError(settings);
+ if (error) {
|
I tested |
Here's my attempt at this which I think is more reliable, because it modifies VapourSynth's shared library: I also changed VSEdit to allow adding plugins without rebuilding: |
Thanks @rnhmjoj for the suggestions. That looks cleaner than my workaround. @tadeokondrak: Your attempt looks very nice, since it doesn’t rely on hacks (environment variables) like mine, thank you! It worked when I tried it with a basic setup in vsedit, however I didn’t yet have the time to test it with mpv or in more depth. Do you want to open a new PR for your implementation? |
Indeed, but it's more complex: I don't think I can review this. Anyway, since plugins path can be configured via |
6465b89
to
837b33d
Compare
I rebased onto master and incorporated @rnhmjoj’s suggestions. I looked into @tadeokondrak’s attempt again. I agree that using a preload is complex, but given vapoursynth’s limited ability to load plugins from dynamic locations, it seems like any solution requires at least some new code. |
I rebased onto master and incorporated @rnhmjoj’s suggestions.
Thanks
it seems like any solution requires at least some new code.
Yes, I came to the same conclusion, unfortunately.
using that mechanism to load plugins from a configuration file outside
of vapoursynth’s store path would also require adding quite a bit of
new code (at least from my understanding, please correct me if I’m
mistaken).
No, you're right: mine wasn't a good idea afterwall :)
Both yours and @tadeokondrak's solutions look good to me, I don't have a
strong preference for either one.
…On 05-02-21, Simon Bruder wrote:
I rebased onto master and incorporated @rnhmjoj’s suggestions.
I looked into @tadeokondrak’s attempt again. `vspipe` in the vapoursynth package isn’t wrapped, but when manually passing `LD_LIBRARY_PATH`, it works. With that, mpv and plain python scripts that import vapoursynth also work.
I agree that using a preload is complex, but given vapoursynth’s limited ability to load plugins from dynamic locations, it seems like any solution requires at least some new code.
It is relatively easy to patch vapoursynth to use a static configuration file in `$out`, but if I understand it correctly, that would require recompiling vapoursynth every time the plugins change. Since vapoursynth uses a hardcoded path to the configuration file (https://github.com/vapoursynth/vapoursynth/blob/d2f6cb0294e725605e8d86596172f3e638de7435/src/core/vscore.cpp#L1339-L1352), using that mechanism to load plugins from a configuration file outside of vapoursynth’s store path would also require adding quite a bit of new code (at least from my understanding, please correct me if I’m mistaken).
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#110365 (comment)
|
837b33d
to
1c127f8
Compare
1c127f8
to
4bf1c8c
Compare
Sorry for the wrong pings/review requests, something went wrong while rebasing. |
I swiched to @tadeokondrak’s approach, wrapped Also, to make |
4bf1c8c
to
51e1ec5
Compare
I asked for feedback from someone else and they asked why the plugin loader doesn't directly use the path to the wrapper store directory. This is because that would be a circular dependency, but it made me think that we should at least link all the plugins to diff --git a/pkgs/development/libraries/vapoursynth/default.nix b/pkgs/development/libraries/vapoursynth/default.nix
index 2bc0d00b521..70fd297610a 100644
--- a/pkgs/development/libraries/vapoursynth/default.nix
+++ b/pkgs/development/libraries/vapoursynth/default.nix
@@ -1,5 +1,5 @@
{ lib, stdenv, fetchFromGitHub, pkg-config, autoreconfHook, makeWrapper
-, runCommandCC, runCommand, vapoursynth, writeText, patchelf
+, runCommandCC, runCommand, vapoursynth, writeText, patchelf, buildEnv
, zimg, libass, python3, libiconv
, ApplicationServices
, ocrSupport ? false, tesseract ? null
@@ -60,6 +60,12 @@ stdenv.mkDerivation rec {
deepPlugins = plugins ++ (getRecursivePropagatedBuildInputs plugins);
+ pluginsEnv = buildEnv {
+ name = "vapoursynth-plugins-env";
+ pathsToLink = [ "/lib/vapoursynth" ];
+ paths = deepPlugins;
+ };
+
pluginLoader = let
source = writeText "vapoursynth-nix-plugins.c" ''
void VSLoadPluginsNix(void (*load)(void *data, const char *path), void *data) {
@@ -102,6 +108,10 @@ stdenv.mkDerivation rec {
--replace "${vapoursynth}" "$out"
done
+ for binaryPlugin in ${pluginsEnv}/lib/vapoursynth/*; do
+ ln -s $binaryPlugin $out/''${binaryPlugin#"${pluginsEnv}/"}
+ done
+
for pythonPlugin in ${pythonEnvironment}/${python3.sitePackages}/*; do
ln -s $pythonPlugin $out/''${pythonPlugin#"${pythonEnvironment}/"}
done |
51e1ec5
to
e0428e1
Compare
That seems like a good idea. I applied your patch. |
I tested again today and everything seems in order. I would perhaps move the code to an external expression and do |
e0428e1
to
7fe4680
Compare
Splitting the function into its own file is a good idea considering it amounts for more than half of the code. I rebased and incorporated this suggestion. |
7fe4680
to
73a34b3
Compare
Co-authored-by: Simon Bruder <simon@sbruder.de>
73a34b3
to
b6f3c4d
Compare
Looks good to me. @tadeokondrak? |
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.
Tested with vspipe and vsedit again to be sure, looks good and works well.
Thanks for helping me with this PR and merging it! |
This PR patches vapoursynth to allow specifying the plugin directory in an environment variable. This allows for a
withPlugins
interface to create a vapoursynth environment with the specified plugins.Motivation for this change
Currently, there is no nice way for using vapoursynth with plugins. A workaround exists (#58859 (comment)), but has its drawbacks (e.g. temporary usage with
nix run
ornix-shell
does not work, it does not support multiple environments with different plugins etc.).Since there is no upstream support for specifying a plugin directory other than in the global configuration file, I added the ability to load plugins from the directory specified in
VAPOURSYNTH_PLUGIN_PATH
as a patch. This is probably not the best option for implementing this, but my other idea (resolving the plugin path relative to the shared library at runtime) would require copying (instead of symlinking) the vapoursynth shared library so the plugin path can be found relative to it. That seemed even worse, so I chose the former option.An example plugin environment can be built from the following expression:
vapoursynth.withPlugins [ vapoursynth-mvtools vapoursynth-editor ]
This includes
vapoursynth-editor
as a plugin (even though it isn’t), since that way the plugin path also gets added to the wrapper ofvsedit
.This also adds the
VAPOURSYNTH_PLUGIN_PATH
environment variable to the mpv wrapper when vapoursynth support is enabled. Example:I’m open for suggestions, as I acknowledge that this solution is somewhat of a hack.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)