Re: [FFmpeg-devel] [PATCH v2] libavformat/vapoursynth: Update to API version 4, load library at runtime

[Ramiro Polla <ramiro.polla@gmail.com>]

Hi Stefan,

On 7/6/24 23:08, Stefan Oltmanns via ffmpeg-devel wrote:
> this is revised patch, to sum up the changes:
> 
> The current VapourSynth implementation is rarely used, as it links the
> VapourSynth library at build time, making the resulting build unable to
> run when VapourSynth is not installed. Therefore barely anyone compiles
> with VapourSynth activated.
> 
> I changed it, so that it loads the library at runtime when a VapourSynth
> script should be opened, just like AviSynth does.
> On Windows the DLL from VapourSynth is not installed in the system
> directory, but the location is stored in the Registry. Therefore I added
> some code to read that information from the registry.
> 
> As the V4 API is designed with dynamic loading in mind (only a single
> import), I updated the implementation to V4 (changes are mostly
> superficial, no structural changes). The V4 API is already several years
> old, fully supported since R55 released in 2021.
> 
> 
> Changes from first patch:
> -Separated the Windows-specific function for getting the DLL location
> from the platform-specific includes
> -It is not enabled by default in configure
> -The header files are not included anymore
> 
> 
> I would like to include the header files for this reason:
> While most Linux distributions ship ffmpeg, only very few contain
> VapourSynth. Therefore ffmpeg won't be compiled with VapourSynth support
> by these distributions, because no VapourSynth headers. Including the
> headers in ffmpeg would mean they can compile with VapourSynth support
> and if a user decided to install VapourSynth from somewhere else or
> compile it by himself, ffmpeg support would be ready and no need for the
> user to install another ffmpeg or compile one.
> I'm not sure what the rules for shipping include files are, as there are
> a few 3rd-party include files in ffmpeg. License is not an issue
> (Vapourynth is LGPL v2.1 or later like ffmpeg).
> 
> 
> 
> make fate runs without any issue. I tested VapourSynth input scripts
> with various color formats on different platforms:
> 
> Ubuntu 22.04
> macOS 13 (x86_64)
> macOS 13 (arm64)
> Windows 10 (msys2/gcc)
> 
> It compiles on these platforms without any warning and runs without any
> issues.

