On Mon, 27 Jan 2025 19:11:16 -0500
Richard Sent <rich...@freakingpenguin.com> wrote:
> ---
> Hi all,
>
> I've had some annoyances where Emms only supports changing the global
> system volume OOTB when I really want to /just/ change mpv's volume. I
> took a first stab at a patch, but it definitely shouldn't be merged
> since a) emms-volume-mpv-change is a bit ugly and b) I haven't done
> any copyright assignment yet.
>
...
> Some thoughts:
>
> 1. Should emms-volume-mpv be merged with emms-player-mpv, similar to
> emms-player-mpd? Or should it remain in a separate file?
Hi,
Personally I have WM/DE hotkeys to tweak volume in audio output for
that particular mpv instance (*), but yeah, it sounds like a good thing
to have bundled with the player backend (to me at least), as it's very
much tied to its API and provides kinda basic player functionality
(that probably should've been there from the start).
(*) Identified via PULSE_PROP_media.role=music in
emms-player-mpv-environment, which also allows for pulseaudio (**)
to remember volume of emms mpv separately from other mpv instances.
(**) Haven't looked at updating it to pipewire yet myself, probably
doesn't work/apply to --ao=pipewire.
> 2. I'd like to add some synchronous paint over the async handler-based
> emms-player-mpv-ipc-req-send functions to support functions like
> emms-volume-mpv-get, but can't think of a good solution. Busy-waiting
> seems like the only option.
One hacky way I've used previously is emms-player-mpv-cmd-prog macro,
which runs code after mpv command, transparently wrapping into a lambda.
It's still there, but marked as deprecated due to issues with lexical
binds - iirc among other issues due to that macro trickery under the hood.
Its usage looks something like this:
(emms-player-mpv-cmd-prog '(get_property vo)
(let ((vo (aref mpv-data 0))) ...)
IMO not very useful, kinda have to build code around that macro,
and it's still same disjointed callbacks under the hood.
But wouldn't emacs coop-multitasking threads work for that?
For example, something like this:
(defun emms-volume-mpv-change (amount)
(make-thread (lambda () (let*
((vol-max (ipc '(get_property volume-max)))
(vol-old (ipc '(get_property volume)))
(vol-new (max (min (+ vol-old amount) vol-max) 0)))
(ipc `(set_property volume ,vol-new))
(message "Volume is %d%%" volume)))))
With idea behind make-thread here being twofold:
- To avoid hanging emacs for the duration of this operation,
i.e. while waiting for mpv to respond, even if it's quick normally.
- To allow using synchronous-looking "ipc" calls, which
actually get resolved by callbacks in (non-blocked) main thread.
Synchronous "ipc" calls can use callback-lambdas setting result + condition:
(defvar ipc-sync (make-mutex "ipc-sync"))
(defvar ipc-sync-check (make-condition-variable ipc-sync "ipc-sync-check"))
(defvar ipc-sync-req nil)
(defvar ipc-sync-reply nil)
(defun ipc (cmd)
"Run mpv command and get result synchronously for current thread"
(with-mutex ipc-sync
(while (or ipc-sync-req ipc-sync-reply) (condition-wait ipc-sync-check))
(setq ipc-sync-req t)
(emms-player-mpv-ipc-req-send (cmd) #'(lambda (data err)
(setq ipc-sync-req nil ipc-sync-reply (list data err))
(with-mutex ipc-sync (condition-notify ipc-sync-check))))
(while (not ipc-sync-reply) (condition-wait ipc-sync-check))
(cl-multiple-value-bind (data err) ipc-sync-reply
(setq ipc-sync-reply nil)
(condition-notify ipc-sync-check)
(if err (error "Failed to run %s with error %s" cmd err) data))))
I think emms-volume-mpv-change above is supposed to work like this:
- emms-volume-mpv-change calls (ipc '(get_property volume-max)).
- It sees empty req/reply, so proceeds to set ipc-sync-req
(for any other (ipc ...) call to wait), and send its request.
- Since ipc-sync-reply isn't set immediately after request was sent,
waits for ipc-sync-check.
- Eventually mpv reply callback resets ipc-sync-req, sets reply and check.
- As ipc-sync-check was set, initial ipc call proceeds,
clears/sets ipc-sync-reply/check for any next call to proceed,
parses and returns reply data (or signals error) for this one.
- emms-volume-mpv-change gets its result, proceeds to next (ipc ...) call, etc.
Haven't tested that code at all though, maybe I'm missing something
basic about why it's not really an option and wasn't considered?
(haven't used threads in emacs myself, so it seems quite possible)
Emacs threads doc:
https://www.gnu.org/software/emacs/manual/html_node/elisp/Threads.html
> 3. Any other useful features? I know [3] brought up ao-volume which is
> worth including, although in certain cases software gain may be the
> only option if the source is exceptionally quiet. I think the perfect
> solution is more complicated than "use ao-volume", especially as
> ao-volume's behavior changes depending on the underlying
> implementation.
Yeah, wrt that earlier [3] comment about volume ("softvol") vs ao-volume,
I think when e.g. raising the volume via some default knob,
ideally something like this would happen:
- Raise softvol up to a limit before sound clipping starts to occur.
- Then start raising ao-volume until it hits its maximum.
- Start raising softvol up again - no other options at this point.
This would raise volume using whatever means mpv has at its disposal,
without knowingly making sound worse (clipping).
"sound clipping starts to occur" point can only be measured by mpv and
its audio processing pipeline, and don't think it exposes that via any
properties in some very generic way.
So best way is probably to just let user decide whether they want to
change "volume" or "ao-volume", kinda like mpv itself does/supports.
Iirc "volume" is the default in mpv (on e.g. 9/0 keys and whatever GUIs),
likely for exactly that kind of "avoid messing with system volume" reason.
(but didn't check, iirc it used to be "ao-volume", so there's probably
an issue/commit with rationale when that default changed)
Alternative can be adding ffmpeg filter that'd either measure or
normalize volume, e.g. dynaudnorm, loudnorm and such, e.g.:
--af=lavfi=[dynaudnorm=f=75:g=25:p=0.55]
But that's probably not something emms should do by default,
as it can be more resource-intensive and change source audio in
unexpected ways (e.g. maybe its volume is supposed to be jumpy).
> My goal with this patch is to avoid tweaking system
> volume, be a shame if that only works for a fraction of users.
On "avoid tweaking system volume" - I think usually "ao-volume" ends up
translating to "volume of mpv audio stream" (with e.g. pulseaudio or
pipewire on linux), and not supposed to affect volume of anything else
on the system.
I.e. not "system volume" as in "single volume knob on speakers".
But it's probably not universally like that, which is why I use that
aforementioned "PULSE_PROP_media.role=music" env-var for example
(so that pulseaudio module-stream-restore remembers this volume separately),
and some configurations use "flat volumes" which can raise shared volume
if that's needed to not go into >100% softvol territory too.
Or maybe there's just no per-stream controls, like in ALSA without dmix.
Just wanted to note that "ao-volume" can translate to different things.
> 4. I really really hate fetching properties one by one and registering
> new handlers every time.
Yeah, definitely not my favorite way to structure it as well.
Maybe that version with make-thread above can be a better alternative.
>
> [1]: https://lists.gnu.org/archive/html/emms-help/2023-01/msg00034.html
> [2]: https://lists.gnu.org/archive/html/emms-help/2020-12/msg00006.html
> [3]: https://lists.gnu.org/archive/html/emms-help/2023-02/msg00000.html
--
Mike Kazantsev // fraggod.net
