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

PID in ipc socket path #3

Closed
JoschuaL opened this issue Feb 4, 2021 · 3 comments
Closed

PID in ipc socket path #3

JoschuaL opened this issue Feb 4, 2021 · 3 comments
Labels
enhancement New feature or request

Comments

@JoschuaL
Copy link

JoschuaL commented Feb 4, 2021

Since I, and probably other people as well, are using the ipc sockets of mpv for different purposes as well, and mpv
seems only be able to handle one of them at a time (or this is a potential OS restriction).

The ipc socket created by mpv-discord also overrides any sockets defined in the mpv.conf.
This would not be a big problem if the path was fixed, other applications could be adjusted to use the ipc socket of mpv-discord.
But since this socket uses the PID in its path, it can usually not be statically resolved, so thats not possible.

After looking in the code, it was not obvious to me why the PID needs to be in the path.
If there is a specific reason I"m just not seeing, then thats fine. Changing the code and running it with a static path seems to work fine for me, so I can keep doing that. But if there is no functional reason to have the path be dynamic, I would suggest changing it to a static path, ideally /tmp/mpvsocket, which is the standart path used by most scripts and applications by default.

I can submit a PR for doing so in that case.

@tnychn
Copy link
Owner

tnychn commented Feb 4, 2021

After looking in the code, it was not obvious to me why the PID needs to be in the path.

LOL guess I was just too naive to think that making the socket path dynamic could avoid collision with other mpv plugins that are also using the IPC socket.

I can submit a PR for doing so in that case.

Thanks! It would be great. Really appreciate your help!

@tnychn tnychn added the enhancement New feature or request label Feb 4, 2021
@JoschuaL
Copy link
Author

JoschuaL commented Feb 5, 2021

AFAIK multiple mpv plugins can use the same IPC socket, but only one socket can be active at a time per application. Therefore
it should theoretically be better to use a common, or at least deterministic path. Though to be honest, I dont know overly much
about the IPC protocol or how mpv implements it. I"ll start working on the PR based on that it works on my system,
though if someone else could test it on their end that would be neat.

@JoschuaL JoschuaL closed this as completed Feb 5, 2021
@tnychn tnychn linked a pull request Feb 6, 2021 that will close this issue
@tnychn
Copy link
Owner

tnychn commented Feb 6, 2021

Implemented at 1fd2498. This can be now achieved with the deafult config. Please upgrade to the latest v1.3.1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants