vf_vapoursynth: update for VapourSynth R74#17731
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.
So I assume the reason why debian.pkgs.org only shows the .so.0.0.0 is because they just ignores symlinks. And I assume it is unlikely there will be new major version release of the old library, so using libvapoursynth-script.so.0 should be the way to go.
If that's the case, the current commit is up-to-date and ready to merge.
There was a problem hiding this comment.
Do you even need to care about soname? What if you load libvsscript.so? getVSScriptAPI_func(VSSCRIPT_API_VERSION); takes VSSCRIPT_API_VERSION, so I assume if it is not compatible it would return null and if the function is not there it would also bail out before that.
There was a problem hiding this comment.
Consider this scenario: User installs a mpv that uses 4_3 as the VSSCRIPT_API_VERSION. He also installs VapourSynth R74, which has libvsscript.so -> libvsscript.so.4 -> libvsscript.so.4.0.0. Then VapourSynth releases R75, which changes the VSSCRIPT_API_VERSION to 5_0, and also make libvsscript.so -> libvsscript.so.5 -> libvsscript.so.5.0.0.
Normally if R75 is direct upgrade of R74, sure, it would fail to load anyways like you said. But what if the two versions can somehow coexist? What if it is like AUR system, where R74 and R75 use different names (like vapoursynth-stable and vapoursynth-nextgen), then mpv using libvsscript.so.4 would still work, but using libvsscript.so will not.
I admit it is a bit far-fetched. If you think that's a scenario we should ignore and keep things simple, I'm OK to update the PR to remove the soversion completely (i.e. load libvsscript.so and libvapoursynth-script.so).
There was a problem hiding this comment.
But what if the two versions can somehow coexist?
Than the soname should be derived from VSSCRIPT_API_VERSION or any other way, such that it will be possible to migrate to "r75" without changing the source, and only recompiling with the new headers. Normally, this would be done by pkg-config, because it would make dependency to certain sover and automatically update on rebuild.
There was a problem hiding this comment.
That would be ideal and doable if VapourSynth makes some sort of contract, like some naming convention between the API version and the library filename. Without, there is risk of failing to load the VSScript library even some version of it is installed.
I updated the code to remove soversion.
|
Generally looks ok now, there are two remaining items:
|
CrendKing
force-pushed
the
vapoursynth
branch
3 times, most recently
from
8e68e43 to
55e50be
Compare
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 modified from their SDK example to utilize mpv's mechanisms. Build dependency is also updated accordingly.
6 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.