diff mbox series

[FFmpeg-devel,v3,1/2] libavformat/vapoursynth: Update to API version 4, load library at runtime

Message ID fdcdcdb3-ce6c-4dec-9759-489d1aa501a4@gmx.net
State New
Headers show
Series [FFmpeg-devel,v3,1/2] libavformat/vapoursynth: Update to API version 4, load library at runtime | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Stefan Oltmanns July 23, 2024, 2:51 p.m. UTC 
Hello,

this is revised patch, this is the first part that just updates to the
API v4 of VapourSynth.

Changes in API v4:
-All functions previously in header are now part of the "vssapi" object
-Renames of different types and functions
-YCoCg is not treated as different format to YUV anymore
-Some pointers to arrays are now arrays inside a struct.

Best regards
Stefan

Comments

Ramiro Polla July 28, 2024, 1:09 p.m. UTC | #1 
On 2024-07-23 16:51, Stefan Oltmanns via ffmpeg-devel wrote:
> this is revised patch, this is the first part that just updates to the
> API v4 of VapourSynth.
> 
> Changes in API v4:
> -All functions previously in header are now part of the "vssapi" object
> -Renames of different types and functions
> -YCoCg is not treated as different format to YUV anymore
> -Some pointers to arrays are now arrays inside a struct.


