[mpv] Overrides `input-ipc-server` set in mpv.conf

[ahmubashshir]

Describe the bug
Syncplay overrides input-ipc-server set in mpv.conf thus breaking tools that depends on known input-ipc-server

To Reproduce
Steps to reproduce the behavior:

1. Set input-ipc-server=/run/user/1000/mpv in mpv.conf
2. Run mpv with syncplay

Expected behavior
Syncplay should check mpv.conf and use ipc path from mpv.conf if it's set.

Version and platform:

• OS: Arch Linux
• Syncplay version and build type: 1.6.9-5 [community/syncplay]
• Media player and version: mpv 1:0.34.1-4 [community/mpv]

edit: replace outdated option name.

[Et0h]

I hope you can find a solution to this problem, but unfortunately it is not something I can be of much assistance with as it falls outside my knowledge and use case.

Syncplay uses JSON IPC to communicate with mpv, and in my understanding JSON IPC sets input-ipc-server in mpv to allow for this communication. As such, it is possible that the problem / solution lies 'upstream' rather than with Syncplay.

If you can figure out a way for JSON IPC to add itself to input-ipc-server without overriding input-ipc-path then feel free to make a pull request to the appropriate repositories. It's possible that liaising with JSON IPC / mpv devs would be of assistance. If you do so then feel free to include a reference to this issue to provide them with relevant context.

[ahmubashshir]

This patch defines a new function that returns either the ipc path set in mpv.conf or the default(temporary) ipc path. Closes #529 Signed-off-by: Mubashshir ***@***.***> --- requirements_gui.txt | 1 + syncplay/players/mpv.py | 26 +++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/requirements_gui.txt b/requirements_gui.txt index a51f665..738e3ba 100644 --- a/requirements_gui.txt +++ b/requirements_gui.txt @@ -1,2 +1,3 @@ pyside2>=5.11.0 requests>=2.20.0; sys_platform == 'darwin' +appdirs>=1.4.4; sys_platform != 'darwin' diff --git a/syncplay/players/mpv.py b/syncplay/players/mpv.py index a5cd06d..7968aec 100755 --- a/syncplay/players/mpv.py +++ b/syncplay/players/mpv.py @@ -7,6 +7,12 @@ import subprocess import threading import ast +if not sys.platform == "darwin": + import appdirs + +from configparser import ConfigParser, NoOptionError as NoConfigKeyError +from pathlib import Path + from syncplay import constants from syncplay.messages import getMessage from syncplay.players.basePlayer import BasePlayer @@ -618,7 +624,7 @@ class MpvPlayer(BasePlayer): env['PATH'] = python_executable + ':' + env['PATH'] env['PYTHONPATH'] = pythonPath try: - socket = self.mpv_arguments.get('input-ipc-server') + socket = self.__get_mpv_ipc_path() self.mpvpipe = self.playerIPCHandler(mpv_location=self.playerPath, ipc_socket=socket, loglevel="info", log_handler=self.__playerController.mpv_log_handler, quit_callback=self.stop_client, env=env, **self.mpv_arguments) except Exception as e: self.quitReason = getMessage("media-player-error").format(str(e)) + " " + getMessage("mpv-failed-advice") @@ -641,6 +647,24 @@ class MpvPlayer(BasePlayer): cwd = None return cwd + def __get_mpv_ipc_path(self): + if sys.platform == "darwin": + conf_path = Path.home() / ".config" / "mpv" / "mpv.conf" + else: + conf_path = Path(appdirs.user_config_dir("mpv", roaming=True, appauthor=False)) + conf_path /= "mpv.conf" + try: + mpv_conf = ConfigParser( + allow_no_value=True, strict=False, inline_comment_prefixes="#" + ) + mpv_conf.read_string("[root]\n" + conf_path.read_text(encoding="utf-8")) + ipc_path = mpv_conf.get("root", "input-ipc-server") + if not ipc_path: + raise ValueError('ipc-path can not be empty') + except (NoConfigKeyError, KeyError, ValueError): + ipc_path = self.mpv_arguments.get('input-ipc-server') + return ipc_path + def run(self): pass

…

-- 2.36.1

[daniel-123]

I don't feel that just automatically using ipc file path from config is the optimal approach for two reasons:

• Primarily the issue is that Syncplay assumes in its design exclusive access to ipc path. Sharing it with another tool runs the risk of either stepping on each others toes. While it's possible that any given tool will work mostly fine, it's seems like a dangerous assumption to base a default behavior on.
• mpv config can be sourced from a number of places, not just the ~/.config/mpv/mpv.conf. It also differs by platform. I don't think trying to read and parse config files of another piece of software is a good practice.

Because of the above I'd see for example adding a command line switch/config entry to syncplay to allow to customize ipc path as far better option. At very least it doesn't run into the two problems mentioned above.

[Et0h]

Because of the above I'd see for example adding a command line switch/config entry to syncplay to allow to customize ipc path as far better option.

