margual56/radio-cli

[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:

  1. Mention the mpv dependency at the top of the README.
  2. 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 than mpv "https://www.youtube.com/watch?v=5qap5aO4i9A", and being able to sync that between computers is very convenient for me.
  • Disk space wasted:
    Screenshot_20220509_203906
    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!