[Feature] - Make `mpv` dependency optional, use e.g. `symphonia` for playback
virtualritz opened this issue · 2 comments
The README doesn't make it very obvious that radio-cli
is just a thin wrapper that merely parses a JSON
file and then runs mpv
.
This should be at the top of the README. The README gives the impression this was a client that can play (radio) media. It is not.
The fact that I need mpv
is mentioned further down in a section called How it works...
that section is closed by default. And it is after the installation instructions.
I actually only saw the mpv
dependency when I looked at the source code because all that happened on my system when trying to use radio-cli
was this:
? Select a station to play: Los 40 Principales
Los 40 Principales
Info: press 'q' to exit
thread 'main' panicked at 'Failed to execute command: Os { code: 2, kind: NotFound, message: "No such file or directory" }', src/main.rs:169:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
To be blunt: if I always listen to the same station, running mpv
with the link from my shell's history is less work (and less time & disk space wasted) than installing this and editing the ~/.config/radio-cli/config.json
.
Suggestions:
- Mention the
mpv
dependency at the top of the README. - Remove the
mpv
dependency. Use a pure Rust solution that can play back the streams.
Regarding 2.:
While there is the libmpv
crate, this is itself a wrapper around the C libmpv
library.
The latter needs to exist somewhere (i.e. possibly manually built and an env. var pointing to it to be set) so the build/installation of radio-cli
would become potentially complicated.
Because of this I'd rather suggest to use something in pure Rust like the symphonia
crate for playback and only fall back to libmpv
for the few formats not (yet) supported by symphonia
.
To not complicate the build by default, put the libmpv
fallback behind a feature flag that is off by default so using cargo
to install radio-cli
just works.
Hi! First of all, know that I get the point(s). However...
The readme (part 1)
The README doesn't make it very obvious that radio-cli is just a thin wrapper that merely parses a JSON file and then runs mpv
Straight from the README: "[...] Let's say this is just a cli frontend for playing things on mpv".
People who care about these things (maybe 20-30% of the people who see the repo) will read the "How it works section", that is why I wrote that section.
The readme (part 2)
This should be at the top of the README. The README gives the impression this was a client that can play (radio) media. It is not.
It sounds like you are implying that I'm trying to hide that fact, but I am not. In fact, quite the opposite:
I added the "how it works" section for a reason, with links to the original repositories for all mentioned applications.
As you could see in this commit, it wasn't "hidden" until two days ago. To me, the README was too long and I wanted to make it less overwhelming by contracting what people are less interested in. BUT as you can see, in one of the first sections of the readme, there's a mention of the optional dependency yt-dlp
.
You are right in that I should have put mpv
as a dependency too.
Mention the mpv dependency at the top of the README.
Will do!
Installation
I actually only saw the mpv dependency when I looked at the source code because all that happened on my system when trying to use radio-cli was this (a thread panic)
Whoops!! I didn't think about that happening, because in reality, installing the program through cargo is not "supported".
If installed through the AUR, it will actually ask you to install mpv (and optionally yt-dlp), and I didn't give a second thought to the cargo install
thing. Same with the manual compilation: in both cases you need to manually install mpv (and yt-dlp).
I will add that to the README.
Dependencies
Remove the mpv dependency. Use a pure Rust solution that can play back the streams.
Yes, and mpv coud remove yt-dlp as a dependency, but they chose not to. mpv
does all I want, so I don't see why the program would benefit from - what is effectively - bloat ("do one thing and do it well").
However, if you really want to, you can create a pull request with the changes you mentioned and if you are able to replicate all the following, the change will be more than welcome:
- Play audio (obviously)
- Play video (with captions support)
- Play youtube videos and livestreams (with captions support)
- When playing a radio stream, display the title of the current song being played (extracted from the stream's metadata)
- Move backwards and forwards in the media being played
Extra
[...] if I always listen to the same station, running mpv with the link from my shell's history is less work (and less time & disk space wasted) than installing this and editing the ~/.config/radio-cli/config.json
- Time: For me, it's easier to type
radio-cli -s lofi
thanmpv "https://www.youtube.com/watch?v=5qap5aO4i9A"
, and being able to sync that between computers is very convenient for me. - Disk space wasted:
Fortunately, I can afford to loose 4Mb of space, though I understand the concept of minimalism. In that case I suggest not using radio-cli and looking for a more minimalistic approach (since that's not the main goal of this program)
Oh! Also, about making the dependency on mpv
optional (by using other players):
I don't see how that would work, since I explicitly use mpv commands such as --very-quiet
and --no-video
.
Using other programs would imply making a case for each supported player.
I personally find it not worth the work, but feel free to fork the project and do that yourself!