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

vapoursynth: add withPlugins interface #110365

Merged
merged 3 commits into from Feb 21, 2021

Conversation

sbruder
Copy link
Contributor

@sbruder sbruder commented Jan 21, 2021

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 or nix-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 of vsedit.

This also adds the VAPOURSYNTH_PLUGIN_PATH environment variable to the mpv wrapper when vapoursynth support is enabled. Example:

wrapMpv
  (mpv-unwrapped.override {
    vapoursynthSupport = true;
    vapoursynth = vapoursynth.withPlugins [
      vapoursynth-mvtools
    ];
  })
  { }

I’m open for suggestions, as I acknowledge that this solution is somewhat of a hack.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@SuperSandro2000
Copy link
Member

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).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 110365 run on x86_64-darwin 1

1 package failed to build and are new build failure:
2 packages built:
  • vapoursynth
  • vapoursynth-mvtools

@SuperSandro2000
Copy link
Member

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).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 110365 run on x86_64-linux 1

3 packages built:
  • vapoursynth
  • vapoursynth-editor
  • vapoursynth-mvtools

@sbruder sbruder force-pushed the vapoursynth-with-plugins branch 2 times, most recently from ec2e231 to c90519e Compare January 22, 2021 19:15
@sbruder
Copy link
Contributor Author

sbruder commented Jan 22, 2021

Force pushed to rebase and to fix dependencies (propagatedBuildInputs) of plugins not being included in the python path.

Copy link
Contributor

@rnhmjoj rnhmjoj left a 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.

@SuperSandro2000
Copy link
Member

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).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 110365 run on x86_64-darwin 1

1 package failed to build and are new build failure:
1 package built:
  • vapoursynth

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 25, 2021

Ok, I managed to simplify it a bit. I think symlinkJoin instead of buildEnv is more appropriate, because what we really want here are real directories of symlinked files. Here's an updated patch

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

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 25, 2021

I tested symlinkJoin, as before, with mpv and mvtools and everything looks good.

@tadeokondrak
Copy link
Member

Here's my attempt at this which I think is more reliable, because it modifies VapourSynth's shared library:

tadeokondrak@1f00195

I also changed VSEdit to allow adding plugins without rebuilding:

tadeokondrak@d35511d

@sbruder
Copy link
Contributor Author

sbruder commented Jan 25, 2021

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?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 25, 2021

Here's my attempt at this which I think is more reliable, because it modifies VapourSynth's shared library:

Indeed, but it's more complex: I don't think I can review this.

Anyway, since plugins path can be configured via SystemPluginDir, would it be possible to patch vapoursynth to load a custom generated vapoursynth.conf that points to $out? This would not require an environment variable or adding new code.

@ofborg ofborg bot requested a review from rnhmjoj February 5, 2021 21:09
@sbruder
Copy link
Contributor Author

sbruder commented Feb 5, 2021

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

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 6, 2021 via email

@sbruder
Copy link
Contributor Author

sbruder commented Feb 10, 2021

Sorry for the wrong pings/review requests, something went wrong while rebasing.

@sbruder
Copy link
Contributor Author

sbruder commented Feb 10, 2021

I swiched to @tadeokondrak’s approach, wrapped vspipe and added the needed makeWrapper options to mpv.

Also, to make propagatedBuildInputs work, I added a function that recursively gets the propagatedBuildInputs of all plugins and added the python environment as path to the environment. I’m not sure if this is the best way to do it.

@tadeokondrak
Copy link
Member

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 $out/lib/vapoursynth.

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

@sbruder
Copy link
Contributor Author

sbruder commented Feb 12, 2021

That seems like a good idea. I applied your patch.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 21, 2021

I tested again today and everything seems in order. I would perhaps move the code to an external expression and do withPlugins = import ./plugins-interface.nix to keep the main package shorter.

@sbruder
Copy link
Contributor Author

sbruder commented Feb 21, 2021

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.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 21, 2021

Looks good to me. @tadeokondrak?

Copy link
Member

@tadeokondrak tadeokondrak left a 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.

@rnhmjoj rnhmjoj merged commit 310f91c into NixOS:master Feb 21, 2021
@sbruder
Copy link
Contributor Author

sbruder commented Feb 22, 2021

Thanks for helping me with this PR and merging it!

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.

None yet

4 participants