vf_vapoursynth: update for VapourSynth R74#17731
vf_vapoursynth: update for VapourSynth R74#17731CrendKing wants to merge 5 commits intompv-player:masterfrom
Conversation
|
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. |
|
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).
There was a problem hiding this comment.
Please noted that the pkg-config file for vapoursynth-script in R74 was a mistake and has been removed in the latest commit. You should only use dependency('vapoursynth') after R74.
There was a problem hiding this comment.
has been removed
Which file are you referring to? The one from shinchiro is still there.
after R74
But folks above want to be backward compatible, so that it works regardless if mpv is built against pre-R74 or post-R74.
There was a problem hiding this comment.
I was referring to vapoursynth/vapoursynth@feba4b9. I don't know if mpv-winbuild-cmake has anything to do with this repo's meson.build.
There was a problem hiding this comment.
I see. So the pkg-config for VSScript is removed after R74? I guess since the vapoursynth dependency already includes all the header files, and we no longer link the lib, we can just depend on it alone.
Still, I remember pre-R74 vapoursynth dependency contains the libs, so the partial_dependency is still needed, right?
Could you check the latest commit?
There was a problem hiding this comment.
Yes, partial_dependency is good if you don't need the library to link against.
Another issue I found is that lib_name_old is wrong. The soversion was always 0 before R74 so it should be "libvapoursynth-script.0.dylib" or "libvapoursynth-script.so.0".
There was a problem hiding this comment.
Have you considered static loader library, so that it's not on users to figure out what libraries needed to be loaded and from where?
This would also be backward compatible, because we would simply do meson dependency in both cases, while the new one would link loader shim lib.
There was a problem hiding this comment.
The soversion was always 0 before R74 so it should be "libvapoursynth-script.0.dylib" or "libvapoursynth-script.so.0".
I literally copied from their SDK example. Is that a mistake on their side?
Have you considered static loader library, so that it's not on users to figure out what libraries needed to be loaded and from where?
This would also be backward compatible, because we would simply do meson dependency in both cases, while the new one would link loader shim lib.
FYI, after R74, the installation process of VapourSynth would add VSSCRIPT_PATH to system environment, so users don't have to. Before R74, user has to add the location of VapourSynth to PATH.
By "loader library" if you mean to move the current logic to separate module and provide a shim of getVSScriptAPI, while still remain inside mpv, I personally think is unnecessary. Ideally, if VapourSynth could instead provide the logic we are doing here and wrap in a loader library, I'd be happily use that. Maybe we should pitch the idea to them. Things like the soversion mismatch HolyWu mentions above shouldn't be a concern to begin with.
There was a problem hiding this comment.
By "loader library" if you mean to move the current logic to separate module and provide a shim of getVSScriptAPI, while still remain inside mpv
No. This was message for HolyWu, of course it doesn't make sense to make it in mpv. HolyWu is contributing to vapoursynth. I was thinking about the static linking ability provided by the vapoursynth itself.
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.
5 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.