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

src/player/mpvadapter: fix compatibility with mpv 0.38.0 #225

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

SpaghettiBorgar
Copy link
Contributor

Since version 0.38.0, mpv requires an additional integer argument ("index") for the loadfile command that comes before the options, but is ignored in most cases (see documentation). I guess when you tried adjusting for mpv 0.38.0 before in 02ce0bd, the error you mention probably is because it was expecting an integer argument for the position where the options used to be, and passing NULL removes the argument all together, but this should still break all cases where there actually are options that are passed to loadfile, including the very much important start and end times for tempAudioClip(), so I'm confused how this seems to have gone unnoticed.

Anyway, since the order of arguments now depends on the mpv version the user has installed on the system (at least when it is dynamically linked like on Linux), we need to implement a runtime version check that conditionally stuffs an argument before the options string. This value can be arbitrary since it is ignored in our cases.
Another potential option would be using mpv_set_option_string() to set the start,end,aid options (and maybe even the input file?) to avoid having to pass these arguments with loadfile all together, which worked with both mpv 0.37.0 and 0.38.0.

Here is the commit that brings the breaking change, the comments might be worth reading: mpv-player/mpv@c678033

Since mpv client API 2.3 introduced with mpv 0.38.0, the loadfile
command now takes an additional argument, which breaks audio clip
generation. To fix this, optionally add the additional (and ignored)
argument to the loadfile command depending on the loaded mpv version.
@ripose-jp
Copy link
Owner

Thanks for fixing this. I think the checking the version at runtime is fine. Infinitely faster than parsing strings as commands at least.

so I'm confused how this seems to have gone unnoticed.

I don't use the feature and all binary releases still ship with mpv 0.37.0 so most people aren't affected. It only affects people using the AUR or building from source.

Here is the commit that brings the breaking change, the comments might be worth reading: mpv-player/mpv@c678033

Thanks for linking this. I didn't know which commit did it.

@ripose-jp ripose-jp merged commit b5aa03d into ripose-jp:master Jun 18, 2024
3 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.

2 participants