[mpv] Overrides input-ipc-server set in mpv.conf
#529
Comments
ahmubashshir
changed the title
input-ipc-path set in mpv.confinput-ipc-path set in mpv.conf
|
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 If you can figure out a way for JSON IPC to add itself to |
|
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
|
|
I don't feel that just automatically using ipc file path from config is the optimal approach for two reasons:
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. |
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 |
|
@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 |
|
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
|
|
@ahmubashshir Thanks for testing that. What if you change line 248 of python_mpv_jsonipc.py from |
|
Same exception happens... btw, why do you vendor dependencies as part of your distribution? |
We use a slightly modified version of the JSON IPC code. |
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. |
|
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
|
|
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
|
Coming here to echo this sentiment- by setting the config item, the user has explicitly assumed responsibility for any breakage caused by bad actors. @Et0h It would be nice to get the patch merged |
I am not aware of any appropriate patch ready to merge. |
|
This one seems close enough, no? |
I sent the patch via email here #529 (comment) edit: Should I fork and send pr? |
Sorry, I should have been more clear by what I meant by "appropriate patch". The historic patch you linked to was considered previously and concerns were raised about it by @daniel-123 in #529 (comment) with further relevant thoughts about what might be acceptable set out by myself in #529 (comment) In response to these comments you said at #529 (comment) that you would send a patch to "for JSON IPC to add itself to input-ipc-server without overriding input-ipc-path" after you studied the source code. In my understanding you have yet to provide such an updated patch. I'm guessing this is because such a patch is not possible based on mpv's current code, as it wasn't clear to me either how to do such a thing (or else I would have provided such a patch myself). However, I know you previously stated that you were aware of other tools that were successful in working around mpv's limitation of only allowing people to specify one IPC socket per instance of mpv. If an updated patch along the lines previously discussed is not possible due to limitations of mpv then I would probably consider it an upstream issue for mpv to resolve rather than something that merits a hacky fix for Syncplay that would add to Syncplay's complexity and could cause more problems than it solves. |
|
@ahmubashshir quick question- is there a distinction between From what I understood, Syncplay already supports specifying EDIT: just verified that this is indeed true. In this case, iamkroot/trakt-scrobbler#266 is a different bug from the current one - will create a separate issue for it. |
I don't see any |
The term Basically what I'm trying to say is that manually reading the configuration file seems hacky, and what we want is a solution that allows both Syncplay and whatever else is using IPC to both work without adding too much complexity and without breaking stuff that currently works. |
|
how about using |
|
I found another solution... after starting mpv normally, we can set |
|
I'm using this script to start second ipc server when input-ipc-server is different from config # ~/.config/mpv/scripts/syncplay-fix-ipc.run
#!/bin/bash
[[ "${1%%=*}" == "--mpv-ipc-fd" ]] || exit
MPV_FD="${1#--mpv-ipc-fd=}"
test "$MPV_FD" || exit
test "$MPV_FD" -gt 0 || exit
get_config_from_file() {
sed -nE '/^'"$1"'=/s/^[^=]+=(.+)$/\1/p' "$2"
}
get_reply_for_command()
{
local resp
echo '{ "command": '"$1"' }' >&"$MPV_FD"
until [[ $resp ]] && [[ $resp =~ "\"data\":" ]]; do
read -u "$MPV_FD" -r resp
done
sed -nE 's/^.+data":"([^"]+)".+$/\1/p' <<< "$resp"
}
read -r cfg < <(get_reply_for_command '["expand-path", "~~home/mpv.conf"]')
read -r cipc < <(get_reply_for_command '["get_property", "input-ipc-server"]')
read -r dipc < <(get_config_from_file input-ipc-server "$cfg")
if [[ $dipc && ! "$cipc" == "$dipc" ]]; then
echo "set input-ipc-server $dipc" >&"$MPV_FD"
fi
exit 0 |
From 8414e6adb511601b98e1a5d4db26834bbdc9a3e2 Mon Sep 17 00:00:00 2001
Message-ID: <8414e6adb511601b98e1a5d4db26834bbdc9a3e2.1704217301.git.ahmubashshir@gmail.com>
From: Mubashshir <[email protected]>
Date: Tue, 2 Jan 2024 23:36:54 +0600
Subject: [PATCH] mpv: Start second IPC server on user-set path
Currently Syncplay overrides user-set `input-ipc-server` in the config
file when launching mpv.
This patch starts another IPC server on user-set `input-ipc-server` path
so that syncplay can work w/o conflicting with other mpv JSON-IPC
clients.
Closes #529
Signed-off-by: Mubashshir <[email protected]>
---
requirements.txt | 1 +
syncplay/players/mpv.py | 28 ++++++++++++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/requirements.txt b/requirements.txt
index 0d61f2f..bb9cc3b 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,6 +1,7 @@
certifi>=2018.11.29
pem>=21.2.0
twisted[tls]>=16.4.0
+appdirs; sys_platform != 'darwin'
appnope>=0.1.0; sys_platform == 'darwin'
pypiwin32>=223; sys_platform == 'win32'
zope.interface>=4.4.0; sys_platform == 'win32'
diff --git a/syncplay/players/mpv.py b/syncplay/players/mpv.py
index 641a1c0..6403b47 100755
--- a/syncplay/players/mpv.py
+++ b/syncplay/players/mpv.py
@@ -505,6 +505,33 @@ class MpvPlayer(BasePlayer):
time.time() > (self.lastLoadedTime + constants.MPV_NEWFILE_IGNORE_TIME)
)
+ def _start_default_ipc(self):
+ if not sys.platform == "darwin":
+ import appdirs
+
+ from configparser import ConfigParser, NoOptionError as NoConfigKeyError
+
+ ipc_path = None
+ cfg = os.path.join((
+ os.path.join(os.environ.get('HOME'), '.config', 'mpv')
+ if sys.platform == "darwin"
+ else appdirs.user_config_dir("mpv", roaming=True, appauthor=False)
+ ), 'mpv.conf')
+
+ if not os.path.isfile(cfg):
+ return
+
+ try:
+ config = ConfigParser(allow_no_value=True, strict=False, inline_comment_prefixes="#")
+ with open(cfg, "r") as f:
+ config.read_string(f'[root]\n{f.read()}')
+ ipc_path = config.get("root", "input-ipc-server")
+ except (NoConfigKeyError, KeyError, ValueError):
+ return
+
+ if ipc_path:
+ self._listener.sendLine(["set", "input-ipc-server", ipc_path])
+
def __init__(self, client, playerPath, filePath, args):
from twisted.internet import reactor
@@ -514,6 +541,7 @@ class MpvPlayer(BasePlayer):
self._playerIPCHandler = MPV
self._create_listener(playerPath, filePath, args)
+ self._start_default_ipc()
def _set_defaults(self):
self._paused = None
--
2.43.0mpv-Start-second-IPC-server-on-user-set-path.patch.gz |
|
@ahmubashshir Nice work finding that if you set the value it at runtime it creates a second IPC server. I note @daniel-123's previous comment that "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." Reading the config from mpv would require a lot of testing due to the numerous platforms and variations, and wouldn't be able to handle complex config scenarios and could therefore exhibit inconsistent behaviour. It would also add an additional dependency (appdirs) which adds to the complexity of Syncplay. In light of these concerns, it might be better for long-term reliability and maintenance if users instead specified input-ipc-server values as a per-player command line argument for mpv/mplayer, and that is then intercepted to be run once mpv has been initialised. |
That's why the
It doesn't need to either, it only needs to read out the specific config key
Shouldn't what the user set in the config file be respected? I used
If you're integrating with a tool, is ignoring/overriding the user-set config a good practice? |
You don't need to create a new issue as I recently reported it at mpvnet-player/mpv.net#654 but feel free to post on that issue if you can help triangulate or confirm the problem and/or code a solution. |
|
I've updated #529 so that if you don't specify a secondary input-ipc-server socket by default it will automatically try to create one as |
|
@Et0h thank you, with this pr all working fine now! |
|
That's great to hear @soredake - in that case I can merge the PR. Also, in other good news stax76 has speedily updated mpv.net to fix the issue which would prevent Syncplay from connecting to mpv.net if input-ipc-server is specified in mpv.conf! |
|
@Et0h can you please restore creating secondary socket on Windows by default? Otherwise this should be reopened #658, plus even after specifying |
I just tried the trakts test for mpv and it worked for me with Syncplay 1.7.3 using both mpv and mpv.net: To get it to work with my version of mpv/mpv.net I had to set In terms of making it a socket/pipe by default, according to @notpeelz it is improper to do it that way. I don't know enough about how sockets/pipes work to be able to judge, so I'm erring on the side of caution and making it an opt-in feature. |
|
I did some investigating. Depending on the order you pass the e.g. $ mpv --player-operation-mode=pseudo-gui --input-ipc-server=/tmp/user-mpv --input-ipc-server=/tmp/syncplay-mpv
# only /tmp/syncplay-mpv is createdExcept it doesn't "break" (at least not right away) because SyncPlay extracts the Now this is where it gets interesting. if (init || opt_ptr == &opts->ipc_path || opt_ptr == &opts->ipc_client) {
mp_uninit_ipc(mpctx->ipc_ctx);
mpctx->ipc_ctx = mp_init_ipc(mpctx->clients, mpctx->global);
}Essentially, SyncPlay ends up killing its own IPC socket. You can test this behavior like so: # in terminal 1:
$ mpv --player-operation-mode=pseudo-gui --input-ipc-server=/tmp/syncplay-mpv
# in terminal 2:
# check that it works
$ echo '{"command": ["get_property", "input-ipc-server"]}' | socat - /tmp/syncplay-mpv
{"data":"/tmp/syncplay-mpv","request_id":0,"error":"success"}
# set the user-defined socket
$ echo '{"command": ["set_property", "input-ipc-server", "/tmp/user-mpv"]}' | socat - /tmp/syncplay-mpv
{"request_id":0,"error":"success"}
# check that the user-defined socket works
$ echo '{"command": ["get_property", "input-ipc-server"]}' | socat - /tmp/user-mpv
{"data":"/tmp/user-mpv","request_id":0,"error":"success"}
# and finally, check if the original socket is still alive (nope)
$ echo '{"command": ["get_property", "input-ipc-server"]}' | socat - /tmp/syncplay-mpv
socat[135918] E GOPEN: /tmp/syncplay-mpv: Connection refused...except SyncPlay still works??? # in terminal 1:
$ mpv --player-operation-mode=pseudo-gui --input-ipc-server=/tmp/syncplay-mpv
# in terminal 2:
# open an interactive REPL with the socket
$ socat - /tmp/syncplay-mpv
# now type the following: {"command": ["get_property", "input-ipc-server"]}<ENTER>
# you should see a response with the expected path.
# now change the the socket path: {"command": ["set_property", "input-ipc-server", "/tmp/user-mpv"]}<ENTER>
# in terminal 3:
# check that the socket is now dead
$ echo '{"command": ["get_property", "input-ipc-server"]}' | socat - /tmp/syncplay-mpv
socat[137175] E GOPEN: /tmp/syncplay-mpv: Connection refused
# now go back to terminal 2 and run the get_property command again: it still works!I think the real way to fix this would be to ask the mpv devs to support listening on multiple sockets:
Uhh... but what if we want want to override the previous Option 1:
Option 2:
Option 3:
Option 1 is bad because it changes existing behavior in a backward-incompatible way. Other than that, I don't see a reliable way to "restore" the user-defined |
It doesn't work for me, syncplay works, but trakt-scrobbler saying this: Can you please adjust this commit so a secondary socket is still created by default on Windows? I get that you are tried to fix something on linux, but in process you broke functionality that worked in 1.7.2 out of the box, I just needed |
Did you have a media file playing at the time? That's the error I get if I run the test without a file open. If you still have issues please provide more detailed instructions on how to replicate, ideally using trakts test.
I think the mpv.conf thing is entirely due to the current trakt-scrobbler code and is nothing to do with recent Syncplay changes. However, it might be due to your attempt to test the latest changes on different media players. The code for getting the per player argument is basically just guesswork if you have more than one player path specified, and only works if you use As I explain at iamkroot/trakt-scrobbler#266 (comment): Looking at https://github.com/iamkroot/trakt-scrobbler/blob/7eef02fcfdefb665de6a77dfba403464f024f2ff/trakt_scrobbler/player_monitors/mpv_wrappers.py it looks like some further changes will be required to get it to work reliably via trakt-scrobbler. Firstly, if there is a Syncplay.ini or .Syncplay folder in the Syncplay folder then Syncplay will use that in preference to the file in the APPDATA folder. This is what distinguishes the installed version of Syncplay from the portable version of Syncplay. Secondly, someone can have multiple different player entries for perplayerarguments depending on the path of the player. You can get the perplayerargument for the currently selected player by first looking at the value of playerpath and then use that as the key for which which perplayerargument to use. Thirdly, it can be set as input-ipc-server rather than --input-ipc-server so just looking for --input-ipc-server might not find it. One option is to just hard-code a pipe/socket name and then instruct people to run input-ipc-server=thatName as a command line argument. This will work UNLESS you end up having two versions of Syncplay running at the same time. |
Yes, I received this with any media file.
Or have option to create secondary socket, that's the best option for me (I only need to set socket in trakt-scrobbler config, and that's all). I'll gladly pay any dev that can make this option. |
But isn't that already the case? You just need to set the "Player arguments" to
Works fine for me. Tested in a Windows 10 VM:
Steps:
I've also tested with this configuration: ...but it appears that trakt-scrobbler doesn't parse SyncPlay's config file correctly. [client_settings]
perplayerarguments = {'c:\\users\\peelz\\scoop\\apps\\mpv\\current\\mpv.exe': ['--input-ipc-server=\\\\.\\pipe\\mpvsocket']}trakt-scrobbler logs this (notice how it doesn't parse the escape sequences): I can make [client_settings]
perplayerarguments = {'c:\\users\\peelz\\scoop\\apps\\mpv\\current\\mpv.exe': ['--input-ipc-server=\\.\pipe\mpvsocket']}then IMO trakt-scrobbler's At the risk of repeating myself, the way it works currently is an ugly workaround (see my previous message). The better way to fix this would be for mpv to support listening on multiple sockets simultaneously. |
No, by secondary socket I mean |
Syncplay 1.7.3 allows you to create a second socket called whatever you want by specifying it in the per-player argument within Syncplay. If you want to call it |
|
What I've done to receive 'property unavailable' from
If I understand correctly, |
That's normal, those properties can't be queried if you're not playing a file. |
It's not ignored so much as set later. Order of events is:
Try
Makes sense. If trakts wasn't able to connect to mpv it'd never get that error, which is just the normal message for if no file is playing. |
Nothing changed unfortunately. |
|
I found why it was not working for me, I used the same socket for plex-mpv-shim, syncplay and trakt-scrobbler, that caused this |
Glad you got to the bottom of it.
The precise operations and sequencing, perhaps. If I understand correctly mpv opens a socket and Syncplay connects to mpv using that socket and then the socket is closed in mpv but the connection to Syncplay over this socket remains open, and only then is the socket reused for its next purpose which is to allow scrobbler to connect. As Syncplay never disconnects it can still make use of the original connection even after scrobbler connects. It is a bit hacky and probably relies on undocumented functionality but so long as it works consistently I'll take the win. |
|
Can this issue be closed now? |
|
I'll say yes, and someone can re-open it if necessary. |
Describe the bug
Syncplay overrides
input-ipc-serverset inmpv.confthus breaking tools that depends on knowninput-ipc-serverTo Reproduce
Steps to reproduce the behavior:
input-ipc-server=/run/user/1000/mpvin mpv.confExpected behavior
Syncplay should check mpv.conf and use ipc path from mpv.conf if it's set.
Version and platform:
edit: replace outdated option name.
The text was updated successfully, but these errors were encountered: