[emms-help] mpv player alternative with json-ipc

[Mike Kazantsev]

Hi,


Have been using recently-merged dochang/emms-player-mpv emms backend
for a while, but recently had an thought to get track durations from
mpv, as pre-scanning them otherwise from mostly-sshfs sources is quite
slow.

So implemented a different backend for mpv using its bidirectional
"JSON IPC" API with long-running mpv instances (kinda like mpd):

https://github.com/mk-fg/emacs-setup/blob/master/extz/emms-player-mpv.el


Advantages of using mpv this way vs player-simple (that I can think of):

- Access to any properties, including track metadata, any file/stream
info, video/audio output info, playlist info (if added as such).

Can be used with e.g. long-running internet radio URLs to fetch
currently playing track information, as parsed by mpv from icy tags.

- Precise event monitoring - e.g. only call emms-player-started when
audio actually starts playing, taking any delay due to mpv startup,
youtube-dl or file buffering (netwok sources) into account.

- Feedback from mpv on any command (success/error) and playback state
(e.g. get playback position info).

- Maybe some minor performance gains due to mpv staying initialized in
memory, instead of starting from scratch for every track.


Apparent disadvantages:

- More work emacs-side to handle events from JSON-IPC.
For static files, it's usually just ~5-10 JSON lines per file.

- More complex implementation.


As I've also noticed that mpv backend was recently merged info emms,
wanted to present this one here as well, in case it might be for
whatever reason preferrable to player-simple mpv wrapping.

Not sure if it is the case, given more complex code, but also not sure
if maybe someone can think of other simple backend deficiencies which
this one can address nicely.


If it is worthy replacement for simple mpv backend for whatever reason
- let me know, can adjust code style to fit better with other .el
files in the repo, and get rid of dependencies like s.el/dash.el
(only used in couple of places).

Any other comments are also welcome, of course.


Cheers!

[Pierre Neidhardt]

Thanks for the hard work!

Before merging dochang/emms-player-mpv, we also considered
https://github.com/momomo5717/emms-player-simple-mpv which uses IPC as
well.

It turned out that there were many rough edges that needed polishing
before merging. So instead we opted for dochang's simpler version for
now and we will adopt IPC features incrementally.

I haven't looked at your code, but isn't it duplicating momomo5717's
effort?

That said, it's no waste of effort and if you'd like to help merge the
IPC features, you are very welcome!

--
Pierre Neidhardt

[Mike Kazantsev]

On Fri, 13 Apr 2018 21:53:47 +0530

Post by Pierre Neidhardt
Thanks for the hard work!
Before merging dochang/emms-player-mpv, we also considered
https://github.com/momomo5717/emms-player-simple-mpv which uses IPC as
well.
It turned out that there were many rough edges that needed polishing
before merging. So instead we opted for dochang's simpler version for
now and we will adopt IPC features incrementally.
I haven't looked at your code, but isn't it duplicating momomo5717's
effort?

Didn't know about that implementation, though not sure why I missed it
when looking for mpv wrappers earlier.

But yeah, it seem to be implementing same IPC thing, though in a bit
different style using transaction queues.

Code there seem to be ~3x in size, which I guess is mostly due to tq
boilerplate and confirmed command deliverly, while my code just fires
these same as with --input-file= fifo and assigns common
'(when err (message "emms-mpv-ipc-error: %s" (s-trim err))))'
handler to all of these, where user'd presumably just retry (when player
loads stream or whatever), same as with cli/osd inputs.

Oh well, guess if you need another idea for how thing can be wrapped,
there it is :)

Post by Pierre Neidhardt
That said, it's no waste of effort and if you'd like to help merge the
IPC features, you are very welcome!

Not sure if there's any gradual way to merge one implementation into
another, as with define-emms-simple-player you'd have simplier
process-per-track model, and replacing that with something like sending
commands to singleton "(emms-mpv-ipc)" (be it tq or async socket) pretty
much replaces the whole thing (which was rather simple to begin with).

Dunno about rough edges you've mentioned though - I'm kinda used to
writing code/protocols around async sockets (usually in python with
twisted/asyncio), so seemed to be rather simple protocol to me,
especially given how mpv can handle any commands at any time/state
without breaking, same as with --input-file= fire-and-forget fifo.

Will follow-up on that in a reply to Yoni's email, I guess.


Thanks for the feedback!
First time I've seen tq module used in the wild :)

[Mike Kazantsev]

On Sat, 14 Apr 2018 00:00:14 +0500

Post by Mike Kazantsev
On Fri, 13 Apr 2018 21:53:47 +0530

Post by Pierre Neidhardt
It turned out that there were many rough edges that needed polishing
before merging. So instead we opted for dochang's simpler version for
now and we will adopt IPC features incrementally.

Dunno about rough edges you've mentioned though ...

Skimming though ML archives, noticed (mostly-missing) conversation
about json-ipc which seem to mention mpv version issues wrt support for
that feature, which I haven't considered at all as it's the first time
I've used this API (more familiar with controlling mpv from lua),
and kinda-assumed it was there long enough, given how simple and
inadequate --input-file= is comparatively.

In which case guess maybe it makes even more sense to leave simple
implmenetation in for compatibility + simplicity reasons for a while,
as improvements that json-ipc provides don't seem to be huge.

[Ian Dunn]

PN> I haven't looked at your code, but isn't it duplicating momomo5717's
PN> effort?

A brief pass of Mike's code looks like he's using a persistent mpv instance (via the --idle flag), whereas momomo5717's version spawns and destroys a new one with each new track. Mpv supports both methods, so I think a persistent instance would be the way to go.

[Pierre Neidhardt]

Post by Ian Dunn
A brief pass of Mike's code looks like he's using a persistent mpv
instance (via the --idle flag), whereas momomo5717's version spawns
and destroys a new one with each new track. Mpv supports both
methods, so I think a persistent instance would be the way to go.

Sounds reasonable.

--
Pierre Neidhardt

[Yoni Rabkin]

Thank you for the offer. Having the bi-directional communication would
be good. For the 5.0 release on the 1st of May we will keep the current
implementation since it Just Works (TM). After the release I have no
problem with adding API features galore.

Do you want to be the one working on this?

[Mike Kazantsev]

On Fri, 13 Apr 2018 14:44:49 -0400

Post by Yoni Rabkin
Thank you for the offer. Having the bi-directional communication would
be good. For the 5.0 release on the 1st of May we will keep the current
implementation since it Just Works (TM). After the release I have no
problem with adding API features galore.
Do you want to be the one working on this?

Yeah, pretty sure it shouldn't be beyond my humble abilities to maintain
that code, and definitely plan to do it for myself, as I use it every
day here.

Indeed it'd make sense to keep current implementation for at least one
release, and unless you've looked over the code, I'd think jury is
still out on whether it's actually any better than simple-player :)


Will mark the day to check for release and prepare/post a format-patch
here with most basic replacement for review, with follow-up patches for
basic metadata.

Not sure on coding style strictness, tabs-spaces and such, but all such
trivia should probably all be deferred until then.

Thanks!

[Yoni Rabkin]

I have version 0.3.4 of mpv on my machine (Trisquel) so this code
wouldn't work on my box. I think one needs (<= 0.17.0 mpv-version). We'd
need a way not to break mpv for people who start enjoying it in 5.0.

Post by Mike Kazantsev
Not sure on coding style strictness, tabs-spaces and such, but all such
trivia should probably all be deferred until then.

Emms is old (the Savannah project was registered 15 years ago), so code
style is all over the place. As maintainer I try for three things:

* HEAD should be pretty stable before a release, but otherwise it
can be as bleeding edge as we want.

* It should compile without warnings.

* Documentation is good, and should be updated.

[Mike Kazantsev]

On Fri, 13 Apr 2018 15:31:23 -0400

Post by Yoni Rabkin
I have version 0.3.4 of mpv on my machine (Trisquel) so this code
wouldn't work on my box. I think one needs (<= 0.17.0 mpv-version). We'd
need a way not to break mpv for people who start enjoying it in 5.0.

Right.

Guess I'll look for a way to separate simple/json-ipc versions cleanly
and load one or the other depending on mpv version.

Post by Yoni Rabkin
Emms is old (the Savannah project was registered 15 years ago), so code

All noted, thanks.

[Mike Kazantsev]

This post might be inappropriate. Click to display it.

[Pierre Neidhardt]

Wow, great work! I'll keep running your branch for now and report if I
run into any issue.

Thanks again!

[Pierre Neidhardt]

Well, did not take long! :p

- When I first started playing, I could not hear anything. I had to
pause/unpause so that mpv would start make sounds.

- Seeking forward / backward resets the timer (but the seeking works).

[Mike Kazantsev]

On Mon, 16 Apr 2018 11:11:41 +0530

Post by Pierre Neidhardt
Well, did not take long! :p

Oh well, not entirely unexpected :)

Post by Pierre Neidhardt
- When I first started playing, I could not hear anything. I had to
pause/unpause so that mpv would start make sounds.

Suspect this one is due to mpv configuration (which maybe needs to
be reset on first connection) or different behavior of --idle in
previous versions.

Can you do:

