search

darkhz / invidtui

A TUI based Invidious client
MIT License
191 stars 6 forks source link

Invidtui crashes if MPV window is closed #42

Open paulie-g opened 6 months ago

paulie-g commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

https://github.com/darkhz/invidtui/assets/44058754/73c24752-de62-4f74-9f61-785894a6f2a3

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.