Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vf_vapoursynth: add user-data parameter to allow user to pass arbitrary data to script #14312

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

CrendKing
Copy link
Contributor

Currently the vapoursynth video filter does not accept any argument for passing arbitrary user data. This limits what the VS script can do.

Ideally, the vapoursynth filter has an user-data parameter that contains string value. mpv passes that value to the VS script just like container_fps and others. Once the VS script gets the data, it can do all sorts of data extraction and transformation.

Another benefit is that instead of mpv always have to catch up to user needs for this filter, with this users can just pass whatever needed themselves, thus becomes more future-proof.

Fixes #14214

Discussion: I did not put any restriction on how big the user data can be. Could there be any security vulnerability without such restriction?

Copy link

github-actions bot commented Jun 6, 2024

Download the artifacts for this pull request:

Windows
macOS

@CrendKing
Copy link
Contributor Author

CrendKing commented Jun 6, 2024

Note that currently the build script on Windows does not turn on the "vapoursynth" feature, thus the CI and build artifact does not actually test the change.

Shall I flip the option in the build script?

DOCS/man/vf.rst Outdated Show resolved Hide resolved
@kasper93
Copy link
Contributor

kasper93 commented Jun 6, 2024

Can't really comment on VS stuff, idea to pass user data is fine. Probably can do it already with something like environment variable and read it in the VS script, but we can pass string from options too.

Shall I flip the option in the build script?

Probably need more than just a flip to build it correctly, but if you want, feel free to do it. Note that vapoursynth is built in MSYS2 builds, so it gets build coverage there.

@CrendKing
Copy link
Contributor Author

CrendKing commented Jun 7, 2024

environment variable

The only environment variable can be read from VapourSynth side must be set before the mpv process is created (i.e. system ev), as I mentioned in #14214. You can't pass video-specific data.

@CrendKing
Copy link
Contributor Author

Probably need more than just a flip to build it correctly, but if you want, feel free to do it. Note that vapoursynth is built in MSYS2 builds, so it gets build coverage there.

How about I add -Dvapoursynth=disabled to the script to make others easier to flip if needed?

@CrendKing CrendKing force-pushed the master branch 4 times, most recently from 9833f0f to b196473 Compare June 7, 2024 07:25
@kasper93
Copy link
Contributor

kasper93 commented Jun 7, 2024

How about I add -Dvapoursynth=disabled to the script to make others easier to flip if needed?

Nah, either it is build fully or not mentioned at all, we would need to do it for all options.

The only environment variable can be read from VapourSynth side must be set before the mpv process is created (i.e. system ev), as I mentioned in #14214. You can't pass video-specific data.

Indeed, mpv reads env only once at the beginning, this could be changed, but probably setting user_data directly is better.

@CrendKing
Copy link
Contributor Author

CrendKing commented Jun 7, 2024

Nah, either it is build fully or not mentioned at all, we would need to do it for all options.

Gotcha.

Indeed, mpv reads env only once at the beginning, this could be changed, but probably setting user_data directly is better.

Thanks. I prefer user_data because the env var route 1) requires at least Lua script, meaning we can't use it in a mpv profile; 2) even in Lua, we need either 3rd party Lua dependency since vanilla Lua can't change env var just for current process, or need to subprocess to a 3rd party app to change. Both are not ideal.

Currently the vapoursynth video filter does not accept any argument for
passing arbitrary user data. This limits what the VS script can do.

Ideally, the vapoursynth filter has an user-data parameter that contain
string value. mpv passes that value to the VS script just like
container_fps and others. Once the VS script gets the data, it can do
all sorts of data extraction and transformation.

Another benefit is that instead of mpv always have to catch up to user
needs for this filter, with this users can just pass whatever needed
themselves, thus becomes more future-proof.

Fixes mpv-player#14214
@kasper93 kasper93 merged commit fe709c9 into mpv-player:master Jun 8, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A way to pass arbitrary user data to VapourSynth script
2 participants