(require 'emms-player-mpv)
(add-to-list 'emms-player-list 'emms-player-mpv)
(setq emms-mpv-debug t)
(emms-player-mpv-start (emms-playlist-current-selected-track))

Then send whatever ends up in *Messages* when running these?

"loadfile" seem to be "Load the given file and play it." so works fine
for starting playback here, gotta check it for previous versions, in
case it might be the cause for the issue, though log can probably clear
this out faster.

Post by Pierre Neidhardt
- Seeking forward / backward resets the timer (but the seeking works).

Wasn't the case before, but can definitely see this happening in
current version, missed it during testing.

Should be easy to fix though.


Thanks!

[Mike Kazantsev]

On Mon, 16 Apr 2018 12:03:26 +0500

Post by Mike Kazantsev
On Mon, 16 Apr 2018 11:11:41 +0530

Post by Pierre Neidhardt
Well, did not take long! :p
- Seeking forward / backward resets the timer (but the seeking works).

Was due to issuing emms-player-started on every playback-restart, fixed here:
https://git.savannah.gnu.org/cgit/emms.git/commit/?h=mpv-json-ipc&id=e04cbaa1

Post by Mike Kazantsev

Post by Pierre Neidhardt
- When I first started playing, I could not hear anything. I had to
pause/unpause so that mpv would start make sounds.

Found that this can also happen if both these are true:

- Track is an url.
- It's not the only track in the emms playlist.

Due to the same emms-player-started change, should be fixed by the same
commit as above.
Might fix issue in your case too, if cause is the same.

If still happens, would help greatly if you can post the log or maybe
more details on specific sequence of actions and tracks involved
(so that I can try to reproduce it here).

[Pierre Neidhardt]

Post by Mike Kazantsev
(require 'emms-player-mpv)
(add-to-list 'emms-player-list 'emms-player-mpv)
(setq emms-mpv-debug t)
(emms-player-mpv-start (emms-playlist-current-selected-track))

From which state do you expect this? Anyways, here is a message when I
ran the last command from the playlist buffer with multiple tracks and
one track was playing.

--8<---------------cut here---------------start------------->8---
emms-mpv-ipc-json >> {"command":["loadfile","/home/ambrevar/music/Juno Reactor/2012 - From the Land of the Rising Sun - Inside the Reactor II/Juno Reactor - 02 - Tokyo Dub - Tri-Force Remix.ogg","replace"],"request_id":31}
(lambda (data err) (if (eq err (quote connection-error)) (progn (emms-player-mpv-cmd cmd))))
emms-mpv-ipc-json << {"data":null,"request_id":31,"error":"success"}
emms-mpv-ipc-json << {"event":"audio-reconfig"}
emms-mpv-ipc-json << {"event":"tracks-changed"}
emms-mpv-ipc-json << {"event":"end-file"}
emms-mpv-ipc-json << {"event":"start-file"}
emms-mpv-ipc-json << {"event":"property-change","id":1,"name":"duration","data":null}
emms-mpv-ipc-json << {"event":"tracks-changed"}
emms-mpv-ipc-json << {"event":"metadata-update"}
emms-mpv-ipc-json << {"event":"audio-reconfig"}
emms-mpv-ipc-json << {"event":"file-loaded"}
emms-mpv-ipc-json << {"event":"seek"}
emms-mpv-ipc-json << {"event":"property-change","id":1,"name":"duration","data":454.961633}
emms-mpv-ipc-json << {"event":"audio-reconfig"} [2 times]
emms-mpv-ipc-json << {"event":"playback-restart"}
--8<---------------cut here---------------end--------------->8---

I noticed the issue happens when going to the next track. I'm not
sure it exactly when it happens, but it happens very frequently, so I'm
quite puzzle that you did not experience it.

Post by Mike Kazantsev

Post by Pierre Neidhardt
- Seeking forward / backward resets the timer (but the seeking works).

Last commit fixed it.

[Mike Kazantsev]

On Tue, 17 Apr 2018 17:24:38 +0530

Post by Pierre Neidhardt
From which state do you expect this?

I thought you'd run it from scratch, with all the requires and init
stuff in there, so that everything that mpv instance does will be
captured, assuming that issue happens there, of course.

But any log is fine, really :)

Post by Pierre Neidhardt
Anyways, here is a message when I
ran the last command from the playlist buffer with multiple tracks and
one track was playing.

...

Post by Pierre Neidhardt
I noticed the issue happens when going to the next track. I'm not
sure it exactly when it happens, but it happens very frequently, so I'm
quite puzzle that you did not experience it.

Logs for mpv my configuration are identical, except for one thing -
'{"event":"seek"}' in there right before playback is supposed to start
("file-loaded -> audio-reconfig x2 -> duration -> playback-restart"
in my case).
But that shouldn't really affect anything, guess maybe it's maybe some
kind of "skip initial silence" script/filter that you might have there.


I wonder if maybe I'm misunderstanding the situation though.

Are steps to reproduce the issue same as following:

- Start any kind of mpv playback, it starts normally.
- Pause currently-playing track via e.g. emms-player-mpv-pause.
- Run emms-next.
- Playlist is switched to next track, but playback is still paused.

With expected result: next track starts playing after (emms-next).
Vs actual result: playback is still paused after (emms-next).

This is the issue that you meant, right?

And e.g. test-case proposed in my previous mail doesn't demonstrate the
issue as there's no pausing involved there?


I think this situation is indeed a bug, as guess emms-next should
also unpause the track, or at least not reset emms-player-paused-p if
it doesn't - will fix in a moment, haven't considered what should
happen on pause+switch like that.

[Mike Kazantsev]

On Tue, 17 Apr 2018 20:26:49 +0500

Post by Mike Kazantsev
I think this situation is indeed a bug, as guess emms-next should
also unpause the track, or at least not reset emms-player-paused-p if
it doesn't - will fix in a moment, haven't considered what should
happen on pause+switch like that.

Fixed in last commit by always removing paused state in
emms-player-start, which seem to make perfect sense there.

[Pierre Neidhardt]

Post by Mike Kazantsev
Fixed in last commit by always removing paused state in
emms-player-start, which seem to make perfect sense there.

OK, seems to work fine so far. I'll have a look at code later when I
have more time.

Yoni?

[Mike Kazantsev]

On Wed, 18 Apr 2018 11:50:25 +0530

Post by Pierre Neidhardt

Post by Mike Kazantsev
Fixed in last commit by always removing paused state in
emms-player-start, which seem to make perfect sense there.

OK, seems to work fine so far. I'll have a look at code later when I
have more time.

But was it the issue that I've described in mail before that?

Post by Pierre Neidhardt
...
This is the issue that you meant, right?
And e.g. test-case proposed in my previous mail doesn't demonstrate the
issue as there's no pausing involved there?

(see that message for full context for these)


I mean, if it's not, and you didn't pause playback in the first place,
wonder if this pause-reset just masks something else that makes it
paused without your action in the first place.

If so, should be possible to add observe_property for pause, see where
it gets enabled, track it down that way, among other tricks.

[Pierre Neidhardt]

Sorry for the lack of info, I have very little time at the moment.
I'll come back to this later, just bump me if I forget.

[Pierre Neidhardt]

I've noticed another issue: with mpv constantly running, it keeps
pulseaudio always up and seems to prevent power management on the audio
card.

I'm not sure about the details but a consequence of switching to Mike's
implementation of the mpv backed has resulted in a 25% drop in battery
life.

I'm not saying that this is necessarily emms-player-mpv.el's fault: it
could be an issue with power management on my end. Pulseaudio should be
able to sleep when no track is playing I guess. The issue could be with
mpv itself.

[Mike Kazantsev]

On Sun, 22 Apr 2018 14:44:30 +0530

Post by Pierre Neidhardt
I've noticed another issue: with mpv constantly running, it keeps
pulseaudio always up and seems to prevent power management on the audio
card.
I'm not sure about the details but a consequence of switching to Mike's
implementation of the mpv backed has resulted in a 25% drop in battery
life.

Pretty sure drain is purely due to hardware not being suspended, not
pulseaudio using CPU when idle or anything like that, though you can
check the latter via "top" or similar tools.

Pulse shouldn't exit with --exit-idle-time while there are clients,
which long-running mpv instance is, as well as any volume applets and
such.


Don't think clients or even paused streams should normally prevent
hw/sink from being suspended though.

I'd suggest checking following:


- "pactl list modules | grep module-suspend-on-idle" - should be there.

More info on the module:
https://wiki.freedesktop.org/www/Software/PulseAudio/Documentation/User/Modules/#index64h3

Should be loaded in default configuration.


- "pactl list sinks" and check "State: ..." line there.

For me it changes like this:

- RUNNING when mpv playback is active and I can hear sounds.

- IDLE immediately after playback in mpv is paused or stopped, with
mpv process still running.

- SUSPENDED after 5 seconds from pausing/stopping mpv playback.

This should be due to module-suspend-on-idle default timeout= value
(5s).


- Check if pulseaudio suspends ALSA device - in pulseaudio logs.

E.g. stop the thing if it's running, and start in verbose mode:

% systemctl --user stop stop pulseaudio.socket pulseaudio
% pulseaudio --daemonize=no\
--disallow-exit --exit-idle-time=99999999 -vvvvv -n 2>&1 |
tee > pa.log

And check for output like this there:

I: [pulseaudio] module-suspend-on-idle.c: Source alsa_input.pci-0000_00_14.2.analog-stereo idle for too long, suspending ...
D: [pulseaudio] source.c: Suspend cause of source alsa_input.pci-0000_00_14.2.analog-stereo is 0x0004, suspending
I: [alsa-source-Generic Analog] alsa-source.c: Device suspended...
...
D: [pulseaudio] module-suspend-on-idle.c: Sink alsa_output.pci-0000_00_14.2.analog-stereo becomes busy, resuming.
D: [pulseaudio] sink.c: Suspend cause of sink alsa_output.pci-0000_00_14.2.analog-stereo is 0x0000, resuming
D: [pulseaudio] reserve-wrap.c: Successfully acquired reservation lock on device 'Audio0'
I: [alsa-sink-Generic Analog] alsa-sink.c: Trying resume...


- If you can hear when audio hardware is suspended vs active (e.g. pop
then silence instead of low background hum), then I'd suggest also
making sure that it follows what pulseaudio does, in case it's
failing to do so despite the log output.

Alternatively, check output of something like this:

# lsof -p $(pgrep -x pulseaudio) | grep /dev/snd/pcm

It should have stuff like "/dev/snd/pcmC0D0p" listed in there when
sink is RUNNING, but close them completely when SUSPENDED, thus
cutting off any possible use of actual hardware, allowing it to be
suspended by kernel.


- If all these work fine, and audio hw indeed seem to be suspended by
pulse, but you still get power drain, I'd suggest checking powertop
or such, once with pulseaudio process killed (make sure it's not
auto-started), and once with it running.

And see if it can point to specific process or kernel routine
doing something weird with pulse running, might be a bug somewhere,
I think.

Post by Pierre Neidhardt
I'm not saying that this is necessarily emms-player-mpv.el's fault: it
could be an issue with power management on my end. Pulseaudio should be
able to sleep when no track is playing I guess. The issue could be with
mpv itself.

Don't think mpv does anything special - just runs a persistent pulse
client, same as e.g. volume applet in whatever desktop environment
would do, and that shouldn't cause an issue.

When it's playing or paused, it also has a stream running, but that
should be nothing new, as it does that without --idle as well.

And indeed, pulseaudio should suspend sink and underlying hardware by
default if there are no actual audio samples going through there,
regardless of whether there are clients or streams created in it.


It is possible to do what module-suspend-on-idle does in pulse with mpv
process though, i.e. setup run-at-time timer on emms-player-stopped-hook
to check emms-player-playing-p or run emms-mpv-proc-stop there.

Doing that by default would break some of those nice use-cases for
long-running mpv though.

Knowing what's the exact issue in your case might open a possibility to
check and workaround for it, at least if it looks common enough, so
would be cool if you could find the cause of that thing.

[Pierre Neidhardt]

Thanks for all the tips, that's very helpful.
After further testing, I don't think this has anything to do with Emms.

I use GuixSD and I experience strange behaviour when I re-log to my
session: pulseaudio is started multiple times.

[Mike Kazantsev]

On Mon, 16 Apr 2018 01:00:17 +0500

Post by Mike Kazantsev
Pushed new backend option now to mpv-json-ipc branch, with support for
both older mpv versions with one-pid-per-track + --input-file operation
and newer versions running as "mpv --idle".
Tried to address all the feedback and suggestions
(thanks again, let me know if I missed any).
Guess I'll leave it there until next release, and maybe ping about way
ahead for it afterwads, fixing any issues that might pop-up there in the
meantime.

Did promise to send ping-mail after 5.0 release, here it is.

Have spotted and fixed a few minor issues and added optional metadata
updates since initial push, with more details on these below.

Should be ready to merge, either as a replacement or in addition to
mpv-simple backend (dunno if redundancy is worth it though, should be
easy to fix any issues here).

Don't think I should have access to master branch, but if you want me
to merge/push it there myself, can easily do that.

No rush on it though.
I'll probably keep using the thing myself for a while (until mpv morphs
into mplayer3 or such), so should always be in roughly working state.


