Invidtui crashes if MPV window is closed

[paulie-g]

Config: Up-to-date Arch with invidtui from aur/invidtui-bin
Steps to reproduce: launch a video with 'V', mpv starts playing video in own window, close window
Actual result: invidtui crashes; screws up the terminal most of the time on crash; sometimes leaves behind $XDG_HOME/.config/invidtui/socket on crash
Expected result: detects mpv exit and handles it cleanly

Discovered because mpv can't be terminated normally via keybind as per #41

[darkhz]

Interesting. Does it not show a "Player has exited" message in the terminal, when the mpv process has exited? I will take a look into this.

[darkhz]

invidtui-theme-demo-2024-03-10_04.51.37.mp4

This is how it usually handles MPV exits. I assume that closing the window will end the MPV process, similar to using killall mpv.

[paulie-g]

Not always, I sometimes get crashes, terminal state corruption and mpv ipc socket left over. Beyond that, closing invidtui when mpv exits is very undesirable, especially since invidtui doesn't retain state on exit+restart.

I saw the reasoning you linked to in the other issue and understand it. However, that choice is causing a lot of problems with usability. It would be better to start a new mpv instance for every playlist item. It doesn't make sense to pay all these penalties for having a single persistent mpv child exist - launching mpv is cheap and it has no valuable state to preserve, unlike invidtui.

I'm also not sure the issues couldn't be resolved with a better (error-checked and fault-tolerant) implementation of the mpv API.

(My apologies. Normally, this is where I'd volunteer to implement this. However, IRL issues mean I won't have time for a while and I don't want to promise a patch I can't deliver in a reasonable timeframe.)

[darkhz]

The points you list are valid. Although, I would think that one MPV instance per invidtui instance would be better than launching MPV for every playlist item, especially considering resource-constrained systems.

I will look into this.

EDIT: If possible, could you record a gif/video showing the terminal state after you close the MPV window?

[paulie-g]

I haven't been able to reproduce it, it must be racy. Wouldn't have told you much: terminal was just not echoing or showing output. In the socket left behind case, invidtui would just complain about the socket already existing (and I think you decided to implement a change that would make this moot in #43 )

There are several things here:

1. It makes sense to make the invidtui <-> mpv interface more robust and conformant
2. For audio, people with tiling WMs and potentially people running invidtui-in-terminal and its mpv via XMbed, it makes sense to reuse mpv iff: a) it can be made robust, and b) it doesn't grow its RSS indefinitely because of ytdl/other plugins
3. For video, people with floating WMs or using a floating WM mode for video (me, I've cobbled together a hybrid WM on top of openbox), it may make sense to have separate mpv processes

The upshot might be: make mpv handling more robust (including relaunching it if it goes away), then make it configurable whether it gets launched per item or reused.

In any case, independent of what you choose to do, simply exiting on mpv terminating is very undesirable.