Building on this suggestion, Syncplay already has a system for allowing custom player paths. As such if you can figure out a way to tap into that to allow for input-ipc-path to be specified in the player argument for that specific player then that seems like it would be better than adding a new command line switch to Syncplay itself which can't be easily accessed via the GUI.

[Et0h]

@ahmubashshir The player arguments show up if you click the 'show more settings' button in the configuration window GUI. Could you please tell me what happens when you add input_ipc_server=PATH to the player arguments for mpv (with PATH being the path of your desired input-ipc-path value)?

[ahmubashshir]

what happens when you add input_ipc_server=PATH to the player arguments for mpv (with PATH being the path of your desired input-ipc-path value)?

This happens when I run with extra argument.

MPV failed with returncode 1. MPV start failed. Traceback (most recent call last): File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 463, in _start_mpv self.mpv_process = MPVProcess(ipc_socket, mpv_location, **kwargs) File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 218, in __init__ self._start_process(ipc_socket, args, env=env) File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 239, in _start_process raise MPVError("MPV not started.") syncplay.vendor.python_mpv_jsonipc.python_mpv_jsonipc.MPVError: MPV not started. MPV failed with returncode 1. MPV start failed. Traceback (most recent call last): File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 463, in _start_mpv self.mpv_process = MPVProcess(ipc_socket, mpv_location, **kwargs) File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 218, in __init__ self._start_process(ipc_socket, args, env=env) File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 239, in _start_process raise MPVError("MPV not started.") syncplay.vendor.python_mpv_jsonipc.python_mpv_jsonipc.MPVError: MPV not started. MPV failed with returncode 1. MPV start failed. Traceback (most recent call last): File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 463, in _start_mpv self.mpv_process = MPVProcess(ipc_socket, mpv_location, **kwargs) File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 218, in __init__ self._start_process(ipc_socket, args, env=env) File "/usr/lib/syncplay/syncplay/vendor/python_mpv_jsonipc/python_mpv_jsonipc.py", line 239, in _start_process raise MPVError("MPV not started.") syncplay.vendor.python_mpv_jsonipc.python_mpv_jsonipc.MPVError: MPV not started. Unhandled Error Traceback (most recent call last): File "/usr/lib/python3.10/site-packages/twisted/python/context.py", line 118, in callWithContext return self.currentContext().callWithContext(ctx, func, *args, **kw) File "/usr/lib/python3.10/site-packages/twisted/python/context.py", line 83, in callWithContext return func(*args, **kw) File "/usr/lib/syncplay/syncplay/vendor/qt5reactor.py", line 164, in _read self.reactor._iterate(fromqt=True) File "/usr/lib/syncplay/syncplay/vendor/qt5reactor.py", line 270, in _iterate self.runUntilCurrent() --- <exception caught here> --- File "/usr/lib/python3.10/site-packages/twisted/internet/base.py", line 967, in runUntilCurrent f(*a, **kw) File "/usr/lib/python3.10/site-packages/twisted/internet/defer.py", line 662, in callback self._startRunCallbacks(result) File "/usr/lib/python3.10/site-packages/twisted/internet/defer.py", line 757, in _startRunCallbacks raise AlreadyCalledError twisted.internet.defer.AlreadyCalledError:

Mubashshir

[Et0h]

@ahmubashshir Thanks for testing that. What if you change line 248 of python_mpv_jsonipc.py from self._set_default(kwargs, "input_ipc_server", self.ipc_socket) to self._set_default(kwargs, "input-ipc-server", self.ipc_socket) and then specify the path using input-ipc-server=PATH?

[ahmubashshir]

Same exception happens... btw, why do you vendor dependencies as part of your distribution?

[Et0h]

btw, why do you vendor dependencies as part of your distribution?

We use a slightly modified version of the JSON IPC code.

[ahmubashshir]

Primarily the issue is that Syncplay assumes in its design exclusive access to ipc path.

IMO, if someone set some option in their config, it's better to assume that they knows what they're doing, and we should respect their configs, instead of overriding them, and I don't think anybody would set input-ipc-path in their config without a reason.

[ahmubashshir]

mpv config can be sourced from a number of places, not just the ~/.config/mpv/mpv.conf. It also differs by platform. I don't think trying to read and parse config files of another piece of software is a good practice.

That's why the `appdirs` module is used. Mubashshir

[ahmubashshir]

As such, it is possible that the problem / solution lies 'upstream' rather than with Syncplay.

I think that's not the problem, cause other tools that uses json-ipc to communicate with mpv checks mpv.conf for `input-ipc-path` first, if it's unset then they use the command-line to set the ipc path, otherwise they use the existing one.

If you can figure out a way for JSON IPC to add itself to input-ipc-server without overriding input-ipc-path then feel free to make a pull request to the appropriate repositories.

I'll send a patch after I study the source code, thanks. Mubashshir