> From 164a440ffbb5951ca38bfff56e7b62bd677d1f52 Mon Sep 17 00:00:00 2001
> From: Stefan Oltmanns <stefan-oltmanns@gmx.net>
> Date: Tue, 23 Jul 2024 16:15:36 +0200
> Subject: [PATCH 1/2] avformat/vapoursynth: Update to API version 4
> 
> Signed-off-by: Stefan Oltmanns <stefan-oltmanns@gmx.net>
> ---
>  configure                 |  2 +-
>  libavformat/vapoursynth.c | 84 +++++++++++++++++++++------------------
>  2 files changed, 46 insertions(+), 40 deletions(-)
> 
> diff --git a/configure b/configure
> index f6f5c29fea..c50b5ad4b4 100755
> --- a/configure
> +++ b/configure
> @@ -7085,7 +7085,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_pkg_config vapoursynth "vapoursynth-script >= 55" VSScript4.h getVSScriptAPI
>  
>  
>  if enabled gcrypt; then
> diff --git a/libavformat/vapoursynth.c b/libavformat/vapoursynth.c
> index 8a2519e19a..ce15f68180 100644
> --- a/libavformat/vapoursynth.c
> +++ b/libavformat/vapoursynth.c
> @@ -25,8 +25,7 @@
>  
>  #include <limits.h>
>  
> -#include <VapourSynth.h>
> -#include <VSScript.h>
> +#include <VSScript4.h>
>  
>  #include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
> @@ -41,6 +40,7 @@
>  #include "internal.h"
>  
>  struct VSState {
> +    const VSSCRIPTAPI *vssapi;
>      VSScript *vss;
>  };
>  
> @@ -49,10 +49,10 @@ typedef struct VSContext {
>  
>      AVBufferRef *vss_state;
>  
> +    const VSSCRIPTAPI *vssapi;
>      const VSAPI *vsapi;
> -    VSCore *vscore;
>  
> -    VSNodeRef *outnode;
> +    VSNode *outnode;
>      int is_cfr;
>      int current_frame;
>  
> @@ -75,8 +75,7 @@ static void free_vss_state(void *opaque, uint8_t *data)
>      struct VSState *vss = opaque;
>  
>      if (vss->vss) {
> -        vsscript_freeScript(vss->vss);
> -        vsscript_finalize();
> +        vss->vssapi->freeScript(vss->vss);
>      }
>  }
>  
> @@ -90,7 +89,6 @@ static av_cold int read_close_vs(AVFormatContext *s)
>      av_buffer_unref(&vs->vss_state);
>  
>      vs->vsapi = NULL;
> -    vs->vscore = NULL;
>      vs->outnode = NULL;
>  
>      return 0;
> @@ -106,7 +104,7 @@ static av_cold int is_native_endian(enum AVPixelFormat pixfmt)
>      return pd && (!!HAVE_BIGENDIAN == !!(pd->flags & AV_PIX_FMT_FLAG_BE));
>  }
>  
> -static av_cold enum AVPixelFormat match_pixfmt(const VSFormat *vsf, int c_order[4])
> +static av_cold enum AVPixelFormat match_pixfmt(const VSVideoFormat *vsf, int c_order[4])
>  {
>      static const int yuv_order[4] = {0, 1, 2, 0};
>      static const int rgb_order[4] = {1, 2, 0, 0};
> @@ -128,13 +126,12 @@ static av_cold enum AVPixelFormat match_pixfmt(const VSFormat *vsf, int c_order[
>              pd->log2_chroma_h != vsf->subSamplingH)
>              continue;
>  
> -        is_rgb = vsf->colorFamily == cmRGB;
> +        is_rgb = vsf->colorFamily == cfRGB;
>          if (is_rgb != !!(pd->flags & AV_PIX_FMT_FLAG_RGB))
>              continue;
>  
> -        is_yuv = vsf->colorFamily == cmYUV ||
> -                 vsf->colorFamily == cmYCoCg ||
> -                 vsf->colorFamily == cmGray;
> +        is_yuv = vsf->colorFamily == cfYUV ||
> +                 vsf->colorFamily == cfGray;
>          if (!is_rgb && !is_yuv)
>              continue;
>  
> @@ -176,15 +173,30 @@ static av_cold int read_header_vs(AVFormatContext *s)
>      int64_t sz = avio_size(pb);
>      char *buf = NULL;
>      char dummy;
> +    char vsfmt[32];
>      const VSVideoInfo *info;
>      struct VSState *vss_state;
>      int err = 0;
>  
> +    if (!(vs->vssapi = getVSScriptAPI(VSSCRIPT_API_VERSION))) {
> +        av_log(s, AV_LOG_ERROR, "Failed to initialize VSScript (possibly PYTHONPATH not set).\n");
> +        err = AVERROR_EXTERNAL;
> +        goto done;
> +    }
> +
> +    if (!(vs->vsapi = vs->vssapi->getVSAPI(VAPOURSYNTH_API_VERSION))) {
> +        av_log(s, AV_LOG_ERROR, "Could not get VSAPI. "
> +                                "Check VapourSynth installation.\n");
> +        err = AVERROR_EXTERNAL;
> +        goto done;
> +    }
> +
>      vss_state = av_mallocz(sizeof(*vss_state));
>      if (!vss_state) {
>          err = AVERROR(ENOMEM);
>          goto done;
>      }
> +    vss_state->vssapi = vs->vssapi;
>  
>      vs->vss_state = av_buffer_create(NULL, 0, free_vss_state, vss_state, 0);
>      if (!vs->vss_state) {
> @@ -193,16 +205,9 @@ static av_cold int read_header_vs(AVFormatContext *s)
>          goto done;
>      }
>  
> -    if (!vsscript_init()) {
> -        av_log(s, AV_LOG_ERROR, "Failed to initialize VSScript (possibly PYTHONPATH not set).\n");
> -        err = AVERROR_EXTERNAL;
> -        goto done;
> -    }
> -
> -    if (vsscript_createScript(&vss_state->vss)) {
> +    if (!(vss_state->vss = vs->vssapi->createScript(NULL))) {
>          av_log(s, AV_LOG_ERROR, "Failed to create script instance.\n");
>          err = AVERROR_EXTERNAL;
> -        vsscript_finalize();
>          goto done;
>      }
>  
> @@ -235,17 +240,14 @@ static av_cold int read_header_vs(AVFormatContext *s)
>      }
>  
>      buf[sz] = '\0';
> -    if (vsscript_evaluateScript(&vss_state->vss, buf, s->url, 0)) {
> -        const char *msg = vsscript_getError(vss_state->vss);
> +    if (vs->vssapi->evaluateBuffer(vss_state->vss, buf, s->url)) {
> +        const char *msg = vs->vssapi->getError(vss_state->vss);
>          av_log(s, AV_LOG_ERROR, "Failed to parse script: %s\n", msg ? msg : "(unknown)");
>          err = AVERROR_EXTERNAL;
>          goto done;
>      }
>  
> -    vs->vsapi = vsscript_getVSApi();
> -    vs->vscore = vsscript_getCore(vss_state->vss);
> -
> -    vs->outnode = vsscript_getOutput(vss_state->vss, 0);
> +    vs->outnode = vs->vssapi->getOutputNode(vss_state->vss, 0);
>      if (!vs->outnode) {
>          av_log(s, AV_LOG_ERROR, "Could not get script output node.\n");
>          err = AVERROR_EXTERNAL;
> @@ -260,7 +262,7 @@ static av_cold int read_header_vs(AVFormatContext *s)
>  
>      info = vs->vsapi->getVideoInfo(vs->outnode);
>  
> -    if (!info->format || !info->width || !info->height) {
> +    if (!info->format.colorFamily || !info->width || !info->height) {
>          av_log(s, AV_LOG_ERROR, "Non-constant input format not supported.\n");
>          err = AVERROR_PATCHWELCOME;
>          goto done;
> @@ -280,18 +282,22 @@ static av_cold int read_header_vs(AVFormatContext *s)
>      st->codecpar->codec_id = AV_CODEC_ID_WRAPPED_AVFRAME;
>      st->codecpar->width = info->width;
>      st->codecpar->height = info->height;
> -    st->codecpar->format = match_pixfmt(info->format, vs->c_order);
> +    st->codecpar->format = match_pixfmt(&info->format, vs->c_order);
>  
>      if (st->codecpar->format == AV_PIX_FMT_NONE) {
> -        av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format %s\n", info->format->name);
> +        if(vs->vsapi->getVideoFormatName(&info->format, vsfmt))
> +            av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format %s\n", vsfmt);
> +        else
> +            av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format\n");
>          err = AVERROR_EXTERNAL;
>          goto done;
>      }
> -    av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n", info->format->name,
> -           av_get_pix_fmt_name(st->codecpar->format));
> -
> -    if (info->format->colorFamily == cmYCoCg)
> -        st->codecpar->color_space = AVCOL_SPC_YCGCO;
> +    if (vs->vsapi->getVideoFormatName(&info->format, vsfmt))
> +        av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n",
> +               vsfmt, av_get_pix_fmt_name(st->codecpar->format));
> +    else
> +        av_log(s, AV_LOG_VERBOSE, "VS format -> pixfmt %s\n",
> +               av_get_pix_fmt_name(st->codecpar->format));

Could you change this to have a single call go av_log()? Possibly using 
a %s with a string for the unknown format. Same thing for the other 
av_log() above.

[...]

Also could you give us a very minimal test script to test this?

Regards,
Ramiro
Ramiro Polla July 28, 2024, 1:15 p.m. UTC | #2 
On 2024-07-23 16:59, Stefan Oltmanns via ffmpeg-devel wrote:
> This is the second part for loading the library at runtime, changes
> compared to previous patch revisions:
> 
> -No atexit anymore
> -No global states anymore
> -Moved the registry read for Windows from a separate function inside the
> function to load the dynamic library and simplified it, reducing the
> amount windows-specific code.
> 
> Tested with 2 VapourSynth inputs on these platforms, no problems and
> clean exit:
> 
> -Linux x86_64 (Ubuntu 22.04)
> -Windows 10 x86_64
> -macOS 14 aarch64

> From 6a8e8b7d5bfcfb8eb3cb24ea1f7e14ca117882c4 Mon Sep 17 00:00:00 2001
> From: Stefan Oltmanns <stefan-oltmanns@gmx.net>
> Date: Tue, 23 Jul 2024 16:19:46 +0200
> Subject: [PATCH 2/2] avformat/vapoursynth: load library at runtime
> 
> Signed-off-by: Stefan Oltmanns <stefan-oltmanns@gmx.net>
> ---
>  configure                 |  2 +-
>  libavformat/vapoursynth.c | 65 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index c50b5ad4b4..1b6670505a 100755
> --- a/configure
> +++ b/configure
> @@ -7085,7 +7085,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 >= 55" VSScript4.h getVSScriptAPI
> +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 ce15f68180..ad1d6eac61 100644
> --- a/libavformat/vapoursynth.c
> +++ b/libavformat/vapoursynth.c
> @@ -25,7 +25,7 @@
>  
>  #include <limits.h>
>  
> -#include <VSScript4.h>
> +#include <vapoursynth/VSScript4.h>
>  
>  #include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
> @@ -39,11 +39,26 @@
>  #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"
> +#else
> +  #include <dlfcn.h>
> +  #define VSSCRIPT_NAME "libvapoursynth-script"
> +  #define VSSCRIPT_LIB VSSCRIPT_NAME SLIBSUF
> +#endif
> +
>  struct VSState {
>      const VSSCRIPTAPI *vssapi;
>      VSScript *vss;
>  };
>  
> +typedef const VSSCRIPTAPI *(*VSScriptGetAPIFunc)(int version);
> +
>  typedef struct VSContext {
>      const AVClass *class;
>  
> @@ -51,6 +66,7 @@ typedef struct VSContext {
>  
>      const VSSCRIPTAPI *vssapi;
>      const VSAPI *vsapi;
> +    void *vslibrary;
>  
>      VSNode *outnode;
>      int is_cfr;
> @@ -70,6 +86,40 @@ static const AVOption options[] = {
>      {NULL}
>  };
>  
> +static av_cold void* vs_load_library(VSScriptGetAPIFunc *get_vssapi)
> +{
> +    void *vslibrary = NULL;
> +#ifdef _WIN32
> +    const HKEY hkeys[] = {HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE};
> +    LONG r;
> +    WCHAR vss_path[512];
> +    DWORD buf_size = sizeof(vss_path) - 2;
> +    char *vss_path_utf8;
> +    int i;
> +
> +    for (i = 0; i < sizeof(hkeys); i++) {

FF_ARRAY_ELEMS(hkeys)

> +        if ((r = RegGetValueW(hkeys[i], L"SOFTWARE\\VapourSynth",
> +                      L"VSScriptDLL", RRF_RT_REG_SZ, NULL,
> +                      &vss_path, &buf_size)) == ERROR_SUCCESS)
> +            break;
> +    }
> +    if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8) == 0) {
> +        vslibrary = dlopen(vss_path_utf8, RTLD_NOW | RTLD_GLOBAL);

I think calling win32_dlopen() with a full path will be problematic for 
systems without KB2533623. win32_dlopen() might need to be fixed in a 
separate patch.

[...]

Regards,
Ramiro
Stefan Oltmanns July 29, 2024, 3:31 a.m. UTC | #3 
Am 28.07.24 um 15:09 schrieb Ramiro Polla:
>>      if (st->codecpar->format == AV_PIX_FMT_NONE) {
>> -        av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format %s\n",
>> info->format->name);
>> +        if(vs->vsapi->getVideoFormatName(&info->format, vsfmt))
>> +            av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format
>> %s\n", vsfmt);
>> +        else
>> +            av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format\n");
>>          err = AVERROR_EXTERNAL;
>>          goto done;
>>      }
>> -    av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n",
>> info->format->name,
>> -           av_get_pix_fmt_name(st->codecpar->format));
>> -
>> -    if (info->format->colorFamily == cmYCoCg)
>> -        st->codecpar->color_space = AVCOL_SPC_YCGCO;
>> +    if (vs->vsapi->getVideoFormatName(&info->format, vsfmt))
>> +        av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n",
>> +               vsfmt, av_get_pix_fmt_name(st->codecpar->format));
>> +    else
>> +        av_log(s, AV_LOG_VERBOSE, "VS format -> pixfmt %s\n",
>> +               av_get_pix_fmt_name(st->codecpar->format));
>
> Could you change this to have a single call go av_log()? Possibly using
> a %s with a string for the unknown format. Same thing for the other
> av_log() above.
>
> [...]

It now prints "(unknown)" for a video format that VapourSynth cannot
resolve. "(unknown)" is also printed at other places in ffmpeg in
similar cases, so it's consistent.

>
> Also could you give us a very minimal test script to test this?

I have attached a minimal test script, it will generate 10 frames each
black, red, green, blue in 640x480, RGB24


Best regards
Stefan
import vapoursynth as vs
core = vs.core
a = core.std.BlankClip(width=640,height=480,length=10)
b = core.std.BlankClip(width=640,height=480,length=10,color=[255,   0,   0])
c = core.std.BlankClip(width=640,height=480,length=10,color=[  0, 255,   0])
d = core.std.BlankClip(width=640,height=480,length=10,color=[  0,   0, 255])
r = a + b + c + d
r.set_output(0)
Stefan Oltmanns July 29, 2024, 3:46 a.m. UTC | #4 
Am 28.07.24 um 15:15 schrieb Ramiro Polla:
>> +    void *vslibrary = NULL;
>> +#ifdef _WIN32
>> +    const HKEY hkeys[] = {HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE};
>> +    LONG r;
>> +    WCHAR vss_path[512];
>> +    DWORD buf_size = sizeof(vss_path) - 2;
>> +    char *vss_path_utf8;
>> +    int i;
>> +
>> +    for (i = 0; i < sizeof(hkeys); i++) {
>
> FF_ARRAY_ELEMS(hkeys)

fixed

>
>> +        if ((r = RegGetValueW(hkeys[i], L"SOFTWARE\\VapourSynth",
>> +                      L"VSScriptDLL", RRF_RT_REG_SZ, NULL,
>> +                      &vss_path, &buf_size)) == ERROR_SUCCESS)
>> +            break;
>> +    }
>> +    if (r == ERROR_SUCCESS && wchartoutf8(vss_path, &vss_path_utf8)
>> == 0) {
>> +        vslibrary = dlopen(vss_path_utf8, RTLD_NOW | RTLD_GLOBAL);
>
> I think calling win32_dlopen() with a full path will be problematic for
> systems without KB2533623. win32_dlopen() might need to be fixed in a
> separate patch.
>

Yes, win32_dlopen would need to check if a full path is already given
and if yes skip all the stuff to determine it's own and system32 path,
but instead just use the given parameter directly. To check if it's a
full path it should be enough to check if it either starts with "\??\"
(NT-style path) or if the second character is ":" (win32 style path).

But is this really is needed for an operating system that reached
support end over 4 years ago and does not have a security patch applied
released over 13 years ago?
I don't know what ffmpeg's exact policy is in this case, just asking.

Best regards
Stefan
Ramiro Polla July 30, 2024, 2:12 p.m. UTC | #5 
On Mon, Jul 29, 2024 at 5:56 AM Stefan Oltmanns via ffmpeg-devel
<ffmpeg-devel@ffmpeg.org> wrote:
> Am 28.07.24 um 15:15 schrieb Ramiro Polla:
> > I think calling win32_dlopen() with a full path will be problematic for
> > systems without KB2533623. win32_dlopen() might need to be fixed in a
> > separate patch.
>
> Yes, win32_dlopen would need to check if a full path is already given
> and if yes skip all the stuff to determine it's own and system32 path,
> but instead just use the given parameter directly. To check if it's a
> full path it should be enough to check if it either starts with "\??\"
> (NT-style path) or if the second character is ":" (win32 style path).
>
> But is this really is needed for an operating system that reached
> support end over 4 years ago and does not have a security patch applied
> released over 13 years ago?
> I don't know what ffmpeg's exact policy is in this case, just asking.

Makes sense. I sent a patchset to clean this, but I haven't been able
to test on a real Windows system.

I'll test the vapoursynth patches later on Linux.

Ramiro
Stefan Oltmanns Aug. 11, 2024, 10:21 a.m. UTC | #6 
Am 30.07.24 um 16:12 schrieb Ramiro Polla:
> On Mon, Jul 29, 2024 at 5:56 AM Stefan Oltmanns via ffmpeg-devel
> <ffmpeg-devel@ffmpeg.org> wrote:
>> Am 28.07.24 um 15:15 schrieb Ramiro Polla:
>>> I think calling win32_dlopen() with a full path will be problematic for
>>> systems without KB2533623. win32_dlopen() might need to be fixed in a
>>> separate patch.
>>
>> Yes, win32_dlopen would need to check if a full path is already given
>> and if yes skip all the stuff to determine it's own and system32 path,
>> but instead just use the given parameter directly. To check if it's a
>> full path it should be enough to check if it either starts with "\??\"
>> (NT-style path) or if the second character is ":" (win32 style path).
>>
>> But is this really is needed for an operating system that reached
>> support end over 4 years ago and does not have a security patch applied
>> released over 13 years ago?
>> I don't know what ffmpeg's exact policy is in this case, just asking.
>
> Makes sense. I sent a patchset to clean this, but I haven't been able
> to test on a real Windows system.
>
> I'll test the vapoursynth patches later on Linux.
>

Any progress on this? Anything I can do?

Best regards
Stefan
Ramiro Polla Aug. 23, 2024, 12:25 p.m. UTC | #7 
On Tue, Jul 30, 2024 at 4:12 PM Ramiro Polla <ramiro.polla@gmail.com> wrote:
> On Mon, Jul 29, 2024 at 5:56 AM Stefan Oltmanns via ffmpeg-devel
> <ffmpeg-devel@ffmpeg.org> wrote:
> > Am 28.07.24 um 15:15 schrieb Ramiro Polla:
> > > I think calling win32_dlopen() with a full path will be problematic for
> > > systems without KB2533623. win32_dlopen() might need to be fixed in a
> > > separate patch.
> >
> > Yes, win32_dlopen would need to check if a full path is already given
> > and if yes skip all the stuff to determine it's own and system32 path,
> > but instead just use the given parameter directly. To check if it's a
> > full path it should be enough to check if it either starts with "\??\"
> > (NT-style path) or if the second character is ":" (win32 style path).
> >
> > But is this really is needed for an operating system that reached
> > support end over 4 years ago and does not have a security patch applied
> > released over 13 years ago?
> > I don't know what ffmpeg's exact policy is in this case, just asking.
>
> Makes sense. I sent a patchset to clean this, but I haven't been able
> to test on a real Windows system.

I finally managed to test the patches on a real Windows system.

They both look good to me, I'll apply them in a couple of days if
there are no other comments.

It would be helpful to write a page in the trac wiki with a basic
howto and common pitfalls. I had problems installing python and
vapoursynth for my user only, and then for all users on the Windows
machine. I had to uninstall everything, remove some registry keys, and
try again using administrator to install to all users. I also had to
manually install Microsoft Visual C++ Redistributable. There was no
good error message telling me what was going wrong. When I added an
av_log() with FormatMessage() after LoadLibraryW() failed, the error
message was also unhelpful.

Ramiro
Stefan Oltmanns Aug. 24, 2024, 11:06 a.m. UTC | #8 
Am 23.08.24 um 14:25 schrieb Ramiro Polla:
>
> I finally managed to test the patches on a real Windows system.
>
> They both look good to me, I'll apply them in a couple of days if
> there are no other comments.
>
> It would be helpful to write a page in the trac wiki with a basic
> howto and common pitfalls. I had problems installing python and
> vapoursynth for my user only, and then for all users on the Windows
> machine. I had to uninstall everything, remove some registry keys, and
> try again using administrator to install to all users. I also had to
> manually install Microsoft Visual C++ Redistributable. There was no
> good error message telling me what was going wrong. When I added an
> av_log() with FormatMessage() after LoadLibraryW() failed, the error
> message was also unhelpful.

Just to make that clear: You had problem installing Python/VapourSynth
on Windows, not with the patch? I know Python can be somewhat
problematic on Windows from other contexts, but VapourSynth is usually
straight forward.

The install guide for VapourSynth lists some problems you may have
experienced:

http://www.vapoursynth.com/doc/installation.html#windows-installation

-Both Python and VapourSynth must be installed as either current user or
all users/system
-On current user installs it cannot install the required Microsoft
Visual C++ Redistributable

To ensure installation of VapourSynth worked you can run "vspipe -v",
this will show the installed version if it's working or an error message
if not.

Common issue on Linux/macOS is that the Python package cannot be found,
you have to set PYTHONPATH for example like this:
PYTHONPATH=/usr/local/lib/python3.12/site-packages
ffmpeg will display the PYTHONPATH issue as a possible problem if can
find the vapoursynth library, but not initialize it.

On recent versions of macOS, it will not look for libraries in
/usr/local/lib anymore, you may have to set
DYLD_LIBRARY_PATH=/usr/local/lib

Best regards
Stefan
Ramiro Polla Aug. 26, 2024, 8:36 a.m. UTC | #9 
On Sat, Aug 24, 2024 at 1:06 PM Stefan Oltmanns via ffmpeg-devel
<ffmpeg-devel@ffmpeg.org> wrote:
> Am 23.08.24 um 14:25 schrieb Ramiro Polla:
> >
> > I finally managed to test the patches on a real Windows system.
> >
> > They both look good to me, I'll apply them in a couple of days if
> > there are no other comments.

Applied. Thank you for the patches.

> > It would be helpful to write a page in the trac wiki with a basic
> > howto and common pitfalls. I had problems installing python and
> > vapoursynth for my user only, and then for all users on the Windows
> > machine. I had to uninstall everything, remove some registry keys, and
> > try again using administrator to install to all users. I also had to
> > manually install Microsoft Visual C++ Redistributable. There was no
> > good error message telling me what was going wrong. When I added an
> > av_log() with FormatMessage() after LoadLibraryW() failed, the error
> > message was also unhelpful.
>
> Just to make that clear: You had problem installing Python/VapourSynth
> on Windows, not with the patch? I know Python can be somewhat
> problematic on Windows from other contexts, but VapourSynth is usually
> straight forward.

Yes, it was unrelated to the patch.

> The install guide for VapourSynth lists some problems you may have
> experienced:
>
> http://www.vapoursynth.com/doc/installation.html#windows-installation
>
> -Both Python and VapourSynth must be installed as either current user or
> all users/system
> -On current user installs it cannot install the required Microsoft
> Visual C++ Redistributable
>
> To ensure installation of VapourSynth worked you can run "vspipe -v",
> this will show the installed version if it's working or an error message
> if not.
>
> Common issue on Linux/macOS is that the Python package cannot be found,
> you have to set PYTHONPATH for example like this:
> PYTHONPATH=/usr/local/lib/python3.12/site-packages
> ffmpeg will display the PYTHONPATH issue as a possible problem if can
> find the vapoursynth library, but not initialize it.
>
> On recent versions of macOS, it will not look for libraries in
> /usr/local/lib anymore, you may have to set
> DYLD_LIBRARY_PATH=/usr/local/lib

Something like this could be added to the wiki.
diff mbox series

Patch

From 164a440ffbb5951ca38bfff56e7b62bd677d1f52 Mon Sep 17 00:00:00 2001
From: Stefan Oltmanns <stefan-oltmanns@gmx.net>
Date: Tue, 23 Jul 2024 16:15:36 +0200
Subject: [PATCH 1/2] avformat/vapoursynth: Update to API version 4

Signed-off-by: Stefan Oltmanns <stefan-oltmanns@gmx.net>
---
 configure                 |  2 +-
 libavformat/vapoursynth.c | 84 +++++++++++++++++++++------------------
 2 files changed, 46 insertions(+), 40 deletions(-)

diff --git a/configure b/configure
index f6f5c29fea..c50b5ad4b4 100755
--- a/configure
+++ b/configure
@@ -7085,7 +7085,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_pkg_config vapoursynth "vapoursynth-script >= 55" VSScript4.h getVSScriptAPI
 
 
 if enabled gcrypt; then
diff --git a/libavformat/vapoursynth.c b/libavformat/vapoursynth.c
index 8a2519e19a..ce15f68180 100644
--- a/libavformat/vapoursynth.c
+++ b/libavformat/vapoursynth.c
@@ -25,8 +25,7 @@ 
 
 #include <limits.h>
 
-#include <VapourSynth.h>
-#include <VSScript.h>
+#include <VSScript4.h>
 
 #include "libavutil/avassert.h"
 #include "libavutil/avstring.h"
@@ -41,6 +40,7 @@ 
 #include "internal.h"
 
 struct VSState {
+    const VSSCRIPTAPI *vssapi;
     VSScript *vss;
 };
 
@@ -49,10 +49,10 @@  typedef struct VSContext {
 
     AVBufferRef *vss_state;
 
+    const VSSCRIPTAPI *vssapi;
     const VSAPI *vsapi;
-    VSCore *vscore;
 
-    VSNodeRef *outnode;
+    VSNode *outnode;
     int is_cfr;
     int current_frame;
 
@@ -75,8 +75,7 @@  static void free_vss_state(void *opaque, uint8_t *data)
     struct VSState *vss = opaque;
 
     if (vss->vss) {
-        vsscript_freeScript(vss->vss);
-        vsscript_finalize();
+        vss->vssapi->freeScript(vss->vss);
     }
 }
 
@@ -90,7 +89,6 @@  static av_cold int read_close_vs(AVFormatContext *s)
     av_buffer_unref(&vs->vss_state);
 
     vs->vsapi = NULL;
-    vs->vscore = NULL;
     vs->outnode = NULL;
 
     return 0;
@@ -106,7 +104,7 @@  static av_cold int is_native_endian(enum AVPixelFormat pixfmt)
     return pd && (!!HAVE_BIGENDIAN == !!(pd->flags & AV_PIX_FMT_FLAG_BE));
 }
 
-static av_cold enum AVPixelFormat match_pixfmt(const VSFormat *vsf, int c_order[4])
+static av_cold enum AVPixelFormat match_pixfmt(const VSVideoFormat *vsf, int c_order[4])
 {
     static const int yuv_order[4] = {0, 1, 2, 0};
     static const int rgb_order[4] = {1, 2, 0, 0};
@@ -128,13 +126,12 @@  static av_cold enum AVPixelFormat match_pixfmt(const VSFormat *vsf, int c_order[
             pd->log2_chroma_h != vsf->subSamplingH)
             continue;
 
-        is_rgb = vsf->colorFamily == cmRGB;
+        is_rgb = vsf->colorFamily == cfRGB;
         if (is_rgb != !!(pd->flags & AV_PIX_FMT_FLAG_RGB))
             continue;
 
-        is_yuv = vsf->colorFamily == cmYUV ||
-                 vsf->colorFamily == cmYCoCg ||
-                 vsf->colorFamily == cmGray;
+        is_yuv = vsf->colorFamily == cfYUV ||
+                 vsf->colorFamily == cfGray;
         if (!is_rgb && !is_yuv)
             continue;
 
@@ -176,15 +173,30 @@  static av_cold int read_header_vs(AVFormatContext *s)
     int64_t sz = avio_size(pb);
     char *buf = NULL;
     char dummy;
+    char vsfmt[32];
     const VSVideoInfo *info;
     struct VSState *vss_state;
     int err = 0;
 
+    if (!(vs->vssapi = getVSScriptAPI(VSSCRIPT_API_VERSION))) {
+        av_log(s, AV_LOG_ERROR, "Failed to initialize VSScript (possibly PYTHONPATH not set).\n");
+        err = AVERROR_EXTERNAL;
+        goto done;
+    }
+
+    if (!(vs->vsapi = vs->vssapi->getVSAPI(VAPOURSYNTH_API_VERSION))) {
+        av_log(s, AV_LOG_ERROR, "Could not get VSAPI. "
+                                "Check VapourSynth installation.\n");
+        err = AVERROR_EXTERNAL;
+        goto done;
+    }
+
     vss_state = av_mallocz(sizeof(*vss_state));
     if (!vss_state) {
         err = AVERROR(ENOMEM);
         goto done;
     }
+    vss_state->vssapi = vs->vssapi;
 
     vs->vss_state = av_buffer_create(NULL, 0, free_vss_state, vss_state, 0);
     if (!vs->vss_state) {
@@ -193,16 +205,9 @@  static av_cold int read_header_vs(AVFormatContext *s)
         goto done;
     }
 
-    if (!vsscript_init()) {
-        av_log(s, AV_LOG_ERROR, "Failed to initialize VSScript (possibly PYTHONPATH not set).\n");
-        err = AVERROR_EXTERNAL;
-        goto done;
-    }
-
-    if (vsscript_createScript(&vss_state->vss)) {
+    if (!(vss_state->vss = vs->vssapi->createScript(NULL))) {
         av_log(s, AV_LOG_ERROR, "Failed to create script instance.\n");
         err = AVERROR_EXTERNAL;
-        vsscript_finalize();
         goto done;
     }
 
@@ -235,17 +240,14 @@  static av_cold int read_header_vs(AVFormatContext *s)
     }
 
     buf[sz] = '\0';
-    if (vsscript_evaluateScript(&vss_state->vss, buf, s->url, 0)) {
-        const char *msg = vsscript_getError(vss_state->vss);
+    if (vs->vssapi->evaluateBuffer(vss_state->vss, buf, s->url)) {
+        const char *msg = vs->vssapi->getError(vss_state->vss);
         av_log(s, AV_LOG_ERROR, "Failed to parse script: %s\n", msg ? msg : "(unknown)");
         err = AVERROR_EXTERNAL;
         goto done;
     }
 
-    vs->vsapi = vsscript_getVSApi();
-    vs->vscore = vsscript_getCore(vss_state->vss);
-
-    vs->outnode = vsscript_getOutput(vss_state->vss, 0);
+    vs->outnode = vs->vssapi->getOutputNode(vss_state->vss, 0);
     if (!vs->outnode) {
         av_log(s, AV_LOG_ERROR, "Could not get script output node.\n");
         err = AVERROR_EXTERNAL;
@@ -260,7 +262,7 @@  static av_cold int read_header_vs(AVFormatContext *s)
 
     info = vs->vsapi->getVideoInfo(vs->outnode);
 
-    if (!info->format || !info->width || !info->height) {
+    if (!info->format.colorFamily || !info->width || !info->height) {
         av_log(s, AV_LOG_ERROR, "Non-constant input format not supported.\n");
         err = AVERROR_PATCHWELCOME;
         goto done;
@@ -280,18 +282,22 @@  static av_cold int read_header_vs(AVFormatContext *s)
     st->codecpar->codec_id = AV_CODEC_ID_WRAPPED_AVFRAME;
     st->codecpar->width = info->width;
     st->codecpar->height = info->height;
-    st->codecpar->format = match_pixfmt(info->format, vs->c_order);
+    st->codecpar->format = match_pixfmt(&info->format, vs->c_order);
 
     if (st->codecpar->format == AV_PIX_FMT_NONE) {
-        av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format %s\n", info->format->name);
+        if(vs->vsapi->getVideoFormatName(&info->format, vsfmt))
+            av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format %s\n", vsfmt);
+        else
+            av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format\n");
         err = AVERROR_EXTERNAL;
         goto done;
     }
-    av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n", info->format->name,
-           av_get_pix_fmt_name(st->codecpar->format));
-
-    if (info->format->colorFamily == cmYCoCg)
-        st->codecpar->color_space = AVCOL_SPC_YCGCO;
+    if (vs->vsapi->getVideoFormatName(&info->format, vsfmt))
+        av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n",
+               vsfmt, av_get_pix_fmt_name(st->codecpar->format));
+    else
+        av_log(s, AV_LOG_VERBOSE, "VS format -> pixfmt %s\n",
+               av_get_pix_fmt_name(st->codecpar->format));
 
 done:
     av_free(buf);
@@ -311,13 +317,13 @@  static int get_vs_prop_int(AVFormatContext *s, const VSMap *map, const char *nam
     int64_t res;
     int err = 1;
 
-    res = vs->vsapi->propGetInt(map, name, 0, &err);
+    res = vs->vsapi->mapGetInt(map, name, 0, &err);
     return err || res < INT_MIN || res > INT_MAX ? def : res;
 }
 
 struct vsframe_ref_data {
     const VSAPI *vsapi;
-    const VSFrameRef *frame;
+    const VSFrame *frame;
     AVBufferRef *vss_state;
 };
 
@@ -339,7 +345,7 @@  static int read_packet_vs(AVFormatContext *s, AVPacket *pkt)
     AVStream *st = s->streams[0];
     AVFrame *frame = NULL;
     char vserr[80];
-    const VSFrameRef *vsframe;
+    const VSFrame *vsframe;
     const VSVideoInfo *info = vs->vsapi->getVideoInfo(vs->outnode);
     const VSMap *props;
     const AVPixFmtDescriptor *desc;
@@ -381,7 +387,7 @@  static int read_packet_vs(AVFormatContext *s, AVPacket *pkt)
         goto end;
     }
 
-    props = vs->vsapi->getFramePropsRO(vsframe);
+    props = vs->vsapi->getFramePropertiesRO(vsframe);
 
     frame = av_frame_alloc();
     if (!frame) {
@@ -410,7 +416,7 @@  static int read_packet_vs(AVFormatContext *s, AVPacket *pkt)
 
     desc = av_pix_fmt_desc_get(frame->format);
 
-    for (i = 0; i < info->format->numPlanes; i++) {
+    for (i = 0; i < info->format.numPlanes; i++) {
         int p = vs->c_order[i];
         ptrdiff_t plane_h = frame->height;
 
-- 
2.34.1