Changes since initial push in more detail:

- Fixed handling for playback start failures, when mpv goes into
"idle" state instead of "start-file" for whatever reason - missing
file, access error, etc.

Previously this would require manually switching to next track, now
works same as with simple players - skips track/file silently.

- Made track duration updates optional, with
emms-player-mpv-update-duration defcustom (default on) to add/remove
hooks that do that.

Uses same emms-mpv-event-functions hook now as user might define, so
easy to rewrite/disable or override, and makes sure hook works
(via "dogfooding" logic).

- Added optional track metadata update(s) when mpv plays it,
controllable via emms-player-mpv-update-metadata defcustom
(default OFF, to not disrupt any other emms-info setup).

Also works via same hooks, and can be used as an example for parsing
any custom metadata (which differs slightly across many media types),
or extended to do that.

Found last option rather useful for both network streams (metadata
updates cause with desktop notifications on track changes) as well as
playback from network mountpoints (pre-parsing info from there is a
bad idea in general).

[Pierre Neidhardt]

This is great! Thanks for the update.
I'll review the code as soon as possible.

It's been working well for me so far.

Post by Mike Kazantsev
Don't think I should have access to master branch, but if you want me
to merge/push it there myself, can easily do that.

Yoni, what do you think? :)

[Yoni Rabkin]

Post by Mike Kazantsev
On Mon, 16 Apr 2018 01:00:17 +0500

Post by Mike Kazantsev
Pushed new backend option now to mpv-json-ipc branch, with support for
both older mpv versions with one-pid-per-track + --input-file operation
and newer versions running as "mpv --idle".
Tried to address all the feedback and suggestions
(thanks again, let me know if I missed any).
Guess I'll leave it there until next release, and maybe ping about way
ahead for it afterwads, fixing any issues that might pop-up there in the
meantime.

Did promise to send ping-mail after 5.0 release, here it is.

It works fine with my mpv 0.3.4. I currently don't have a machine with a
newer version of mpv to test, but I'll be setting one up Real Soon Now
(TM).

Since it supports both the old and new mpv and provides an interface for
interesting new features, it should be included in master so that
everyone can hammer on it. I had Zhang's emms-player-mpv.el set up and
in moving to your version I didn't need to change anything; it doesn't
even break existing setups thanks to the auto-detection of the mpv
version (smart).

Please feel free to merge this into master so that more people can
hammer on it.

Before you do, please change the dependence on the cl package to the
newer cl-lib (clean namespace), and make sure that emms-mpv-ipc-proc is
defined before it is referenced.

[Mike Kazantsev]

On Fri, 25 May 2018 16:02:10 -0400

Post by Yoni Rabkin
Before you do, please change the dependence on the cl package to the
newer cl-lib (clean namespace), and make sure that emms-mpv-ipc-proc is
defined before it is referenced.

Oh, right, forgot to check it for warnings after updates, fixed both of
these, thanks.

Second one by adding dummy define for emms-mpv-ipc-proc to keep
defcustoms in the head of the file before any internals for readability
(as I tend to skim code to find tunables myself, not customize via ui).

Post by Yoni Rabkin
Please feel free to merge this into master so that more people can
hammer on it.

Done.