> From 759b097865953ee66949ecbcdadbebfad623c29a Mon Sep 17 00:00:00 2001
> From: Stefan Oltmanns <stefan-oltmanns@gmx.net>
> Date: Sat, 6 Jul 2024 22:56:53 +0200
> Subject: [PATCH] avformat/vapoursynth: Update to API version 4, load library
>  at runtime
> 
> Signed-off-by: Stefan Oltmanns <stefan-oltmanns@gmx.net>
> ---
>  configure                 |   3 +-
>  libavformat/vapoursynth.c | 171 +++++++++++++++++++++++++++++---------
>  2 files changed, 136 insertions(+), 38 deletions(-)
> 
> diff --git a/configure b/configure
> index b28221f258..e43f3827ec 100755
> --- a/configure
> +++ b/configure
> @@ -3575,6 +3575,7 @@ libxevd_decoder_deps="libxevd"
>  libxeve_encoder_deps="libxeve"
>  libxvid_encoder_deps="libxvid"
>  libzvbi_teletext_decoder_deps="libzvbi"
> +vapoursynth_deps_any="libdl LoadLibrary"
>  vapoursynth_demuxer_deps="vapoursynth"
>  videotoolbox_suggest="coreservices"
>  videotoolbox_deps="corefoundation coremedia corevideo"
> @@ -7080,7 +7081,7 @@ enabled rkmpp             && { require_pkg_config rkmpp rockchip_mpp  rockchip/r
>                                 { enabled libdrm ||
>                                   die "ERROR: rkmpp requires --enable-libdrm"; }
>                               }
> -enabled vapoursynth       && require_pkg_config vapoursynth "vapoursynth-script >= 42" VSScript.h vsscript_init
> +enabled vapoursynth       && require_headers "vapoursynth/VSScript4.h vapoursynth/VapourSynth4.h"
>  
>  
>  if enabled gcrypt; then
> diff --git a/libavformat/vapoursynth.c b/libavformat/vapoursynth.c
> index 8a2519e19a..9c82f8f3b8 100644
> --- a/libavformat/vapoursynth.c
> +++ b/libavformat/vapoursynth.c
> @@ -25,9 +25,6 @@
>  
>  #include <limits.h>
>  
> -#include <VapourSynth.h>
> -#include <VSScript.h>
> -
>  #include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
>  #include "libavutil/eval.h"
> @@ -40,19 +37,46 @@
>  #include "demux.h"
>  #include "internal.h"
>  
> +/* Platform-specific directives. */
> +#ifdef _WIN32
> +  #include <windows.h>
> +  #include "compat/w32dlfcn.h"
> +  #include "libavutil/wchar_filename.h"
> +  #undef EXTERN_C
> +  #define VSSCRIPT_LIB "VSScript.dll"
> +  #define VS_DLOPEN() ({ void *handle = NULL; \
> +                        char *dll_name = get_vs_script_dll_name(); \
> +                        handle = dlopen(dll_name, RTLD_NOW | RTLD_GLOBAL); \
> +                        av_free(dll_name); \
> +                        handle; })
> +#else
> +  #include <dlfcn.h>
> +  #define VSSCRIPT_NAME "libvapoursynth-script"
> +  #define VSSCRIPT_LIB VSSCRIPT_NAME SLIBSUF
> +  #define VS_DLOPEN() dlopen(VSSCRIPT_LIB, RTLD_NOW | RTLD_GLOBAL)
> +#endif
> +
> +#include <vapoursynth/VSScript4.h>
> +
>  struct VSState {
>      VSScript *vss;
>  };
>  
> +typedef const VSSCRIPTAPI *(*VSScriptGetAPIFunc)(int version);
> +
> +typedef struct VSScriptLibrary {
> +    void *library;
> +    const VSSCRIPTAPI *vssapi;
> +} VSScriptLibrary;
> +
>  typedef struct VSContext {
>      const AVClass *class;
>  
>      AVBufferRef *vss_state;
>  
>      const VSAPI *vsapi;
> -    VSCore *vscore;
>  
> -    VSNodeRef *outnode;
> +    VSNode *outnode;
>      int is_cfr;
>      int current_frame;
>  
> @@ -70,19 +94,72 @@ static const AVOption options[] = {
>      {NULL}
>  };
>  
> +static VSScriptLibrary vs_script_library;

Does vs_script_library have to be a global?

> +
> +static int vs_atexit_called = 0;
> +
> +static av_cold void vs_atexit_handler(void);
> +
> +#ifdef _WIN32
> +static av_cold char* get_vs_script_dll_name(void) {
> +     LONG r;
> +     WCHAR vss_path[512];
> +     char *vss_path_utf8;
> +     DWORD buf_size = sizeof(vss_path) - 2;
> +     r = RegGetValueW(HKEY_CURRENT_USER, L"SOFTWARE\\VapourSynth",
> +                      L"VSScriptDLL", RRF_RT_REG_SZ, NULL,
> +                      &vss_path, &buf_size);
> +     if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) == 0)
> +         return vss_path_utf8;
> +     r = RegGetValueW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\VapourSynth",
> +                      L"VSScriptDLL", RRF_RT_REG_SZ, NULL,
> +                      &vss_path, &buf_size);
> +     if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) == 0)
> +         return vss_path_utf8;
> +     if (wchartoutf8(L"VSScript.dll", &vss_path_utf8) == 0)
> +         return vss_path_utf8;
> +     return 0;
> +}
> +#endif

Don't fetch the path on the registry on Windows. The user should set the 
path outside of FFmpeg.

> +
> +static av_cold int vs_load_library(void)
> +{
> +    VSScriptGetAPIFunc get_vs_script_api;
> +    vs_script_library.library = VS_DLOPEN();
> +    if (!vs_script_library.library)
> +        return -1;
> +    get_vs_script_api = (VSScriptGetAPIFunc)dlsym(vs_script_library.library,
> +                                                  "getVSScriptAPI");
> +    if (!get_vs_script_api) {
> +        dlclose(vs_script_library.library);
> +        return -2;
> +    }
> +    vs_script_library.vssapi = get_vs_script_api(VSSCRIPT_API_VERSION);
> +    if (!vs_script_library.vssapi) {
> +        dlclose(vs_script_library.library);
> +        return -3;
> +    }
> +    atexit(vs_atexit_handler);

Can you get rid of the call to atexit()?

[...]

Could you split the patch into first moving to API 4 and then working on 
the runtime loading? The first part might be reviewed and merged faster.

Ramiro
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".