[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() --- --- 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

[iamkroot]

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.

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

[Et0h]

@Et0h It would be nice to get the patch merged

I am not aware of any appropriate patch ready to merge.

[iamkroot]

This one seems close enough, no? https://github.com/Syncplay/syncplay/issues/529#issuecomment-1126777547

[ahmubashshir]

I am not aware of any appropriate patch ready to merge.

I sent the patch via email here https://github.com/Syncplay/syncplay/issues/529#issuecomment-1126777547

edit: Should I fork and send pr?

[Et0h]

I am not aware of any appropriate patch ready to merge.

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 https://github.com/Syncplay/syncplay/issues/529#issuecomment-1126895026 with further relevant thoughts about what might be acceptable set out by myself in https://github.com/Syncplay/syncplay/issues/529#issuecomment-1126895696

In response to these comments you said at https://github.com/Syncplay/syncplay/issues/529#issuecomment-1274355847 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.

[iamkroot]

@ahmubashshir quick question- is there a distinction between input-ipc-server and input-ipc-path?

From what I understood, Syncplay already supports specifying --input-ipc-server arg to MPV (via mpv_arguments.get('input-ipc-server')) and it correctly picks up this value instead of a randomly generated one.

EDIT: just verified that this is indeed true. In this case, https://github.com/iamkroot/trakt-scrobbler/issues/266 is a different bug from the current one - will create a separate issue for it.

[iamkroot]

JSON IPC to add itself to input-ipc-server without overriding input-ipc-path

I don't see any input-ipc-path config for MPV. So I'm confused what this sentence means. Could you please clarify @Et0h ?

[Et0h]

JSON IPC to add itself to input-ipc-server without overriding input-ipc-path

I don't see any input-ipc-path config for MPV. So I'm confused what this sentence means. Could you please clarify @Et0h ?

The term input-ipc-path came from @ahmubashshir rather than myself. I know mpv keeps renaming stuff, so maybe it is synonymous with --input-ipc-server.

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.

[ahmubashshir]

how about using --input-ipc-client on supported platforms?

[ahmubashshir]

I found another solution... after starting mpv normally, we can set input-ipc-server to another value to start a second ipc server 🙃

[ahmubashshir]

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

[ahmubashshir]

From 8414e6adb511601b98e1a5d4db26834bbdc9a3e2 Mon Sep 17 00:00:00 2001
Message-ID: <8414e6adb511601b98e1a5d4db26834bbdc9a3e2.1704217301.git.ahmubashshir@gmail.com>
From: Mubashshir <ahmubashshir@gmail.com>
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 <ahmubashshir@gmail.com>
---
 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.0

mpv-Start-second-IPC-server-on-user-set-path.patch.gz (use git am to apply the patch)

[Et0h]

@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.

[ahmubashshir]

@Et0h

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."

That's why the appdirs module is used... they're doing the heavy lifting of path management. appdirs module will return platform specific location... like platform	path
windows	%APPDATA%/mpv/mpv.conf
linux/*bsd(XDG_CONFIG_HOME unset)	~/.config/mpv/mpv.conf
linux/*bsd(XDG_CONFIG_HOME set)	${XDG_CONFIG_HOME}/mpv/mpv.conf
OSX	~/.config/mpv/mpv.conf

wouldn't be able to handle complex config scenarios

It doesn't need to either, it only needs to read out the specific config key input-ipc-server, nothing else. Even regex could've been used...

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.

Shouldn't what the user set in the config file be respected?

I used appdirs module to keep the changes to minimum (you can get exact config path by sending expand-path ~~home/mpv.conf to the IPC, implementing that seems complex)

@daniel-123

I don't think trying to read and parse config files of another piece of software is a good practice.

If you're integrating with a tool, is ignoring/overriding the user-set config a good practice?

[Et0h]

Shouldn't what the user set in the config file be respected?

I'm not sure if it can be respected in a consistent manner.

In my understanding mpv has a system-wide configuration file but also user-specific files and you can even specify profiles within a file. This means there will always be some uncertainty as to whether Syncplay is accurately reading the correct input-ipc-server value.

Your use case seems to be having a single simple config file, and that might be the case for many users. However, the sort of users who set input-ipc-server in their config are probably more likely than normal to be people with their own complex custom config options that might be hard for Syncplay to parse.

The more accurate Syncplay is in matching the mpv process for parsing config the more complex you make Syncplay itself. And we would not just have to replicate mpv's behaviour, but also the mpv.net behaviour which might be the same but might differ in relevant respects.

While there are some upsides to reading from the main config file, there are also downsides such as potential consistency issues, increased complexity of Syncplay code (which can make maintenance and testing harder), more dependencies, etc. As such we need to make a judgment call as to whether or not we consider it to be worth it.

As noted at https://github.com/Syncplay/syncplay/issues/315 when we make decisions about Syncplay development we have to balance a number of different factors including code simplicity, consistency/predictability, intentionality, disruption minimisation, functionality, responsiveness, privacy and compatibility. This balancing act is not an exact science and others might come to different conclusions, but the core developers do have the advantage of having been developing Syncplay for more than a decade to help guide our judgment. We are also the ones who often end up having to maintain code that people contribute, making us important stakeholders in determining what should be accepted into the code base.

Having considered the suggestions against the relevant factors, the view that @daniel-123 and I have come to is that we do not want Syncplay to start trying to extract config information from the mpv config file. As such, while we are grateful for the time and effort you have put in your proposed code is not deemed to be acceptable in its current form.

As previously discussed, I think there could however be an alternative way forward that you will hopefully consider adequate if not ideal. Allowing users who use IPC to intentionally specify their preference for an override via a Syncplay media player command line argument setting seems like an acceptable alternative as it allows input-ipc-server to be overridden in a way that is easier for us to test, implement, and maintain and without needing any additional dependencies. Generally speaking those who want to use the functionality will probably already be specifying various player arguments, so it would just fit in as part of the normal approach our users will take. We can explain it in the FAQ for those who were not expecting that Syncplay would otherwise override or ignore their config file in that respect.

[soredake]

It would be really nice if I can just add input-ipc-server=\\.\pipe\mpvsocket and syncplay will just work. I wasted time on this https://github.com/Syncplay/syncplay/issues/658 and https://github.com/Syncplay/syncplay/issues/654 (without bing chat I would waste days writing this script)

[Et0h]

Is anyone aware of an input IPC server usage other than Syncplay that people can use to test this with Windows that doesn't require them to have an account on a different server? The example usage I've seen is trakt-scrobbler but that is tied to a specific service.

[iamkroot]

Is anyone aware of an input IPC server usage other than Syncplay that people can use to test this with Windows that doesn't require them to have an account on a different server? The example usage I've seen is trakt-scrobbler but that is tied to a specific service.

Technically, you don't need a trakt account to run the monitor part of the scrobbler. If you disable the Scrobbler thread (you'll need to edit main.py and comment out the line that starts the thread), you can use the rest of the infrastructure to run the mpv monitor. In particular, trakts test smplayer@mpv should work

Edit: now that I look at the source code, it seems you don't even need to edit any code. trakts test is completely independent of the server - it only checks the player connection. So all you'd need to do is enable the monitor (trakts config set players.monitored smplayer@mpv) followed by the test command

[Et0h]

@iamkroot That's really helpful, thanks for sharing.

@soredake @ahmubashshir @iamkroot Please test my attempt to fix this issue at and let me know if it works for you. You can change the code yourself using the changes at https://github.com/Syncplay/syncplay/pull/669/files or just download the test versions at https://github.com/Syncplay/syncplay/actions/runs/7688463987?pr=669

Instructions: Set input-ipc-server=\.\pipe\mpvsocket in the mpv.conf folder and then put it in the player arguments for mpv in Syncplay, and then it should allow trakt-scrobbler with the version of mpv that Syncplay runs.

I tested with trakts test mpv and it responded with the media and %.

Change:

From 5419358e5f92099176db6646b4dc58b0c52c9cf5 Mon Sep 17 00:00:00 2001
From: Etoh <etoh@syncplay.pl>
Date: Sun, 28 Jan 2024 21:16:40 +0000
Subject: [PATCH] Pass input-ipc-server player argument to mpv (#529)

---
 syncplay/players/mpv.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/syncplay/players/mpv.py b/syncplay/players/mpv.py
index 641a1c07..64ae9f80 100755
--- a/syncplay/players/mpv.py
+++ b/syncplay/players/mpv.py
@@ -420,6 +420,8 @@ def sendMpvOptions(self):
         options_string = ", ".join(options)
         self._listener.sendLine(["script-message-to", "syncplayintf", "set_syncplayintf_options",  options_string])
         self._setOSDPosition()
+        if self._listener.mpv_arguments.get("input-ipc-server"):
+            self._setProperty("input-ipc-server", self._listener.mpv_arguments.get("input-ipc-server"))

     def _handleUnknownLine(self, line):
         self.mpvErrorCheck(line)

[soredake]

@Et0h nothing changed for me, unfortunately.

[Et0h]

@Et0h nothing changed for me, unfortunately.

Could you please provide step by step instructions for how I can reproduce your issue?

[soredake]

@Et0h I've described my problems in these reports already https://github.com/Syncplay/syncplay/issues/658 https://github.com/Syncplay/syncplay/issues/654

[Et0h]

@soredake I've now tried to replicate your issue and wasn't able to replicate your problem.

Replication of #654 (with appropriate tweaks to allow testing)

1. I did not run "winget install Syncplay.Syncplay mpv.net" instruction as this would install the wrong version of Syncplay (too old) and the wrong version of mpv.net (too new as the latest version of mpv.net breaks Syncplay if input-ipc-server is specified in config), but also used mpv rather than mpv.net because only mpv is listed in trakts test
2. For Syncplay I used the portable version of Syncplay 1.7.2 dev from https://github.com/Syncplay/syncplay/actions/runs/7688463987?pr=669 and more specifically https://github.com/Syncplay/syncplay/actions/runs/7688463987/artifacts/1200977075
3. For mpv.net I would have used v6.0.4.0-stable portable available from https://github.com/mpvnet-player/mpv.net/releases and more specifically https://github.com/mpvnet-player/mpv.net/releases/download/v6.0.4.0-stable/mpv.net-v6.0.4.0-stable.zip but as I'm using mpv I used https://sourceforge.net/projects/mpv-player-windows/files/release/mpv-0.37.0-x86_64.7z/download
4. Added input-ipc-server=\\.\pipe\mpvsocket to top of %APPDATA%\mpv\mpv.conf (I would have used %APPDATA%\mpv.net\mpv.conf but didn't have trakts testing as above)
5. For testing, followed above guidance to install trakts using Python 3.8, and ran trakts config set players.monitored mpv
6. In Syncplay, set mpv path to the portable version of mpv
7. Added input-ipc-server=\\.\pipe\mpvsocket to "Player arguments (if any)" in Syncplay
8. Ran Syncplay with a test room on a public server. mpv.net says it had connected.
9. In Syncplay: Added a file (BigBuckBunny.mp4) to playlist. Set media directory and added file to playlist. It then loaded in mpv.net. I sought to half-way through the file.
10. Re-opened command line prompt and ran trakts test mpv (as above).

Result:

 Trying to connect
 Connected

 Waiting for events
 Got info

Playing BigBuckBunny at 49.06%

Test: Pass.

[soredake]

1. mpv.net (too new as the latest version of mpv.net breaks Syncplay if input-ipc-server is specified in config)

I see, I will report this to mpv.net repository.

[Et0h]

1. mpv.net (too new as the latest version of mpv.net breaks Syncplay if input-ipc-server is specified in config)

I see, I will report this to mpv.net repository.

You don't need to create a new issue as I recently reported it at https://github.com/mpvnet-player/mpv.net/issues/654 but feel free to post on that issue if you can help triangulate or confirm the problem and/or code a solution.

[Et0h]

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 mpvSyncplaySocket. This should hopefully make it easier for finding the socket using trakt-scrobbler, etc.

[soredake]

@Et0h thank you, with this pr all working fine now!

[Et0h]

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]

From Syncplay 1.7.3 the user will need to specify the pipe name (e.g. input-ipc-server=\\.\pipe\examplepipe) to connect to an mpv pipe. See #674 and #675 for details.

[soredake]

@Et0h can you please restore creating secondary socket on Windows by default? Otherwise this should be reopened https://github.com/Syncplay/syncplay/issues/658, plus even after specifying input-ipc-server=\\.\pipe\mpvsocket in player arguments trakt-scrobbler is not working again. I don't use linux, everything worked fine in previous release on Windows.

[Et0h]

@Et0h can you please restore creating secondary socket on Windows by default? Otherwise this should be reopened #658, plus even after specifying input-ipc-server=\\.\pipe\mpvsocket in player arguments trakt-scrobbler is not working again. I don't use linux, everything worked fine in previous release on Windows.

I just tried the trakts test for mpv and it worked for me with Syncplay 1.7.3 using both mpv and mpv.net:

c:\Python38>trakts test mpv
Testing connection with mpv.
Please ensure that mpv is playing some media.
 Trying to connect
 Connected

 Waiting for events
 Got info

Playing BigBuckBunny at 13.61%

To get it to work with my version of mpv/mpv.net I had to set input-ipc-server=\\.\pipe\mpvsocket in the mpv.conf file so it knew where it should connect to, and then also put input-ipc-server=\\.\pipe\mpvsocket in the per-player argument for mpv/mpv.net.

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.

[notpeelz]

I did some investigating. It turns out mpv doesn't care for multiple --input-ipc-server= options. The last value specified will override the previous ones.

Depending on the order you pass the --input-ipc-server= options, this would either break SyncPlay's ability to communicate with mpv or prevent the creation of the user's socket.

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 created

Except it doesn't "break" (at least not right away) because SyncPlay extracts the --input-ipc-server=/tmp/user-mpv option from the "Player arguments" field and calls ["set_property", "input-ipc-server", "/tmp/user-mpv"] via its IPC socket.

Now this is where it gets interesting. When SyncPlay calls set_property input-ipc-server, this bit of code is executed:

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??? That's because accepted connections stay open even after the socket is reinitialized! i.e.:

# 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:

config file: input-ipc-server=/tmp/user-mpv CLI params: --input-ipc-server=/tmp/custom-mpv --input-ipc-server=/tmp/syncplay-mpv Outcome: input-ipc-server=/tmp/user-mpv:/tmp/custom-mpv:/tmp/syncplay-mpv

Uhh... but what if we want want to override the previous input-ipc-server values?

Option 1: Using a special syntax to replace the list of sockets (instead of appending by default):

config file: input-ipc-server=/tmp/user-mpv CLI params: --input-ipc-server=:/tmp/syncplay-mpv Outcome: input-ipc-server=/tmp/syncplay-mpv

Option 2: Using a special syntax to append to the list of sockets (instead of replacing by default):

config file: input-ipc-server=/tmp/user-mpv CLI params: --input-ipc-server=:/tmp/custom-mpv --input-ipc-server=:/tmp/syncplay-mpv Outcome: input-ipc-server=/tmp/user-mpv:/tmp/custom-mpv:/tmp/syncplay-mpv

Option 3: Same as option 2, but using mpv's existing property expansion mechanism:

config file: input-ipc-server=/tmp/user-mpv CLI params: --input-ipc-server=${input-ipc-server}:/tmp/custom-mpv --input-ipc-server=${input-ipc-server}:/tmp/syncplay-mpv Outcome: input-ipc-server=/tmp/user-mpv:/tmp/custom-mpv:/tmp/syncplay-mpv

Option 1 is bad because it changes existing behavior in a backward-incompatible way. Option 2 is alright. It's backward-compatible and easier to implement than Option 3. Option 3 is probably too complex/verbose (also ${} is shell syntax, which would require extra escaping, i.e --input-ipc-server='${input-ipc-server}:/tmp/user-mpv'

---

Other than that, I don't see a reliable way to "restore" the user-defined input-ipc-server property after the fact. Also keep in mind that parsing mpv's config file(s) is a bad idea, as @daniel-123 mentioned above. Not only is the config file location unpredictable, but you also have to parse additional config files specified via --include= and include properties.

[soredake]

I just tried the trakts test for mpv and it worked for me with Syncplay 1.7.3 using both mpv and mpv.net:

It doesn't work for me, syncplay works, but trakt-scrobbler saying this:

2024-04-30 09:31:32,334 - ERROR - syncplay@mpv - mpv - Error with command ['get_property', 'path']. Response: {'request_id': 1, 'error': 'property unavailable'}

@notpeelz

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 ipc_path: \\.\pipe\mpvSyncplaySocket in trakt-scrobbler, now I need to set input-ipc-server in mpv.conf and player arguments, and trakt-scrobbler is still not working, and I don't want to spend my time figuring out why because in 1.7.2 release it just worked, and I want it to stay this way on Windows.

[Et0h]

I just tried the trakts test for mpv and it worked for me with Syncplay 1.7.3 using both mpv and mpv.net:

It doesn't work for me, syncplay works, but trakt-scrobbler saying this:

2024-04-30 09:31:32,334 - ERROR - syncplay@mpv - mpv - Error with command ['get_property', 'path']. Response: {'request_id': 1, 'error': 'property unavailable'}

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.

you broke functionality that worked in 1.7.2 out of the box, I just needed ipc_path: \\.\pipe\mpvSyncplaySocket in trakt-scrobbler, now I need to set input-ipc-server in mpv.conf and player arguments, and trakt-scrobbler is still not working, and I don't want to spend my time figuring out why because in 1.7.2 release it just worked, and I want it to stay this way on Windows.

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 --input-ipc-server= rather than input-ipc-server=.

As I explain at https://github.com/iamkroot/trakt-scrobbler/issues/266#issuecomment-1923727292:

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.

[soredake]

Did you have a media file playing at the time?

Yes, I received this with any media file.

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.

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.

[notpeelz]

Or have option to create secondary socket

But isn't that already the case? You just need to set the "Player arguments" to --input-ipc-server=\\.\pipe\mpvsocket. Agreed, the way it's implemented currently is a bit awkward/confusing:

1. When SyncPlay launches mpv, it appends the user-defined "Player arguments" before its own --input-ipc-server= param. This means that SyncPlay will clobber the user-defined IPC path. At this stage, the user-defined socket doesn't exist yet.
2. To "restore" the user-defined socket, SyncPlay extracts the value from the --input-ipc-server argument you defined, then calls {"command": ["set_property", "input-ipc-server", "\\\\.\\pipe\\mpvsocket"]} via its own IPC connection. This causes SyncPlay's temporary socket file (or NamedPipe endpoint on windows) to get terminated, however this doesn't terminate the IPC connection already established between mpv and SyncPlay, allowing SyncPlay to keep working.

---

It doesn't work for me, syncplay works, but trakt-scrobbler saying this: [...]

Works fine for me. Tested in a Windows 10 VM:

Steps:

1. Install Syncplay 1.7.3 (windows installer) and mpv
2. Install scoop
3. Install Python 3.8 via scoop (trakt-scrobbler doesn't work with Python 3.12 because it uses deprecated APIs that have since been removed)

   scoop bucket add versions
   scoop install python38

4. Install trakt-scrobbler

   python38 -m pip install --user pipx
   python38 -m pipx ensurepath
   pipx install trakt-scrobbler

5. Configure trakt-scrobbler

   trakts auth
   trakts config set players.monitored mpv
   trakts config set players.mpv.ipc_path \\.\pipe\mpvsocket
   trakts start --restart

6. Launch syncplay and set "Player arguments" to --input-ipc-server=\\.\pipe\mpvsocket
7. Play big buck bunny
8. At this point, you should see windows notifications from trakt-scrobbler, informing you of the progress.

I've also tested with this configuration:

trakts config set players.monitored syncplay@mpv
trakts config unset players.mpv.ipc_path
trakts start --restart

...but it appears that trakt-scrobbler doesn't parse SyncPlay's config file correctly. e.g: If I set "Player arguments" in the GUI to --input-ipc-server=\\.\pipe\mpvsocket, my %appdata%/syncplay.ini config looks like this:

[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):

DEBUG - MainThread - monitor - Autoloaded syncplay@mpv ipc_path = \\\\.\\pipe\\mpvsocket

I can make syncplay@mpv work if I edit the config file by hand (while SyncPlay + mpv are still running), i.e.:

[client_settings]
perplayerarguments = {'c:\\users\\peelz\\scoop\\apps\\mpv\\current\\mpv.exe': ['--input-ipc-server=\\.\pipe\mpvsocket']}

then trakt start --restart

---

IMO trakt-scrobbler's syncplay@mpv shouldn't even exist. Various tools parsing each other's config files is a surefire way to create an unmaintainable, buggy mess of code. My recommendation is to manually configure players.mpv.ipc_path and avoid all of this config-parsing nonsense.

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.

[soredake]

But isn't that already the case?

No, by secondary socket I mean \\.\pipe\mpvSyncplaySocket that was used in 1.7.2 release.

[Et0h]

But isn't that already the case?

No, by secondary socket I mean \\.\pipe\mpvSyncplaySocket that was used in 1.7.2 release.

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 \\.\pipe\mpvSyncplaySocket then you can call it \\.\pipe\mpvSyncplaySocket .