I did squash all commits of the initial implementation into one, as they
don't seem to be very relevant to overall history, same as it was with
previous implementation.

Indeed, more exposure should hopefully find more interesting uses and
tweaks for many things that mpv offers via its APIs, as well as weed
out bugs faster.

[Yoni Rabkin]

Post by Mike Kazantsev
On Fri, 25 May 2018 16:02:10 -0400

Post by Yoni Rabkin
Before you do, please change the dependence on the cl package to the
newer cl-lib (clean namespace), and make sure that emms-mpv-ipc-proc is
defined before it is referenced.

Oh, right, forgot to check it for warnings after updates, fixed both of
these, thanks.
Second one by adding dummy define for emms-mpv-ipc-proc to keep
defcustoms in the head of the file before any internals for readability
(as I tend to skim code to find tunables myself, not customize via ui).

As do I; I find writing Lisp to be the best user interface experience.

Post by Mike Kazantsev

Post by Yoni Rabkin
Please feel free to merge this into master so that more people can
hammer on it.

Done.
I did squash all commits of the initial implementation into one, as they
don't seem to be very relevant to overall history, same as it was with
previous implementation.

Makes sense.

Once it stabilizes we'll need to add documentation in the Fine
Manual. But that's only after it's been hammered upon.

Post by Mike Kazantsev
Indeed, more exposure should hopefully find more interesting uses and
tweaks for many things that mpv offers via its APIs, as well as weed
out bugs faster.

I use Emms to play long videos often and would like to integrate
bookmarking with the ability to detect when and where the video had been
paused on the mpv side (someone walked into the room and hit the
space-bar on my machine in order to talk to me). So that's at least one
use I'm looking forward to.

[Mike Kazantsev]

On Fri, 25 May 2018 17:44:08 -0400

Post by Yoni Rabkin
I use Emms to play long videos often and would like to integrate
bookmarking with the ability to detect when and where the video had been
paused on the mpv side (someone walked into the room and hit the
space-bar on my machine in order to talk to me). So that's at least one
use I'm looking forward to.

That's a good idea.

Note that at least newer mpv has "write-watch-later-config" command
which stores playback position under ~/.config/mpv/watch_later/ and
will continue from the same place unless mpv is configured not to
(via e.g. --no-resume-playback option).

So unless you want actual bookmarks, i.e. multiples of these for files,
and not just one "where did I stop here" position, binding something
like (emms-player-mpv-cmd '(write-watch-later-config))) should work.

To be fair, same trick should also work with simple player as it's a
one-way command without much need for reply.

[Yoni Rabkin]

Post by Mike Kazantsev
On Fri, 25 May 2018 16:02:10 -0400

Post by Yoni Rabkin
Before you do, please change the dependence on the cl package to the
newer cl-lib (clean namespace), and make sure that emms-mpv-ipc-proc is
defined before it is referenced.

Oh, right, forgot to check it for warnings after updates, fixed both of
these, thanks.
Second one by adding dummy define for emms-mpv-ipc-proc to keep
defcustoms in the head of the file before any internals for readability
(as I tend to skim code to find tunables myself, not customize via ui).

Post by Yoni Rabkin
Please feel free to merge this into master so that more people can
hammer on it.

Done.
I did squash all commits of the initial implementation into one, as they
don't seem to be very relevant to overall history, same as it was with
previous implementation.
Indeed, more exposure should hopefully find more interesting uses and
tweaks for many things that mpv offers via its APIs, as well as weed
out bugs faster.

I also think the code needs to move to the emms-player-... namespace.

Using the file method it doesn't detect the player ending and therefore
doesn't move on to the next track. This had worked before, so I'm unsure
of what changed.

[Mike Kazantsev]

On Fri, 25 May 2018 17:55:56 -0400

Post by Yoni Rabkin
I also think the code needs to move to the emms-player-... namespace.

Yeah, I was kinda afraid of that.

Not hard to change, but that'd make all the names so horribly long,
hurting readability, unless there some way to filter all that noise out
(and fit more than one such name on a line).

Is there no other convention to maybe use much shorter reasonably-unique
prefix instead of much longer one?

Post by Yoni Rabkin
Using the file method it doesn't detect the player ending and
therefore doesn't move on to the next track. This had worked before,
so I'm unsure of what changed.

Will look into that.

Didn't think any of late changes should've affected 'file, so only ran
basic play-stop-next routine, which guess was a mistake, and should
probably be automated anyway.

[Mike Kazantsev]

On Sat, 26 May 2018 03:06:54 +0500

Post by Mike Kazantsev
On Fri, 25 May 2018 17:55:56 -0400

Post by Yoni Rabkin
Using the file method it doesn't detect the player ending and
therefore doesn't move on to the next track. This had worked before,
so I'm unsure of what changed.

Will look into that.

Should be fixed now, was due to imperfect logic reversal between
stopped/playing flag on the process in an earlier update.

Guess an immediate counter-point to me squashing history just a
few dozen minutes earlier.

[Yoni Rabkin]

Post by Mike Kazantsev
On Fri, 25 May 2018 17:55:56 -0400

Post by Yoni Rabkin
I also think the code needs to move to the emms-player-... namespace.

Yeah, I was kinda afraid of that.
Not hard to change, but that'd make all the names so horribly long,
hurting readability, unless there some way to filter all that noise out
(and fit more than one such name on a line).
Is there no other convention to maybe use much shorter reasonably-unique
prefix instead of much longer one?

None that I can think of, unless we move to emms-plr-... or something,
which isn't a huge improvement; I think that players should have their
own namespace.

I've always kind of visually "tuned out" the long names. I guess I could
also write up some code to abbreviate the display of these names on the
Emacs-level.

What do other people think of this?

Post by Mike Kazantsev

Post by Yoni Rabkin
Using the file method it doesn't detect the player ending and
therefore doesn't move on to the next track. This had worked before,
so I'm unsure of what changed.

Will look into that.
Didn't think any of late changes should've affected 'file, so only ran
basic play-stop-next routine, which guess was a mistake, and should
probably be automated anyway.

That is exactly why we throw stuff into master; we can't fix it until we
know it's broken (people who want stability now have the mpv-enable 5.0
release.)

[Mike Kazantsev]

On Fri, 25 May 2018 18:22:59 -0400

Post by Yoni Rabkin

Post by Mike Kazantsev
On Fri, 25 May 2018 17:55:56 -0400

Post by Yoni Rabkin
I also think the code needs to move to the emms-player-... namespace.

