no need for socat and netcat

[Hill-98]

Here's another implementation that optimizes communication with subprocesses (yeah, I know there are a lot of similar PRs).

It can communicate directly with sockets on all platforms.

ref: https://mpv.io/manual/master/#alternative-ways-of-starting-clients

[Hill-98]

I didn't test on MacOS (I don't have it)

I haven't compared performance with other PRs either, but on my pc it's fine.

[christoph-heinrich]

I was looking for a way of writing to the socket as if it was a file, but I didn't find anything, so I made #42 instead. But you managed to do it :tada:.

Unfortunately it doesn't work all that well for me. Updates don't happen all that reliably. Reducing or even removing the throttling on seeking does helps, but there is clearly some problem that seeks get dropped for some reason. And even worse, after a while (usually 10-30 seconds) the thumbnails stop getting updated at all and I have two mpv processes that use 100% of one cpu core each.

https://user-images.githubusercontent.com/8932183/194911720-962b9040-9222-49ac-925e-9ae0326c9e0e.mp4

Also note how it spawns 2 of each process. spawn() gets called two times at the beginning because of spawn_first=yes, which is normal, master also behaves that way. However without this PR I don't end up with duplicate processes.

[Hill-98]

I haven't done much testing for reliability, but I think the existing code is messy and not so reliable in many places, maybe we need a big refactoring.

edit: I'm using Windows, I'll go to Linux later to see why the above problem occurs.

[christoph-heinrich]

With spawn_first=no there are no duplicate processes and the thumbnail generation works much more reliably. However it still stops creating new thumbnails after a while and one mpv process goes to 100% cpu usage on one core.

I've put a print statement in the spawn function and it does get executed with updated times, so the problem is not the rest of the code.

[christoph-heinrich]

Performance wise I don't notice a difference between this and #42. In theory this should be faster and have less cpu usage but I couldn't notice a difference when testing both solutions and trying out different times for seek throttling.

42 does an exact seek at the end which makes it seem like the last frame comes a bit later, but when I comment that out so that it only does keyframe seeks like this here, there is no difference anymore.

I do hope you can make it reliable, because then this is the preferred solution. But I'll keep using my PR for now.

Edit: Maybe the problem is on the mpv side? The subprocess must be entering an endless loop somewhere because of the cpu usage, and I don't see how this solution could cause that in the subprocess (an endless loop in thumbfast.lua would cause cpu usage in the main process).

[Hill-98]

edit: need further testing

[Hill-98]

I fixed it!

It should be that the event triggered by seek fills up the buffer, but it does not read it, so mpv has been unable to write to the new buffer.

[christoph-heinrich]

That thing is great now. :partying_face:

I still end up with twice as many sub processes as necessary when spawn_first=yes, but I've checked and that also happens in #42 (didn't check master) so there must be a problem somewhere else.

It's so fast that we could even remove the seek throttling all together, but that can increase the cpu usage quite a bit. But reducing it to 3/60s = 50ms like I did in #42 makes it noticeably more responsive (when the system can decode fast enough) and the increase in cpu usage ain't that bad.

Also out of mere curiosity, I don't really understand how that bash stuff from your last change works. Would you mind explaining it please?

Edit: I just noticed my linter complains about a missing nil check when writing the script file.

[christoph-heinrich]

Found the reason for double the processes. I'll make a PR for it in a few hours.

[pintassilgo]

After applying this patch, mpv no longer displays thumbnails for me on Linux. Is it needed to change something else besides thumbfast.lua?

[Hill-98]

I just noticed my linter complains about a missing nil check when writing the script file.

I'm not using any lua linter, I'm guessing this? (local file = io.open(client_script_path, "w+"))

As for the seek throttling, I didn't delve into it, so it's unclear to me how it works.

Also out of mere curiosity, I don't really understand how that bash stuff from your last change works. Would you mind explaining it please?

It just keeps reading fd with bash's built-in read command, the echo command is useless, but bash doesn't seem to have an empty loop.

[Hill-98]

@pintassilgo This is very strange, can you provide information about your system environment? Bash version, etc.

If you customize the script-opts, please post.

[pintassilgo]

openSUSE Tumbleweed, KDE Wayland, following official regular updates, nothing special.

GNU bash, version 5.1.16(1)-release (x86_64-suse-linux)

I don't use custom options for thumbfast.

Operating System: openSUSE Tumbleweed 20221008
KDE Plasma Version: 5.25.5
KDE Frameworks Version: 5.98.0
Qt Version: 5.15.6
Kernel Version: 6.0.0-1-default (64-bit)
Graphics Platform: Wayland
Processors: 12 × AMD Ryzen 5 1600 Six-Core Processor
Memory: 31,3 GiB of RAM
Graphics Processor: AMD Radeon RX 570 Series

[pintassilgo]

Console log:

[Hill-98]

@pintassilgo It looks like you applied this patch in an older version, I didn't find this variable in the latest version.

try the original version of my repository

[pintassilgo]

I'm already using it.

[pintassilgo]

Log is about mpv internals:

https://github.com/mpv-player/mpv/blob/dfcd561ba9087c2a62cb7034c5e661d0b57ad660/player/lua/defaults.lua#L637

[Hill-98]

Log is about mpv internals:

https://github.com/mpv-player/mpv/blob/dfcd561ba9087c2a62cb7034c5e661d0b57ad660/player/lua/defaults.lua#L637

Ok, I got it, I didn't set a callback function for the new command_native_async, it works in the latest git version, I'll fix it.

[Hill-98]

Fixed, try again. @pintassilgo

[pintassilgo]

Thanks, now it's working fine. 👍

[christoph-heinrich]

I'm not using any lua linter, I'm guessing this? (local file = io.open(client_script_path, "w+"))

Yes

As for the search limit, I didn't delve into it, so it's unclear to me how it works.

When sending the seek command, the current time gets stored, and then when another seek request comes, it checks how long ago the last seek was and if it was too soon, it sets a timeout with the remaining time until another seek is allowed. That timeout then executes the next seek.

The echo command is useless, but bash doesn't seem to have an empty loop.

Bash does have a nop and I've tried replacing the loop content with a : and it works fine.

I still don't understand why the buffer needs to be emptied. Shouldn't mpv read from that? Might that lead to commands being read by read before mpv gets a chance to?

[Hill-98]

@christoph-heinrich

Since fd opens a socket, not a normal file, I guess the socket read and write are separated.

I solved the nil and empty loop, seek limit when I wake up and take a look, but I hope your PR merges to fix it.

[Hill-98]

I have optimized the client script, it is now more reliable, the script will exit when FD is unavailable instead of IPC_PATH being unavailable.

[po5]

Merged as f1ba4a670bb5b90e4260e74b9f8c16d0fbe239e4. Thanks for finding a way to remove the socat/netcat dependencies!