Move to JSON IPC API for mpv using iwaltons3's library (#261)

[Et0h]

This should allow Syncplay to support mpv >0.30.0. Raises minimum mpv version to >= 0.17.0.

Many thanks to @iwalton3 for his Python MPV JSONIPC library which is available from https://github.com/iwalton3/python-mpv-jsonipc/

[daniel-123]

I have tested it with mpv 0.23, 0.29.0 and 0.32 - it works except for needing to wait for ipc

[iwalton3]

The library currently assumes MPV is running and has IPC available if start_mpv is set to false. (That is intended for connecting to a copy of MPV the user already has running, like GNOME MPV.) The managed mpv allows you to pass command arguments (when you initialize the MPV constructor), will generate a randomly named Linux socket/Windows pipe (if not specified), and will wait for IPC to open.

Of course you can verify that the socket/pipe is already open before calling the constructor. Just be careful as Windows names pipes behave oddly if you check them twice in a short period of time.

[Et0h]

With 76439a4 I've tried moving over to managed mpv as per @iwalton3's suggestion to see if that fixes the issue raised by @daniel-123.

When testing the latest commit please pay close attention to:

• Errors during initialisation (e.g. due to Syncplay trying to access things too soon)
• Handling of additional command line arguments specified by Syncplay users (as I moved from a list to a dictionary)
• Handling of opening mpv with a filename specified in Syncplay (as I moved over to the delayedFilePath system for all operating systems)
• Handling of Syncplay detecting and reacting to mpv being closed (as I had to change the relevant code)

[iwalton3]

If there is a problem with it detecting events such as window close, you can catch them with an event handler: https://github.com/iwalton3/jellyfin-mpv-shim/blob/master/jellyfin_mpv_shim/player.py#L111

[Et0h]

@iwalton3 In terms of event handling, if two scripts both register for the same on_key_press event (e.g. "space" or "CLOSE_WIN" ) then will mpv only actually honour one of them? That was my vague recollection from looking into it a long time ago, but maybe I'm wrong or out of date.

[iwalton3]

Yes if you bind a key, it will prevent all behavior from all previous key binds for that key, including the default ones.

[daniel-123]

From my tests as of current commit it works with 0.23.0 so I don't see much of a reason to raise required version all the way up to 0.29.0.

[daniel-123]

Quitting mpv raises an exception
EventHandler caught exception from (<bound method MpvPlayer.mpv_log_handler of <syncplay.players.mpv.MpvPlayer object at 0x7f197c031d30>>, ('info', 'cplayer', 'Exiting... (Quit)')).

Expected behavior would be to just quit Syncplay instead.

[iwalton3]

Quitting mpv raises an exception
EventHandler caught exception from (<bound method MpvPlayer.mpv_log_handler of <syncplay.players.mpv.MpvPlayer object at 0x7f197c031d30>>, ('info', 'cplayer', 'Exiting... (Quit)')).

Expected behavior would be to just quit Syncplay instead.

The best way to handle this would probably be to rebind q and CLOSE_WIN to trigger syncplay to quit.

[daniel-123]

Quitting mpv raises an exception
EventHandler caught exception from (<bound method MpvPlayer.mpv_log_handler of <syncplay.players.mpv.MpvPlayer object at 0x7f197c031d30>>, ('info', 'cplayer', 'Exiting... (Quit)')).
Expected behavior would be to just quit Syncplay instead.

The best way to handle this would probably be to rebind q and CLOSE_WIN to trigger syncplay to quit.

I feel iffy about rebinding keys. It won't do anything if somebody uses different key to quit mpv (or just clicks the X). It also won't handle the situation where mpv crashes for some reason.

I think it's just much simpler to ensure consistent behavior if Syncplay quits the instant mpv process is no longer there regardless of the reason.

Edit: an exception further down is a simple BrokenPipeError on the ipc socket, which should be reasonably foolproof way of detecting that mpv is gone.

[iwalton3]

Edit: an exception further down is a simple BrokenPipeError on the ipc socket, which should be reasonably foolproof way of detecting that mpv is gone.

Yeah that is probably your best bet, since it still works even if it is connecting to a copy the user launches. That would have to be caught in the separate pipe implementations and sent back as some kind of event (since they're separate threads).

[iwalton3]

It looks like the actual "BrokenPipeError: [Errno 32] Broken pipe" doesn't happen until you try call something on the MPV instance after it has crashed. (Currently the broken pipe is ignored in the actual socket implementations.) I'll add a callback you can register to know when MPV terminates.

[iwalton3]

Please see: https://github.com/iwalton3/python-mpv-jsonipc/releases/tag/v1.1.11

[blaenk]

Thanks so much for working on this @Et0h! Can we expect any kind of release (stable or otherwise) once this is merged? I would love to try it out at that point.

[Et0h]

Thanks so much for working on this @Et0h! Can we expect any kind of release (stable or otherwise) once this is merged? I would love to try it out at that point.

If no showstoppers are reported then I expect this PR will be merged into the main branch before the end of this week after it has been tested on Mac. I'm hoping to get at a release candidate of Syncplay v1.6.5 out before the end of the month.