Yeah, I was kinda afraid of that.
Not hard to change, but that'd make all the names so horribly long,
hurting readability, unless there some way to filter all that noise out
(and fit more than one such name on a line).
Is there no other convention to maybe use much shorter reasonably-unique
prefix instead of much longer one?

None that I can think of, unless we move to emms-plr-... or something,
which isn't a huge improvement; I think that players should have their
own namespace.
I've always kind of visually "tuned out" the long names. I guess I could
also write up some code to abbreviate the display of these names on the
Emacs-level.
What do other people think of this?

I didn't think of it much before either, but it indeed sounds like a
great idea, and kinda easy to implement too.

Have used something like this before for python:
https://github.com/mk-fg/emacs-setup/blob/master/extz/lambda-mode.el

Just need to have same code generate prefix from file name and replace
it with some unique letter.

Not sure if it'll be an improvement, but it's always hard to tell with
these things until you try.

Will post a follow-up here if it works for me at least.

[Pierre Neidhardt]

Post by Yoni Rabkin
None that I can think of, unless we move to emms-plr-... or something,
which isn't a huge improvement; I think that players should have their
own namespace.
I've always kind of visually "tuned out" the long names. I guess I could
also write up some code to abbreviate the display of these names on the
Emacs-level.
What do other people think of this?

What about https://github.com/Malabarba/names?

[Mike Kazantsev]

On Sat, 26 May 2018 01:42:47 +0200

Post by Yoni Rabkin
None that I can think of, unless we move to emms-plr-... or
something, which isn't a huge improvement; I think that players
should have their own namespace.
I've always kind of visually "tuned out" the long names. I guess I
could also write up some code to abbreviate the display of these
names on the Emacs-level.
What do other people think of this?

What about https://github.com/Malabarba/names ?

It does look like a nice solution.

Applying it to existing code was not entirely without surprises to me,
despite the claim, as one has to keep in mind that quoted symbols
aren't namespaced (as documented), so prefix will poke through
abstraction in some cases.


Abbreviating prefix in editor (or some post-save mangler hook) shouldn't
have such issue, and doesn't impose extra runtime/learning dependency,
so at first glance looks like a better idea.

But on the other hand it actually requires everyone in the target reader
audience to setup such ad-hoc abbreviation to apply, which is probably a
lot more work than figuring out what that "define-namespace" thing does.

So if not already widespread (similar to something like git), IMO a
non-starter for anyone but authors, who maybe don't benefit much from it
anyway, being most familiar with the code in question.

I think "(define-namespace ...)" is a better idea, if the goal is to
remove significant amount of visual noise and boilerplate from the code.


Whether that "visual noise" due to prefix is significant enough to
e.g. include whole additional dependency maybe can be best
tested by an example:

- Check out this code:
https://gist.github.com/mk-fg/04f12b67e3c5f53ff3e93d764991ef73

...especially places outside of defcustom/defvar and such, i.e.
actual implementation stuff:

https://gist.github.com/mk-fg/04f12b67e3c5f53ff3e93d764991ef73#file-emms-player-mpv-el-L426-L460
https://gist.github.com/mk-fg/04f12b67e3c5f53ff3e93d764991ef73#file-emms-player-mpv-el-L327-L371

- Compare to this - same two highlights:

https://gist.github.com/mk-fg/a3976db50206b54e1f13d78507d77837#file-emms-player-mpv-el-L430-L464
https://gist.github.com/mk-fg/a3976db50206b54e1f13d78507d77837#file-emms-player-mpv-el-L331-L375

- How much noise is there: Loading Image...
It's pretty much on every other line, outside of headers.

Both gists have working code.

emms-player-mpv- is maybe among the longest prefixes, so improvements
aren't necessarily that clear elsewhere.

Also note that weird indentation of the code above never uses
alignment, which would probably make even more difference, as it
drastically reduces available space on the line where applied.


Where names.el notably breaks abstraction in this case:

- All hooks, timers, make-process-likes, some map-like calls.

I.e. ":filter 'emms-player-mpv-ipc-filter" still have to be
pre-expanded, same as "(run-hooks ...)" and "(run-at-time ...)".

- All docstrings with links like `emms-player-mpv-stuff'.

- Macros and string names.


I'd say these downsides are still worth large improvement in
readability, and looks like it'd be much easier to work with as well.
So definitely would include names.el into project with long prefixes.

But don't think it should be up to me here, and I'm not sure about
copyright issues, though header in names.el seem to suggest assignment.

Maybe someone unfamiliar with code above can provide an opinion?
I'm definitely in a worst position to judge whether it's readable :)


Thanks a lot for a great suggestion, Pierre.

Didn't consider such option at all, though admittedly never looked into
the issue either.

[Yoni Rabkin]

Post by Mike Kazantsev
On Sat, 26 May 2018 01:42:47 +0200

Post by Yoni Rabkin
None that I can think of, unless we move to emms-plr-... or
something, which isn't a huge improvement; I think that players
should have their own namespace.
I've always kind of visually "tuned out" the long names. I guess I
could also write up some code to abbreviate the display of these
names on the Emacs-level.
What do other people think of this?

What about https://github.com/Malabarba/names ?

It does look like a nice solution.

Copyright wouldn't be a problem since an ELPA package would have the FSF
as the copyright holder.

However, I would be against adding an ad-hoc packaging system to
Emms. As someone who has written a lot of Common Lisp to for a living I
can safely say that a package system replaces one set of problems with
another (`shadowing-import' anyone?); it's a matter of which set of
problems you want to deal with (1). I've have chosen the set of problems
for Emms that come with a global namespace in the name of making the
code accessible and uniform both internally and within Emacs. Solutions
should probably be in the reader instead (2).

(1) https://lists.gnu.org/archive/html/emacs-devel/2014-12/msg00906.html

(2) https://lists.gnu.org/archive/html/emacs-devel/2013-07/msg00820.html

[Mike Kazantsev]

On Sun, 27 May 2018 22:20:30 -0400

Post by Yoni Rabkin

Post by Mike Kazantsev
On Sat, 26 May 2018 01:42:47 +0200

Post by Yoni Rabkin
None that I can think of, unless we move to emms-plr-... or
something, which isn't a huge improvement; I think that players
should have their own namespace.
I've always kind of visually "tuned out" the long names. I guess I
could also write up some code to abbreviate the display of these
names on the Emacs-level.
What do other people think of this?

What about https://github.com/Malabarba/names ?

