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

input-ipc-server command line switch overridden by config in mpv.net v7 #654

Closed
Et0h opened this issue Jan 31, 2024 · 5 comments
Closed

Comments

@Et0h
Copy link

Et0h commented Jan 31, 2024

Describe the bug
Syncplay uses JSON IPC to connect to mpv.net. While this works fine with mpv.net v6, in Syncplay does not work with mpv.net v7 if the user specifies a different value for input-ipc-server in config.

I believe this is because mpv.net is prioritising the value in the config over the value used by Syncplay's value set via the input-ipc-server command line switch.

To Reproduce

  1. Download and install Syncplay 1.7.1.
  2. Download and install both mpv.net-v6.0.4.0-stable and mpv.net-v7.1.0.0-portable from https://github.com/mpvnet-player/mpv.net/releases
  3. Run Syncplay, choose a server and room name, and set the player path to mpv.net 6.0.40. Syncplay and mpv.net should run fine
  4. Run Syncplay again, but change the player path to mpv.net-v7.1.0.0-portable. Syncplay will run fine.
  5. Set input-ipc-server value in C:\Users(user)\AppData\Roaming\mpv.net\mpv.conf manually or via mpvnet
  6. Run Syncplay, choose a server and room name, and set the player path to mpv.net 6.0.40. Syncplay and mpv.net will still run fine
  7. Run Syncplay again, but change the player path to mpv.net-v7.1.0.0-portable. Syncplay will freeze and not load properly, presumably because it cannot connect.

Expected behavior
For mpv.net to allow IPC-Server to be overridden so Syncplay will start.

Additional context
Add any other context about the problem here.

  1. mpv.net version: mpv.net-v7.1.0.0-portable
  2. Windows version: Windows 10
  3. Syncplay uses a custom implementation of python-mpv-jsonipc by @iwalton3 for communication with mpv and mpv.net.
  4. This might relate to v7.0.0.3 Beta (2023-12-15) having added input-ipc-server to the conf editor, but I've not tested this.
@stax76
Copy link
Collaborator

stax76 commented Feb 1, 2024

In mpv.net v6 most CLI options were set after the initialization of the mpv core, in v7 this was changed to all CLI options being set before the initialization of the mpv core, I was told by a mpv developer that's the way to do it. It's probably this change causing the problem. I'll try to better understand it and fix it, but it will take time, as I'm currently taking a development break.

@soredake
Copy link
Contributor

soredake commented Feb 1, 2024

@Et0h can this be fixed on syncplay side by reading input-ipc-server value from mpv.conf and just using it?

@Et0h
Copy link
Author

Et0h commented Feb 2, 2024

@soredake It would be too complicated for Syncplay to do this. For example, there could be several different conf files and in those files there might be one than one input-ipc-server value, e.g. due to profiles. It is just way too hacky and unpredictable. It'd be far better to just tell the user to delete their input-ipc-server value.

@stax76 It's interesting that an mpv developer called for this behaviour as it results in different results to the main MPV.

Let's provide an easier to test example of osd-msg1.

In mpv-0.37.0-x86_64 if I put osd-msg1=Bob in mpv.conf but run with the argument osd-msg1=Alice, it will display Alice.
On the other hand, if I do the same in mpv.net v7 it will display Bob.

For most things this isn't a big deal as I can just override the value via the IPC connection once mpv.net has started, but if the input-ipc-server value is set then I don't have an IPC connection in the first place.

If this is a known issue then people can just be instructed to not set input-ipc-server in their mpv.net configuration if they want to use Syncplay, and instead to set it via Syncplay itself (a feature I'm working on).

As such, this probably doesn't need to be seen as a high priority issue, but is worth knowing about.

@stax76
Copy link
Collaborator

stax76 commented Feb 2, 2024

I could reproduce it and think I understand now what's wrong, I fix it in the next couple of days.

@stax76
Copy link
Collaborator

stax76 commented Feb 3, 2024

A new release fixing it is available.

@stax76 stax76 closed this as completed Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants