Re: [PATCH v2] * emms-volume-mpv.el: New file.

[Mike Kazantsev]

On Fri, 14 Feb 2025 11:03:15 -0500
Richard Sent <richard@freakingpenguin.com> wrote:

> ---
> Hi all,
> 
> > 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).  
> 
> Understood. For now I'll keep it separate since it's easier for me to
> track mentally but we can move it later.

Yeah, thanks, it looks great.

Guess I can merge and push it, unless anyone spots any issues in a
couple days?

I should probably add it to docs and to emms-volume.el as well,
so that it'd be easier to know about and find/use in that case.

It didn't occur to me at the time, but separate file seem to also make
more sense in context of being a part of emms-volume.el subsystem too.


> > "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.  
> 
> I've added emms-volume-mpv-method. This allows for pure "volume"
> control, pure "ao-volume" control, and a "smart" mode. The smart mode
> tries to avoid clipping by ensuring both "volume" and "ao-volume" are at
> 100% before raising "volume" above 100%. When lowering volume, it first
> drops "volume" to 100%, then lowers "ao-volume" to 0. This should allow
> for mostly-system-integrated volume management while still supporting
> mpv's software volume boost.

Sorry, didn't mean it as in "should be changed to work like that" at
all, was just thinking to comment on these volumes, but in retrospect
guess it's easy to read like that. 
(as I'm supposed to be a maintainer or something, not just random
person in a thread)

But with a method-switch it definitely seems more flexible, and won't
need e.g. someone wanting ao-volume directly to patch it later.


> While ao-volume properties above 100% should be possible, I don't think
> mpv supports them. Attempting to raise ao-volume above 100% causes this
> error:
> 
> (error "Failed to run (set_property ao-volume 105.0) with error unsupported 
> format for accessing property")
> 
> Because ao-volume-max doesn't exist [1], this cap can't be changed. Ergo
> I capped ao-volume at 100%.

Don't think it really matters how it's done, but other way around that
error that comes to mind is how mpv inputs do it - sending something
like "add ao-volume 5" command, which should have mpv increase and cap
the value as-needed.

Iirc these commands don't return resulting value, so volume will still
need to be fetched for minibuffer message, which seems important to
echo instead of only changing stuff blindly.


Guess alternative way to implement this with "add" commands in general
can be to use emms-player-mpv-observe-property on 'volume and 'ao-volume
from emms-player-mpv-event-connect-hook and process those to echo changing
volume anytime via emms-player-mpv-event-functions.

So that after e.g. "add volume -5" is processed and mpv changes volume,
it'd notify emacs about "volume" property change and it'd be printed in
minibuffer in a similar way.

One advantage with observe_property would be that changing volume in 
mpv externally somehow - e.g. via pavucontrol or pressing hotkeys 
and GUI elements in mpv window - will also print info about changes 
in emacs, but dunno if it's needed there on those external changes.

Also this allows to track and change these volumes via emacs-side
variable (as observing property will keep it in sync), but don't think
that's how emms-volume.el does it anyway, and maybe a bit too magical.


Don't mean to say that anything needs changes at all, as I don't know
how keeping volumes more in-sync like that would be useful.
It just didn't occur to me earlier, so thought to mention that it can
also be implemented in this other alternative way as well.

(...guess maybe if there was "display emms volume emoji in mode line"
widget or something like that, to avoid it needing to poll for
volume(s) every N seconds, but then such widget can probably register
its own observer hooks for its own purposes, doesn't really need to be
rely on volume-changing implementation details)


> While developing this patch I found some odd behavior when volume was
> changed rapidly. Threads would indefinitely block on
> emms-volume-mpv--ipc-sync-check and the volume would suddenly jump to
> unrelated values. I think this was because of multiple threads calling
> emms-volume-mpv-synchronous-ipc, which interacted with each other in odd
> ways.

It's also been my experience with threads that "put one sync mutex
around as much as possible" tends to be the best way to side-step most
quirks, and usually worth doing just in case, if possible.

And most obvious emacs docs in particular seem to not guarantee strict
cooperative multitasking behavior (where switching threads only happens
at exact thread-yield calls or mutex/condition waits), saying that it's
"mostly cooperative", so IMO seems best to just make these calls as
sequential/non-concurrent as they can be, to be as future-proof against 
hard-to-debug threading quirks as possible.


> One limitation of the current patch: ao-volume is only exposed while
> mpv's audio output is active. With the 'system or 'smart methods, if the
> user tries adjusting volume before mpv starts playing, or if the user
> plays media without audio, the volume cannot be adjusted. I don't know
> if this can be resolved. The initial value for ao-volume may change
> depending on what it was when mpv was last run.

Afaik that's also how mpv hotkeys (and commands, other UI elements) work -
prints some error until ao-stream is actually created somehow after
playback starts, where latter part can be delayed if it's from network 
source/fs,
spun-down hdd or whatever.
So seems like a current feature or mpv limitation in general.
(and using e.g. "add ao-volume 1" command won't work around this either)


I think if it's useful enough for emms to control this in a stricter
way than how mpv itself does it, then emms-player-mpv-observe-property +
emms-player-mpv-event-functions seem to be one obvious way to do it.

I.e. observe 'ao-volume, which I think mpv should send immediately when
it's available, and if it doesn't match what's set in emms - adjust it asap.
To avoid emms completely taking control over ao-volume from other mpv inputs,
alternatively maybe it can only be set by emms once, when it first sees that
properly from mpv, and then unobserve or ignore it (leave up to mpv).

Can see this kind of extra control being useful as an alternative to
PULSE_PROP_media.role=music env var which I've mentioned in an earlier 
email (makes pulseaudio store emms-mpv stream volume separately).

But same as above, it's just how I can think of implementing "this can
be resolved" part, which I've no idea if useful enough or worth doing.


-- 
Mike Kazantsev // fraggod.net