vf_vapoursynth: update for VapourSynth R74#17731
vf_vapoursynth: update for VapourSynth R74#17731CrendKing wants to merge 4 commits intompv-player:masterfrom
Conversation
VapourSynth R74 changed two things relevant to mpv build process: 1) VSScript no longer has static library to link to, and only support runtime loading. This is reflected on the new VSScript example in VapourSynth SDK. 2) The VSScript library name is changed from libvapoursynth-script to libvsscript for Linux and MacOS. The new VSScript initialization code is slightly modified from their SDK example to fit in the mpv logic. Build dependency version is also update.
|
Not good at all. It should be implemented in a backward-compatible way instead. |
This comment was marked as outdated.
This comment was marked as outdated.
|
This approach should be backward compatible. I removed the dependency related changes. Will squash all commits later. |
|
|
||
| vapoursynth = dependency('vapoursynth', version: '>= 74', required: get_option('vapoursynth')) | ||
| vapoursynth_script = dependency('vapoursynth-script', version: '>= 74', | ||
| vapoursynth = dependency('vapoursynth', version: '>= 56', required: get_option('vapoursynth')) |
There was a problem hiding this comment.
If we're going to load Vapoursynth on runtime why is this meson feature necessary?
There was a problem hiding this comment.
We still need the header files, right?
There was a problem hiding this comment.
Unfortunately this will also link library. It should be header only. I don't know if meson already support header only dependencies, it was long standing limitation.
|
|
||
| #ifdef _WIN32 | ||
| const wchar_t *vsscript_path = _wgetenv(L"VSSCRIPT_PATH"); | ||
| const HMODULE lib = LoadLibraryExW(vsscript_path ? vsscript_path : L"VSScript.dll", NULL, LOAD_WITH_ALTERED_SEARCH_PATH); |
There was a problem hiding this comment.
You should use our dlopen and getenv helpers from io.h
There was a problem hiding this comment.
Are you sure? First, I see io.h only support getenv() with char version, no wchar_t. Doesn't that potentially have UTF-8 problem? Second, the mp_dlopen() always resolve relative path to full path, so a VSScript.dll becomes <mpv_exe_dir>\VSScript.dll, which pretty much never exists. Previous behavior was effectively LoadLibrary(), which will search the standard paths including the directories in PATH.
There was a problem hiding this comment.
mp_dlopen is not correct in this case.
There was a problem hiding this comment.
I agree. I think we should keep the original _wgetenv + LoadLibrary pattern. Also, do you guys prefer LoadLibrary or LoadLibraryEx? Do you care about DLL hijacking, so no LOAD_WITH_ALTERED_SEARCH_PATH?
There was a problem hiding this comment.
I prefer _wgetenv + LoadLibraryExW(..., LOAD_WITH_ALTERED_SEARCH_PATH) pattern. LOAD_WITH_ALTERED_SEARCH_PATH also helps locate dependent DLLs (e.g. vapoursynth.dll alongside VSScript.dll, making it preferable over plain LoadLibraryW.
There was a problem hiding this comment.
LOAD_LIBRARY_SEARCH_DEFAULT_DIRS | LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR
Since I only use portable ver before r74. From my view...
- IF users add VS DLLs in the same dir of mpv.exe → APPLICATION_DIR could find.
- IF users put VS in the another dir which has been added into PATH. It cannot work, right?
There was a problem hiding this comment.
Sorry, I was wrong. LOAD_WITH_ALTERED_SEARCH_PATH is the way to go.
| #else | ||
| const char *lib_name_r74 = "libvsscript.so.4"; | ||
| const char *lib_name_old = "libvapoursynth-script.so.4"; | ||
| #endif |
There was a problem hiding this comment.
You can define it at the top and assign as VSSCRIPT_PATH or something.
There was a problem hiding this comment.
Can you elaborate? Are you suggesting to first put these in the VSSCRIPT_PATH environment variable if that variable wasn't exist?
|
Here is a version using the |
| vapoursynth = dependency('vapoursynth', version: '>= 56', required: get_option('vapoursynth')) | ||
| vapoursynth_script = dependency('vapoursynth-script', version: '>= 56', | ||
| required: get_option('vapoursynth')) | ||
| vapoursynth_script = dependency('vapoursynth-script', version: '>= 56', required: get_option('vapoursynth')) |
There was a problem hiding this comment.
It may break r74 compat too.
There is no vapoursynth_script in r74 right? Does it disable mpv's vs filter feature when compling with r74 already installed in system?
There was a problem hiding this comment.
The vapoursynth script (VSScript) still exists in R74. The only change is that they renamed the dynamic library name in Linux and MacOS. The header file name is still VSScript4.h. Since we are requiring less from the dependency, from compile point of view there is no change at all. Should work on all VapourSynth versions (after R56).
4 participants
VapourSynth R74 changed two things relevant to mpv build process:
The new VSScript initialization code is slightly modified from their SDK example to fit in the mpv logic. Build dependency version is also update.
Previously, the VapourSynth directory (which contains VapourSynth and VSScript DLLs) must be in
PATH(which VapourSynth installer should takes care). After R74, there is new installation instructions which setsVSSCRIPT_PATHenvironment variable instead. Change toPATHis no longer needed.