search

ripose-jp / Memento

An mpv-based video player for studying Japanese
https://ripose-jp.github.io/Memento/
GNU General Public License v2.0
472 stars 22 forks source link

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

Closed SpaghettiBorgar closed 3 months ago

SpaghettiBorgar commented 3 months ago

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 https://github.com/ripose-jp/Memento/commit/02ce0bdfba2f3fe485ab6b33b85f8d9022f58e19, 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: https://github.com/mpv-player/mpv/commit/c678033c1d60b48ae02fbbe4815869b9504a17f6

ripose-jp commented 3 months ago

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: https://github.com/mpv-player/mpv/commit/c678033c1d60b48ae02fbbe4815869b9504a17f6

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