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

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

Merged
merged 15 commits into from May 15, 2020
Merged

Conversation

Et0h
Copy link
Contributor

@Et0h Et0h commented May 11, 2020

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
Copy link
Contributor

@daniel-123 daniel-123 commented May 11, 2020

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

@iwalton3
Copy link

@iwalton3 iwalton3 commented May 11, 2020

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
Copy link
Contributor Author

@Et0h Et0h commented May 11, 2020

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
Copy link

@iwalton3 iwalton3 commented May 11, 2020

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
Copy link
Contributor Author

@Et0h Et0h commented May 12, 2020

@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
Copy link

@iwalton3 iwalton3 commented May 12, 2020

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 daniel-123 linked an issue May 12, 2020 that may be closed by this pull request
syncplay/constants.py Outdated Show resolved Hide resolved
Copy link
Contributor

@daniel-123 daniel-123 left a comment

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.

Copy link
Contributor

@daniel-123 daniel-123 left a comment

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
Copy link

@iwalton3 iwalton3 commented May 12, 2020

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
Copy link
Contributor

@daniel-123 daniel-123 commented May 12, 2020

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
Copy link

@iwalton3 iwalton3 commented May 12, 2020

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
Copy link

@iwalton3 iwalton3 commented May 12, 2020

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
Copy link

@iwalton3 iwalton3 commented May 12, 2020

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

@Et0h Et0h self-assigned this May 13, 2020
@blaenk
Copy link
Contributor

@blaenk blaenk commented May 14, 2020

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
Copy link
Contributor Author

@Et0h Et0h commented May 14, 2020

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.

@Et0h Et0h merged commit 793e55d into master May 15, 2020
1 check passed
albertosottile pushed a commit to albertosottile/syncplay that referenced this issue Sep 30, 2020
…Syncplay#310)

* Separate mpv from mplayer, increase min mpv ver to >= 0.17, refactor

* Further separation of mpv from mplayer

* Fix reference to isASCII

* Add iwalton3's Python MPV JSONIPC library (Apache 2.0)

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

* Add empty init in Python MPV JSONIPC to make py2exe happy

* Use managed version of Python MPV JSONIPC to improve initialisation reliability

* Set mpv min version to >=0.29.0 to ensure compatibility

* Allow mpv >=0.23.0 based on daniel-123's tests

* Update mpv compatibility message

* Revert to old OSC compat message

* Removed mpv option that's no longer used afer switching to IPC.

* Update python-mpv-jsonipc to v1.1.11

* Use python-mpv-jsonipc's mpv quit handler

* Shorten mpv paused/position update message

Co-authored-by: daniel-123 <wrobel.dan@gmail.com>
albertosottile pushed a commit to albertosottile/syncplay that referenced this issue Sep 30, 2020
…Syncplay#310)

* Separate mpv from mplayer, increase min mpv ver to >= 0.17, refactor

* Further separation of mpv from mplayer

* Fix reference to isASCII

* Add iwalton3's Python MPV JSONIPC library (Apache 2.0)

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

* Add empty init in Python MPV JSONIPC to make py2exe happy

* Use managed version of Python MPV JSONIPC to improve initialisation reliability

* Set mpv min version to >=0.29.0 to ensure compatibility

* Allow mpv >=0.23.0 based on daniel-123's tests

* Update mpv compatibility message

* Revert to old OSC compat message

* Removed mpv option that's no longer used afer switching to IPC.

* Update python-mpv-jsonipc to v1.1.11

* Use python-mpv-jsonipc's mpv quit handler

* Shorten mpv paused/position update message

Co-authored-by: daniel-123 <wrobel.dan@gmail.com>
@daniel-123 daniel-123 deleted the mpv_JSON branch Apr 10, 2021
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.

4 participants