tnychn / mpv-discord

🎈 A cross-platform Discord Rich Presence integration for mpv with no external dependencies.
MIT License
172 stars 14 forks source link

PID in ipc socket path #3

Closed JoschuaL closed 3 years ago

JoschuaL commented 3 years ago

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 commented 3 years ago

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!

JoschuaL commented 3 years ago

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.

tnychn commented 3 years ago

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