It does look like a nice solution.

However, I would be against adding an ad-hoc packaging system to
Emms. As someone who has written a lot of Common Lisp to for a living I
can safely say that a package system replaces one set of problems with
another (`shadowing-import' anyone?); it's a matter of which set of
problems you want to deal with (1). I've have chosen the set of problems
for Emms that come with a global namespace in the name of making the
code accessible and uniform both internally and within Emacs. Solutions
should probably be in the reader instead (2).

Hm, yeah, guess it indeed makes more sense for new project to pick
such packaging scheme than having inconsistency and downsides of both
approaches within existing one, as also pointed out in that thread.

https://lists.gnu.org/archive/html/emacs-devel/2014-12/msg00718.html

Also, some of the downsides I had with my simple code conversion can
apparently be fixed by using proper sharp-quoted symbols for functions.


Reader aliases would seem to work in a similar way, so definitely like
that approach as well, though I think it'd bring same non-uniformity
issue with it, which is maybe why it's not implemented in emacs already
- not flawless either.


Thanks for the explaination and pointers to that discussion.
Should've probably looked it up earlier - nothing new under the sun :)

[Yoni Rabkin]

Post by Mike Kazantsev
On Fri, 25 May 2018 17:55:56 -0400

Post by Yoni Rabkin
I also think the code needs to move to the emms-player-... namespace.

Yeah, I was kinda afraid of that.

At least I'm not asking you to modify your idiosyncratic
indentation. Now _that's_ a red line I wouldn't ask anyone to cross.

[Mike Kazantsev]

(sorry for duplicate, mailed it off-list accidentally)

On Fri, 25 May 2018 18:25:13 -0400

Post by Yoni Rabkin

Post by Mike Kazantsev
On Fri, 25 May 2018 17:55:56 -0400

Post by Yoni Rabkin
I also think the code needs to move to the emms-player-... namespace.

Yeah, I was kinda afraid of that.

At least I'm not asking you to modify your idiosyncratic
indentation. Now _that's_ a red line I wouldn't ask anyone to cross.

It's some kind of auto-indenter that does that weird tabs-and-spaces
thing which I never looked into, no?

In which case fix there should be pretty straightforward - run that
formatter before commit and not bother with indentation.

I'll look into it a bit later myself though, sounds like both issues
can be easy to address by in one half-automated style-change commit,
which I don't really mind that much.

[Mike Kazantsev]

On Sat, 26 May 2018 03:36:34 +0500

Post by Mike Kazantsev
On Fri, 25 May 2018 18:25:13 -0400

Post by Yoni Rabkin
At least I'm not asking you to modify your idiosyncratic
indentation. Now _that's_ a red line I wouldn't ask anyone to cross.

It's some kind of auto-indenter that does that weird tabs-and-spaces
thing which I never looked into, no?
In which case fix there should be pretty straightforward - run that
formatter before commit and not bother with indentation.

Found that lispy-multiline formatter seem to produce good aligned
sexp formatting, which seem to be much more in-line with the existing
code, so used that.

Some existing emms code seem to have a mix of tabs and spaces, which I
don't think is consistent (main emms.el can be good example), and
probably simply due to indent automation, where emacs auto-inserts tab
instead of 4 or 8 spaces (depending on tab-size) sometimes.

Used strictly spaces with lispy-multiline formatter for consistency,
but shouldn't be a problem to tweak that to use tabs + spaces mix as
well, if preferrable.

Please let me know if it's gotten better or worse this way.

My indentation practices are indeed a bit unorthodox, so I think it'd
make sense to ditch these in any collaborative project, especially as it
doesn't really take much effort with great tools like lispy fixing it
all for you.

[Yoni Rabkin]

Post by Mike Kazantsev
On Sat, 26 May 2018 03:36:34 +0500

Post by Mike Kazantsev
On Fri, 25 May 2018 18:25:13 -0400

Post by Yoni Rabkin
At least I'm not asking you to modify your idiosyncratic
indentation. Now _that's_ a red line I wouldn't ask anyone to cross.

It's some kind of auto-indenter that does that weird tabs-and-spaces
thing which I never looked into, no?
In which case fix there should be pretty straightforward - run that
formatter before commit and not bother with indentation.

Found that lispy-multiline formatter seem to produce good aligned
sexp formatting, which seem to be much more in-line with the existing
code, so used that.
Some existing emms code seem to have a mix of tabs and spaces, which I
don't think is consistent (main emms.el can be good example), and
probably simply due to indent automation, where emacs auto-inserts tab
instead of 4 or 8 spaces (depending on tab-size) sometimes.
Used strictly spaces with lispy-multiline formatter for consistency,
but shouldn't be a problem to tweak that to use tabs + spaces mix as
well, if preferrable.
Please let me know if it's gotten better or worse this way.
My indentation practices are indeed a bit unorthodox, so I think it'd
make sense to ditch these in any collaborative project, especially as it
doesn't really take much effort with great tools like lispy fixing it
all for you.

The new indentation seems fine to me since it's what Emacs produces by
default with indent-region. There were two lines that didn't get
indented at the end so I went ahead and pushed a fix for that.

[Mike Kazantsev]

On Mon, 28 May 2018 11:07:31 -0400

Post by Yoni Rabkin

Post by Mike Kazantsev
Please let me know if it's gotten better or worse this way.

The new indentation seems fine to me since it's what Emacs produces by
default with indent-region. There were two lines that didn't get
indented at the end so I went ahead and pushed a fix for that.

Thanks.

Weird about those two, as I did whole file via one script, and they seem
identical to ones around them in previous version, yet indeed left
misindented. Gotta be a bug to fix in there somewhere...

[Yoni Rabkin]

Post by Mike Kazantsev
On Fri, 25 May 2018 16:02:10 -0400

Post by Yoni Rabkin
Before you do, please change the dependence on the cl package to the
newer cl-lib (clean namespace), and make sure that emms-mpv-ipc-proc is
defined before it is referenced.

Oh, right, forgot to check it for warnings after updates, fixed both of
these, thanks.
Second one by adding dummy define for emms-mpv-ipc-proc to keep
defcustoms in the head of the file before any internals for readability
(as I tend to skim code to find tunables myself, not customize via ui).

Post by Yoni Rabkin
Please feel free to merge this into master so that more people can
hammer on it.

Done.
I did squash all commits of the initial implementation into one, as they
don't seem to be very relevant to overall history, same as it was with
previous implementation.
Indeed, more exposure should hopefully find more interesting uses and
tweaks for many things that mpv offers via its APIs, as well as weed
out bugs faster.

Another change needed is to add a regexp to the player so that
(emms-player-get emms-player-mpv 'regex) will return a useful value that
emms-source-file-regex can use.

Among other things this breaks is that the various
emms-add-... functions will add files that the player doesn't know how
to play. For instance, jpegs that litter album directories will start
showing up in the playlists, etc.

Adding this:

(apply #'emms-player-simple-regexp emms-player-base-format-list)

...is typically what we start with.

[Mike Kazantsev]

On Fri, 25 May 2018 18:59:06 -0400

Post by Yoni Rabkin
Another change needed is to add a regexp to the player so that
(emms-player-get emms-player-mpv 'regex) will return a useful value that
emms-source-file-regex can use.
Among other things this breaks is that the various
emms-add-... functions will add files that the player doesn't know how
to play. For instance, jpegs that litter album directories will start
showing up in the playlists, etc.

Didn't think that it makes sense for mpv - it can totally "play" jpegs
and other images, and has options for these, as well as any binary file
one can imagine (e.g. with the right ffmpeg pipeline setup in lua).
So these internal restrictions seemed to be kinda bogus for it.

But do see what you mean with skipping files in context of music albums.

Guess it'd indeed be best to restrict it like that by default and let
people who want to use mpv for non-audio/video override it.

[Mike Kazantsev]

On Sat, 26 May 2018 04:14:52 +0500

Post by Mike Kazantsev
On Fri, 25 May 2018 18:59:06 -0400

Post by Yoni Rabkin
Another change needed is to add a regexp to the player so that
(emms-player-get emms-player-mpv 'regex) will return a useful value that
emms-source-file-regex can use.
Among other things this breaks is that the various
emms-add-... functions will add files that the player doesn't know how
to play. For instance, jpegs that litter album directories will start
showing up in the playlists, etc.

Didn't think that it makes sense for mpv - it can totally "play" jpegs
and other images, and has options for these, as well as any binary file
one can imagine (e.g. with the right ffmpeg pipeline setup in lua).
So these internal restrictions seemed to be kinda bogus for it.

One notable thing about other backends which don't seem to apply to mpv
in particular btw is URL formats - in default configuration on Arch,
for example, it passes these to youtube-dl, if can't process them
directly, and that tool has all kinds of unique url types for hundreds
of online media platforms.

And playlist of ytdl:// links seem to be a kinda nice thing to have,
which I'll try to have working with default regexps.

Post by Mike Kazantsev
But do see what you mean with skipping files in context of music albums.
Guess it'd indeed be best to restrict it like that by default and let
people who want to use mpv for non-audio/video override it.

[Mike Kazantsev]

On Sat, 26 May 2018 04:21:46 +0500

Post by Mike Kazantsev
On Sat, 26 May 2018 04:14:52 +0500

Post by Mike Kazantsev
On Fri, 25 May 2018 18:59:06 -0400

Post by Yoni Rabkin
Another change needed is to add a regexp to the player so that
(emms-player-get emms-player-mpv 'regex) will return a useful value that
emms-source-file-regex can use.
Among other things this breaks is that the various
emms-add-... functions will add files that the player doesn't know how
to play. For instance, jpegs that litter album directories will start
showing up in the playlists, etc.

Didn't think that it makes sense for mpv - it can totally "play" jpegs
and other images, and has options for these, as well as any binary file
one can imagine (e.g. with the right ffmpeg pipeline setup in lua).
So these internal restrictions seemed to be kinda bogus for it.

One notable thing about other backends which don't seem to apply to mpv
in particular btw is URL formats ...

Didn't realize that it's emms-player-simple that derives playable-p
function from that regexp, and otherwise it's apparently only used for
files, so shouldn't cause any issues for URLs.

Ended up using basic emms-player-base-format-list as suggested, as it
indeed seem to include all popular A/V extensions already.

[Ian Dunn]

MK> Hi,


MK> Have been using recently-merged dochang/emms-player-mpv emms backend for
MK> a while, but recently had an thought to get track durations from mpv, as
MK> pre-scanning them otherwise from mostly-sshfs sources is quite slow.

MK> So implemented a different backend for mpv using its bidirectional "JSON
MK> IPC" API with long-running mpv instances (kinda like mpd):

MK> https://github.com/mk-fg/emacs-setup/blob/master/extz/emms-player-mpv.el


MK> Advantages of using mpv this way vs player-simple (that I can think of):

This looks great! Couple of comments after a brief look at it:

1a. You should require s and dash in the header, as they both appear to be dependencies. I can't immediately see any others.

1b. Neither is currently a dependency of EMMS, so either they'd have to be added, or removed from your code.

2. In "emms-mpv-ipc-line", give the option for users to introduce their own event handlers. This makes it easier to extend to support additional events.

3. As stated in another email, you use the --idle flag, which seems to exist for this very purpose. Thanks for that.

4. A general convention when dealing with external processes in emacs is to break up command line args and the actual program. It especially helps when users want to use a specific binary. Otherwise, I'd have to modify just the first argument of a list if I wanted to modify it at all in a robust way.

5. Adding to 4, I'd recommend making emms-mpv-proc-cmd a defcustom instead of a defvar.

[Mike Kazantsev]

On Fri, 13 Apr 2018 15:23:05 -0400

Post by Ian Dunn
1a. You should require s and dash in the header, as they both appear to be dependencies. I can't immediately see any others.
1b. Neither is currently a dependency of EMMS, so either they'd have to be added, or removed from your code.
2. In "emms-mpv-ipc-line", give the option for users to introduce their own event handlers. This makes it easier to extend to support additional events.
3. As stated in another email, you use the --idle flag, which seems to exist for this very purpose. Thanks for that.
4. A general convention when dealing with external processes in emacs is to break up command line args and the actual program. It especially helps when users want to use a specific binary. Otherwise, I'd have to modify just the first argument of a list if I wanted to modify it at all in a robust way.
5. Adding to 4, I'd recommend making emms-mpv-proc-cmd a defcustom instead of a defvar.

Completely agree.

Definitely need to make quite a few defvar's there into defcustom's,
including separate binary/args.

s/dash deps (pretty sure I use cl-return and let* too) are only
omitted because it's in the repo where I use them all over the place,
and would be better to drop them entirely for emms code to avoid new
requires there.

Will be sure to address all these, thanks a lot for pointing them out!